Stage 3 Specification Review: Waldemar Horwat

[rbuckton]

This is a placeholder task for the Stage 3 Specification Review feedback from Waldemar Horwat (@waldemarhorwat), per https://github.com/tc39/notes/blob/master/meetings/2019-07/july-25.md#explicit-resource-management-for-stage-2-continuation-from-tuesday

[waldemarhorwat]

• Spelling in spec: hygenic → hygienic

• The invariants are a bit confusing. I was trying to figure out whether the [[DisposeMethod]] of a DisposableResource can be undefined. CreateDisposableResource calls GetDisposeMethod, which can return undefined in its first statement, and stores the result into [[DisposeMethod]]. On the other hand, the first statement of GetDisposeMethod is actually dead code since it's never called with V set to null or undefined.

• Why is for (using const x in …) allowed by the grammar but turned into a syntax error by a static semantic rule? The simplest thing is to allow it, just like in for-of. I wouldn't worry about mandating early errors for obscure cases where we can determine that x doesn't have a . There's not much point in trying to turn things like using const x = 5 into early errors.

• Please document the return values of HasCallInTailPosition. It's hard to read the spec without knowing what possible values it can return and what they mean. For example, does "not-found" mean that nothing blocking the call from being considered to be in a tail position was found?

• CaseBlock without default clause: "first CaseClauses" → "CaseClauses"

• A using const directly inside a switch statement will cause all subsequent statements to be non-tail-call even if they are in separate case clauses with an intervening unconditional break or return. It's an obscure case though, and I'm not sure trying to fix it is worth it.

• Various for statements have HasCallInTailPosition defined as "Return HasCallInTailPosition of Statement with argument call". But don't some more of them need to account, for example, a LexicalDeclaration inside a three-clause for's parentheses containing using const?

• The readme is inconsistent with the semantics. Which one is correct?

The readme states that multiple exceptions are handled as:

    if ($$try.errors.length > 1) {
      throw new AggregateError($$try.errors);
    }
    if ($$try.errors.length === 1) {
      throw $$try.errors[0];
    }

whereas the semantics seem to unconditionally create an AggregateError if any disposers produce errors. I like the version in the semantics much better. However, it still suffers from the potential issue that if you have several consecutive using const statements then you might get a tree of AggregateErrors.

[rbuckton]

• The invariants are a bit confusing. I was trying to figure out whether the [[DisposeMethod]] of a DisposableResource can be undefined. CreateDisposableResource calls GetDisposeMethod, which can return undefined in its first statement, and stores the result into [[DisposeMethod]]. On the other hand, the first statement of GetDisposeMethod is actually dead code since it's never called with V set to null or undefined.

A DisposableResource's [[DisposeMethod]] can't be undefined. The logic that handles that is in AddDisposableResource, so some of these algorithm steps are redundant. I'll clean this up.

• Why is for (using const x in …) allowed by the grammar but turned into a syntax error by a static semantic rule? The simplest thing is to allow it, just like in for-of. I wouldn't worry about mandating early errors for obscure cases where we can determine that x doesn't have a . There's not much point in trying to turn things like using const x = 5 into early errors.

A using const is allowed in for..of, but not for..in, but both statements use ForDeclaration. I can add a [lookahead ≠ `using`] to the for..in definition instead if that's clearer.

• Please document the return values of HasCallInTailPosition. It's hard to read the spec without knowing what possible values it can return and what they mean. For example, does "not-found" mean that nothing blocking the call from being considered to be in a tail position was found?

I'll add a table describing the values, as well as make them clearer: ~not-found~ -> ~tail-call-not-found~, etc.

• A using const directly inside a switch statement will cause all subsequent statements to be non-tail-call even if they are in separate case clauses with an intervening unconditional break or return. It's an obscure case though, and I'm not sure trying to fix it is worth it.

I agree. The current logic is a best effort to avoid a blanket Contains |UsingConst| so that the following refactor wouldn't change whether a function is tail-call recursive:

  function f() {
-   const resource = ...;
-   try {
+   {
+     using const resource = ...;
      ...
    }
-   finally {
-     resource.close();
-   }
    return f();
  }

Handling fall-through for cases in a switch statement may be something we can investigate further, if necessary.

• Various for statements have HasCallInTailPosition defined as "Return HasCallInTailPosition of Statement with argument call". But don't some more of them need to account, for example, a LexicalDeclaration inside a three-clause for's parentheses containing using const?

Ah, I neglected to ban for (using const x = ...; ; ) {}. It made sense to me to allow using const in a for..of statement, as you could be generating and iterating over disposable resources. It seems less useful in a regular for-statement, so the simplest case would be to ban for (using const x = ...;;). However, there's no reason why it shouldn't be allowed, so I will look into fixing this before outright banning it altogether.

I explicitly ban it in for..in because a String can never be disposable.

• The readme is inconsistent with the semantics. Which one is correct?

The readme states that multiple exceptions are handled as:

    if ($$try.errors.length > 1) {
      throw new AggregateError($$try.errors);
    }
    if ($$try.errors.length === 1) {
      throw $$try.errors[0];
    }

whereas the semantics seem to unconditionally create an AggregateError if any disposers produce errors. I like the version in the semantics much better. However, it still suffers from the potential issue that if you have several consecutive using const statements then you might get a tree of AggregateErrors.

The README is out of date in this case. I will update it shortly.

[rbuckton]

Also, regarding switch: HasCallInTailPosition is a Static Semantics rule, so it doesn't differentiate between case 2 and case 3 below anyways:

function f() {
    switch (x) {
        case 1:
            return f(); // probably tail callable
        case 2:
            using const y = ...;
            // falls through
        case 3:
            return f(); // possibly not a tail callable?
    }
}

Even if we could solve for case 1, we would have to assume that the return in case 3: is not tail-callable since we can reach it from two different case clauses, only one of which contains a using const. If we did some kind of control flow analysis of switch, then we would have cases where the code "seems" tail callable but isn't due to a fall-through case clause, which would make it more difficult for a developer to determine if their function will be properly tail-call optimized. Instead, having a coarse-grained rule about tail calls in switch is actually more predictable.

[rbuckton]

I addressed the above comments in #75. You find the rendered version of the spec with these changes here: https://tc39.es/proposal-explicit-resource-management/pr/75

[rbuckton]

However, it still suffers from the potential issue that if you have several consecutive using const statements then you might get a tree of AggregateErrors.

That's always a possibility (and is already possible with Promise.any). Now that this proposal uses cause to hold the exception from the body of the containing block, a tree of AggregateErrors provides the most fidelity with respect to exception information.

In .NET, the AggregateException class has a Flatten() method to flatten a tree of AggregateException instances. We might want to consider adding something similar to AggregateError in the future.

[waldemarhorwat]

I don't like the "[lookahead ≠ using]" restriction in the for-in rule of the grammar because it makes the grammar non-LR(1). The issue is that there are two parallel productions that start with "for (ForDeclaration[?Yield, ?Await]", one of which has the "[lookahead ≠ using]" restriction and one which doesn't, which causes a grammar conflict on how the ForDeclaration should be parsed.

If you want to syntactically outlaw using in for-in loops, the standard grammar technique would be better and simpler. Factor LetOrConstForDeclaration out of ForDeclaration so that LetOrConstForDeclaration contains only the let/const form whereas ForDeclaration expands into either the using const form or a LetOrConstForDeclaration. Then have the for-in grammar use LetOrConstForDeclaration instead of ForDeclaration.

[rbuckton]

I don't like the "[lookahead ≠ using]" restriction in the for-in rule of the grammar because it makes the grammar non-LR(1). The issue is that there are two parallel productions that start with "for (ForDeclaration[?Yield, ?Await]", one of which has the "[lookahead ≠ using]" restriction and one which doesn't, which causes a grammar conflict on how the ForDeclaration should be parsed.

We already do something similar in ForInOfStatement with respect to how LeftHandSideExpression is treated in for..in vs. for..of:

If [lookahead ≠ `using`] isn't enough, I can do the same as for..of and use [lookahead ≠ `using` `const`] as a better disambiguator.

If you want to syntactically outlaw using in for-in loops, the standard grammar technique would be better and simpler. Factor LetOrConstForDeclaration out of ForDeclaration so that LetOrConstForDeclaration contains only the let/const form whereas ForDeclaration expands into either the using const form or a LetOrConstForDeclaration. Then have the for-in grammar use LetOrConstForDeclaration instead of ForDeclaration.

The same could be said for LeftHandSideExpression, and I doubt we want to refactor that.

[rbuckton]

Also, refactoring ForDeclaration might require a number of additional changes to SDOs that reference it currently. I'd argue the lookahead restriction is actually simpler for the narrower case of banning using const in for..in.

[rbuckton]

for..in and for..of are already ambiguous in the spec. If we truly wanted this to be an LR(1) production we'd need to rewrite ForStatement and ForInOfStatement to use a cover grammar and then reparse once we encounter ,, ;, in, or of.

[rbuckton]

I've updated the lookahead to [lookahead != `using` `const`] to further disambiguate between for (using in x) vs for (using const y in x). We can refactor ForDeclaration if necessary, but I'd rather reduce the number of disruptive changes to the spec text to only that which is absolutely necessary to achieve the syntax and semantics of the proposal.

[waldemarhorwat]

Given a choice between a complex lookahead and a plain production, the plain production is much better. The let and async of lookaheads are hard to get rid of for practical reasons, but the using const lookahead isn't needed and shouldn't be added.

I don't see any significant problem with SDOs and abstracting LetOrConstForDeclaration out of ForDeclaration. If there is one, what is it?

[rbuckton]

• BoundNames — ForDeclaration : LetOrConst ForBinding
• IsDestructuring — ForDeclaration : LetOrConst ForBinding
• ForDeclarationBindingInitialization — ForDeclaration : LetOrConst ForBinding
• ForDeclarationBindingInitialization — ForDeclaration : LetOrConst ForBinding
• Static Semantics: Early Errors (for ForInOfStatement) — ForInOfStatement : `for` `(` ForDeclaration `in` Expression `)` Statement
• ForInOfLoopEvaluation — ForInOfStatement : `for` `(` ForDeclaration `in` Expression `)` Statement
• ForIn/OfBodyEvaluation — has Assert: lhs is a ForDeclaration
• Additions and Changes That Introduce Incompatibilities with Prior Editions — References ForDeclaration in relation to for..in

Wouldn't we need to change all of these to support that refactor?

[waldemarhorwat]

These seem like trivial updates. They're either just trivial passthroughs or change ForDeclaration to LetOrConstForDeclaration.

[waldemarhorwat]

ForStatement:

"Let LexicalDeclaration Contains UsingConst, return contains-using-const." → "If LexicalDeclaration Contains UsingConst, return contains-using-const."

Also note that this definition is fairly brittle. It works for now but will break if do-expressions land in the language.

[waldemarhorwat]

The refactoring hazard issue I raised at the meeting hasn't been addressed yet.

{
 using const x = ...;
 using const y = ...;
 z();
}

has observably different behavior when exceptions are thrown from:

{
 using const x = ...;
 {
  using const y = ...;
  z();
 }
}

We should make the behavior the same. One way to do this is to avoid merging multiple errors from the same scope together into a single AggregateError and instead always nest errors one at a time. If several resource disposers in the same scope throw errors, the last-destroyed one should create a record whose cause points to the record of the previously-destroyed one whose cause points to … and so on. This would fix the refactoring hazard above.

[ljharb]

Why should we make them the same?

[bakkot]

Because a reader of those two snippets of code would not expect them to have different behavior.

[ljharb]

Why wouldn't they? Two different scopes have different behavior wrt const/let, and certainly would with the scope of using. Why is it reasonable to assume that adding curly braces around code leaves the behavior unchanged?

[bakkot]

Well, let me make a weaker claim: I definitely would not expect those two snippets of code to have different behavior. I strongly suspect many readers will share my intuition, regardless of how reasonable it is.

[mhofman]

Just to clarify my understanding, it's only the shape of the error that would diverge, right? All disposal would still be performed, in the same order, regardless of any error occuring during disposal. And all errors would still be available, just through a different path (flat errors vs errors+cause).
I personally do not believe it's that much of an hazard if that's the case.

[Jack-Works]

The let and async of lookaheads are hard to get rid of for practical reasons, but the using const lookahead isn't needed and shouldn't be added.

Since it already has to lookahead, I guess it's not too problematic for implementors to add another case

[rbuckton]

Just to clarify my understanding, it's only the shape of the error that would diverge, right? All disposal would still be performed, in the same order, regardless of any error occuring during disposal. And all errors would still be available, just through a different path (flat errors vs errors+cause).

That is correct, the only difference would be in the shape of the AggregateError. All other errors would still be reachable.

[rbuckton]

@waldemarhorwat: I'd appreciate another look at the specification text prior to the September meeting, if possible.

[waldemarhorwat]

Has the refactoring hazard issue been fixed? See the proposed fix above, which as far as I can tell has not been implemented.

[waldemarhorwat]

The jagged AggregateError fix has not been implemented in the version of the spec I can see.

[rbuckton]

The jagged AggregateError fix has not been implemented in the version of the spec I can see.

The change can be found here and is waiting for review: #117

[waldemarhorwat]

Thank you for the link. The proposal looks good with the change in #117.

[rbuckton]

#117 has merged, along with a few other minor issues noticed by @zloirock. @waldemarhorwat, should I consider this sign-off on the proposal specification text?