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 Clarification: Contains definition in 5.2.4 may be misleading #2251

Closed
codehag opened this issue Dec 11, 2020 · 4 comments · Fixed by #2271
Closed

Editorial Clarification: Contains definition in 5.2.4 may be misleading #2251

codehag opened this issue Dec 11, 2020 · 4 comments · Fixed by #2271

Comments

@codehag
Copy link
Contributor

codehag commented Dec 11, 2020

I don't know if I am the only person who will be confused with this, but worth asking. When preparing to introduce top level await on the stream, I came across the following line:

Let async be body Contains await. -- Knowing what top level await does, obviously, this must be ignoring functions right?

I double checked in the spec. The first definition, when looking it up in the spec is this one here: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-static-semantic-rules

    For each child node child of this Parse Node, do
        If child is an instance of symbol, return true.
        If child is an instance of a nonterminal, then
            Let contained be the result of child Contains symbol.
            If contained is true, return true.
    Return false.

This is a bit misleading, as later on we have notes like this one:

https://www.ecma-international.org/ecma-262/11.0/index.html#sec-object-initializer-static-semantics-contains

Static semantic rules that depend upon substructure generally do not look into function definitions.

That seems pretty important. Maybe we should add this note to the description of the algorithm above?

@codehag codehag changed the title Editorial: Contains definition in 5.2.4 may be misleading Editorial Clarification: Contains definition in 5.2.4 may be misleading Dec 11, 2020
@codehag codehag added question and removed question labels Dec 11, 2020
@bakkot
Copy link
Contributor

bakkot commented Dec 11, 2020

Hm. The text surrounding the algorithm steps you quote is pretty explicit about the fact that the given definition is overridden for some productions, to my eyes. Is the ask that it be clearer about which ones, or am I misunderstanding?

Either way, I'll be sure to revise this as part of #1950, which will hopefully make this totally clear by putting the whole definition in a single place.

@michaelficarra
Copy link
Member

I agree with @bakkot, I don't think this would've been a problem if you could see the definition of Contains all in one place.

@codehag
Copy link
Contributor Author

codehag commented Dec 11, 2020

That sounds like a much more comprehensive change, and I would be in favor of that!

@bakkot
Copy link
Contributor

bakkot commented Jan 15, 2021

Contains is now defined in a single place, and all usages of it link there. Hopefully it's clearer now.

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 a pull request may close this issue.

3 participants