editorial review

Question

editorial review

michaelficarra opened this issue 3 years ago · 4 comments

This is a strictly editorial review. I didn't review for correctness.

Instead of "if X is Y or Z", you should use "if X is either Y or Z". See tc39/ecma262#2877 for more conventions around comparisons.
Instead of "If Type(V) is not Object", you should use "If V is not an Object", and similar throughout. See tc39/ecma262#2874.
In "in reverse list order", "List" should be capitalised.
IsUsingAwaitDeclaration is missing a definition for ForDeclaration : LetOrConst ForBinding
I would probably try to have the definition of IsUsingDeclaration mirror the structure of IsUsingAwaitDeclaration, replacing some falses with trues. We don't need to take maximum advantage of the chain rule. I would also combine all the other productions into one big "Return false" case.
You're missing return types on your AOs. Please add them. All AO return types should be listed explicitly now.
AddDisposableResource always returns NormalCompletion(empty). AOs that always return normal completions should not use completion records.
- Also, AOs whose value is never consumed should return ~unused~, not ~empty~. Ecmarkup will enforce that these AOs are treated like procedures.
- Relatedly, AOs that are declared to return completions do not need to use NormalCompletion() wrappers around the returned values. See https://tc39.es/ecma262/#sec-implicit-normal-completion.
Similarly, when completion records enter an algorithm, you must use the Completion AO. See for example the call of Dispose in DisposeResources or any of the calls of DisposeResources itself. If the AO return types were annotated, I believe ecmarkup would have reported this.
The type "either normal, sync-dispose, or async-dispose" should be written "one of normal, sync-dispose, or async-dispose" since it is 3 or more values. See tc39/ecma262#2972 (comment).
You can use the ? macro with SDOs (you already use it in some places), so you don't need to have a separate ReturnIfAbrupt step.
There's two separate, non-interacting Using flags introduced. One seems to mean "a using declaration is allowed here" and another seems to mean "this production is nested within a using declaration". I would probably try to name them two different things to communicate that they are distinct.
I would probably pull the undefined check out of DisposeResources. It appears that the first parameter of DisposeResources isn't something that just dynamically arrives at an undefined value; instead, the call sites are explicitly setting it to undefined. In those cases, I prefer the guard to be outside the AO.
"Let result be result of evaluating FunctionStatementList". You're missing the word "the", but also we don't use this form anymore. Instead, there's now an Evaluation SDO. See tc39/ecma262#2744.
lookahead ∉ { using }: we have a lookahead ≠ form that should be preferred when the set only contains 1 element
In the SuppressedError constructor, I don't like having an alias named message and a separate alias named msg. Please rename one or both.
I don't see the need for "DisposableStack adopt callback function". Why isn't this just a record? Why does it need to be a function object? Is one ever exposed to user code somewhere?

Answer 1 · 2023-01-29T23:47:46.000Z

I would probably try to have the definition of IsUsingDeclaration mirror the structure of IsUsingAwaitDeclaration, replacing some falses with trues. We don't need to take maximum advantage of the chain rule. I would also combine all the other productions into one big "Return false" case.

I mostly based it on IsConstDeclaration, which doesn't combine everything for the false branches either. I also tried to minimize the changes to IsUsingDeclaration from the sync version of the proposal.

You're missing return types on your AOs. Please add them. All AO return types should be listed explicitly now.

I'll update the version of the proposal in this repo. I already have them in this PR: rbuckton/ecma262#3.

Also, AOs whose value is never consumed should return ~unused~, not ~empty~. Ecmarkup will enforce that these AOs are treated like procedures.

Also addressed in rbuckton/ecma262#3, but I'll update here as well.

You can use the ? macro with SDOs (you already use it in some places), so you don't need to have a separate ReturnIfAbrupt step.

Can you point out where this needs to be fixed? I can't seem to find it except in places where the original algorithm might have changed (i.e., it's not occuring in any of the <ins> covered blocks).

I would probably pull the undefined check out of DisposeResources. It appears that the first parameter of DisposeResources isn't something that just dynamically arrives at an undefined value; instead, the call sites are explicitly setting it to undefined. In those cases, I prefer the guard to be outside the AO.

I may have already done this, I just pushed up some changes that I also made in the ECMA262 PR for the sync version of the proposal.

