Closed Bug 622646 Opened 9 years ago Closed 5 years ago

catch/let name collision error message should be more descriptive


(Core :: JavaScript Engine, defect)

Not set





(Reporter: cdleary, Assigned: sgowzilla)


(Whiteboard: [good first bug][lang=c++][mentor wanted])


(3 files, 3 obsolete files)

Should be an easy fix.

<hdon> interesting: if you shadow the variable "e" in (catch e) jsapi says "redeclaration of let e"
<evilpie> is there some bug about JSObject size?
<cdleary-lappy> hdon: the catch scoping uses the let semantics, since they're identical
<jimb> hdon: Yeah, that's by analogy with 'var e'.  It doesn't read well, does it?
<hdon> yeah it's clear what's going on now, but i started my investigation by searching let.*\<e\>
<hdon> or something like that
<jimb> hdon: (catch variables are treated like let bindings, in that they're not floated.)
* hdon nods
Can I be assigned this bug?
Sure! If you have any questions, feel free to join #jsapi on
Assignee: general → vandrew88
Also, to make your patches easier to review and check in, please read the page below to give you some submission guidelines.
Hey guys, 

I'm interested to work on this issue, can I help at this point?
Hi Afshin,
The relevant line throwing the error is (search for JSMSG_REDECLARED_VAR, declared at Waldo, could you make sure whoever knows the parsing code sees this and can be available for questions?
Flags: needinfo?(jwalden+bmo)
I think the fix should be easy, can I be the fixer?
Assignee: vandrew88 → afshin.meh
Just need some information about this issue, I'm available on irc with `afshinmeh`.
Feel free to ask questions here.
So my question is how we should change the message to make it more descriptive? Any clue?
The message itself lives in js/src/js.msg.  As for how to change it to make it more understandable, I suspect what we want to do is add a new message to that file, patterned off the existing message line that's being used right now, and tweak the wording a bit.

As for what wording: suppose you were a developer who accidentally triggered this error.  What would *you* want to see as the error message?  If you can answer that, then there you go.

I would perhaps try to provide more of a comment/answer here, right now, but I'm at the far end of my day and my brain's turned to mush.  But I wanted to at least say something here as help, and to show I'm following progress here, even if it's not as 100% helpful as I might ideally want it to be.  If my comment here isn't enough, just needinfo? me again and I can look harder.  :-)
Flags: needinfo?(jwalden+bmo)
About the current message, should be abandon it and make a new message or just alter the old one? 

And and the meaning of the new message, I believe show the line and column of the old declared variable would be helpful, what you think?
Flags: needinfo?(jwalden+bmo)
Sorry for the delay, been busy lately.  :-(

A new message is probably better.  (Of course, if that's the only use of this, you might as well just change this one -- but I don't think it is, offhand.)

The error-reporting mechanism accepts a node as a reference point, so you'll get *some* sort of coordinates in that (assuming a non-null node is being specified -- check that first, if necessary).  If you mean to show/expose coordinates for both, that may be slightly trickier.  Wouldn't hurt to show both, but it depends how ambitious you are.  Just wording changes, or being more precise about the issue, is certainly good on its own.
Flags: needinfo?(jwalden+bmo)
Let me know what you think guys.
Attachment #8344327 - Flags: review?(jwalden+bmo)
Comment on attachment 8344327 [details] [diff] [review]

Review of attachment 8344327 [details] [diff] [review]:

The wording is enough to cancel this, I think.  But I also need more clarity about the exact set of scenarios where this can be hit, to say whether we're handling this all fully correctly with this new error message.

::: js/src/js.msg
@@ +222,5 @@
>  MSG_DEF(JSMSG_TOO_MANY_CATCH_VARS,    169, 0, JSEXN_SYNTAXERR, "too many catch variables")
>  MSG_DEF(JSMSG_NEGATIVE_REPETITION_COUNT, 170, 0, JSEXN_RANGEERR, "repeat count must be non-negative")
>  MSG_DEF(JSMSG_INVALID_FOR_OF_INIT,    171, 0, JSEXN_SYNTAXERR, "for-of loop variable declaration may not have an initializer")
>  MSG_DEF(JSMSG_INVALID_MAP_ITERABLE,   172, 0, JSEXN_TYPEERR, "iterable for map should have array-like objects")
> +MSG_DEF(JSMSG_REDECLARED_VAR_OR_CONST,173, 2, JSEXN_TYPEERR, "there is already a '{0}' named '{1}' exist")

The English of this is weird.  :-)  I think something like "a '{0}' named '{1}' already exists" would probably be better.

That said, I don't quite understand the set of cases where this hits now, to understand exactly whether this style of error message is the right fit for the situation.  Is this what should be happening with this patch applied?

[jwalden@find-waldo-now src]$ dbg/js
js> let x; var x;
js> const x; var x;
typein:2:13 TypeError: there is already a 'const' named 'x' exist:
typein:2:13 const x; var x;
typein:2:13 .............^
js> function x() {}; var x;
js> function x() {}; let x; 
js> try { } catch (e) { var e; }
js> try { } catch (e) { let e; } 
typein:6:24 TypeError: redeclaration of variable e:
typein:6:24 try { } catch (e) { let e; }
typein:6:24 ........................^

That last bit is especially sort of maybe worrisome, because a catch-block |e| isn't really a variable...I think.  Whatever new error happens here, I want to be sure we avoid referring to catch-block variables as "let" variables.  They might be under the hood, and semantically.  But that's not what they are to web developers, and we shouldn't refer to them that way, when talking to web developers.
Attachment #8344327 - Flags: review?(jwalden+bmo)
Thanks for the review Jeff. I understand the wording of the new message has some problems, so I fixed that.

For the next issue, as far as I know currently the catch parameters knows as a `let`. At least this test case shows this:

js> try { } catch (e) { const e; }  
typein:109:26 TypeError: a 'let' named 'e' already exists:
typein:109:26 try { } catch (e) { const e; } 
typein:109:26 ..........................^
js> try { } catch (e) { let e; }    
typein:110:24 TypeError: redeclaration of variable e:
typein:110:24 try { } catch (e) { let e; } 
typein:110:24 ........................^

I believe the last error message was happened here:

However, is there any more informations that you can give me to make a new patch for this bug? All I have currently is a new message (with a new wording). 

Should I submit a new patch for new message or not? Accept my apologize for such a question, I'm new in Mozilla :-)
Flags: needinfo?(jwalden+bmo)
I'm still waiting for more information on this issue :-)
Flags: needinfo?
No worries about questions, we were all new at some point.


js> try { } catch (e) { const e; }  
typein:109:26 TypeError: a 'let' named 'e' already exists:
typein:109:26 try { } catch (e) { const e; } 
typein:109:26 ..........................^

has me leery.  There's no |let| in sight here at all, so we shouldn't be claiming there is one.  And actually, now that I reread this bug, comment 0 is *specifically* about this issue -- that we're claiming there's a |let| when there's only a |catch|.  So this isn't fixable solely by changing an error message.  We need extra logic to detect when the name being shadowed is a catch-variable, and in such cases we need to have a custom message specifically tailored to the situation.

So, yes: a new patch is needed.  But one that simply adjusts the error message isn't enough -- it'll have to have extra logic to detect when a let-ful definition is actually a catch-variable, to merit that custom message.
Flags: needinfo?(jwalden+bmo)
Attached patch catch_name_collision.patch (obsolete) — Splinter Review
I saw that this bug had been sitting dormant for a few months, so I decided to go ahead and take a crack at it.

Example output:

js> try {} catch(e) { const e; }
typein:2:24 TypeError: redeclaration of identifier 'e' in catch:
typein:2:24 try {} catch(e) { const e; }
typein:2:24 ........................^
Attachment #8406373 - Flags: review?(jwalden+bmo)
Perhaps I jumped the gun on this one--sorry about that. May I be assigned to this bug?
hi Stephen, I just assigned this bug to you. Don't worry about "jumping the gun." Assigning a bug to someone is just a formality. You did the right thing by posting your patch and asking for Jeff's review. He has many review requests, so sometimes he is a little slow to respond. :)

If you have any questions or would like to ping Jeff, feel free to join #jsapi on
Assignee: afshin.meh → sgowzilla
Flags: needinfo?
Whiteboard: [good first bug] → [good first bug][lang=c++][mentor=?]
Comment on attachment 8406373 [details] [diff] [review]

Review of attachment 8406373 [details] [diff] [review]:

A drive-by nit: looks like you have several instances of trailing whitespace in this patch, which should be removed! :)
Attached patch catch_name_collision.patch (obsolete) — Splinter Review
Ah, good catch!  I've removed all instances of trailing whitespace from this patch.
Attachment #8406373 - Attachment is obsolete: true
Attachment #8406373 - Flags: review?(jwalden+bmo)
Attachment #8413718 - Flags: review?(n.nethercote)
Attachment #8413718 - Flags: review?(jwalden+bmo)
Comment on attachment 8413718 [details] [diff] [review]

