editorial review · Issue #9 · tc39/proposal-async-explicit-resource-management (original) (raw)

This repository was archived by the owner on Jun 20, 2023. It is now read-only.

Description

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 various editorial changes for comparisons 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 Editorial: avoid using Type macro for simple type tests 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 Editorial: Examine+reduce the diversity of forms in PR #2877 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 Editorial: Make Evaluation more like regular SDOs 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?