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.

Sign up for GitHub

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

Jump to bottom

Normative: plug some holes in the coverage of syntax-directed operations #1301

Merged
merged 1 commit into from
Feb 19, 2020
Merged

Normative: plug some holes in the coverage of syntax-directed operations #1301

merged 1 commit into from
Feb 19, 2020

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Sep 7, 2018

(I think there are still some coverage holes in Evaluation, but that's a bit tricky.)

(Plus there's the bug in VarScopedDeclarations addressed by PR #1284.)

bmeck
bmeck previously requested changes Sep 11, 2018
View reviewed changes
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
<emu-alg>
1. Return *false*.
</emu-alg>
<emu-grammar>ObjectBindingPattern : `{` BindingPropertyList `,` BindingRestProperty `}`</emu-grammar>
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird since we don't list the chain production, but makes sense.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 12, 2018

@bmeck, are you still requesting changes on this PR?

TimothyGu
TimothyGu reviewed Jan 2, 2019
View reviewed changes
spec.html Outdated
<emu-grammar>
ObjectBindingPattern :
`{` `}`
`{` BindingRestProperty `}`
Copy link
Member

@TimothyGu TimothyGu Jan 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively (and more analogous to BindingRestElement), we could return true false from BindingRestProperty : ... BindingIdentifier instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why true?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I meant false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, and let the chain rule take care of ContainsExpression for

    ObjectBindingPattern : `{` BindingRestProperty `}`

but then for consistency, we'd also have to change the alg for

    ObjectBindingPattern : `{` BindingPropertyList `,` BindingRestProperty `}`

from

    1. Return ContainsExpression of |BindingPropertyList|.

to

    1. Let _has_ be ContainsExpression of |BindingPropertyList|.
    1. If _has_ is *true*, return *true*.
    1. Return ContainsExpression of |BindingRestProperty|.

Personally, I don't think the analogy with BindingRestElement is worth the added complication.

@ljharb ljharb requested review from ljharb and a team and removed request for bterlson February 28, 2019 19:57
@jmdyck jmdyck added the spec bug label Jun 1, 2019
@jmdyck jmdyck mentioned this pull request Jul 3, 2019
Merged
@jmdyck jmdyck changed the title Editorial: plug some holes in the coverage of syntax-directed operations Normative: plug some holes in the coverage of syntax-directed operations Jul 3, 2019
@jmdyck jmdyck added normative change Affects behavior required to correctly evaluate some ECMAScript source text and removed editorial change labels Jul 3, 2019
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jul 3, 2019

Changing from Editorial to Normative based on this comment from @ljharb.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 1, 2019

Rebased to master and added 3 more commits.

@jmdyck jmdyck mentioned this pull request Nov 4, 2019
Merged
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Nov 4, 2019
@ljharb ljharb requested review from syg and bakkot November 9, 2019 04:24
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Nov 20, 2019
@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 22, 2019

forced-pushed to resolve a merge-conflict.

(PR #1608 added a definition for IsSimpleParameterList in a way that was different from (but equivalent to) my former 5th commit, which has now disappeared.)

ljharb
ljharb approved these changes Jan 29, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

rubberstamp based on editor call and previous approvals

@ljharb ljharb self-assigned this Jan 29, 2020
@bakkot
Copy link
Contributor

bakkot commented Feb 1, 2020 •
edited
Loading

(See also #1861, though it's slightly nonobvious what the right thing is in that case.)

@jmdyck @ljharb
Normative: plug some holes in the coverage of syntax-directed operati…
efa4e7e 
…ons (tc39#1301)

 - CoveredCallExpression: change production
      When an SDO is applied to a Parse Node, the appropriate definition of the SDO
      is the one for the production of which the Parse Node is an instance
      (ignoring the complication of chain productions).

      The only invocation of CoveredCallExpression
      is on a |CoverCallExpressionAndAsyncArrowHead|,
      so we expect CoveredCallExpression to be defined on
          CoverCallExpressionAndAsyncArrowHead : MemberExpression Arguments
      not
          CallExpression : CoverCallExpressionAndAsyncArrowHead
 - BoundNames: add def for ObjectBindingPattern
      BoundNames was missing a definition for
          ObjectBindingPattern : `{` BindingPropertyList `,` BindingRestProperty `}`
 - ContainsExpression: add a few defs
      ContainsExpression was missing definitions for:
          ObjectBindingPattern : `{` BindingRestProperty `}`
          ObjectBindingPattern : `{` BindingPropertyList `,` BindingRestProperty `}`
          ArrowParameters : CoverParenthesizedExpressionAndArrowParameterList
 - ExpectedArgumentCount: add a few defs
      ExpectedArgumentCount was missing definitions for:
          FormalParameters : FunctionRestParameter
          FormalParameterList : FormalParameter
          ArrowParameters : CoverParenthesizedExpressionAndArrowParameterList
 - CoveredFormalsList: add a def
      CoveredFormalsList was missing a definition for
          CoverParenthesizedExpressionAndArrowParameterList : `(` Expression `,` `)`
 - IteratorBindingInitialization: add a def
      IteratorBindingInitialization was missing a definition for:
          ArrowParameters : CoverParenthesizedExpressionAndArrowParameterList
 - HasCallInTailPosition: add a few defs
      HasCallInTailPosition was missing definitions for the 3
          IterationStatement : `for` `await` `(` productions.
 - define 3 SDOs on empty FunctionStatementList
      specifically:
       - ContainsDuplicateLabels
       - ContainsUndefinedBreakTarget
       - ContainsUndefinedContinueTarget
 - MV: add def for Hex4Digits
      PR tc39#984 added a reference to "the MV of |Hex4Digits|"
      (in the definition of CharacterValue)
      but didn't add a definition for it.

      Extract one from the definition for "the SV of |Hex4Digits|".
 - eliminate unsupported "SV of |SourceCharacter|"
      We have rules that say the TRV of something is:
       - the SV of the |SourceCharacter| that is that single code point; or
       - the SV of the |SourceCharacter| that is that |HexDigit|.

      but there's no definition for the SV of |SourceCharacter|.

      We could add such a definition, except that these usages are already odd,
      in that they introduce a |SourceCharacter| nonterminal when there isn't one
      in the parse tree. So instead, fix that oddity by eliminating the references
      to SV and |SourceCharacter|.

      And since I'm in the neighborhood,
         The TRV of a |HexDigit| ...
      is weird. No other SDO rule is written that way. Change it to:
         The TRV of <emu-grammar>HexDigit :: ....</emu-grammar> ...
@ljharb ljharb merged commit efa4e7e into tc39:master Feb 19, 2020
@jmdyck jmdyck deleted the sdo-coverage branch February 20, 2020 20:07
@jmdyck jmdyck mentioned this pull request Feb 20, 2020
Merged
@jmdyck jmdyck restored the sdo-coverage branch February 20, 2020 20:32
@jmdyck jmdyck deleted the sdo-coverage branch February 20, 2020 20:54
@jmdyck jmdyck mentioned this pull request Feb 25, 2020
Open
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Feb 26, 2020
@jmdyck @ljharb
Editorial: supply args to IteratorBindingInitialization (tc39#1877)
435ae46 
Fixes a bug in PR tc39#1301
@bakkot bakkot mentioned this pull request May 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmeck bmeck bmeck left review comments

@TimothyGu TimothyGu TimothyGu left review comments

@ljharb ljharb ljharb approved these changes

@michaelficarra michaelficarra michaelficarra approved these changes

@bakkot bakkot bakkot approved these changes

@zenparsing zenparsing zenparsing approved these changes

@waldemarhorwat waldemarhorwat Awaiting requested review from waldemarhorwat

@syg syg Awaiting requested review from syg

Assignees

@ljharb ljharb

Labels
normative change Affects behavior required to correctly evaluate some ECMAScript source text spec bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jmdyck @bakkot @ljharb @michaelficarra @bmeck @TimothyGu @zenparsing @bterlson