Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Ban new.target and arguments from initializers #41

Closed
wants to merge 1 commit into from
Closed

Ban new.target and arguments from initializers #41

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions spec/new-productions.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ <h1>|PublicFieldDefinition|</h1>
</emu-grammar>
</emu-clause>

<emu-clause id="public-field-definition-early-errors">
<h1>Static Semantics: Early Errors</h1>
PublicFieldDefinition :
PropertyName[?Yield] Initializer?
<emu-grammar>
</emu-grammar>
<emu-alg>
1. It is a SyntaxError if |Initializer| contains |NewTarget|.
Copy link
Member

Choose a reason for hiding this comment

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

These need to use Contains instead of contains. Contains is a formally defined spec algorithm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean. The spec defines "contains" formally, but it's consistently used in lower case.

Copy link
Member

Choose a reason for hiding this comment

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

See the definition (and usage) of Contains in this section: https://tc39.github.io/ecma262/#sec-static-semantic-rules

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I see there are a couple Contains usages, but it's mostly contains. It doesn't matter which one is used; probably the spec could use an editorial change to normalize.

Copy link
Member

Choose a reason for hiding this comment

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

In the current state, this and the below line are poorly enough specified that I would object to the proposal's advancement. I legitimately do not know what is meant by each of them.

Copy link
Member

Choose a reason for hiding this comment

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

Note that advancement right now means Stage2 -- which doesn't require finalized spec text (i.e. spec text can have TODOS and require editorial work so long as the major aspects of it are in place)

But of course we should address your concerns: The purpose here is to specify that it is a static error if new.target or arguments is syntactically present in the initializer position. Is there more context around your misunderstanding you can provide?

Copy link
Member

Choose a reason for hiding this comment

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

Summarising the offline discussion I had with @jeffmo.

  1. Agreed, we don't need to finalise the spec text for advancement to stage 2, but we do need to explain what we're trying to specify.
  2. The Contains operation is different from the natural usage of "contains" in that it does not cross function boundaries.
  3. @jeffmo claims his intent is for new.target to be disallowed within the initialiser, regardless of function boundaries; I think this is not a great decision.
  4. @jeffmo claims his intent is for arguments to be disallowed only up to function boundaries (Contains semantics); I think this is a good decision.
  5. We can't specify the arguments restriction in terms of Contains. We'll need a new spec operation (ContainsArguments?) to do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable. I didn't realize that detail. Still, I think if we agree on what the semantics should be, we don't need to get every little detail like this worked out before Stage 2.

1. It is a SyntaxError if |Initializer| contains an |Identifier| with the StringValue `"arguments"`.
Copy link
Member

Choose a reason for hiding this comment

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

This too, if that wasn't clear.

</emu-alg>
</emu-clause>

<emu-clause id="static-semantics-class-public-fields">
<h1>Static Semantics: ClassPublicFields</h1>

Expand Down Expand Up @@ -101,6 +113,8 @@ <h1>InitializePublicInstanceFields ( _O_, _constructor_ )</h1>
1. Assert: Assert _constructor_ is an ECMAScript function object.
1. Let _lex_ be the Lexical Environment of the running execution context.
1. Let _initializerEnv_ be NewFunctionEnvironment ( _constructor_, *undefined* ).
1. Perform ! _envRec_.CreateImmutableBinding(`"arguments"`, ~false~).
1. Perform ! _envRec_.InitializeBinding(`"arguments"`, ~undefined~).
1. Let _initializerEnvRec_ be the value of _initializerEnv_'s EnvironmentRecord.
1. Perform _initializerER_.BindThisValue ( _O_ ).
1. Set the running execution context's LexicalEnvironment to _initializerEnv_.
Expand Down