Closed Bug 1428672 Opened 6 years ago Closed 3 years ago

Redeclaration of a class needs a specific error

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox59 --- affected

People

(Reporter: peter.kehl, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: good-second-bug)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180107100322

Steps to reproduce:

Run Scratchpad (Shift-F4) and evaluate *twice*:
class A {}


Actual results:

On the second evaluation:

SyntaxError: redeclaration of let


Expected results:

Generate a more specific error, please. "let" is confusing.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Arai, I guess this is something you might want to do, or mentor. Otherwise feel free to forward it to someone else ;)
Flags: needinfo?(arai.unmht)
Priority: -- → P3
So, this is similar to bug 1282431.
the difference is that, the fix in bug 1282431 is applied to frontend to handle the case that the 2 classes are in same source,
but this issue happens when the 2 classes are in different sources, so that this needs some fix in binding itself.
Blocks: jserror
Mentor: arai.unmht
Flags: needinfo?(arai.unmht)
Whiteboard: good-second-bug
Hello, I would like to work on this issue.

This is the exact output

class A{}

/*
Exception: SyntaxError: redeclaration of let A
@Scratchpad/1:1:1
*/

What needs to be done?
(In reply to Ashish Verma [:ashish1500616] from comment #3)
> Hello, I would like to work on this issue.
> 
> This is the exact output
> 
> class A{}
> 
> /*
> Exception: SyntaxError: redeclaration of let A
> @Scratchpad/1:1:1
> */
> 
> What needs to be done?

hi, thank you for your comment :)

the error message needs to be changed to "redeclaration of class A".
please see bug 1282431, that is similar bug.

the issue is that, class declaration is treated as let binding inside the engine,
and when re-defining it, it conflicts with the existing binding for `let`, and the error says "let A",
but it should say "class A".

I haven't checked which actual part needs fix, but I think some code inside js/src/frontend [1] and js/src/vm/EnvironmentObject.* [2]
If you have any other questions, feel free to ask here, or #jsapi IRC channel [3]

[1] https://searchfox.org/mozilla-central/source/js/src/frontend
[2] https://searchfox.org/mozilla-central/source/js/src/vm/EnvironmentObject.cpp
[3] https://wiki.mozilla.org/IRC
Sorry But I am not getting any Idea Where to start from or I don't know these things.Do you think It's a bug that can be solved by a beginner?
I think, this may be a bit difficult than good-first-bugs category, but if you have fixed one more some bugs in JavaScript Engine, this will be good for the next step.

to investigate the issue, here's the workflow:
  1. build SpiderMonkey JS shell with debug mode [1]
  2. run the testcase (I think you'll need to prepare a single-file testcase that causes the issue)
  3. figure out where the error message is generated
  4. figure out how class declaration is converted into let binding
  5. figure out how we can embed the information about "it's class declaration", into the let binding

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Hi, I'm Issei.
Could I work on this issue if it's still open?
Flags: needinfo?(arai.unmht)
Sure.  this is still open.
Assignee: nobody → is2ei.horie
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
Sorry for late. Now I found the place where the name and kind of the binding are passed from frontend.
https://searchfox.org/mozilla-central/source/js/src/frontend/Parser.cpp#1774

I will try to pass the declaration kind there.
See Also: → 1467052
have you already started changing code?
I'm about to modify binding data in Scope object in bug 1467052, to distinguish between var and top-level function in different way than current one (offset range in binding),
and I think what we need is very similar between both bugs.  (so we could reuse the code)

if you've already started, can you share your idea about how to distinguish between let vs class?
if you haven't, I'm going to fix bug 1467052 so that this bug can be fixed based on it.
Flags: needinfo?(is2ei.horie)
(In reply to Tooru Fujisawa [:arai] from comment #10)
> have you already started changing code?
> I'm about to modify binding data in Scope object in bug 1467052, to
> distinguish between var and top-level function in different way than current
> one (offset range in binding),
> and I think what we need is very similar between both bugs.  (so we could
> reuse the code)
> 
> if you've already started, can you share your idea about how to distinguish
> between let vs class?
> if you haven't, I'm going to fix bug 1467052 so that this bug can be fixed
> based on it.

I started but haven't done much yet. I'm still checking how existing code works.
Please fix bug 1467052 first. Sorry for blocking this issue!
Flags: needinfo?(is2ei.horie) → needinfo?(arai.unmht)
thanks!
I'll update the description (the code structure and what's need to be done) once bug 1467052 gets fixed :)
Flags: needinfo?(arai.unmht)
okay, now bug 1467052 patch is landed.
here's update for the background:

  * Currently the error is reported in ReportRuntimeRedeclaration function [1], which has 2 callsites
  * "let vs const vs something else" is checked by looking up in the environment object [2]
  * while runtime, the binding's kind (var, let, const, etc) is defined in *Scope::Data objects [3], as the offset range,
    which doesn't distinguish between let and class
  * bug 1467052 added a way to distinguish between var and function, both are inside `vars` range in Scope, by flag in BindingName [4][5]
  * BindingName is stored in *Scope::Data and accessible with BindingIter [6][7]

and the things to do in this bug:
  * store the "let vs class" info into BindingKind when creating it [8]
  * pass BindingIter to the caller of ReportRuntimeRedeclaration and check if it's class, when it's let binding


[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js26ReportRuntimeRedeclarationEP9JSContextN2JS6HandleIPNS_12PropertyNameEEEPKc&redirect=false
[2] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/js/src/vm/EnvironmentObject.cpp#3335
[3] example: https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/js/src/vm/Scope.h#765-774
[4] https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/js/src/vm/Scope.h#142-144
[5] https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/js/src/vm/Scope.h#1414-1419
[6] https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/js/src/vm/Scope.h#1149-1161
[7] https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/js/src/vm/Scope.h#1394-1412
[8] https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/js/src/frontend/Parser.cpp#1807-1812
Assignee: is2ei.horie → nobody
Status: ASSIGNED → NEW

The bug as originally opened here is resolved; the error message now reads redeclaration of let A, which at the very least is better than it was.

Bug 1564825 has a nice STR and is up to date, so I'm closing this one in lieu of that one.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.