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

RESOLVED FIXED in Firefox 54

Status

()

--
enhancement
RESOLVED FIXED
17 years ago
2 years ago

People

(Reporter: timeless, Assigned: arai)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 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

17 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.
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
(Reporter)

Comment 4

17 years ago
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

17 years ago
Target Milestone: --- → Future
(Reporter)

Updated

16 years ago
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

13 years ago
Assignee: khanson → general
QA Contact: pschwartau → general
Target Milestone: Future → ---
Assignee: general → nobody
(Assignee)

Updated

4 years ago
Blocks: 622261
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1106161

Updated

3 years ago
Duplicate of this bug: 611374
(Assignee)

Updated

2 years ago
Depends on: 1283712
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7afbd9e37e72098abe275afd4b41e48b3b2a0a16
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
(Assignee)

Comment 8

2 years ago
Created attachment 8838202 [details] [diff] [review]
Part 1: Report the position and the kind of previous declaration for redeclaration error.

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.
(Assignee)

Comment 10

2 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.
(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

2 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

2 years ago
Created attachment 8838904 [details] [diff] [review]
Part 1: Report the position and the kind of previous declaration for redeclaration error.

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+
(Assignee)

Comment 15

2 years ago
Created attachment 8839788 [details] [diff] [review]
Part 1: Report the position and the kind of previous declaration for redeclaration error. r=anba

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

2 years ago
Created attachment 8839790 [details] [diff] [review]
Part 2: Add a testcase for devtools and note.

Here's actual testcase for error note added in bug 1283712.
Attachment #8839790 - Flags: review?(chevobbe.nicolas)
(Assignee)

Comment 17

2 years ago
Created attachment 8839791 [details] [diff] [review]
Part 3: Add test input for mocha test.

and input for mocha test
Attachment #8839791 - Flags: review?(chevobbe.nicolas)
(Assignee)

Comment 18

2 years ago
Created attachment 8839792 [details] [diff] [review]
Part 4: Update stub.

and updated stub
Attachment #8839792 - Flags: review?(chevobbe.nicolas)
(Assignee)

Comment 19

2 years ago
Created attachment 8839793 [details] [diff] [review]
Part 5: Add another testcase for devtools and note.

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.
(Assignee)

Comment 22

2 years ago
Created attachment 8840083 [details] [diff] [review]
Part 4: Update stub. r=nchevobbe
Attachment #8839792 - Attachment is obsolete: true
Attachment #8840083 - Flags: review+
(Assignee)

Comment 23

2 years ago
Created attachment 8840084 [details] [diff] [review]
Part 5: Add another testcase for devtools and note.

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+
(Assignee)

Comment 26

2 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.
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
(Assignee)

Comment 29

2 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
FYI I filed Bug 1342215 to fix this and submitted a patch.
(Assignee)

Comment 31

2 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

2 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

2 years ago
turns out that expectUncaughtException() should be called for each uncaught exception.
if I call it before ContentTask.spawn, the test passes...
(Assignee)

Updated

2 years ago
Depends on: 1342293
You need to log in before you can comment on or make changes to this bug.