Closed Bug 104442 Opened 23 years ago Closed 7 years ago

JS Engine doesn't appear to know where a variable was declared

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: timeless, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

Javascript Strict warnings indicate where a variable is redeclared, this is sort
of useful, however it would really help people to be able to see where the
original declaration was.

timeless@sorefbsd:/tmp: cat a.cpp
int main(void){
int a;
int a;
}
timeless@sorefbsd:/tmp: g++ a.cpp
a.cpp: In function `int main()':
a.cpp:3: redeclaration of `int a'
a.cpp:2: `int a' previously declared here

js> var a=1; var a=2;
typein:1: strict warning: redeclaration of var a:
typein:1: strict warning: var a=1; var a=2;
typein:1: strict warning: .............^
Reassigning to Kenton; cc'ing others for consideration of this RFE -  
Assignee: rogerl → khanson
Summary: JS Engine doesn't appear to know where a variable was declared → [RFE] JS Engine doesn't appear to know where a variable was declared
Oh, almost forgot, bugs are places for this kind of discussion.  The people
getting spammed have asked for it.  That is why I respond here, and not directly
to your email.
I made note somewhere (bug, comment, cvs log, maybe all three) of the difficulty
here.  We'd need to track declarations and compilation units, something we don't
waste time and space doing currently.  That'll hit page load perf, possibly in
the noise, but who knows?  Any top100 page-set contains plenty of scripts.

Maybe we could do this tracking only optionally.

/be
i only want this if we're doing jsstrict.

i once tried to figure out how to do this, it looked really hard, hence this 
rfe
Target Milestone: --- → Future
Summary: [RFE] JS Engine doesn't appear to know where a variable was declared → JS Engine doesn't appear to know where a variable was declared
Assignee: khanson → general
QA Contact: pschwartau → general
Target Milestone: Future → ---
Assignee: general → nobody
Blocks: jserror
Depends on: 1283712
bug 1283712 added a mechanism to add notes to error message,
now we can point multiple file positions for single error, as error notes.

this patch adds error note for redeclaration error, to point the place that the variable was previously declared.

the information about the declaration is stored to DeclaredNameInfo, so I added a position (pos_) member to it.
the position is recorded when declaring it first.
and when it gets redeclared, the position of the initial declaration is passed to reportRedeclaration function,
and that information is attached to the error, as error note.

it works like following:

code:
  const a = 10;
  let x, y, a;

jsshell output:
  test.js:2:10 SyntaxError: redeclaration of const a:
  test.js:2:10 let x, y, a;
  test.js:2:10 ..........^
  test.js:1:6 note: a is previously declared at line 1

devtools output:
  SyntaxError: redeclaration of const a          test.js:2:10
  note: a is previously declared at line 1       test.js:1:6

since the note points certain position in the file, we could say "a is previously declared here" or something tho,
I choose saying the line number explicitly to make it clearer.
Attachment #8838202 - Flags: review?(andrebargull)
(In reply to Tooru Fujisawa [:arai] from comment #8)
> the information about the declaration is stored to DeclaredNameInfo, so I
> added a position (pos_) member to it.

Do we need to worry about the increased size of DeclaredNameInfo or is this a non-critical data structure?

> 
> it works like following:
> 
> code:
>   const a = 10;
>   let x, y, a;
> 
> jsshell output:
>   test.js:2:10 SyntaxError: redeclaration of const a:
>   test.js:2:10 let x, y, a;
>   test.js:2:10 ..........^
>   test.js:1:6 note: a is previously declared at line 1

I think I'd prefer to use the same line prefix when printing the information for a single error. Like:

jsshell output:
  test.js:2:10 SyntaxError: redeclaration of const a:
  test.js:2:10 let x, y, a;
  test.js:2:10 ..........^
  test.js:2:10 Previously declared at line 1, column 6

IMO this makes it clearer that the error output belongs to a single error.
thanks :)

(In reply to André Bargull from comment #9)
> (In reply to Tooru Fujisawa [:arai] from comment #8)
> > the information about the declaration is stored to DeclaredNameInfo, so I
> > added a position (pos_) member to it.
> 
> Do we need to worry about the increased size of DeclaredNameInfo or is this
> a non-critical data structure?

it's allocated per each declaration, so that won't matter so much.

http://logs.glob.uno/?c=mozilla%23jsapi&s=25+Oct+2016&e=25+Oct+2016&h=104442#c804699


> I think I'd prefer to use the same line prefix when printing the information
> for a single error. Like:
> 
> jsshell output:
>   test.js:2:10 SyntaxError: redeclaration of const a:
>   test.js:2:10 let x, y, a;
>   test.js:2:10 ..........^
>   test.js:2:10 Previously declared at line 1, column 6
> 
> IMO this makes it clearer that the error output belongs to a single error.

it's following the output of C compiler like clang:
  a.cc:6:13: error: redefinition of 'x' with a different type: 'int' vs 'char'
    int a, b, x;
              ^
  a.cc:5:8: note: previous definition is here
    char x;
         ^


for devtools output, error and notes are shown in single log entry, so it's already clear that it's single error,
and also it needs to point the previously declared position to make it jump-able.
(In reply to Tooru Fujisawa [:arai] from comment #10)
> > Do we need to worry about the increased size of DeclaredNameInfo or is this
> > a non-critical data structure?
> 
> it's allocated per each declaration, so that won't matter so much.
> 
> http://logs.glob.uno/
> ?c=mozilla%23jsapi&s=25+Oct+2016&e=25+Oct+2016&h=104442#c804699

Okay, great to hear!


> it's following the output of C compiler like clang:
>   a.cc:6:13: error: redefinition of 'x' with a different type: 'int' vs
> 'char'
>     int a, b, x;
>               ^
>   a.cc:5:8: note: previous definition is here
>     char x;
>          ^
> 
> 
> for devtools output, error and notes are shown in single log entry, so it's
> already clear that it's single error,
> and also it needs to point the previously declared position to make it
> jump-able.

In that case disregard my previous comment. ^^
>   test.js:2:10 Previously declared at line 1, column 6

anyway, saying column number sounds more helpful.
I'll change it.

redeclaration error is SyntaxError and it shouldn't happen in hot path,
so formatting one more number should be fine in term of performance.
Changed to say column number.

test.js:2:10 SyntaxError: redeclaration of const a:
test.js:2:10 let x, y, a;
test.js:2:10 ..........^
test.js:1:6 note: Previously declared at line 1, column 6
Attachment #8838202 - Attachment is obsolete: true
Attachment #8838202 - Flags: review?(andrebargull)
Attachment #8838904 - Flags: review?(andrebargull)
Comment on attachment 8838904 [details] [diff] [review]
Part 1: Report the position and the kind of previous declaration for redeclaration error.

Review of attachment 8838904 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: js/src/frontend/Parser.cpp
@@ +1014,5 @@
> +    if (!notes)
> +        return;
> +
> +    uint32_t lineno, column;
> +    char lineNumber[16], columnNumber[16];

Why is the array length 16 when we just need to print uint32_t values? How about we simply copy the printing code from https://dxr.mozilla.org/mozilla-central/rev/d0462b0948e0b1147dcce615bddcc46379bdadb2/js/src/vm/JSONParser.cpp#92-96 ?

@@ +1017,5 @@
> +    uint32_t lineno, column;
> +    char lineNumber[16], columnNumber[16];
> +    tokenStream.srcCoords.lineNumAndColumnIndex(prevPos, &lineno, &column);
> +    SprintfLiteral(lineNumber, "%u", lineno);
> +    SprintfLiteral(columnNumber, "%u", column);

Do we prefer |"%u"| or |"%" PRIu32| for uint32_t? I think the latter is 'more correct', but I'm not sure we actually care about that.

@@ +1254,5 @@
>              } else if (kind == DeclarationKind::VarForAnnexBLexicalFunction) {
>                  MOZ_ASSERT(DeclarationKindIsParameter(declaredKind));
>  
>                  // Annex B.3.3.1 disallows redeclaring parameter names.
>                  *redeclaredKind = Some(declaredKind);

I wonder if it makes sense to add comment why it's not necessary to set |prevPos| in this case.
Attachment #8838904 - Flags: review?(andrebargull) → review+
Thanks :)
updated.

maybe we could add a class for converting number to string and use it everywhere.
something like following (not sure if it worth using template tho...)

class UInt32ToChars {
  static const size_t MaxWidth = sizeof("4294967295");
  char buf[MaxWidth];

public:
  UInt32ToChars(uint32_t n) {
    sprintf(buf, "%" PRIu32, n);
  }

  const char* get() const {
    return buf;
  }
};

anyway it's for another bug.
Attachment #8838904 - Attachment is obsolete: true
Attachment #8839788 - Flags: review+
Here's actual testcase for error note added in bug 1283712.
Attachment #8839790 - Flags: review?(chevobbe.nicolas)
and input for mocha test
Attachment #8839791 - Flags: review?(chevobbe.nicolas)
Attached patch Part 4: Update stub. (obsolete) — Splinter Review
and updated stub
Attachment #8839792 - Flags: review?(chevobbe.nicolas)
and mocha test with actual data.
Attachment #8839793 - Flags: review?(chevobbe.nicolas)
Attachment #8839790 - Flags: review?(chevobbe.nicolas) → review+
Attachment #8839791 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8839792 [details] [diff] [review]
Part 4: Update stub.

Review of attachment 8839792 [details] [diff] [review]:
-----------------------------------------------------------------

r+ anyway, but I think networkEvent.js shouldn't be commited here.

::: devtools/client/webconsole/new-console-output/test/fixtures/stubs/networkEvent.js
@@ +25,5 @@
>    "response": {},
>    "source": "network",
>    "type": "log",
>    "groupId": null,
> +  "timeStamp": 1487025973770

Can you try to on latest m-c ? This shouldn't be modified now (the stub generation should now ignore properties that are likely to change - timestamp, totalTime, startedTime, ...)
Attachment #8839792 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8839793 [details] [diff] [review]
Part 5: Add another testcase for devtools and note.

Review of attachment 8839793 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks arai, all is looking good !
I do not R+ though, because when you rebase on latest m-c, it will probably impact some of your stubs, and thus tests.

::: devtools/client/webconsole/new-console-output/test/components/page-error.test.js
@@ +238,5 @@
> +
> +    // There should be the location.
> +    const locationLink = note.find(`.message-location`);
> +    expect(locationLink.length).toBe(1);
> +    expect(locationLink.text()).toBe("test-tempfile.js:2:6");

There were some changes on the function that generate stubs lately, and I think this will impact the location content.
Attachment #8839792 - Attachment is obsolete: true
Attachment #8840083 - Flags: review+
Thank you for reviewing :)

indeed, I should've checked after rebase.
changed at=>eq and filename.
Attachment #8839793 - Attachment is obsolete: true
Attachment #8839793 - Flags: review?(chevobbe.nicolas)
Attachment #8840084 - Flags: review?(chevobbe.nicolas)
Comment on attachment 8840083 [details] [diff] [review]
Part 4: Update stub. r=nchevobbe

Review of attachment 8840083 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/new-console-output/test/fixtures/stubs/pageError.js
@@ +254,5 @@
> +    ]
> +  }
> +});
> +
> +stubPackets.set("SyntaxError: redeclaration of let a", {

we already have the same packet in line 213, is this wanted ?
Comment on attachment 8840084 [details] [diff] [review]
Part 5: Add another testcase for devtools and note.

Review of attachment 8840084 [details] [diff] [review]:
-----------------------------------------------------------------

Seems good to me, thanks !
Attachment #8840084 - Flags: review?(chevobbe.nicolas) → review+
Thank you for reviewing :)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #24)
> Comment on attachment 8840083 [details] [diff] [review]
> Part 4: Update stub. r=nchevobbe
> 
> Review of attachment 8840083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> devtools/client/webconsole/new-console-output/test/fixtures/stubs/pageError.
> js
> @@ +254,5 @@
> > +    ]
> > +  }
> > +});
> > +
> > +stubPackets.set("SyntaxError: redeclaration of let a", {
> 
> we already have the same packet in line 213, is this wanted ?

it's automatically generated by stub generator,
and I have only one entry in input.
not sure why only this is duplicated tho...
(running again won't duplicate, right?)

I'll look into it.
Weird, there might be an error in the stub generation I guess. I'll check on my side too
okay, there's a problem in the stub generation, I'll fix this.
In the meantime, feel free to land your patches
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #28)
> okay, there's a problem in the stub generation, I'll fix this.
> In the meantime, feel free to land your patches

thanks :D
FYI I filed Bug 1342215 to fix this and submitted a patch.
I'm having trouble in try run, that doesn't reproduce locally on OSX.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a18bbfb808c0d0954f15b26bd153b087d067fa66&selectedJob=79776963
> INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_check_stubs_page_error.js | uncaught exception - SyntaxError: redeclaration of let a at 
> INFO - Stack trace:
> INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1648
> INFO - chrome://mochikit/content/tests/BrowserTestUtils/content-task.js line 52 > eval:null:6
> INFO - chrome://mochikit/content/tests/BrowserTestUtils/content-task.js:null:53
> INFO - JavaScript error: http://example.com/browser/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/test-console-api.html, line 2: SyntaxError: redeclaration of let a
> INFO - JavaScript note: http://example.com/browser/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/test-console-api.html, line 2: Previously declared at line 2, column 6
> INFO - Console message: [JavaScript Error: "SyntaxError: redeclaration of let a" {file: "http://example.com/browser/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/test-console-api.html" line: 2 column: 9 source: "  let a, a;
> INFO - "}]
> INFO - 
> INFO - JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/new-console-output/new-console-output-wrapper.js, line 45: TypeError: this.jsterm.hud is null
> INFO - Console message: [JavaScript Error: "TypeError: this.jsterm.hud is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/new-console-output/new-console-output-wrapper.js" line: 45}]
> INFO - Removing tab.

and also the error message doesn't make sense... :/
it looks like complaining about the error that is thrown intentionally by stub generator,
that shouldn't make the test fail.

not sure about latter part.


triggered OSX mochitest to see if it's linux only or an issue in my environment.
just noticed that now e10s is default in mochitest and it needs --disable-e10s to run non-e10s test :P
and it's reproduced locally.
turns out that expectUncaughtException() should be called for each uncaught exception.
if I call it before ContentTask.spawn, the test passes...
Depends on: 1342293
You need to log in before you can comment on or make changes to this bug.