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: Set [[SourceText]] on classes before static blocks evaluate #2840

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jul 30, 2022

Test case: class Foo { static { this.toString() } }

This will require the [[SourceText]] of Foo to be set.

@ljharb
Copy link
Member

ljharb commented Jul 30, 2022

This doesn’t seem editorial; maybe a normative bugfix?

@devsnek
Copy link
Member Author

devsnek commented Jul 30, 2022

it hits a spec assertion... we generally consider that type of bugfix to be editorial right?

@bakkot
Copy link
Contributor

bakkot commented Jul 30, 2022

The rule we've been using for bugfixes is that if the text is coherent but does not match committee intent it's Normative, whereas if it's outright incoherent it's Editorial. This is kind of in the middle, but since there's a failed assertion I'm fine calling it Editorial.

spec.html Outdated
@@ -24755,6 +24755,7 @@ <h1>
Runtime Semantics: ClassDefinitionEvaluation (
_classBinding_: unknown,
_className_: unknown,
_sourceText_: unknown,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_sourceText_: unknown,
_sourceText_: ECMAScript source text,

spec.html Outdated
@@ -24917,15 +24919,15 @@ <h1>Runtime Semantics: Evaluation</h1>
</emu-note>
<emu-grammar>ClassExpression : `class` ClassTail</emu-grammar>
<emu-alg>
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments *undefined* and *""*.
1. Set _value_.[[SourceText]] to the source text matched by |ClassExpression|.
1. Set _sourceText_ be the source text matched by |ClassExpression|.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Set _sourceText_ be the source text matched by |ClassExpression|.
1. Let _sourceText_ be the source text matched by |ClassExpression|.

spec.html Outdated
1. Return _value_.
</emu-alg>
<emu-grammar>ClassExpression : `class` BindingIdentifier ClassTail</emu-grammar>
<emu-alg>
1. Let _className_ be StringValue of |BindingIdentifier|.
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments _className_ and _className_.
1. Set _value_.[[SourceText]] to the source text matched by |ClassExpression|.
1. Set _sourceText_ be the source text matched by |ClassExpression|.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Set _sourceText_ be the source text matched by |ClassExpression|.
1. Let _sourceText_ be the source text matched by |ClassExpression|.

spec.html Outdated
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments *undefined* and *"default"*.
1. Set _value_.[[SourceText]] to the source text matched by |ClassDeclaration|.
1. Let _sourceText_ be the source text matched by |ClassDeclaration|.
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments *undefined*, *"default"*, and _sourceText_.
Copy link
Member

Choose a reason for hiding this comment

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

We can return directly in these cases instead of introducing an alias.

Suggested change
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments *undefined*, *"default"*, and _sourceText_.
1. Return ? ClassDefinitionEvaluation of |ClassTail| with arguments *undefined*, *"default"*, and _sourceText_.

@devsnek devsnek force-pushed the class-source-text branch from 6ab0dc2 to 2287414 Compare January 3, 2023 06:25
@bathos
Copy link
Contributor

bathos commented Jan 3, 2023

This will fix #2669 (I think. Does this fix it for only static blocks, or both static blocks and static initializer expressions?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants