Conditional assignments used for side-effects trigger "never used"

[dbushong]

If you're using the return value of x ?= y() for cheap memoization, that triggers "Variable ... is never used"

Code sample

cache = null

work = ->
  console.error 'doing work'
  42

fn = -> cache ?= work()

console.log fn()
console.log fn()

Output / stack trace

  ⚡ foo.coffee
     ⚡ #1: Variable "cache" is never used.
     ⚡ #7: Variable "cache" is never used (first defined on line 1).

⚡ Warning! » 0 errors and 2 warnings in 1 file

coffeelint.json

{
  "check_scope": {
    "module": "coffeescope2",
    "environments": ["node"],
    "overwrite": false
  }
}

[za-creature]

Thanks for reporting this.

IMO, this doesn't require a behavioral change as I don't consider it a bug (see #10) but I did update the unused-var error message to be more descriptive and hopefully cause less confusion.

As of 0.4.2 (just released), your code will produce: Variable "cache" is assigned to but never read which is a bit more descriptive of what's happening.

To fix the error, you need to actually do something with cache:

cache = null

work = ->
  console.error 'doing work'
  42

fn = ->
  cache ?= work()
  return cache

console.log fn()
console.log fn()

Because in this case the two samples are functionally equivalent (the "return cache" statement is implicit), there's some debate to be had here about what does return cache = "value" really means. Does it:

a) Return the value of cache after (conditionally) assigning "value"
b) Return the value of the assignment expression itself (which is defined as the value on the right hand side)

The generated javascript code for conditional assignments is somewhere in between:

return cache != null ? cache : cache = "value"

coffeescope2 assumes that the result of an assignment is the value of the assignment, thus it does not consider it as reading the assigned variable. This is to catch cases like this:

foo = bar = baz = "qux"

console.log(foo)
# bar is never read
console.log(baz)

If you have strong feelings the other way, please reopen and I'll put this (better handling of implicit returns) on the roadmap for 1.0

[dbushong]

I don't know about strong feelings, but given the implicit return is what this code was aiming for (this was a real example that came up in my code, btw), and given that cache ?= work() in this context compiles to the exact same JS as return cache ?= work(), I feel it's worth Doing This Right

FWIW, great tool - coffeelint without coffeescope2 is sadly lacking :-)

[za-creature]

Anything bigger than 'meh' is a strong feeling in my book :)

Reopening

[dbushong]

Keep in mind, this problem isn't specific to implicit returns; it's any use of the "return value" of any assignment-like: ?= and ||= being the obvious ones, but there are arguments to be made for &&=, *=, +=, /=, and maybe even =

We had some code that looked like:

module.exports = descriptiveFnName = -> whatever

Where the name in the middle gets you better error traces. I had to convert that code to:

descriptiveFnName = -> whatever
module.exports = descriptiveFnName

...which is debatably better code, but seems like a silly exercise in tricking the linter.

[za-creature]

There's definitely an exception to be made for module.exports = descriptiveFnName = -> as there's precedent for that: https://github.com/za-creature/coffeescope/blob/master/test/ScopeLinter/unused.coffee#L69

I also agree on the handling of *= et all, I think there's an inconsistency there in the way += is handled in comparison with the way ++ is handled. I am however on the fence here on whether to go by the spec or attempt to guess what the user meant (in the end, isn't that what linters are for? all code that compiles is technically by-the-spec)

This is a longer discussion and one that I'm happy to have under normal circumstances; unfortunately, right now is not a good time for me to think about this stuff as I'm a bit stretched thin with other things. I do however want it on the record that I have no intention of abandoning or even just keeping this project on life support: all open issues will be fixed, it's just a matter of when I find the time to do it.

Thanks for your report and thoughts! All feedback is appreciated as it helps me better understand how coffeescript is used in the wild (when I started this project, I figured most people just wrote python-like code, which is exactly why this issue has been overlooked: it's nowhere near to what the python equivalent would be).

[za-creature]

Closing this in the context of #43