Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: token after strict mode block is evaluated in strict mode #11186

Merged
merged 1 commit into from Mar 24, 2020

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Feb 28, 2020

Q                       A
Fixed Issues? Fixes #11183
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We're currently eating the closing bracket token of the class body before restoring the outer scope's strict mode state. This PR changes it so that we restore the out strict mode state before eating this final token.

Question: Does this seem like the correct way to fix this?

@kaicataldo kaicataldo added pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 28, 2020
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we delay throwing octal error when reading a number token. So even in this.nextToken() the next token is read in an incorrect strict mode, we can still throw later when we are sure this.state.strict is correct.

return this.parseLiteral(this.state.value, "NumericLiteral");

We can delay throwing octal literals error when we finished parsing a numeric literal

        const start = this.state.start;
        const strict = this.state.strict;
        node = this.parseLiteral(this.state.value, "NumericLiteral");
        if (strict && this.state.containsOctal) {
          this.raise(start, "Legacy octal literals are not allowed in strict mode");
        }
        this.state.containsOctal = null;
        return node;

And in the tokenizer part we can set containsOctal: true only and allow tokens be generated, so it will be checked later with correct strict.

if (this.state.strict) {

I think we should also get rid of

this.state.containsOctal = false;
this.state.octalPosition = null;

as long as we correctly reset these states after parsing literals.

Sidenote: other than tokenizer#readNumber(), we also risk incorrect state.strict in tokenizer#readEscapedChar.

} else if (this.state.strict) {

But I think it is okay to leave it as-is because this function will never called in the nextToken of a tt.braceR, which may be a boundary of strict mode. tokenizer#readEscapedChar will only called after consuming a tt.backslash token.

@@ -0,0 +1,17 @@
class A {}
Copy link
Contributor

@JLHwung JLHwung Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test case about function declaration, which is failing on master

function A() { "use strict"; }
05

@kaicataldo
Copy link
Member Author

Thanks for the review! Will take a look later this afternoon.

@kaicataldo
Copy link
Member Author

Waiting to land #11188 first since the two PRs actually overlap a bit.

@JLHwung
Copy link
Contributor

JLHwung commented Mar 5, 2020

FYI acornjs/acorn@44e52b2 is how acorn fixes this issue. That is a neat fix, maybe we can port it here?

@kaicataldo kaicataldo force-pushed the fix-class-strict-mode branch 2 times, most recently from 601b800 to b808ce2 Compare March 23, 2020 22:14
@kaicataldo kaicataldo changed the title fix: token after a class body is evaluated in strict mode fix: token after strict mode block is evaluated in strict mode Mar 23, 2020
@kaicataldo
Copy link
Member Author

This is ready for re-review!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@nicolo-ribaudo nicolo-ribaudo merged commit 4e6c9c5 into babel:master Mar 24, 2020
@kaicataldo kaicataldo deleted the fix-class-strict-mode branch March 25, 2020 18:26
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 25, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Parser]: First token after a class is considered strict mode
4 participants