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.

Sign up for GitHub

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

Jump to bottom

Editorial: Fix indirect eval environment check #1949

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Editorial: Fix indirect eval environment check #1949

merged 1 commit into from
Apr 15, 2020

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Apr 14, 2020

indirect eval should not allow new.target, super, etc., and GetThisEnvironment is not defined in terms of non-ecmascript execution contexts.

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Apr 14, 2020
bakkot
bakkot reviewed Apr 14, 2020
View reviewed changes
spec.html Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Apr 14, 2020

I would call this an editorial bugfix, since an assertion is violated in the current text.

@devsnek devsnek force-pushed the fix-indirect-eval-getthisenvironment branch 2 times, most recently from 72088e6 to a7962fe Compare April 14, 2020 18:36
@devsnek devsnek changed the title Normative: Fix indirect eval environment check Editorial: Fix indirect eval environment check Apr 14, 2020
@syg
Copy link
Contributor

syg commented Apr 14, 2020

I agree with editorial here -- this is a spec bug. There's no consensus needed here, and certainly no tests needed. There is no other interpretation of indirect eval possible in the current spec or in any implementation.

@bakkot
Copy link
Contributor

bakkot commented Apr 14, 2020

Also there are already tests: eg here is the test that you cannot use new.target in an indirect eval and here (x, x) are the tests that you can only use new.target in a direct eval when it is in a function.

@ljharb ljharb added editorial change spec bug and removed needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Apr 14, 2020
@ljharb ljharb requested review from syg, michaelficarra, ljharb, bakkot and a team and removed request for ljharb April 14, 2020 18:45
bakkot
bakkot reviewed Apr 14, 2020
View reviewed changes
spec.html Show resolved Hide resolved
syg
syg approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm with @bakkot's suggestion

@devsnek devsnek force-pushed the fix-indirect-eval-getthisenvironment branch from a7962fe to 9352535 Compare April 14, 2020 21:13
@ljharb ljharb self-assigned this Apr 14, 2020
michaelficarra
michaelficarra reviewed Apr 14, 2020
View reviewed changes
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

@ljharb ljharb force-pushed the fix-indirect-eval-getthisenvironment branch from 9352535 to cbe3b05 Compare April 15, 2020 00:14
@ljharb ljharb merged commit cbe3b05 into tc39:master Apr 15, 2020
@devsnek devsnek deleted the fix-indirect-eval-getthisenvironment branch April 15, 2020 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelficarra michaelficarra michaelficarra left review comments

@bakkot bakkot bakkot approved these changes

@syg syg syg approved these changes

Assignees

@ljharb ljharb

Labels
editorial change spec bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devsnek @bakkot @syg @michaelficarra @ljharb