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?