21

I'm trying to understand why I'm getting the following error, not how to work around it.

Passing the following code to JSLint or JSHint yields the error 'err' is already defined.

/*jslint white: true, devel: true, onevar: true, browser: true, undef: true, nomen: true, regexp: true, plusplus: true, windows: true, bitwise: true, newcap: true, strict: true, maxerr: 50, indent: 4 */
function xyzzy() {

    "use strict";

    try { /*Step 1*/ } catch (err) { }
    try { /*Step 2*/ } catch (err) { }

}

The obvious assumption here is that catch behaves, or should behave, like a function. Thus, err is neither a global variable, nor a local variable to xyzzy, but a parameter for the catch block.

In browsing the ECMA-262 Standard, section 12.14 describing The try Statement indicates that the catch clause takes an Identifier that is bound to an exception. Additionally the semantic production rule for catch refers to a parameter that's passed calling out Identifier as an argument.

This seems to suggest to the casual reader that the above code is valid and that perhaps the lint tools have a bug.

Even IntelliJ's strictest JavaScript code inspection analysis doesn't report there being a problem with err being redefined.

More of a concern, if it is a variable scoping concern, then one might surmise that the err is bleeding into the global space, which poses a whole host of other problems, and that instead one should declare it up front, like this:

/*jslint white: true, devel: true, onevar: true, browser: true, undef: true, nomen: true, regexp: true, plusplus: true, windows: true, bitwise: true, newcap: true, strict: true, maxerr: 50, indent: 4 */
function xyzzy() {

    "use strict";
    var err;  // DECLARE err SO IT IS CERTAINLY LOCAL

    try { /*Step 1*/ } catch (err) { }
    try { /*Step 2*/ } catch (err) { }

}

But this only results now in two errors about err at each of the catch statements, making the problem worse and potentially introducing variable shadowing.

The lint tools are suggesting that each catch block introduces not just it's own lexical scope, but a new variable as well. This can't be right.

Simply making err1, err2, ... to placate the static analysis tools merely hides the symptom and doesn't contribute to cleaner code.

JavaScript Gurus: Is this a bug in the lint tool, a dark corner with the JavaScript spec, or a fundamental misunderstanding of what's happening here?

UPDATE: Wrote to Douglas Crockford, author of JSLint, and it turns out there's a very valid reason for this warning. See answer below.

Walt Stoneburner
  • 2,562
  • 4
  • 24
  • 37
  • 4
    It's important to limit the amount of credence given to complaints from JSLint. – Pointy May 23 '11 at 16:40
  • 1
    @Pointy - understood, but to date JSLint has raised some very valid points when it's come to subtle issues, which is why I use it. If it's actually a bug, I'll contact Douglas as I have in the past, but I figured I'd explore the issue further before doing so. It always helps to understand a language inside and out, as well as the warnings a linter produces. I'll ignore those warnings only when I know the lint tool is wrong, which is possible, though unlikely. – Walt Stoneburner May 23 '11 at 16:53
  • REMINDER ALL: The goal is is not about scope, what happens in the catch block, or shadowing of other variables. It's about why is lint treating a parameter to catch as if it were a unique variable declaration. Is there a subtle use case why lint would do this? It's not just enough to ignore it if it's legal, but if the practice of doing the above could introduce a subtle bug. That's the exploration, which in turn dictates an appropriate fix. – Walt Stoneburner May 23 '11 at 17:03
  • Well the spec is pretty clear that the identifier is bound in a "new declarative environment", so this must be considered a style comment rather than a warning about actually dangerous code. (Of course, one might consider questionable coding style to be "dangerous", but it's in the realm of opinion.) (in my opinion :-) – Pointy May 23 '11 at 17:03
  • @Point - I'll certainly take "style" as an acceptable answer; I just need to figure out the 'why' that style would be problematic. (Ever see how JSLint treats switch/case statement indentations? Feels foreign, makes sense after a while.) I figure the check was added after someone, somewhere got bitten by something really evil and subtle. I'd love to know what that was. – Walt Stoneburner May 23 '11 at 17:15
  • Another confirmation that it's a parameter; used IntelliJ's refactoring tool to rename `err` in my actual code and it only affected the ones you'd suspect, specifically the stuff inside the scope block and not beyond. Slowly growing more convinced it's a JSLint/JSHint bug and not based on a useful practice; I've written Crockford in the event I've missed something ultra subtle. – Walt Stoneburner May 23 '11 at 17:38

4 Answers4

11

Wrote to Douglas Crockford, author of JSLint, about this issue.

There turns out to be a very valid reason after all...

Douglas writes:

Catch variables are not scoped correctly, so I recommend that you use a different name in each one.

If you look at this similar StackOverflow question you'll note that PleaseStand started to touch on it. Not all browsers, especially historic ones, handle scoping correctly or consistently.

JSLint recognizes that your code may work in one browser, but not in another, leaving a very nasty and subtle bug to track down. The warning is real.

By using a different name, which, yes, doesn't feel clean or concise at all, because it isn't, happens to be the universal way of not running into the problem.

Thank you Douglas for clarifying! Mystery solved.

Community
  • 1
  • 1
Walt Stoneburner
  • 2,562
  • 4
  • 24
  • 37
3

The specification is quite clear that any name defined a catch statement will do nothing more than shadow a surrounding name. Beyond that I would not consider these errors as nothing more than a warning. Just using pure intuition I believe that this is simply overzealous analysis on the part of the designer of those Lint tools.

Since a catch block introduces a new scope, using the same name will simply shadow any similar names in the enclosing scope. This isn't necessarily a bad thing if you are aware of the semantics. If you are coding under the assumption that the enclosing err will be accessible than you'll need to change your assumptions.

Specification

The production Catch : catch ( Identifier ) Block is evaluated as follows:

  1. Let C be the parameter that has been passed to this production.
  2. Let oldEnv be the running execution context’s LexicalEnvironment.
  3. Let catchEnv be the result of calling NewDeclarativeEnvironment passing oldEnv as the argument.
  4. Call the CreateMutableBinding concrete method of catchEnv passing the Identifier String value as the argument.
  5. Call the SetMutableBinding concrete method of catchEnv passing the Identifier, C, and false as arguments. Note that the last argument is immaterial in this situation.
  6. Set the running execution context’s LexicalEnvironment to catchEnv.
  7. Let B be the result of evaluating Block.
  8. Set the running execution context’s LexicalEnvironment to oldEnv.
  9. Return B.

NOTE No matter how control leaves the Block the LexicalEnvironment is always restored to its former state.

ChaosPandion
  • 77,506
  • 18
  • 119
  • 157
  • The second code block was merely a test to see if that would help or hurt the problem. (It hurt.) JSLint is trying to tell me something or steer away from somewhere that I could hurt myself, I want to know what that is -- not ignore warnings. The actual code would not have a local variable. – Walt Stoneburner May 23 '11 at 16:50
  • 1
    Do you have a reference that catch blocks definitively introduce a new scope? Not doubting you, just want to read up on it. – JaredPar May 23 '11 at 16:52
  • 1
    @JaredPar: Catch blocks don't introduce a new variable environment, in ES3 they simply introduce an object at the top of the scope chain, that has a property named like the catch identifier, and its value is the exception that has been raised, in ES5 they introduce a LexicalEnvironment, but again not a "new scope" (variable and function declarations are always hoisted). – Christian C. Salvadó May 23 '11 at 16:56
  • 2
    Catch doesn't introduce a new scope. `catch (e) { ... this is not a new scope ... }` http://jsfiddle.net/robert/sZrGb/ – Robert May 23 '11 at 16:57
  • @Robert nice proof by example :) – JaredPar May 23 '11 at 16:58
  • 1
    Rereading his answer, I think he means (err) will be scoped to the catch block, which is true, but the catch block itself is not a scope. Maybe clarify that in your answer. – Robert May 23 '11 at 17:01
  • @JaredPar: In reading section 12.14 for the production rules, it appears according to the ECMA-262 that it's actually creating a new lexical environment (see steps 2-3 where it invokes NewDeclarativeEnvironment). Admittedly, I'm still trying to tangle through what that all means at the lowest nuts'n'bolts, but my take is that the catch block can introduce its own local variables while, of course, having access to the passed parameter, and the outer scope as well. I'd suspect that 'this' is still the current object. But good point to challenge. – Walt Stoneburner May 23 '11 at 17:10
2

Check this answer out: JSLint complaining about my try/catch

As has mentioned, try opens a new block-scope. See https://developer.mozilla.org/en/JavaScript/Reference/Scope_Cheatsheet

Indeed the top of the document explains that it's not all standard, but in ES5, section 12.14 the section on executing a catch block clearly defines MDC's description as standard:

No matter how control leaves the Block the LexicalEnvironment is always restored to its former state.

Community
  • 1
  • 1
davin
  • 44,863
  • 9
  • 78
  • 78
  • I wish I had bumped into that question before hand, as I would have most certainly referenced it. I was able to deduce what JSLint was doing, but what I don't get from that post's answer is the why. I'm thinking it may be time to break from speculation and just ask Crockford. I'll let everyone know what I find out. – Walt Stoneburner May 23 '11 at 17:17
  • Your Scope_Cheatsheet link further supports the idea that `err` is a parameter and not a variable. I'm starting to suspect it's merely a lint bug. I've asked Douglas if he could expound if there's a reasoning behind it that I'm just not seeing. He's usually very good with turnaround times. – Walt Stoneburner May 23 '11 at 17:33
0

You can have a problem with reusing the same parameter name in sequential try-catches if you later need to refer to the error.

If more than one catch was called, only the last one will be in scope for a finally or function scope expression.

jsLint is conservative- if you can prevent a possible screw up with a unique variable, why not use it?

kennebec
  • 102,654
  • 32
  • 106
  • 127
  • Ideally one shouldn't be trying to access an error in another catch block based on the parameter that was passed into it; if needed, squirrel it away. According to the Scope_Cheatsheet in davin's answer, it's not even possible. As Crocker pointed out, not all browsers handle scoping the same way (having read some IE8 and older material). We've just learned JSLint isn't conservative, it's actually right -- especially if you want to deal with historic, non-compliant browsers. The natural gut reaction was not to introduce the unnecessary symbols, but this "trick" solves a greater problem. – Walt Stoneburner May 23 '11 at 18:18