Review of attachment 8413718 [details] [diff] [review]:

Looks ok to me, but Waldo should also review because he knows this stuff much better than I do.

::: js/src/frontend/Parser.cpp
@@ +2966,5 @@
> +                                JSMSG_REDECLARED_CATCH_IDENTIFIER, bytes.ptr()))
> +            {
> +                return false;
> +            }
> +            if(!parser->report(reporter, false, pn, JSMSG_REDECLARED_VAR,

Space after |if|, please.
Attachment #8413718 - Flags: review?(n.nethercote) → review+
Attached patch catch_name_collision.patch (obsolete) — Splinter Review
Added a missing space after |if|.

Nicholas, thanks again for your help!
Attachment #8413718 - Attachment is obsolete: true
Attachment #8413718 - Flags: review?(jwalden+bmo)
Attachment #8414508 - Flags: review?(jwalden+bmo)
Hi, Stephen - Are you still working on this?
Flags: needinfo?(sgowzilla)
With all due respect, the onus is on Jeff to actually review the patch here, not Stephen.
Flags: needinfo?(sgowzilla)
Sorry, thanks for clarifying. 'sup, Jeff?
Flags: needinfo?(jwalden+bmo)
Yea, I should've sent a review ping some time ago. I'm still available to work on this should the need arise.
Comment on attachment 8414508 [details] [diff] [review]

Review of attachment 8414508 [details] [diff] [review]:

Sorry for the horrendous delay here.  Been distracted dealing with pressing, tricky, complicated security concerns, and I couldn't spare the time to keep up with requests in that time.  :-(

This mess of statements, names, declaration types, the peculiarities of the callers, etc. makes my head hurt to comprehend.  But I *think* this is what we want.  Still, unless I'm misreading, I'd like to see this done very slightly differently to avoid the apparent problem of potentially warning twice here.

We want tests with patches, whenever possible, and it's possible here.  Copy js/src/tests/js1_8_5/extensions/array-length-protochange.js to a new redeclaration-of-catch-warning.js, put "// |reftest| skip-if(!" at the top of it, and do something like this in the guts of it:

    evaluate("try {} catch(e) { const e; }");
    throw new Error("didn't throw");
  catch (e)
    assertEq(e.message.indexOf("catch") > 0, true,
             "wrong error, got " + e.message);

You can run your test using

  python tests/ path-to-builddir/dist/bin/js js1_8_5/extensions/redeclaration-of-catch

and add the -o flag if you need to examine its output for some reason.

::: js/src/frontend/Parser.cpp
@@ +2967,5 @@
> +            {
> +                return false;
> +            }
> +            if (!parser->report(reporter, false, pn, JSMSG_REDECLARED_VAR,
> +                               Definition::kindString(dn_kind), bytes.ptr()))

So this will double-warn in some cases, I think.  Won't it?  Testing this a bunch I can't figure out *how* to get it to double-warn, and maybe it's not possible.  But in any event the hazard *appears* to exist with this code structuring, if |inCatchBody| doesn't also imply |error|.  So why not structure like so instead:

            if (!(inCatchBody
                  ? parser->report(reporter, false, pn,
                                   JSMSG_REDECLARED_CATCH_IDENTIFIER, bytes.ptr())
                  : parser->report(reporter, false, pn, JSMSG_REDECLARED_VAR,
                                   Definition::kindString(dn_kind), bytes.ptr())))
                return false;
Attachment #8414508 - Flags: review?(jwalden+bmo) → feedback+
Flags: needinfo?(jwalden+bmo)
Whiteboard: [good first bug][lang=c++][mentor=?] → [good first bug][lang=c++][mentor wanted]
No worries about the delay Jeff, I too have been busy lately. 

So, I was never able to trigger a double-warning with the way my code was structured. However, I'm also unable to prove it couldn't happen, so I did the safe thing and made your suggested change. I've also added the test you provided.

While testing, I noticed the error message you get when you shadow an exception identifier with a |let| statement is incorrect so I decided to fix that too.  I wasn't sure if this change was outside the scope of this bug so I included it as a separate patch.

Before applying catch_let_name_collision.patch:
js> try {} catch(e) { let e; }
typein:3:22 TypeError: redeclaration of variable e:
typein:3:22 try {} catch(e) { let e; }
typein:3:22 ......................^


js> try {} catch (e) { let e; }
typein:2:23 TypeError: redeclaration of identifier 'e' in catch:
typein:2:23 try {} catch (e) { let e; }
typein:2:23 .......................^
This patch contains the suggestions Jeff made in comment 30.
Attachment #8414508 - Attachment is obsolete: true
Attachment #8444214 - Flags: review?(jwalden+bmo)
This patch contains changes that correct the error message when a |let| statement shadows an exception identifier.
Attachment #8444216 - Flags: review?(jwalden+bmo)
Comment on attachment 8444214 [details] [diff] [review]

Review of attachment 8444214 [details] [diff] [review]:

This patch is (believe it or not) already bitrotted.  js.msg changes pretty quickly, if you're trying to squat unused error messages.  Easily fixed locally, tho.  (I tend to squat one of the later-in-the-file messages to reduce the chances of conflicts, but maybe I shouldn't have told you that.  ;-) )

