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

Minor spec bugs #226

Closed
Fayti1703 opened this issue Sep 21, 2021 · 6 comments · Fixed by #255
Closed

Minor spec bugs #226

Fayti1703 opened this issue Sep 21, 2021 · 6 comments · Fixed by #255
Labels
bug Something isn’t working as intended in spec documentation Improvements or additions to documentation

Comments

@Fayti1703
Copy link

The current draft specification reads:
§1.2.1 (Static Semantics: Contains)

PipeBody : PipeExpression

  1. If symbol is ^, return false.

§4.2 (Pipe Operator)

PipeBody[In, Yield, Await] :
AssignmentExpression[?In, ?Yield, ?Await]

§4.2.1 (Static Semantics: Early Errors)

PipeBody : PipeExpression

  1. It is a Syntax Error if PipeBody Contains ^ is false

Given my current understanding of how Static Semantics work, there's two issues here:

  1. PipeBody Contains ^ is always false, so any occurrence of the production PipeBody : PipeExpression would be a syntax error.
  2. There is no production PipeBody : PipeExpression, so the early error static semantics, Contains semantics and the semantics in §4.2.1 directly below the quoted one never fire.

I suspect the intention was to attach the semantics to PipeBody : AssignmentExpression instead and perform the Contains check on AssignmentExpression, but I'm not sure on this.

@js-choi js-choi added documentation Improvements or additions to documentation bug Something isn’t working as intended in spec labels Sep 21, 2021
@lightmare
Copy link

I think the Contains check should be something like:

PipeExpression : ShortCircuitExpression |> PipeBody
  1. If symbol is ^, return ShortCircuitExpression Contains symbol
    (i.e. dive into the LHS, and ignore the RHS, because it has its own topic)
  2. ...

For example, 1 |> (2 |> 3 + ^) is illegal because there's no topic reference in 2 (and the one in 3 + ^ is in a different pipe).
The inner PipeExpression 2 |> 3 + ^ will pass the Contains check, because its PipeBody is 3 + ^.
The outer PipeExpression will fail the check, because its PipeBody is 2 |> 3 + ^ which matches the above production and won't dive into the inner PipeBody.

However, I was under the impression that you wanted 42 |> (function() { return ^ }) to be legal syntax, and that is not possible with the current definition of Contains, which doesn't dive into function expressions.

@js-choi
Copy link
Collaborator

js-choi commented Sep 26, 2021

I agree with both bugs; thank you for finding them.

In fact, I believe the latter bug was what @waldemarhorwat was referring to in the 2021-08 plenary meeting:

WH: Next question I have is, and the spec contradicts itself on that, can you use % inside the bodies of nested functions?

a |> (%, x => %) |> (y => %)

JSC: I believe it is. Okay. Yeah. Trying to minimize special rules—but whatever happens.

WH: Part of the spec says that’s allowed; part of the spec says that’s not allowed. What is the intended behavior?

JSC: I’ll deal with issues like that after the meeting, whenever you want: happy to hash out after the meeting. The intent is to allow them in functions. Although I wouldn’t say that might be good style.

TAB: The percent is just a variable binding that applies just within the context of the things. So you can lexically bind over and should be able to lexically bind over that in any way that you could a normal variable.

WH: Okay, so it’s not like this where you can’t use it inside nested functions? Thank you. All of these syntax problems are solvable and I see no real showstoppers here. % would work as far as the syntax is concerned.

I’ll try to deal with these two bugs at the same time. My plan is:

  1. Move the Contains check to the left-hand ShortCircuitExpression.
  2. Add a special case to the definition of Contains for ^ in function declarations.

I think that’s all we need to do; both are pretty simple edits. I’ll try to get to fixing these sometime.

@waldemarhorwat
Copy link
Collaborator

In addition to function bodies, there are other productions that prevent the propagation of Contains, some of which need to be modified with some care.

There is also a simpler problem I alluded to during the meeting: The definition of Contains has turned the script 3^4 into a syntax error instead of returning 7 as it should.

@waldemarhorwat
Copy link
Collaborator

The syntax error appears due to the static semantics added in 5.1.1 of the current proposal, which turns scripts that use ^ as an xor operator into syntax errors. Another version of that error happens in CreateDynamicFunction.

@js-choi
Copy link
Collaborator

js-choi commented Sep 28, 2021

@waldemarhorwat: Excellent points; thank you.

We probably would fix the latter problem by hiding ^ in BitwiseXORExpression from Contains. This should not be developer-observable.

So my plan is:

  1. Move the Contains check to the left-hand ShortCircuitExpression.
  2. Add special cases for ^ to the definition of Contains, for each separately defined production that could contain ^.
  3. Add a special case for ^ to the definition of Contains for BitwiseXORExpression.

(FYI: We are talking about tentatively switching the spec back to % soon, but these changes would be the same regardless.)

@js-choi js-choi changed the title Static Semantics Confusion Minor spec bugs Sep 28, 2021
@tc39 tc39 deleted a comment from aadamsx Sep 28, 2021
@waldemarhorwat
Copy link
Collaborator

The scope handling of the check for ^ is divergent enough from other uses of Contains that it would likely be simpler to write a separate static semantic function instead of trying to re-purpose Contains.

js-choi added a commit that referenced this issue Oct 29, 2021
js-choi added a commit that referenced this issue Nov 8, 2021
Fixes #226.
The specification’s early errors were not updated for the new PipeBody : AssignmentExpression production.
`a |> (x => %)` was incorrectly invalid because Contains always returned false for functions.
The script `a % 2` was incorrectly invalid because Contains `%` is true for that script.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn’t working as intended in spec documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants