622646 - catch/let name collision error message should be more descriptive

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

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

Comment 1

[Andrew]

Hi, 
Can I be assigned this bug?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Sure! If you have any questions, feel free to join #jsapi on irc.mozilla.org.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Also, to make your patches easier to review and check in, please read the page below to give you some submission guidelines.
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 4

[Afshin Mehrabani]

Hey guys, 

I'm interested to work on this issue, can I help at this point?

Comment 5

[Josh Matthews [:jdm]]

Hi Afshin,
The relevant line throwing the error is http://mxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#2904 (search for JSMSG_REDECLARED_VAR, declared at http://mxr.mozilla.org/mozilla-central/source/js/src/js.msg#208). Waldo, could you make sure whoever knows the parsing code sees this and can be available for questions?

Comment 6

[Afshin Mehrabani]

I think the fix should be easy, can I be the fixer?

Comment 7

[Afshin Mehrabani]

Just need some information about this issue, I'm available on irc with `afshinmeh`.

Comment 8

[Josh Matthews [:jdm]]

Feel free to ask questions here.

Comment 9

[Afshin Mehrabani]

So my question is how we should change the message to make it more descriptive? Any clue?

Comment 10

[Jeff Walden [:Waldo]]

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.  :-)

Comment 11

[Afshin Mehrabani]

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?

Comment 12

[Afshin Mehrabani]

ping

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Afshin Mehrabani]

Attachment: fix_exist_let.patch

Let me know what you think guys.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8344327 [details] [diff] [review]
fix_exist_let.patch

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.

Comment 16

[Afshin Mehrabani]

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: http://mxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#2646

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 :-)

Comment 17

[Afshin Mehrabani]

I'm still waiting for more information on this issue :-)

Comment 18

[Jeff Walden [:Waldo]]

No worries about questions, we were all new at some point.

So,

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.

Comment 19

[Stephen Gowan]

Attachment: catch_name_collision.patch

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 ........................^

Comment 20

[Stephen Gowan]

Perhaps I jumped the gun on this one--sorry about that. May I be assigned to this bug?

Comment 21

[Chris Peterson [:cpeterson]]

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 irc.mozilla.org.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8406373 [details] [diff] [review]
catch_name_collision.patch

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! :)

Comment 23

[Stephen Gowan]

Attachment: catch_name_collision.patch

Ah, good catch!  I've removed all instances of trailing whitespace from this patch.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8413718 [details] [diff] [review]
catch_name_collision.patch

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.

Comment 25

[Stephen Gowan]

Attachment: catch_name_collision.patch

Added a missing space after |if|.

Nicholas, thanks again for your help!

Comment 26

[Mike Hoye [:mhoye]]

Hi, Stephen - Are you still working on this?

Comment 27

[Josh Matthews [:jdm]]

With all due respect, the onus is on Jeff to actually review the patch here, not Stephen.

Comment 28

[Mike Hoye [:mhoye]]

Sorry, thanks for clarifying. 'sup, Jeff?

Comment 29

[Stephen Gowan]

Yea, I should've sent a review ping some time ago. I'm still available to work on this should the need arise.

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 8414508 [details] [diff] [review]
catch_name_collision.patch

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(!xulRuntime.shell)" at the top of it, and do something like this in the guts of it:

  options("strict");
  try
  {
    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/jstests.py 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;
            }

Comment 31

[Stephen Gowan]

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 ......................^

After:

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 .......................^

Comment 32

[Stephen Gowan]

Attachment: catch_const_name_collision.patch

This patch contains the suggestions Jeff made in comment 30.

Comment 33

[Stephen Gowan]

Attachment: catch_let_name_collision.patch

This patch contains changes that correct the error message when a |let| statement shadows an exception identifier.

Comment 34

[Jeff Walden [:Waldo]]

Comment on attachment 8444214 [details] [diff] [review]
catch_const_name_collision.patch

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)

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 8444216 [details] [diff] [review]
catch_let_name_collision.patch

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.  :-)

https://tbpl.mozilla.org/?tree=Try&rev=51d1e4e5e63a

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>
bool
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",
               printable.ptr());
    }
    return false;
}

Comment 36

[Jeff Walden [:Waldo]]

And a few days later, finally hit a break in tree action to land these:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b89b64668f99
https://hg.mozilla.org/integration/mozilla-inbound/rev/187a759bff17

Thanks for the patches!  Any ideas what you're interested in tackling next at all?

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b89b64668f99
https://hg.mozilla.org/mozilla-central/rev/187a759bff17

Comment 38

[Stephen Gowan]

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: https://bugzilla.mozilla.org/show_bug.cgi?id=892702

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.