Closed
Bug 104442
Opened 23 years ago
Closed 8 years ago
JS Engine doesn't appear to know where a variable was declared
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
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)
(deleted),
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nchevobbe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nchevobbe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nchevobbe
:
review+
|
Details | Diff | Splinter Review |
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: .............^
Comment 1•23 years ago
|
||
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
Comment 2•23 years ago
|
||
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.
Comment 3•23 years ago
|
||
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
Updated•23 years ago
|
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
Updated•19 years ago
|
Assignee: khanson → general
QA Contact: pschwartau → general
Target Milestone: Future → ---
Updated•10 years ago
|
Assignee: general → nobody
Assignee | ||
Comment 7•8 years ago
|
||
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
(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. ^^
Assignee | ||
Comment 12•8 years ago
|
||
> 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.
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
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+
Assignee | ||
Comment 16•8 years ago
|
||
Here's actual testcase for error note added in bug 1283712.
Attachment #8839790 -
Flags: review?(chevobbe.nicolas)
Assignee | ||
Comment 17•8 years ago
|
||
and input for mocha test
Attachment #8839791 -
Flags: review?(chevobbe.nicolas)
Assignee | ||
Comment 18•8 years ago
|
||
and updated stub
Attachment #8839792 -
Flags: review?(chevobbe.nicolas)
Assignee | ||
Comment 19•8 years ago
|
||
and mocha test with actual data.
Attachment #8839793 -
Flags: review?(chevobbe.nicolas)
Updated•8 years ago
|
Attachment #8839790 -
Flags: review?(chevobbe.nicolas) → review+
Updated•8 years ago
|
Attachment #8839791 -
Flags: review?(chevobbe.nicolas) → review+
Comment 20•8 years ago
|
||
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 21•8 years ago
|
||
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.
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8839792 -
Attachment is obsolete: true
Attachment #8840083 -
Flags: review+
Assignee | ||
Comment 23•8 years ago
|
||
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 24•8 years ago
|
||
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 25•8 years ago
|
||
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+
Assignee | ||
Comment 26•8 years ago
|
||
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.
Comment 27•8 years ago
|
||
Weird, there might be an error in the stub generation I guess. I'll check on my side too
Comment 28•8 years ago
|
||
okay, there's a problem in the stub generation, I'll fix this.
In the meantime, feel free to land your patches
Assignee | ||
Comment 29•8 years ago
|
||
(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
Comment 30•8 years ago
|
||
FYI I filed Bug 1342215 to fix this and submitted a patch.
Assignee | ||
Comment 31•8 years ago
|
||
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.
Assignee | ||
Comment 32•8 years ago
|
||
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.
Assignee | ||
Comment 33•8 years ago
|
||
turns out that expectUncaughtException() should be called for each uncaught exception.
if I call it before ContentTask.spawn, the test passes...
Assignee | ||
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/effa71064b2918dbab3f63716419e84264588f7b
Bug 104442 - Part 1: Report the position and the kind of previous declaration for redeclaration error. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/175610403a4ebb73d3e99ef36762fc4e8d6f8efd
Bug 104442 - Part 2: Add a testcase for devtools and note. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/b90365c52f059c88f1a4ad8358781378da438f87
Bug 104442 - Part 3: Add test input for mocha test. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/12c83a596d7b21cda79ee07c0d196139b5ddff3c
Bug 104442 - Part 4: Update stub. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/01bd2d505daa10f606a06ed6103ddcb9c1df0ebf
Bug 104442 - Part 5: Add another testcase for devtools and note. r=nchevobbe
Comment 35•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/effa71064b29
https://hg.mozilla.org/mozilla-central/rev/175610403a4e
https://hg.mozilla.org/mozilla-central/rev/b90365c52f05
https://hg.mozilla.org/mozilla-central/rev/12c83a596d7b
https://hg.mozilla.org/mozilla-central/rev/01bd2d505daa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•