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: Introduce abstract closure and use them in RegExps #1889

Merged
merged 1 commit into from
Mar 9, 2020

Conversation

syg
Copy link
Contributor

@syg syg commented Mar 4, 2020

We don't have a well-defined notion of closures purely for use within the spec. This PR introduces the notion of an abstract closure and replaces the ad-hoc "internal procedures" and "internal closures" used by RegExps with them.

Also intended for use in #1597 for speccing Jobs.

@syg syg requested a review from a team March 4, 2020 22:22
@syg syg added the editor call to be discussed in the next editor call label Mar 4, 2020
@syg syg changed the title Introduce abstract closure and use them in RegExps Editorial: Introduce abstract closure and use them in RegExps Mar 4, 2020
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added the es2020 label Mar 4, 2020
spec.html Show resolved Hide resolved
@syg
Copy link
Contributor Author

syg commented Mar 5, 2020

Rebased and force pushed.

Incorporated all of @michaelficarra and @bakkot's suggestions.

@ljharb ljharb requested review from ljharb and a team March 5, 2020 00:17
@ljharb ljharb added editorial change and removed editor call to be discussed in the next editor call labels Mar 5, 2020
@devsnek
Copy link
Member

devsnek commented Mar 5, 2020

Why can't we just say "spec steps may capture variables as needed"?

@syg
Copy link
Contributor Author

syg commented Mar 5, 2020

Why can't we just say "spec steps may capture variables as needed"?

We can, but I'm of the opinion requiring explicit captures is a good editorial direction to avoid accidental capture.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Mar 5, 2020

@domenic That's a great point! I think a natural followup to this PR would be to change CreateBuiltInFunction so that it took an abstract closure as its first argument, rather than a list of algorithm steps. (Edit: that followup is #2109.)

@TimothyGu
Copy link
Member

Under https://deploy-preview-1889--ecma262-snapshots.netlify.com/#sec-properties-of-regexp-instances is:

The value of the [[RegExpMatcher]] internal slot is an implementation-dependent representation of the Pattern of the RegExp object.

It should probably be changed to "abstract closure" as well.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb requested review from jmdyck and TimothyGu March 6, 2020 00:49
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

The 'evaluate' semantics for Pattern :: Disjunction include the step:

1. Let _c_ be a Continuation that always returns its State argument as a successful MatchResult.

which should probably be changed to:

1. Let _c_ be a new Continuation with parameters (_y_) that captures nothing and performs the following steps when called:
  1. Assert: _y_ is a State.
  1. Return _y_.

(as in the semantics for lookaround assertions.)

Similarly, the semantics for Alternative :: [empty] has:

1. Return a Matcher that takes two arguments, a State _x_ and a Continuation _c_, and returns the result of calling _c_(_x_).

which should probably be changed to:

1. Return a new Matcher with parameters (_x_, _c_) that captures nothing and performs the following steps when called:
  1. Assert: _x_ is a State.
  1. Assert: _c_ is a Continuation.
  1. Return _c_(_x_).

spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@syg
Copy link
Contributor Author

syg commented Mar 6, 2020

Thanks for the catches, @jmdyck!

@ljharb ljharb requested a review from jmdyck March 7, 2020 19:10
jmdyck
jmdyck previously requested changes Mar 8, 2020
spec.html Show resolved Hide resolved
<h1>The Abstract Closure Specification Type</h1>
<p>The <dfn>abstract closure</dfn> specification type is used to refer to algorithm steps together with a collection of values. Abstract closures are meta-values and are invoked using function application style such as _closure_(_arg1_, _arg2_). Like abstract operations, invocations perform the algorithm steps described by the abstract closure.</p>
<p>In algorithm steps that create an abstract closure, values are captured with the verb "capture" followed by a list of aliases. The value associated with each alias at creation time is captured. In steps that specify the algorithm to be performed when an abstract closure is called, each captured value is referred to by the alias that was used to capture the value.</p>
<p>If an abstract closure returns a Completion Record, that Completion Record's [[Type]] must be either ~normal~ or ~throw~.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<p>If an abstract closure returns a Completion Record, that Completion Record's [[Type]] must be either ~normal~ or ~throw~.</p>
<p>If an abstract closure returns a Completion Record, that Completion Record's [[Type]] is either ~normal~ or ~throw~.</p>

(because "must be" suggests a requirement on an implementation, whereas this is just a self-imposed restriction on the spec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disagree. There are no requirements on implementation anywhere when talking about specification types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh? I'm not even sure how that relates to what I said. Are you disagreeing with my statement that "must be" suggests a requirement on an implementation, or with my statement that the sentence in question is a self-imposed restriction on the spec?

There are no requirements on implementation anywhere when talking about specification types.

If that were true, then when talking about specification types, we could say completely different things without changing any of the requirements on implementations. But that's clearly false. For example, imagine that, when talking about Lists, we said that elements are in an unpredictable order, or that duplicates are removed. That would certainly change the requirements that the spec imposes on implementations.

Copy link
Member

Choose a reason for hiding this comment

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

The observable differences are requirements on implementations, anything else is purely editorial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, but again, I'm not sure how that relates to my suggested change.

@ljharb ljharb dismissed jmdyck’s stale review March 9, 2020 20:46

changes addressed/responded to

ljharb pushed a commit to syg/ecma262 that referenced this pull request Mar 9, 2020
ljharb pushed a commit to syg/ecma262 that referenced this pull request Mar 9, 2020
@ljharb ljharb merged commit 8e15cd0 into tc39:master Mar 9, 2020
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 23, 2020
PR tc39#1889 (among other things) inserted a step before the former 2.a,
but didn't update the note that referenced it.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 24, 2020
PR tc39#1889 (among other things) inserted a step before the former 2.a,
but didn't update the note that referenced it.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 30, 2020
PR tc39#1889 (among other things) inserted a step before the former 2.a,
but didn't update the note that referenced it.
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request May 6, 2020
Editorial: Reinstate an SDO rule

PR tc39#1933 deleted SDO rules that are handled by the chain production rule,
but it also deleted this one which isn't.
(It has "TV" on the left and "TRV" on the right.)

Editorial: Move an <emu-note> element

PR tc39#1490 (among other things) moved the evaluation semantics for ArrowFunction
from the Evaluation SDO to the NamedEvaluation SDO.
The accompanying <emu-note> should have moved at the same time
(in particular because of the reference to "step 3").

Editorial: Delete <emu-note> in TimeClip clause

PR tc39#1827 (among other things) removed step 4 from the algorithm for TimeClip,
obsoleting the accompanying emu-note that describes "the point of step 4".

Conceivably, the note could be reworded to describe the effect of
'ToInteger' on step 3, but I don't think it'd be worth the bother.

Editorial: Change "Step 2.a" to "Step 2.b" in RepeatMatcher note

PR tc39#1889 (among other things) inserted a step before the former 2.a,
but didn't update the note that referenced it.

Editorial: Change step 7 to step 6 in SortCompare note

Commit 9c1e076 (2015-10-26) introduced the '?' abbreviation for ReturnIfAbrupt.
This caused the ToString call on step 7 to move to step 6,
but the note that referred to it wasn't updated.

Editorial: Fix typo: "Descritor" -> "Descriptor"

Editorial: Fix typo: "GeneratorObject" -> "generator object"

(There's no such thing as a GeneratorObject.)

Editorial: Delete "as a parameter" after "is present"

(It's the only place in the spec where we use that phrasing.)

Editorial: Change "which" to "that"

... in "{String,Array,Map,Set} methods which return such iterators"

Editorial: Insert a comma in SetDefaultGlobalBindings()

Formerly, it read like "containing" modified "the property",
when it actually modified "the property descriptor".

Editorial: Change "lexical environment" to "Environment Record"

... in FunctionDeclarationInstantiation,
to balance the NOTE in the other arm of the if-else,
and also for consistency with the NOTE at 27.a.

(I should have done this in PR tc39#1697.)

Editorial: Change "step 3" to "step 4" in Note

... that accompanies the NamedEvaluation semantics for
    ArrowFunction : ArrowParameters `=&gt;` ConciseBody

PR tc39#1870 (among other things) inserted a step before the former step 3,
but didn't update the note that referenced it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants