try/catch unnecessarily defines unused variable error

[benjie]

The CS code

main = ->
  try
    throw new Error "Hello"
  catch e
    console.log e
  return

Compiles to:

// Generated by CoffeeScript 1.10.0
(function() {
  var main;

  main = function() {
    var e, error;
    try {
      throw new Error("Hello");
    } catch (error) {
      e = error;
      console.log(e);
    }
    return;
  };

}).call(this);

However catch (error) defines error within the scope of the catch block - defining it outside the scope of the catch block has no effect (and means there's a defined but unused variable which causes ESLint issues). This demonstrates that var error is not used:

function main() {
  var error = null;
  try {
    throw new Error("Hello");
  } catch (error) {
    console.log("Error 1", error);
  }
  console.log("Error 2", error);
}
main();

which results in:

Error 1 [Error: Hello]
Error 2 null

If I'm reading them right, section 12.14 of both v3 and v5 of the ECMA specs seem to imply that the catch block adds a new scope to the scope chain.

[vendethiel]

...Sadly, that last part isn't true on JScript, which is why CoffeeScript has this behavior. See #1476 / #2422 and a few duplicates.

[benjie]

@vendethiel I suspected JScript may be at fault here; however v1.9.3 didn't define the additional variable:

// Generated by CoffeeScript 1.9.3
(function() {
  var main;

  main = function() {
    var e;
    try {
      throw new Error("Hello");
    } catch (_error) {
      e = _error;
      console.log(e);
    }
    return console.log(e);
  };

}).call(this);

The only negative I can see to this approach is that on the faulty JScript it will define/clobber a global _error variable.

The v1.10.0 solution creates an ESLint error which, if disabled, will allow many more issues in a codebase to go undetected (defining a variable but never using it is often a sign that something's awry). Sadly the unused variable detection in CoffeeLint is not as powerful as that of ESLint so we tend to use both (we even wrote an ESLint formatter that reports the location of the issue honouring sourcemaps).

I think this particular solution to the issue needs revisiting.

[jashkenas]

Yes, it looks like a potential regression. I think the scoping was refactored a bit more recently.

[vendethiel]

oh whoops, sorry again. I didn't look closely enough... my bad.