Skip to content

for(const x in y) #541

Open
Open
for(const x in y)#541

Description

@classilla
classilla
opened on Jan 12, 2019

This is needed for full functionality in Github's current release, and probably fixes other sites. Unfortunately it also seems to have been fixed by the frontend binding rewrite that is the crux of #533.

It may be possible to hack in something that allows const as a synonym for var as a temporary fix. The underlying issues, which were never fully repaired, are https://bugzilla.mozilla.org/show_bug.cgi?id=449811 and https://bugzilla.mozilla.org/show_bug.cgi?id=1101653 .

Activity

  1. classilla commented on Jan 12, 2019

    @classilla
    classilla
    on Jan 12, 2019
    OwnerAuthor

    In Parser.cpp, look at lines 4585 and 4791. Then, in BytecodeEmitter.cpp, fix the assertions in lines 5499 and remove the assertion at 5521. This should be enough to get const treated as let which should have similar enough semantics for a first pass. However, this is too risky to put in FPR12.

  2. roytam1 commented on Jan 23, 2019

    @roytam1
    roytam1
    on Jan 23, 2019

    so these changes "hackfixes" for(const x in y) problems:

    diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp
index df0d8a7b0..dafb61f05 100644
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -5496,7 +5496,7 @@ BytecodeEmitter::emitIterator()
 bool
 BytecodeEmitter::emitForInOrOfVariables(ParseNode* pn)
 {
-    MOZ_ASSERT(pn->isKind(PNK_VAR) || pn->isKind(PNK_LET));
+    MOZ_ASSERT(pn->isKind(PNK_VAR) || pn->isKind(PNK_LET) || pn->isKind(PNK_CONST));
 
     // ES6 specifies that loop variables get a fresh binding in each iteration.
     // This is currently implemented for C-style for(;;) loops, but not
@@ -5518,7 +5518,7 @@ BytecodeEmitter::emitForInOrOfVariables(ParseNode* pn)
         if (!emitVariables(pn, DefineVars))
             return false;
     } else {
-        MOZ_ASSERT(pn->isKind(PNK_LET));
+        MOZ_ASSERT(pn->isKind(PNK_LET) || pn->isKind(PNK_CONST));
         if (!emitVariables(pn, InitializeVars))
             return false;
     }
diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
index 5830e7d90..3d6757fd9 100644
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -4583,10 +4583,10 @@ Parser<ParseHandler>::declarationPattern(Node decl, TokenKind tt, BindData<Parse
         if (*forHeadKind != PNK_FORHEAD) {
             // |for (const ... in ...);| and |for (const ... of ...);| are
             // syntax errors for now.  We'll fix this in bug 449811.
-            if (handler.declarationIsConst(decl)) {
+            /*if (handler.declarationIsConst(decl)) {
                 report(ParseError, false, pattern, JSMSG_BAD_CONST_DECL);
                 return null();
-            }
+            }*/
 
             if (!checkDestructuringPattern(data, pattern))
                 return null();
@@ -4788,7 +4788,7 @@ Parser<ParseHandler>::declarationName(Node decl, TokenKind tt, BindData<ParseHan
             if (isForIn || isForOf) {
                 // XXX Uncomment this when fixing bug 449811.  Until then,
                 //     |for (const ... in/of ...)| remains an error.
-                //constRequiringInitializer = false;
+                constRequiringInitializer = false;
 
                 *forHeadKind = isForIn ? PNK_FORIN : PNK_FOROF;
             } else {

    and now complaining async/await keyword.
    xref: #521

  3. roytam1
    added a commit that references this issue on Feb 1, 2019

    hack: treat `const` in `for(const x in y)` as `let`.

    f0dd403

  4. classilla commented on Feb 6, 2019

    @classilla
    classilla
    on Feb 6, 2019
    OwnerAuthor

    This isn't worth it, might regress other code, and doesn't fix enough to get Github working properly. Wontfixing for TenFourFox.

  5. classilla
    closed this as completedon Feb 6, 2019

  6. classilla commented on Mar 16, 2019

    @classilla
    classilla
    on Mar 16, 2019
    OwnerAuthor

    If we can get async functions working, we might reexamine this hack.

  7. classilla commented on May 7, 2019

    @classilla
    classilla
    on May 7, 2019
    OwnerAuthor

    This may also fix Discord, see https://tenfourfox.tenderapp.com/discussions/problems/8804-discord-problem

  8. classilla
    reopened this on May 7, 2019

  9. classilla commented on May 18, 2019

    @classilla
    classilla
    on May 18, 2019
    OwnerAuthor

    This enables login to Discord, but now it just "crashes" (the page complains Discord has crashed). Changing the user agent makes no difference.

  10. classilla commented on May 21, 2019

    @classilla
    classilla
    on May 21, 2019
    OwnerAuthor

    There are various failures, but they aren't unexpected:

    /Volumes/BruceDeuce/src/tenfourfox/js/src/jit-test/tests/parser/letContextualKeyword.js:7:5 Error: Assertion failed: got false, expected true
    Stack:
    expectError@/Volumes/BruceDeuce/src/tenfourfox/js/src/jit-test/tests/parser/letContextualKeyword.js:7:5
    @/Volumes/BruceDeuce/src/tenfourfox/js/src/jit-test/tests/parser/letContextualKeyword.js:21:1

    ecma_6/LexicalEnvironment/const-declaration-in-for-loop.js: rc = 3, run time = 0.723338

    1146644: Don't crash compiling a non-body-level for-loop whose loop declaration is a const
    ecma_6/LexicalEnvironment/const-declaration-in-for-loop.js:95:3 Error: Assertion failed: got false, expected true: unexpected error: expected SyntaxError, got Error: Congratulations on making for (const & in/of &) work! Please remove the try/catch and this throw.
    Stack:
    @ecma_6/LexicalEnvironment/const-declaration-in-for-loop.js:95:3

    ecma_6/LexicalEnvironment/const-declaration-in-for-loop.js: rc = 3, run time = 1.333542

    1146644: Don't crash compiling a non-body-level for-loop whose loop declaration is a const
    ecma_6/LexicalEnvironment/const-declaration-in-for-loop.js:95:3 Error: Assertion failed: got false, expected true: unexpected error: expected SyntaxError, got Error: Congratulations on making for (const & in/of &) work! Please remove the try/catch and this throw.
    Stack:
    @ecma_6/LexicalEnvironment/const-declaration-in-for-loop.js:95:3

    js1_8_5/reflect-parse/for-loop-destructuring.js: rc = 3, run time = 1.013104

    assertError@js1_8_5/reflect-parse/PatternAsserts.js:92:11
    test@js1_8_5/reflect-parse/for-loop-destructuring.js:29:1
    runtest@js1_8_5/reflect-parse/shell.js:59:9
    @js1_8_5/reflect-parse/for-loop-destructuring.js:51:1

    js1_8_5/reflect-parse/PatternAsserts.js:92:11 Error: expected SyntaxError for "for (const x in foo);"
    Stack:
    assertError@js1_8_5/reflect-parse/PatternAsserts.js:92:11
    test@js1_8_5/reflect-parse/for-loop-destructuring.js:29:1
    runtest@js1_8_5/reflect-parse/shell.js:59:9
    @js1_8_5/reflect-parse/for-loop-destructuring.js:51:1

    js1_8_5/reflect-parse/for-loop-destructuring.js: rc = 3, run time = 5.893012

    assertError@js1_8_5/reflect-parse/PatternAsserts.js:92:11
    test@js1_8_5/reflect-parse/for-loop-destructuring.js:29:1
    runtest@js1_8_5/reflect-parse/shell.js:59:9
    @js1_8_5/reflect-parse/for-loop-destructuring.js:51:1

    js1_8_5/reflect-parse/PatternAsserts.js:92:11 Error: expected SyntaxError for "for (const x in foo);"
    Stack:
    assertError@js1_8_5/reflect-parse/PatternAsserts.js:92:11
    test@js1_8_5/reflect-parse/for-loop-destructuring.js:29:1
    runtest@js1_8_5/reflect-parse/shell.js:59:9
    @js1_8_5/reflect-parse/for-loop-destructuring.js:51:1

  11. classilla commented on May 29, 2019

    @classilla
    classilla
    on May 29, 2019
    OwnerAuthor

    No offense, but I don't make one-offs since we don't have continuous integration and builds are lengthy (and they're too large to attach here anyway). The patch above is essentially what's on tree, so you could do it off the FPR14 tag with that added.

  12. classilla commented on Jun 16, 2019

    @classilla
    classilla
    on Jun 16, 2019
    OwnerAuthor

    Since the latest attempt at #521 ended in failure again, and this might add some partial functionality on some sites, we'll try it (with test fixes) for FPR15.

  13. classilla
    added a commit that references this issue on Jun 20, 2019

    #541: hack for(const x in y) with test changes

    f47c9db

  14. classilla commented on Jul 21, 2019

    @classilla
    classilla
    on Jul 21, 2019
    OwnerAuthor

    Leaving open while I ponder if more work is needed.

  15. classilla commented on Oct 29, 2019

    @classilla
    classilla
    on Oct 29, 2019
    OwnerAuthor

    This is now causing github to go into infinite errors, essentially seizing the browser (primarily on issues) due to an uncaught check op. I removed this op as a temporary measure but this makes the hack even more disgusting since const will be valid to assign to pretty much anywhere. It will also break a lot of tests. I haven't thought of a better way around it.

  16. roytam1 commented on Oct 29, 2019

    @roytam1
    roytam1
    on Oct 29, 2019

    maybe create a pref to change behavior in runtime?

  17. classilla commented on Oct 29, 2019

    @classilla
    classilla
    on Oct 29, 2019
    OwnerAuthor

    Maybe, except the check op is generated in the frontend bytecode emitter, so a previously compiled script wouldn't change its behaviour. (Conversely, it would be highly impractical (and slow) to actually make the check during execution depend on a runtime setting, so this would have to live in the frontend if I did it at all.)

  18. classilla
    added a commit that references this issue on Nov 22, 2019

    #541: temporary workaround for infinite errors on github

    8c40900
  19. classilla
    added a commit that references this issue on Feb 21, 2020

    #541: more disgusting hacks to fix jsfiddle

    1f3d8fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions