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: Remove unused steps from definitions of Contains #1519

Open
wants to merge 2 commits into
base: master
from

Conversation

@jmdyck
Copy link
Collaborator

jmdyck commented Apr 26, 2019

... specifically, steps of the form:

    If _symbol_ is an |Identifier| and StringValue of _symbol_
    is the same value as the StringValue of |IdentifierName|,
    return *true*.

Resolves #831.

As @allenwb points out, the intended use case for such steps does not occur in the spec.

And as I point out, these steps are semantically problematic.

If the intended use case ever does occur, I suspect we'll find a different way to handle it.

jmdyck added 2 commits Apr 26, 2019
... specifically, steps of the form:
  If _symbol_ is an |Identifier| and StringValue of _symbol_
  is the same value as the StringValue of |IdentifierName|,
  return *true*.

Resolves issue #831.

As @allenwb points out, the intended use case for such steps
does not occur in the spec.

And as I point out, these steps are semantically problematic.

If the intended use case ever *does* occur,
I'm guessing we'll find a different way to handle it.
... after previous commit. Specifically, when code says:
```
1. If <something>, return *false*.
1. Return *false*.
```
we can drop the first step, because the result is `*false*` either way.
@ljharb ljharb requested review from allenwb, waldemarhorwat, zenparsing, ljharb and tc39/ecma262-editors Apr 26, 2019
@zenparsing

This comment has been minimized.

Copy link
Member

zenparsing commented May 1, 2019

If the intended use case ever does occur, I suspect we'll find a different way to handle it.

Such as introducing a new static semantic rule like "ContainsIdentifier" or somesuch?

@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented May 2, 2019

Such as introducing a new static semantic rule like "ContainsIdentifier" or somesuch?

Yup, e.g. ContainsArguments in the Public and private instance fields proposal.

Or we might try to introduce a form that abstracts over this kind of ContainsFoo operation, so that defining a new one isn't too burdensome.

@waldemarhorwat

This comment has been minimized.

Copy link

waldemarhorwat commented Jul 23, 2019

Is Contains ever used with identifiers?

If not, then it might be better to do the following:

  • Delete these overrides of Contains
  • Add an override of Contains on IdentifierName that always returns false so that IdentifierName doesn't pick up reserved words in any context.
  • Clarify that Contains on reserved words not in IdentifierName does match

It's currently unclear where Contains stops. Does it jump from the syntactic grammar to the lexical grammar? I hope it doesn't, but the spec is ambiguous. While doing the above it would be good to clarify how Contains deals with that boundary (and on which side of that boundary reserved words and IdentifierName lie) in the definition of Contains.

@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Jul 23, 2019

Is Contains ever used with identifiers?

The right-hand argument of Contains is never |Identifier|, if that's what you mean.

Here are all the right-hand args:

`super`
|AwaitExpression|
|NewTarget|
|NotEscapeSequence|
|ScriptBody|
|SuperCall|
|SuperProperty|
|YieldExpression|

If not, then it might be better to do the following:

  • Delete these overrides of Contains

  • Add an override of Contains on IdentifierName that always returns false so that IdentifierName doesn't pick up reserved words in any context.

Yeah, I think that would work (assuming the syntactic/lexical boundary isn't a problem, see below).

* Clarify that Contains on reserved words not in IdentifierName does match

What do you mean by "reserved words not in IdentifierName"? It seems to me that there's no such thing. (That is, any sequence of characters that matches ReservedWord also matches IdentifierName.)

It's currently unclear where Contains stops. Does it jump from the syntactic grammar to the lexical grammar? I hope it doesn't, but the spec is ambiguous.

I believe it doesn't (currently) matter. That is, for the cases in which it is currently used, Contains will deliver the same result whether you think it jumps or not. (Mind you, I think there's a fair chance that a new use could break that without anyone noticing, so it's kind of fragile.)

While doing the above it would be good to clarify how Contains deals with that boundary (and on which side of that boundary reserved words and IdentifierName lie) in the definition of Contains.

It seems to me that the spec prefers to avoid talking about that boundary. Which might be one reason that these rules stop the recursion one step away from it.

@syg syg self-requested a review Oct 17, 2019
@ljharb ljharb requested review from bakkot and removed request for tc39/ecma262-editors and zenparsing Nov 9, 2019
@bakkot
bakkot approved these changes Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.