104442 - Part 1: Report the position and the kind of previous declaration for redeclaration error.

5 files changed, 330 insertions, 35 deletions

diff --git a/js/src/frontend/NameAnalysisTypes.h b/js/src/frontend/NameAnalysisTypes.h
index d39e177fb..b94ca6cc0 100644
--- a/js/src/frontend/NameAnalysisTypes.h
+++ b/js/src/frontend/NameAnalysisTypes.h
@@ -124,6 +124,7 @@ DeclarationKindIsLexical(DeclarationKind kind)
 // Used in Parser to track declared names.
 class DeclaredNameInfo
 {
+    uint32_t pos_;
     DeclarationKind kind_;
 
     // If the declared name is a binding, whether the binding is closed
@@ -132,8 +133,9 @@ class DeclaredNameInfo
     bool closedOver_;
 
   public:
-    explicit DeclaredNameInfo(DeclarationKind kind)
-      : kind_(kind),
+    explicit DeclaredNameInfo(DeclarationKind kind, uint32_t pos)
+      : pos_(pos),
+        kind_(kind),
         closedOver_(false)
     { }
 
@@ -144,6 +146,12 @@ class DeclaredNameInfo
         return kind_;
     }
 
+    static const uint32_t npos = uint32_t(-1);
+
+    uint32_t pos() const {
+        return pos_;
+    }
+
     void alterKind(DeclarationKind kind) {
         kind_ = kind;
     }
diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
index ffece5721..c96e8c065 100644
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -19,6 +19,8 @@
 
 #include "frontend/Parser.h"
 
+#include "mozilla/Sprintf.h"
+
 #include <new>
 
 #include "jsapi.h"
@@ -200,11 +202,12 @@ ParseContext::Scope::addCatchParameters(ParseContext* pc, Scope& catchParamScope
 
     for (DeclaredNameMap::Range r = catchParamScope.declared_->all(); !r.empty(); r.popFront()) {
         DeclarationKind kind = r.front().value()->kind();
+        uint32_t pos = r.front().value()->pos();
         MOZ_ASSERT(DeclarationKindIsCatchParameter(kind));
         JSAtom* name = r.front().key();
         AddDeclaredNamePtr p = lookupDeclaredNameForAdd(name);
         MOZ_ASSERT(!p);
-        if (!addDeclaredName(pc, p, name, kind))
+        if (!addDeclaredName(pc, p, name, kind, pos))
             return false;
     }
 
@@ -343,7 +346,8 @@ ParseContext::init()
                 namedLambdaScope_->lookupDeclaredNameForAdd(fun->explicitName());
             MOZ_ASSERT(!p);
             if (!namedLambdaScope_->addDeclaredName(this, p, fun->explicitName(),
-                                                    DeclarationKind::Const))
+                                                    DeclarationKind::Const,
+                                                    DeclaredNameInfo::npos))
             {
                 return false;
             }
@@ -1001,13 +1005,42 @@ Parser<ParseHandler>::hasValidSimpleStrictParameterNames()
 
 template <typename ParseHandler>
 void
-Parser<ParseHandler>::reportRedeclaration(HandlePropertyName name, DeclarationKind kind,
-                                          TokenPos pos)
+Parser<ParseHandler>::reportRedeclaration(HandlePropertyName name, DeclarationKind prevKind,
+                                          TokenPos pos, uint32_t prevPos)
 {
     JSAutoByteString bytes;
     if (!AtomToPrintableString(context, name, &bytes))
         return;
-    errorAt(pos.begin, JSMSG_REDECLARED_VAR, DeclarationKindString(kind), bytes.ptr());
+
+    if (prevPos == DeclaredNameInfo::npos) {
+        errorAt(pos.begin, JSMSG_REDECLARED_VAR, DeclarationKindString(prevKind), bytes.ptr());
+        return;
+    }
+
+    auto notes = MakeUnique<JSErrorNotes>();
+    if (!notes)
+        return;
+
+    uint32_t line, column;
+    tokenStream.srcCoords.lineNumAndColumnIndex(prevPos, &line, &column);
+
+    const size_t MaxWidth = sizeof("4294967295");
+    char columnNumber[MaxWidth];
+    SprintfLiteral(columnNumber, "%" PRIu32, column);
+    char lineNumber[MaxWidth];
+    SprintfLiteral(lineNumber, "%" PRIu32, line);
+
+    if (!notes->addNoteLatin1(pc->sc()->context,
+                              getFilename(), line, column,
+                              GetErrorMessage, nullptr,
+                              JSMSG_REDECLARED_PREV,
+                              lineNumber, columnNumber))
+    {
+        return;
+    }
+
+    errorWithNotesAt(Move(notes), pos.begin, JSMSG_REDECLARED_VAR,
+                     DeclarationKindString(prevKind), bytes.ptr());
 }
 
 // notePositionalFormalParameter is called for both the arguments of a regular
@@ -1022,6 +1055,7 @@ Parser<ParseHandler>::reportRedeclaration(HandlePropertyName name, DeclarationKi
 template <typename ParseHandler>
 bool
 Parser<ParseHandler>::notePositionalFormalParameter(Node fn, HandlePropertyName name,
+                                                    uint32_t beginPos,
                                                     bool disallowDuplicateParams,
                                                     bool* duplicatedParam)
 {
@@ -1046,7 +1080,7 @@ Parser<ParseHandler>::notePositionalFormalParameter(Node fn, HandlePropertyName
         *duplicatedParam = true;
     } else {
         DeclarationKind kind = DeclarationKind::PositionalFormalParameter;
-        if (!pc->functionScope().addDeclaredName(pc, p, name, kind))
+        if (!pc->functionScope().addDeclaredName(pc, p, name, kind, beginPos))
             return false;
     }
 
@@ -1150,8 +1184,8 @@ DeclarationKindIsParameter(DeclarationKind kind)
 
 template <typename ParseHandler>
 bool
-Parser<ParseHandler>::tryDeclareVar(HandlePropertyName name, DeclarationKind kind,
-                                    Maybe<DeclarationKind>* redeclaredKind)
+Parser<ParseHandler>::tryDeclareVar(HandlePropertyName name, DeclarationKind kind, uint32_t beginPos,
+                                    Maybe<DeclarationKind>* redeclaredKind, uint32_t* prevPos)
 {
     MOZ_ASSERT(DeclarationKindIsVar(kind));
 
@@ -1224,23 +1258,29 @@ Parser<ParseHandler>::tryDeclareVar(HandlePropertyName name, DeclarationKind kin
 
                 if (!annexB35Allowance && !annexB33Allowance) {
                     *redeclaredKind = Some(declaredKind);
+                    *prevPos = p->value()->pos();
                     return true;
                 }
             } else if (kind == DeclarationKind::VarForAnnexBLexicalFunction) {
                 MOZ_ASSERT(DeclarationKindIsParameter(declaredKind));
 
                 // Annex B.3.3.1 disallows redeclaring parameter names.
+                // We don't need to set *prevPos here since this case is not
+                // an error.
                 *redeclaredKind = Some(declaredKind);
                 return true;
             }
         } else {
-            if (!scope->addDeclaredName(pc, p, name, kind))
+            if (!scope->addDeclaredName(pc, p, name, kind, beginPos))
                 return false;
         }
     }
 
-    if (!pc->sc()->strict() && pc->sc()->isEvalContext())
+    if (!pc->sc()->strict() && pc->sc()->isEvalContext()) {
         *redeclaredKind = isVarRedeclaredInEval(name, kind);
+        // We don't have position information at runtime.
+        *prevPos = DeclaredNameInfo::npos;
+    }
 
     return true;
 }
@@ -1248,11 +1288,15 @@ Parser<ParseHandler>::tryDeclareVar(HandlePropertyName name, DeclarationKind kin
 template <typename ParseHandler>
 bool
 Parser<ParseHandler>::tryDeclareVarForAnnexBLexicalFunction(HandlePropertyName name,
-                                                            bool* tryAnnexB)
+                                                            uint32_t beginPos, bool* tryAnnexB)
 {
     Maybe<DeclarationKind> redeclaredKind;
-    if (!tryDeclareVar(name, DeclarationKind::VarForAnnexBLexicalFunction, &redeclaredKind))
+    uint32_t unused;
+    if (!tryDeclareVar(name, DeclarationKind::VarForAnnexBLexicalFunction, beginPos,
+                       &redeclaredKind, &unused))
+    {
         return false;
+    }
 
     if (!redeclaredKind && pc->isFunctionBox()) {
         ParseContext::Scope& funScope = pc->functionScope();
@@ -1324,11 +1368,12 @@ Parser<ParseHandler>::noteDeclaredName(HandlePropertyName name, DeclarationKind
       case DeclarationKind::BodyLevelFunction:
       case DeclarationKind::ForOfVar: {
         Maybe<DeclarationKind> redeclaredKind;
-        if (!tryDeclareVar(name, kind, &redeclaredKind))
+        uint32_t prevPos;
+        if (!tryDeclareVar(name, kind, pos.begin, &redeclaredKind, &prevPos))
             return false;
 
         if (redeclaredKind) {
-            reportRedeclaration(name, *redeclaredKind, pos);
+            reportRedeclaration(name, *redeclaredKind, pos, prevPos);
             return false;
         }
 
@@ -1345,7 +1390,7 @@ Parser<ParseHandler>::noteDeclaredName(HandlePropertyName name, DeclarationKind
             return false;
         }
 
-        if (!pc->functionScope().addDeclaredName(pc, p, name, kind))
+        if (!pc->functionScope().addDeclaredName(pc, p, name, kind, pos.begin))
             return false;
 
         break;
@@ -1368,7 +1413,7 @@ Parser<ParseHandler>::noteDeclaredName(HandlePropertyName name, DeclarationKind
                 (p->value()->kind() != DeclarationKind::LexicalFunction &&
                  p->value()->kind() != DeclarationKind::VarForAnnexBLexicalFunction))
             {
-                reportRedeclaration(name, p->value()->kind(), pos);
+                reportRedeclaration(name, p->value()->kind(), pos, p->value()->pos());
                 return false;
             }
 
@@ -1376,7 +1421,7 @@ Parser<ParseHandler>::noteDeclaredName(HandlePropertyName name, DeclarationKind
             // declaration that shadows the VarForAnnexBLexicalFunction.
             p->value()->alterKind(kind);
         } else {
-            if (!scope->addDeclaredName(pc, p, name, kind))
+            if (!scope->addDeclaredName(pc, p, name, kind, pos.begin))
                 return false;
         }
 
@@ -1416,7 +1461,7 @@ Parser<ParseHandler>::noteDeclaredName(HandlePropertyName name, DeclarationKind
         if (pc->isFunctionExtraBodyVarScopeInnermost()) {
             DeclaredNamePtr p = pc->functionScope().lookupDeclaredName(name);
             if (p && DeclarationKindIsParameter(p->value()->kind())) {
-                reportRedeclaration(name, p->value()->kind(), pos);
+                reportRedeclaration(name, p->value()->kind(), pos, p->value()->pos());
                 return false;
             }
         }
@@ -1433,12 +1478,12 @@ Parser<ParseHandler>::noteDeclaredName(HandlePropertyName name, DeclarationKind
                 p = scope->lookupDeclaredNameForAdd(name);
                 MOZ_ASSERT(!p);
             } else {
-                reportRedeclaration(name, p->value()->kind(), pos);
+                reportRedeclaration(name, p->value()->kind(), pos, p->value()->pos());
                 return false;
             }
         }
 
-        if (!p && !scope->addDeclaredName(pc, p, name, kind))
+        if (!p && !scope->addDeclaredName(pc, p, name, kind, pos.begin))
             return false;
 
         break;
@@ -2188,8 +2233,11 @@ Parser<ParseHandler>::declareFunctionThis()
         ParseContext::Scope& funScope = pc->functionScope();
         AddDeclaredNamePtr p = funScope.lookupDeclaredNameForAdd(dotThis);
         MOZ_ASSERT(!p);
-        if (!funScope.addDeclaredName(pc, p, dotThis, DeclarationKind::Var))
+        if (!funScope.addDeclaredName(pc, p, dotThis, DeclarationKind::Var,
+                                      DeclaredNameInfo::npos))
+        {
             return false;
+        }
         funbox->setHasThisBinding();
     }
 
@@ -2231,8 +2279,11 @@ Parser<ParseHandler>::declareDotGeneratorName()
     ParseContext::Scope& funScope = pc->functionScope();
     HandlePropertyName dotGenerator = context->names().dotGenerator;
     AddDeclaredNamePtr p = funScope.lookupDeclaredNameForAdd(dotGenerator);
-    if (!p && !funScope.addDeclaredName(pc, p, dotGenerator, DeclarationKind::Var))
+    if (!p && !funScope.addDeclaredName(pc, p, dotGenerator, DeclarationKind::Var,
+                                        DeclaredNameInfo::npos))
+    {
         return false;
+    }
     return true;
 }
 
@@ -2481,8 +2532,11 @@ Parser<ParseHandler>::declareFunctionArgumentsObject()
     if (tryDeclareArguments) {
         AddDeclaredNamePtr p = funScope.lookupDeclaredNameForAdd(argumentsName);
         if (!p) {
-            if (!funScope.addDeclaredName(pc, p, argumentsName, DeclarationKind::Var))
+            if (!funScope.addDeclaredName(pc, p, argumentsName, DeclarationKind::Var,
+                                          DeclaredNameInfo::npos))
+            {
                 return false;
+            }
             funbox->declaredArguments = true;
             funbox->usesArguments = true;
         } else if (hasExtraBodyVarScope) {
@@ -2964,8 +3018,8 @@ Parser<ParseHandler>::functionArguments(YieldHandling yieldHandling, FunctionSyn
                 if (!name)
                     return false;
 
-                if (!notePositionalFormalParameter(funcpn, name, disallowDuplicateParams,
-                                                   &duplicatedParam))
+                if (!notePositionalFormalParameter(funcpn, name, pos().begin,
+                                                   disallowDuplicateParams, &duplicatedParam))
                 {
                     return false;
                 }
@@ -3689,7 +3743,7 @@ Parser<ParseHandler>::functionStmt(uint32_t preludeStart, YieldHandling yieldHan
             // early error, do so. This 'var' binding would be assigned
             // the function object when its declaration is reached, not at
             // the start of the block.
-            if (!tryDeclareVarForAnnexBLexicalFunction(name, &tryAnnexB))
+            if (!tryDeclareVarForAnnexBLexicalFunction(name, pos().begin, &tryAnnexB))
                 return null();
         }
 
diff --git a/js/src/frontend/Parser.h b/js/src/frontend/Parser.h
index 824dddb58..6b5ade425 100644
--- a/js/src/frontend/Parser.h
+++ b/js/src/frontend/Parser.h
@@ -146,9 +146,9 @@ class ParseContext : public Nestable<ParseContext>
         }
 
         MOZ_MUST_USE bool addDeclaredName(ParseContext* pc, AddDeclaredNamePtr& p, JSAtom* name,
-                                          DeclarationKind kind)
+                                          DeclarationKind kind, uint32_t pos)
         {
-            return maybeReportOOM(pc, declared_->add(p, name, DeclaredNameInfo(kind)));
+            return maybeReportOOM(pc, declared_->add(p, name, DeclaredNameInfo(kind, pos)));
         }
 
         // Remove all VarForAnnexBLexicalFunction declarations of a certain
@@ -1437,15 +1437,17 @@ class Parser final : public ParserBase, private JS::AutoGCRooter
 
     bool hasValidSimpleStrictParameterNames();
 
-    void reportRedeclaration(HandlePropertyName name, DeclarationKind kind, TokenPos pos);
-    bool notePositionalFormalParameter(Node fn, HandlePropertyName name,
+    void reportRedeclaration(HandlePropertyName name, DeclarationKind prevKind, TokenPos pos,
+                             uint32_t prevPos);
+    bool notePositionalFormalParameter(Node fn, HandlePropertyName name, uint32_t beginPos,
                                        bool disallowDuplicateParams, bool* duplicatedParam);
     bool noteDestructuredPositionalFormalParameter(Node fn, Node destruct);
     mozilla::Maybe<DeclarationKind> isVarRedeclaredInEval(HandlePropertyName name,
                                                           DeclarationKind kind);
-    bool tryDeclareVar(HandlePropertyName name, DeclarationKind kind,
-                       mozilla::Maybe<DeclarationKind>* redeclaredKind);
-    bool tryDeclareVarForAnnexBLexicalFunction(HandlePropertyName name, bool* tryAnnexB);
+    bool tryDeclareVar(HandlePropertyName name, DeclarationKind kind, uint32_t beginPos,
+                       mozilla::Maybe<DeclarationKind>* redeclaredKind, uint32_t* prevPos);
+    bool tryDeclareVarForAnnexBLexicalFunction(HandlePropertyName name, uint32_t beginPos,
+                                               bool* tryAnnexB);
     bool checkLexicalDeclarationDirectlyWithinBlock(ParseContext::Statement& stmt,
                                                     DeclarationKind kind, TokenPos pos);
     bool noteDeclaredName(HandlePropertyName name, DeclarationKind kind, TokenPos pos);
diff --git a/js/src/jit-test/tests/parser/redeclaration.js b/js/src/jit-test/tests/parser/redeclaration.js
new file mode 100644
index 000000000..f719021ac
--- /dev/null
+++ b/js/src/jit-test/tests/parser/redeclaration.js
@@ -0,0 +1,230 @@
+// Error message for redeclaration should show the position where the variable
+// was declared.
+
+const npos = -1;
+
+function test_one(fun, filename, name,
+                  [prevLineNumber, prevColumnNumber],
+                  [lineNumber, columnNumber]) {
+    let caught = false;
+    try {
+        fun();
+    } catch (e) {
+        assertEq(e.message.includes("redeclaration"), true);
+        assertEq(e.lineNumber, lineNumber);
+        assertEq(e.columnNumber, columnNumber);
+        let notes = getErrorNotes(e);
+        if (prevLineNumber == npos) {
+            assertEq(notes.length, 0);
+        } else {
+            assertEq(notes.length, 1);
+            let note = notes[0];
+            assertEq(note.message,
+                     `Previously declared at line ${prevLineNumber}, column ${prevColumnNumber}`);
+            assertEq(note.lineNumber, prevLineNumber);
+            assertEq(note.columnNumber, prevColumnNumber);
+            if (filename)
+                assertEq(note.fileName, filename);
+        }
+        caught = true;
+    }
+    assertEq(caught, true);
+}
+
+function test_parse(source, ...args) {
+    test_one(() => {
+        Reflect.parse(source, { source: "foo.js" });
+    }, "foo.js", ...args);
+}
+
+function test_eval(source, ...args) {
+    test_one(() => {
+        eval(source);
+    }, undefined, ...args);
+}
+
+function test(...args) {
+    test_parse(...args);
+    test_eval(...args);
+}
+
+// let
+
+test(`
+let a, a;
+`, "a", [2, 4], [2, 7]);
+
+test(`
+let a;
+let a;
+`, "a", [2, 4], [3, 4]);
+
+test(`
+let a;
+const a = 1;
+`, "a", [2, 4], [3, 6]);
+
+test(`
+let a;
+var a;
+`, "a", [2, 4], [3, 4]);
+
+test(`
+let a;
+function a() {
+}
+`, "a", [2, 4], [3, 9]);
+
+test(`
+{
+  let a;
+  function a() {
+  }
+}
+`, "a", [3, 6], [4, 11]);
+
+// const
+
+test(`
+const a = 1, a = 2;
+`, "a", [2, 6], [2, 13]);
+
+test(`
+const a = 1;
+const a = 2;
+`, "a", [2, 6], [3, 6]);
+
+test(`
+const a = 1;
+let a;
+`, "a", [2, 6], [3, 4]);
+
+test(`
+const a = 1;
+var a;
+`, "a", [2, 6], [3, 4]);
+
+test(`
+const a = 1;
+function a() {
+}
+`, "a", [2, 6], [3, 9]);
+
+test(`
+{
+  const a = 1;
+  function a() {
+  }
+}
+`, "a", [3, 8], [4, 11]);
+
+// var
+
+test(`
+var a;
+let a;
+`, "a", [2, 4], [3, 4]);
+
+test(`
+var a;
+const a = 1;
+`, "a", [2, 4], [3, 6]);
+
+// function
+
+test(`
+function a() {};
+let a;
+`, "a", [2, 9], [3, 4]);
+
+test(`
+function a() {};
+const a = 1;
+`, "a", [2, 9], [3, 6]);
+
+// Annex B lexical function
+
+test(`
+{
+  function a() {};
+  let a;
+}
+`, "a", [3, 11], [4, 6]);
+
+test(`
+{
+  function a() {};
+  const a = 1;
+}
+`, "a", [3, 11], [4, 8]);
+
+// catch parameter
+
+test(`
+try {
+} catch (a) {
+  let a;
+}
+`, "a", [3, 9], [4, 6]);
+
+test(`
+try {
+} catch (a) {
+  const a = 1;
+}
+`, "a", [3, 9], [4, 8]);
+
+test(`
+try {
+} catch (a) {
+  function a() {
+  }
+}
+`, "a", [3, 9], [4, 11]);
+
+// parameter
+
+test(`
+function f(a) {
+  let a;
+}
+`, "a", [2, 11], [3, 6]);
+
+test(`
+function f(a) {
+  const a = 1;
+}
+`, "a", [2, 11], [3, 8]);
+
+test(`
+function f([a]) {
+  let a;
+}
+`, "a", [2, 12], [3, 6]);
+
+test(`
+function f({a}) {
+  let a;
+}
+`, "a", [2, 12], [3, 6]);
+
+test(`
+function f(...a) {
+  let a;
+}
+`, "a", [2, 14], [3, 6]);
+
+test(`
+function f(a=1) {
+  let a;
+}
+`, "a", [2, 11], [3, 6]);
+
+// eval
+// We don't have position information at runtime.
+// No note should be shown.
+
+test_eval(`
+let a;
+eval("var a");
+`, "a", [npos, npos], [1, 4]);
diff --git a/js/src/js.msg b/js/src/js.msg
index ce039cbc8..569928be8 100644
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -63,6 +63,7 @@ MSG_DEF(JSMSG_SPREAD_TOO_LARGE,        0, JSEXN_RANGEERR, "array too large due t
 MSG_DEF(JSMSG_BAD_WEAKMAP_KEY,         0, JSEXN_TYPEERR, "cannot use the given object as a weak map key")
 MSG_DEF(JSMSG_BAD_GETTER_OR_SETTER,    1, JSEXN_TYPEERR, "invalid {0} usage")
 MSG_DEF(JSMSG_BAD_ARRAY_LENGTH,        0, JSEXN_RANGEERR, "invalid array length")
+MSG_DEF(JSMSG_REDECLARED_PREV,         2, JSEXN_NOTE, "Previously declared at line {0}, column {1}")
 MSG_DEF(JSMSG_REDECLARED_VAR,          2, JSEXN_SYNTAXERR, "redeclaration of {0} {1}")
 MSG_DEF(JSMSG_UNDECLARED_VAR,          1, JSEXN_REFERENCEERR, "assignment to undeclared variable {0}")
 MSG_DEF(JSMSG_GETTER_ONLY,             1, JSEXN_TYPEERR, "setting getter-only property {0}")