I also prefer to put |if (typeof reportCompare === "function")| in front of the final reportCompare in a jstest so that I can run it standalone more easily.  No strict *requirement* for this, but it's a nicety.

I have this imported locally, with the minimal fixes made, will push shortly now that a try run has come back good.  (Should be fine, but you never know what dumb test is going to be checking exact error message syntax.  e_e)
Attachment #8444214 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8444216 [details] [diff] [review]

Review of attachment 8444216 [details] [diff] [review]:

A bit long in the tooth for an r+, but the thrust is right enough, and I wanted to run this through try anyway to verify (I accidentally ran your original, ish, through try, and got the stmt-null issue pointed out to me -- tho I was going to suggest it as belt-and-suspenders anyway for other reaosns), so may as well make 'em myself.  :-)

Thanks for the patches!  I'll land them both, with adjustments, shortly.

::: js/src/frontend/Parser.cpp
@@ +2677,3 @@
>      JSAutoByteString name;
> +    if (AtomToPrintableString(context, atom, &name)) {
> +        stmt->type == STMT_CATCH

So just branching on stmt->type like this is not right, because it's possible for it to be nullptr.  So this needs a null-check.

@@ +2677,5 @@
>      JSAutoByteString name;
> +    if (AtomToPrintableString(context, atom, &name)) {
> +        stmt->type == STMT_CATCH
> +          ? report(ParseError, false, pn, JSMSG_REDECLARED_CATCH_IDENTIFIER, name.ptr())
> +          : report(ParseError, false, pn, JSMSG_REDECLARED_VAR, isConst ? "const" : "variable", name.ptr());

Ternary expression as statement is a bit ಠ_ಠ (or e_e, as we like to say around here when we're being lazy).  Also, slightly better to have early failure if ATPS fails -- cuts down on indentation, reads a little better than having one central return.  And, the LexicalLookup should be deferred as late as possible, on general principle.  And, it doesn't matter now (because top-level lets are mutated into vars internally -- a bug), but we should null-check the return of LexicalLookup to defend against the time when lets are treated as such at top level.

And (this was perhaps not immediately obvious, but in looking harder at the code I noticed it) the extra rooting here is unnecessary if the argument type is Handle<PropertyName*> rather than a raw pointer of different type.  Every caller can trivially supply one.  So by adjusting the signature the need for the rooting disappears.

All total, then, the method after all those tweaks reduces to this:

template <typename ParseHandler>
Parser<ParseHandler>::reportRedeclaration(Node pn, bool isConst, HandlePropertyName name)
    JSAutoByteString printable;
    if (!AtomToPrintableString(context, name, &printable))
        return false;

    StmtInfoPC *stmt = LexicalLookup(pc, name, nullptr, (StmtInfoPC *)nullptr);
    if (stmt && stmt->type == STMT_CATCH) {
        report(ParseError, false, pn, JSMSG_REDECLARED_CATCH_IDENTIFIER, printable.ptr());
    } else {
        report(ParseError, false, pn, JSMSG_REDECLARED_VAR, isConst ? "const" : "variable",
    return false;
Attachment #8444216 - Flags: review?(jwalden+bmo) → review+
And a few days later, finally hit a break in tree action to land these:

Thanks for the patches!  Any ideas what you're interested in tackling next at all?
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Thanks again for the reviews and making those code tweaks for me, they've given me plenty to be mindful of in future submissions.  It's quite exciting to see those patches land in mozilla central!

Some time ago this bug caught my eye:

Range analysis is a new (to me) and seemingly interesting topic. However, I'm open to suggestions if you have something else in mind or reservations about me tackling this bug.
You need to log in before you can comment on or make changes to this bug.