Run babel on compilation errors as well as on parsing ones #1861
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unfortunately parsing checks syntax, not whether what the syntax did was actually ... supported(sometimes). As can be seen this fixes a lot of cases where this is a problem. This is actually probably going to be an even bigger problem in the future as goja gets more features in it which will let the parsing to pass even though some feature actually isn't supported but the syntax is now "valid". Good example (which was how this was kind of found) is that destructuring looks a lot like an object with shorthand properties, but this is only noticable once the code is compiled, not while parsing. So in the future when goja for example has support for all the rest of the syntax (like `let`) it will be harder to catch. Even now code such as `let [x] = [2]` which is desctructuring doesn't return an error on the parsing stage but instead in the compilation. This example might be a bug in goja, but I would argue we can have others.
mstoykov
commented
Feb 17, 2021
| @@ -3186,6 +3104,7 @@ | |||
| "test/language/literals/regexp/S7.8.5_A1.4_T2.js-strict:true": "test/language/literals/regexp/S7.8.5_A1.4_T2.js: Test262Error: Code unit: d800 Expected SameValue(«\\\\\\ud800», «\\�») to be true at harness/sta.js:22:9(49)", | |||
| "test/language/literals/regexp/S7.8.5_A2.1_T2.js-strict:true": "test/language/literals/regexp/S7.8.5_A2.1_T2.js: Test262Error: Code unit: d800 Expected SameValue(«nnnn\\ud800», «nnnn�») to be true at harness/sta.js:22:9(49)", | |||
| "test/language/literals/regexp/S7.8.5_A2.4_T2.js-strict:true": "test/language/literals/regexp/S7.8.5_A2.4_T2.js: Test262Error: Code unit: d800 Expected SameValue(«a\\\\\\ud800», «a\\�») to be true at harness/sta.js:22:9(49)", | |||
| "test/language/literals/regexp/early-err-flags-unicode-escape.js-strict:true": "test/language/literals/regexp/early-err-flags-unicode-escape.js: error is not an object (Test262: This statement should not be evaluated.)", | |||
There was a problem hiding this comment.
This is babel/babel#6691 and practically ... not a problem for us.
mstoykov
commented
Feb 17, 2021
| @@ -4724,16 +4643,15 @@ | |||
| "test/language/statements/for-in/head-const-bound-names-dup.js-strict:true": "test/language/statements/for-in/head-const-bound-names-dup.js: unexpected error type (TypeError), expected (SyntaxError)", | |||
| "test/language/statements/for-in/head-const-bound-names-fordecl-tdz.js-strict:true": "test/language/statements/for-in/head-const-bound-names-fordecl-tdz.js: Test262Error: Expected a ReferenceError to be thrown but no exception was thrown at all at harness/sta.js:22:9(49)", | |||
| "test/language/statements/for-in/head-const-bound-names-in-stmt.js-strict:true": "test/language/statements/for-in/head-const-bound-names-in-stmt.js: error is not an object (Test262: This statement should not be evaluated.)", | |||
| "test/language/statements/for-in/head-let-bound-names-dup.js-strict:true": "test/language/statements/for-in/head-let-bound-names-dup.js: unexpected error type (TypeError), expected (SyntaxError)", | |||
There was a problem hiding this comment.
This actually now returns the "correcter":
ERRO[0000] TypeError: file:///home/mstoykov/work/k6_copy/js/tc39/d.js: Duplicate declaration "x"
> 1 | for (let [x, x] in {}) {}
| ^
2 | at <eval>:2:28542(114)
instead of
SyntaxError: Unexpected strict mode reserved word at file:///home/mstoykov/work/k6_copy/js/tc39/d.js:1:6
The error comes from babel transformation so 🤷
mstoykov
commented
Feb 17, 2021
| @@ -4907,9 +4808,9 @@ | |||
| "test/language/statements/for-of/head-const-bound-names-in-stmt.js-strict:true": "test/language/statements/for-of/head-const-bound-names-in-stmt.js: error is not an object (Test262: This statement should not be evaluated.)", | |||
| "test/language/statements/for-of/head-decl-no-expr.js-strict:true": "test/language/statements/for-of/head-decl-no-expr.js: error is not an object (Test262: This statement should not be evaluated.)", | |||
| "test/language/statements/for-of/head-expr-no-expr.js-strict:true": "test/language/statements/for-of/head-expr-no-expr.js: error is not an object (Test262: This statement should not be evaluated.)", | |||
| "test/language/statements/for-of/head-let-bound-names-dup.js-strict:true": "test/language/statements/for-of/head-let-bound-names-dup.js: unexpected error type (TypeError), expected (SyntaxError)", | |||
There was a problem hiding this comment.
na--
previously approved these changes
Feb 18, 2021
na--
requested changes
Feb 18, 2021
Codecov Report
@@ Coverage Diff @@
## master #1861 +/- ##
==========================================
- Coverage 71.58% 71.52% -0.06%
==========================================
Files 180 179 -1
Lines 13955 13961 +6
==========================================
- Hits 9990 9986 -4
- Misses 3329 3336 +7
- Partials 636 639 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
imiric
previously approved these changes
Feb 18, 2021
Co-authored-by: na-- <[email protected]>
That is dependant on goja getting support for more or less everything babel currently provides us ... which will take quite some time, especially given that it is mostly 1 person working on it in their spare time. My contributions are unlikely to make a dent in that unless we prioritize it and that is actually unlikely as well as there is not so much benefit to that as for example working on things that babel doesn't provide us with (generators, Promise, async/await being the interesting ones).
Looking at the code ... it's unlikely any refactoring will make it clearer and this code is rarely touched ... so hopefully whoever do touch it will notice the same block a comment apart :) |
imiric
approved these changes
Feb 18, 2021
na--
approved these changes
Feb 18, 2021
na--
pushed a commit
that referenced
this pull request
Mar 1, 2021
Unfortunately parsing checks syntax, not whether what the syntax expressed is actually ... supported(sometimes). As can be seen, this fixes a lot of cases where this is a problem. This is actually probably going to be an even bigger problem in the future as goja gets more features in it which will let the parsing to pass even though some feature actually isn't supported but the syntax is now "valid". A good example (which was how this was kind of found) is that destructuring looks a lot like an object with shorthand properties, but this is only noticeable once the code is compiled, not while parsing. So in the future when goja for example has support for all the rest of the syntax (like `let`) it will be harder to catch. Even now code such as `let [x] = [2]` which is destructuring doesn't return an error on the parsing stage but instead in the compilation. This example might be a bug in goja, but I would argue we can have others.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unfortunately parsing checks syntax, not whether what the syntax did was
actually ... supported(sometimes).
As can be seen this fixes a lot of cases where this is a problem.
This is actually probably going to be an even bigger problem in the
future as goja gets more features in it which will let the parsing
to pass even though some feature actually isn't supported but the syntax
is now "valid".
Good example (which was how this was kind of found) is that destructuring
looks a lot like an object with shorthand properties, but this is only
noticable once the code is compiled, not while parsing. So in the future
when goja for example has support for all the rest of the syntax (like
let) it will be harder to catch.Even now code such as
let [x] = [2]which is desctructuring doesn'treturn an error on the parsing stage but instead in the compilation.
This example might be a bug in goja, but I would argue we can have
others.