1428672 - Redeclaration of a class needs a specific error

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Peter Lyons Kehl]

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Arai, I guess this is something you might want to do, or mentor. Otherwise feel free to forward it to someone else ;)

Comment 2

[Tooru Fujisawa [:arai]]

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.

Comment 3

[Ashish Verma [:ashish1500616]]

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?

Comment 4

[Tooru Fujisawa [:arai]]

(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

Comment 5

[Ashish Verma [:ashish1500616]]

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?

Comment 6

[Tooru Fujisawa [:arai]]

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

Comment 7

[Issei Horie [:is2ei]]

Hi, I'm Issei.
Could I work on this issue if it's still open?

Comment 8

[Tooru Fujisawa [:arai]]

Sure.  this is still open.

Comment 9

[Issei Horie [:is2ei]]

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.

Comment 10

[Tooru Fujisawa [:arai]]

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.

Comment 11

[Issei Horie [:is2ei]]

(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!

Comment 12

[Tooru Fujisawa [:arai]]

thanks!
I'll update the description (the code structure and what's need to be done) once bug 1467052 gets fixed :)

Comment 13

[Tooru Fujisawa [:arai]]

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

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

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.