Open michaelficarra opened 1 year ago
- I would probably try to have the definition of
IsUsingDeclaration
mirror the structure ofIsUsingAwaitDeclaration
, replacing somefalse
s withtrue
s. We don't need to take maximum advantage of the chain rule. I would also combine all the other productions into one big "Returnfalse
" 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: https://github.com/rbuckton/ecma262/pull/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 https://github.com/rbuckton/ecma262/pull/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 separateReturnIfAbrupt
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 ofDisposeResources
. It appears that the first parameter ofDisposeResources
isn't something that just dynamically arrives at anundefined
value; instead, the call sites are explicitly setting it toundefined
. 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: MakeEvaluation
more like regular SDOs ecma262#2744.
This is already addressed in https://github.com/rbuckton/ecma262/pull/3 and is an artifact of the version of the specification this proposal's draft spec text was created against.
lookahead ∉ { using }
: we have alookahead ≠
form that should be preferred when the set only contains 1 element
Already addressed in https://github.com/rbuckton/ecma262/pull/3, but I'll fix here as well.
- In the
SuppressedError
constructor, I don't like having an alias namedmessage
and a separate alias namedmsg
. 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.
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
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.
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 https://github.com/rbuckton/ecma262/pull/3.
I see you've pulled the
undefined
test out of the AO, but you're still passingundefined
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?
Does that work?
Yep.
This is a strictly editorial review. I didn't review for correctness.
IsUsingAwaitDeclaration
is missing a definition forForDeclaration : LetOrConst ForBinding
IsUsingDeclaration
mirror the structure ofIsUsingAwaitDeclaration
, replacing somefalse
s withtrue
s. We don't need to take maximum advantage of the chain rule. I would also combine all the other productions into one big "Returnfalse
" case.AddDisposableResource
always returnsNormalCompletion(empty)
. AOs that always return normal completions should not use completion records.~unused~
, not~empty~
. Ecmarkup will enforce that these AOs are treated like procedures.NormalCompletion()
wrappers around the returned values. See https://tc39.es/ecma262/#sec-implicit-normal-completion.Dispose
inDisposeResources
or any of the calls ofDisposeResources
itself. If the AO return types were annotated, I believe ecmarkup would have reported this.?
macro with SDOs (you already use it in some places), so you don't need to have a separateReturnIfAbrupt
step.Using
flags introduced. One seems to mean "ausing
declaration is allowed here" and another seems to mean "this production is nested within ausing
declaration". I would probably try to name them two different things to communicate that they are distinct.undefined
check out ofDisposeResources
. It appears that the first parameter ofDisposeResources
isn't something that just dynamically arrives at anundefined
value; instead, the call sites are explicitly setting it toundefined
. In those cases, I prefer the guard to be outside the AO.Evaluation
SDO. See https://github.com/tc39/ecma262/pull/2744.lookahead ∉ { using }
: we have alookahead ≠
form that should be preferred when the set only contains 1 elementSuppressedError
constructor, I don't like having an alias namedmessage
and a separate alias namedmsg
. Please rename one or both.