"Let result be result of evaluating FunctionStatementList". You're missing the word "the", but also we don't use this form anymore. Instead, there's now an Evaluation SDO. See Editorial: Make Evaluation more like regular SDOs ecma262#2744.

This is already addressed in rbuckton/ecma262#3 and is an artifact of the version of the specification this proposal's draft spec text was created against.

lookahead ∉ { using }: we have a lookahead ≠ form that should be preferred when the set only contains 1 element

Already addressed in rbuckton/ecma262#3, but I'll fix here as well.

In the SuppressedError constructor, I don't like having an alias named message and a separate alias named msg. Please rename one or both.

This is the exact same text as in the AggregateError constructor. Should I differ here or should that end up being changed at some point as well?

I don't see the need for "DisposableStack adopt callback function". Why isn't this just a record? Why does it need to be a function object? Is one ever exposed to user code somewhere?

It is a function object to keep AddDisposableResource and DisposeResources tidy. It's not exposed. IIRC, we use function objects in this way elsewhere in the spec (such as Object.fromEntries), though it does seem like the way in which they are created has changed to using the new CreateBuiltinFunction AO, so I will update to use that instead.

Answer 2 · 2023-01-30T00:28:05.000Z

I also tried to minimize the changes to IsUsingDeclaration from the sync version of the proposal.

I still think it would be a better end result if they were structured similarly.

I'll update the version of the proposal in this repo. I already have them in this PR: rbuckton/ecma262#3.

Oops, I didn't realise you had a PR.

Can you point out where this needs to be fixed?

The cases I noticed were in https://tc39.es/proposal-async-explicit-resource-management/#sec-let-and-const-declarations-runtime-semantics-evaluation. They can be

Perform ? BindingEvaluation of BindingList with parameter sync-dispose.
Return empty.

I may have already done this, I just pushed up some changes that I also made in the ECMA262 PR for the sync version of the proposal.

I see you've pulled the undefined test out of the AO, but you're still passing undefined instead of guarding at the call site. See https://tc39.es/proposal-async-explicit-resource-management/#sec-runtime-semantics-forin-div-ofbodyevaluation-lhs-stmt-iterator-lhskind-labelset

Should I differ here or should that end up being changed at some point as well?

Differ here. We can clean up confusing alias names in existing AOs separately.

IIRC, we use function objects in this way elsewhere in the spec (such as Object.fromEntries)

In Object.fromEntries, we use an abstract closure, which would also be acceptable. I prefer to avoid introducing new kinds of things (especially function objects) if we can get away with using Records, ACs, or some other existing spec construct.

Answer 3 · 2023-01-30T00:42:44.000Z

I also tried to minimize the changes to IsUsingDeclaration from the sync version of the proposal.

I still think it would be a better end result if they were structured similarly.

Already done.

I'll update the version of the proposal in this repo. I already have them in this PR: rbuckton/ecma262#3.

Oops, I didn't realise you had a PR.

It's not against tc39/ecma262, yet. I'm hoping I can merge it into tc39/ecma262#3000 following the January plenary because of the overlap, and if not I'll create a separate PR.

The cases I noticed were in https://tc39.es/proposal-async-explicit-resource-management/#sec-let-and-const-declarations-runtime-semantics-evaluation.

Ah. This is another case of drift from the version of the spec this was originally written against. It's already fixed in rbuckton/ecma262#3.

I see you've pulled the undefined test out of the AO, but you're still passing undefined instead of guarding at the call site. See https://tc39.es/proposal-async-explicit-resource-management/#sec-runtime-semantics-forin-div-ofbodyevaluation-lhs-stmt-iterator-lhskind-labelset

Ah, thanks. I'll fix that here and in tc39/ecma262#3000.

Should I differ here or should that end up being changed at some point as well?

Differ here. We can clean up confusing alias names in existing AOs separately.

I can change it, but this is the same text used in Error and NativeError as well.

In Object.fromEntries, we use an abstract closure, which would also be acceptable. I prefer to avoid introducing new kinds of things (especially function objects) if we can get away with using Records, ACs, or some other existing spec construct.

In #10 I've changed this algorithm to use an abstract closure and CreateBuiltinFunction, just as we do with Object.fromEntries. Does that work?

Answer 4 · 2023-01-30T14:39:31.000Z

Does that work?

Yep.