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

Editorial: use proper comparison operation in LabelledEvaluation of LabelledStatement #3162

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

hyp3rflow
Copy link
Contributor

This PR adds missing ~empty~ guard in LabelledEvaluation for LabelledStatement.

Currently, LabelledEvaluation for LabelledStatement is defined as follows:

1. Let label be the StringValue of LabelIdentifier.
2. Let newLabelSet be the list-concatenation of labelSet and « label ».
3. Let stmtResult be Completion(LabelledEvaluation of LabelledItem with argument newLabelSet).
4. If stmtResult.[[Type]] is break and SameValue(stmtResult.[[Target]], label) is true, then
	a. Set stmtResult to NormalCompletion(stmtResult.[[Value]]).
5. Return ? stmtResult.

According to the definition, SameValue, used in step 4, requires two arguments which are both ECMAScript langauge value. In step 3, stmtResult.[[Target]] can be ~empty~ if there is no LabelIdentifier in BreakStatement. Consequently, providing stmtResult.[[Target]] as the argument for SameValue should not be allowed.

Following ECMAScript snippet can occur this case:

switch (1) {
  case 1:
    foo: { break; } // stmtResult.[[Type]] is break and stmtResult.[[Target]] is ~empty~
}

So fix this by adding more condition in step 4 for guard.

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.

We should be able to just change the SameValue comparison to an "is" comparison. See https://tc39.es/ecma262/#sec-identity.

@hyp3rflow hyp3rflow force-pushed the fix-labelledevaluation branch 2 times, most recently from 8fb1a3c to 66c330b Compare September 6, 2023 11:18
@hyp3rflow
Copy link
Contributor Author

@michaelficarra I updated as you commented. :)

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Sep 7, 2023
@ljharb ljharb changed the title Add missing guard in LabelledEvaluation for LabelledStatement Editorial: Add missing guard in LabelledEvaluation for LabelledStatement Sep 19, 2023
@ljharb ljharb changed the title Editorial: Add missing guard in LabelledEvaluation for LabelledStatement Editorial: use proper comparison operation in LabelledEvaluation of LabelledStatement Sep 19, 2023
@ljharb ljharb force-pushed the fix-labelledevaluation branch from 66c330b to 367a07f Compare September 19, 2023 22:03
@ljharb ljharb merged commit 367a07f into tc39:main Sep 19, 2023
zhangenming pushed a commit to zhangenming/ecma262 that referenced this pull request Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants