Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do we need new syntax? #76

Closed
mhofman opened this issue Oct 25, 2021 · 54 comments
Closed

Do we need new syntax? #76

mhofman opened this issue Oct 25, 2021 · 54 comments

Comments

@mhofman
Copy link
Member

mhofman commented Oct 25, 2021

While reviewing the latest state of this proposal the other day, it struck me that, as the readme mentions, the mechanism to perform block scope finalization already exists in JavaScript today in the form of iterators and for-of statements. This can be leveraged to build a purely API based approach to explicit resource management following closely the patterns adopted by this proposal:

for (const { using } of Disposable) {
  const resource = using(getResource());
  resource.doSomething();
  const other = using(resource.getOther());
  const stuff = other.doStuff();
  using(() => cleanUpStuff(stuff));
} // automatically cleanup, even when something throws

I actually spent this weekend implementing such a library to demonstrate that all use cases covered by the readme can seemingly be adapted to this approach, without the need to add any new syntax to the language.
https://github.com/mhofman/disposator/

While a for-of statement can be awkward at first, the code stays fairly readable. A similar approach was proposed in #10. As discussed there, JavaScript iterators are essentially the dispose and cursor patterns mixed together, and what this approach does is to neuter the cursor pattern to only leave the dispose pattern.

With a widely adopted library or built-in API, I believe this approach could become pretty natural. I also believe the block scope is more explicit in some regard than the current direction of the syntax additions, especially around the interleaving points for asynchronous code. See #68

One limitation of using for-of is that errors thrown during iterator close are shadowed by errors thrown during the iteration block (with no way to detect the iterator close is due to a throw). I would argue this is a separate problem that we should consider investigating (e.g. we could add an extra argument to iterator.return()). This is similar (but opposite) of errors thrown in a finally block shadowing an error thrown in the try block.

@rbuckton
Copy link
Collaborator

rbuckton commented Oct 31, 2021

While novel, there are several things that concern me with this approach:

  1. Abusing for..of for non-iteration purposes
  2. Does not meet goals for Scoping, Lifetime, and Reducing Footguns
  3. Action-at-a-Distance of using in expressions

Abusing for..of

This abuses for..of for something that isn't precisely iteration, which could easily be confusing for anyone unfamiliar with the code or the library in question. By conflating disposal with for..of, it will be more difficult to search for and find answers relating to the behavior on MDN, StackOverflow, etc. I'm very much against reusing for..of for this, as it feels like a hammer in search of a nail, when what we have is a screw.

JavaScript iterators are essentially the dispose and cursor patterns mixed together, and what this approach does is to neuter the cursor pattern to only leave the dispose pattern.

While that's certainly true, every language referenced as Prior Art in the explainer has both a cursor pattern and a dispose pattern:

  • C#
    • Cursor — IEnumerable, IEnumerator, yield return, yield break.
    • Dispose — IDisposable, using (...) {}, using x = ....
  • Java
    • Cursor — Iterable, Iterator.
    • Dispose — java.io.Closable, try-with-resources.
  • Python
    • Cursor — __iter__, __next__.
    • Dispose — __enter__, __exit__, with.

The fact that JavaScript conflates these two patterns does not mean it is necessarily the correct approach, and is one of the reasons that this proposal adds @@dispose to built-in iterators as a forwarding call to this.return().

Scoping, Lifetime, and Reducing Footguns

This also does not meet the stated goals regarding coupling scope with lifetime and reducing user footguns. While the using expression in your example is scoped to the for..of statement, it can be used anywhere within the block:

for (const { using } of disposable) {
    const x = using(f()); // scoped to for..of
    {
        const y = using(g()); // oops, still scoped to for..of
    }
    // NOTE: y was not disposed.
}

Instead, a developer must be rigorous about ensuring they don't reuse a using inside of a nested block. This also introduces a local binding for using that can escape its block (i.e., () => using), which could cause even more confusing scoping vs. lifetime issues.

Action-at-a-Distance

One of the discussions at an earlier TC39 meeting was around using an expression form for disposal, and there were concerns raised about code that might bury the expression somewhere in a complex operation. Such an expression would have action-at-a-distance side effects that are not obvious at first glance. Having an explicit declarative form mitigates that concern.


In general I would prefer to have distinct syntax with clear rules around lifetime and scoping vs. leveraging an existing feature that doesn't precisely map to the expected semantics:

// source
{
  ...
  using const x = expr1;
  ...
  {
    ...
    using const y = expr2;
    ...
  }
}

// helper (ES2015+)
var __usingScope = function* () {
  var disposables = [];
  var scope = {
    error: undefined,
    using(resource) {
      if (resource !== null && resource !== undefined) {
        const dispose = resource[Symbol.dispose];
        if (typeof dispose !== "function") throw new TypeError();
        disposables.push({ resource, dispose })
      }
      return resource;
    }
  };
  try {
    yield scope;
  }
  finally {
    var errors = [];
    for (var i = disposables.length - 1; i >= 0; i--) {
      try {
        disposables[i].dispose.call(disposables[i].resource);
      }
      catch (e) {
        errors.push(e);
      }
    }
    if (errors.length) throw new AggregateError(errors, undefined, scope.error); // sets cause
    if (scope.error) throw scope.error.cause; // throws cause
  }
};

// transposed
for (const scope_1 of __usingScope()) try {
  ...
  const x = scope_1.using(expr1);
  ...
  for (const scope_2 of __usingScope()) try {
    ...
    const y = scope_2.using(expr2);
    ...
  } catch (e) { scope_2.error = { cause: e }; }
} catch (e) { scope_1.error = { cause: e }; }

Its possible that a transpiler could leverage such a feature for downleveling to ES2015+ and still maintain this proposal's goals, since the user wouldn't have access to the generated scope.using function.

@rbuckton
Copy link
Collaborator

rbuckton commented Oct 31, 2021

One limitation of using for-of is that errors thrown during iterator close are shadowed by errors thrown during the iteration block (with no way to detect the iterator close is due to a throw). I would argue this is a separate problem that we should consider investigating (e.g. we could add an extra argument to iterator.return()). This is similar (but opposite) of errors thrown in a finally block shadowing an error thrown in the try block.

Its not so simple to just add another argument to .return (or .throw). How would you delegate the aggregated errors to a generator? How would that affect existing code, especially if someone writes a custom Iterator using next/throw/return? for..of is very limited when it comes to working with generators: it can't get the return value from the generator and can't send in values to yield (aside from undefined). I'm not sure changing the iteration protocol is the right way to go, especially to overload it with even more responsibilities to suit a use case for which it was not intended.

@bergus
Copy link

bergus commented Oct 31, 2021

@rbuckton While I totally agree with what you wrote in "Abusing for … of", I don't think the arguments from "Scoping, Lifetime, and Reducing Footguns" are specific to this syntax, they always apply. Sure, the OP's library has a using() method that allows adding disposers dynamically, but this is not necessary. One could just as well do something where the resources are scoped to the block, like

for (const resource of Disposable(getResource())) {
  resource.doSomething();
  for (const other of Disposable(resource.getOther())) {
    const stuff = other.doStuff();
    
  }
} // automatically cleanup, even when something throws

And scopes never did coincide with lifetimes in the presence of closures. I doubt that is a problem we could solve without introducing something like Rust's movement semantics… Regardless what syntax for resource management we come up with.

Finally, the example you gave where y is only disposed at the end of the outer block is actually quite common. There are use cases where you need something like that, and Python has the builtin contextlib.ExitStack for this purpose:

{
    using const stack = new ExitStack();
    const x = stack.push(f()); // scoped to the block
    let y = null;
    if () {
        y = stack.push(g()); // still scoped to the outer block, on purpose
    }
    // y is still usable here
} // x and y are disposed here

And yes, of course it's a footgun to call stack.push outside of the block (or passing it around etc), but IIRC it would throw when you try to use it after the stack has been disposed.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 1, 2021

Your example using for (const resource of Disposable(...)) is something I'd considered as a workaround for years now. It suffers from some of the same issues as the previous try using (const x = ...) syntax in that it requires a new block scope if you need to evaluate code between two resources.

And scopes never did coincide with lifetimes in the presence of closures. I doubt that is a problem we could solve without introducing something like Rust's movement semantics… Regardless what syntax for resource management we come up with.

In C#, the resource lifetime and scope are stilled tied to its block regardless of closures. A callback that is invoked while the resource is still "alive" can access the resource. If that callback is invoked after the resource has been disposed, the binding for the resource is still reachable but its most likely that accessing properties or methods on the object will throw an ObjectDisposedException.

I've been considering the possibility of adding an entry to the binding in the environment record to invalidate the binding, so that accessing the binding once it leaves scope would throw an error not unlike accessing an uninitialized const binding. The problem is that, even if you invalidate the binding, its still possible to copy it to another value so that it can escape the scope:

let y;
{
  using const x = getX();
  y = x;
} // x is disposed
y; // can still access the disposed value from 'x'

Since this isn't a 100% foolproof safeguard, I'm not sure its worth pursuing. In C#, its up to the producer of the Disposable to throw if their object is not in a usable state after being disposed. This also allows some disposables to be reused (such as a database connection with an .Open() method), so its more flexible.

Finally, the example you gave where y is only disposed at the end of the outer block is actually quite common. There are use cases where you need something like that, and Python has the builtin contextlib.ExitStack for this purpose: [...]

With ExitStack you are opting in to this behavior. You can do the same thing with Disposable with some additional user code:

{
  using const void = new Disposable(() => Disposable.from(stack)[Disposable.dispose]()); // scoped to the block
  const stack = [];
  const push = resource => {
    stack.push(resource);
    return resource;
  }; 

  const x = f();
  stack.push(x);
  
  let y = null
  if (...) {
    y = g();
    stack.push(y);
  }
  // y is still usable here
} // x and y are disposed here

I've wanted to keep the API surface of Disposable fairly simple (i.e., essentially immutable with only a Symbol.dispose method), but if something like ExitStack (and AsyncExitStack) seems valuable enough it might be worth adopting either as part of Disposable or as a separate class.

For the base case, however, we want the language to guide the user to the most correct behavior by default (i.e., let the user fall into a "pit of success").

@mhofman
Copy link
Member Author

mhofman commented Nov 1, 2021

By conflating disposal with for..of, it will be more difficult to search for and find answers relating to the behavior on MDN, StackOverflow, etc.

While I agree the for...of syntax is not quite natural, I'm not sure it impair the ability to find answers. I doubt you can find meaningful answers for just the "using const" (nor "for of") terms, but if you see the "Disposable" term, you'll add it to the search and find related answers.

Scoping, Lifetime, and Reducing Footguns

Like @bergus, I disagree with the premise that it's unacceptable to call using in a sub block, or that it's unclear the resource would be disposed when exiting the defining block.

I have however realized that I forgot to invalidate the using function after the aggregated resource has been disposed. That's now fixed by mhofman/disposator@616025d.

One could just as well do something where the resources are scoped to the block

Very true, added the shorthand in mhofman/disposator@120c2ec

for (const res of Disposable.using(getResource())) {

}

Action-at-a-Distance

In my opinion a declarative form introduces other issues, mainly that the syntax is awkward when you want to ignore the result or destructure it. In general anything left of an = impacts the identifiers in that position. The using const syntax however has an effect to the right hand side of the assignment. This is also a surprising action at distance in its own way, and the mitigation is to disallow patterns that are normally valid in LHS (let, destructure)

every language referenced as Prior Art in the explainer has both a cursor pattern and a dispose pattern

To be clear, I am not 100% opposed to new syntax, I just believe the current binding assignment syntax introduces too much complexity and has too many unresolved issues. On top of the above, I'm still concerned with the hidden async interleaving points.

I'm actually wondering if there isn't a way to support RAII style with a try-with statement block for the dispose pattern, combined with an aggregate runtime helper. That would resolve the concern you have with the abuse of for-of, and my concerns with the using const syntax. I know this is a step back, but I'm not sure if runtime aggregation has been explored with such an approach

try with (const res of getResource()) {

}
try with (const { using } of Disposable) {
  const res = using(getResource());
}

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 1, 2021

I'm looking to advance this proposal based on discussions I've had with @syg around shared structs and possible future concurrent JS capabilities. I'd really like a clean and concise syntactic solution for handling thread synchronization and locking, as concurrency is going to be complicated enough without wrapping everything in a for..of:

shared struct class State {
  ready = false;
  ...
}
const mut = new Mutex();
const cv = new ConditionVariable();
...
{
  using const lck = mut.lock();
  cv.wait(lck, () => state.ready);
  ...
}

To be clear, I am not 100% opposed to new syntax, I just believe the current binding assignment syntax introduces too much complexity and has too many unresolved issues. On top of the above, I'm still concerned with the hidden async interleaving points.

I'm not clear on where the complexity is. Are you talking about spec complexity or the developer experience?

try with (const res of getResource()) {

}

We just got rid of try using () {} in favor of using const. Are you arguing that we should go back to that? There were more than a few detractors of try using, and using const was the consensus the last time this came to committee (prior to the most recent October meeting). I've also heard feedback from current and past members of the C# team that C#'s new using declaration form is much preferred over the older using (Type x = expr) {} block syntax, for the same reasons.

@mhofman
Copy link
Member Author

mhofman commented Nov 1, 2021

Its not so simple to just add another argument to .return (or .throw). How would you delegate the aggregated errors to a generator? How would that affect existing code, especially if someone writes a custom Iterator using next/throw/return? for..of is very limited when it comes to working with generators: it can't get the return value from the generator and can't send in values to yield (aside from undefined). I'm not sure changing the iteration protocol is the right way to go, especially to overload it with even more responsibilities to suit a use case for which it was not intended.

Dealing with suppressed errors is a pain, and that's why I believe the problem should be fixed regardless of this proposal. I'm struggling hard with them in my library, and it's plain impossible to do proper error handling in iterators.

At this point I doubt we can change the semantics of suppressed errors, especially in iterators. Aka a finally thrown error will always suppress a try error, and if an iterator is closed due to an error, an error thrown in the iterator's return will always be suppressed. However what we can do is surface the "cause" error, and offer facilities for code that cares to handle the situation.

A strawman I have in mind:

  • Surface a "cause" error in a finally through a finally.error syntax addition
  • Add an extra argument to .return and have the iterator close steps provide the cause (.throw is surprisingly never invoked on its own by the spec, only forwarded).
  • Surface an iterator close error in a generator finally statement through the above finally.error

That still doesn't resolve the issue of surfacing the suppressed error after the iterator close, but at least the iterator logic has more information. For example it could try to attach a suppressed property to the causing error (which may fail on non-objects, or frozen objects), or it could annotate with a side table. I wish we could change the iterator close to suppress the causing error like for try-finally, so that we'd have the opportunity to throw a new wrapping error, but I'm worried some obscure piece of code somewhere is relying on the suppression. Maybe we could make it work through assignment to finally.error = wrappedError.

@mhofman
Copy link
Member Author

mhofman commented Nov 1, 2021

Are you talking about spec complexity or the developer experience?

Both. The spec changes are extremely broad, with some very surprising interactions as @waldemarhorwat went through last week. And IMO questions like #78 do show the developer ergonomics aren't quite right yet either.

There were more than a few detractors of try using

I need to go through the notes and issue history, I was trying to get a feel of where the objections stemmed from. As I mentioned I don't know if resource aggregation with single block syntax was explored to resolve the nesting issues. I'd also propose to avoid any assignment style syntax inside the clause, like for-of, to steer away from confusion regarding what is the resource tracked (right of of, not left).

going to be complicated enough without wrapping everything in a for..of:

You have to wrap a disposable in a block. The question is whether a new explicit block is needed, or if re-using a regular statement's block is acceptable. My concern is that re-using existing blocks is a problem in the case of hidden async interleaving points. Aka the following should not be allowed, and I doubt there is a way to syntactically mark such block statements:

if (condition) {
  using async const res = getResource()
}

If you need to introduce a dedicated block statement, then there isn't much difference if that statement is for-of or try-with-of, or something else.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 1, 2021

Are you talking about spec complexity or the developer experience?

Both. The spec changes are extremely broad, with some very surprising interactions as @waldemarhorwat went through last week. And IMO questions like #78 do show the developer ergonomics aren't quite right yet either.

Waldemar brought up three cases he considered surprising:

  • Block refactoring
  • Tail calls
  • for..in vs for..of

Block refactoring

I've been discussing the block refactoring concern in #74. @ljharb seems to agree that the current approach is correct and what Waldemar considers surprising here is the correct representation.

Tail calls

Tail recursing with using const is equivalent to for..of using the example in the OP:

// with using const
switch (...) {
  case 1:
    return foo(1); // not tail recursive due to `using const` in CaseBlock
  case 2: 
    using const x = ...;
    return foo(2); // not tail recursive due to `using const` in CaseBlock
  case 3:
    return foo(3); // not tail recursive due to `using const` in CaseBlock
}

// with for..of
for (const { using } of Disposable) {
  switch (...) {
    case 1:
      return foo(1); // not tail recursive due to containing `for..of`
    case 2: 
      const x = using(...);
      return foo(2); // not tail recursive due to containing `for..of`
    case 3:
      return foo(3); // not tail recursive due to containing `for..of`
    }
}

There's nothing surprising to me here, tail call semantics are identical. The only spec complexity is that HasCallInTailPosition needs to check whether the Block or CaseBlock directly contains a using const declaration.

If you wanted prevent case 2 from breaking tail recursion, you would need to move the using const (or for..of) into a case-local block:

// with using const
switch (...) {
  case 1:
    return foo(1); // tail recursive position
  case 2: {
    using const x = ...;
    return foo(2); // not tail recursive due to `using const` in Block
  }
  case 3:
    return foo(3); // tail recursive position
}

// with for..of
switch (...) {
  case 1:
    return foo(1); // tail recursive position
  case 2: 
    for (const { using } of Disposable) {
      const x = using(...);
      return foo(2); // not tail recursive due to containing `for..of`
    }
  case 3:
    return foo(3); // tail recursive position
}

for..in vs for..of

Waldemar claims that something is surprising here:

for (using const x in y); // syntax error, `using const` not allowed in `for..in`
for (using const x of y); // no syntax error

I would argue that allowing using const in for..in would be surprising, as the only think a for..in with a using const could do is throw a runtime error, since neither string nor symbol can be an Object with a Symbol.dispose method.


There were more than a few detractors of try using

I need to go through the notes and issue history, I was trying to get a feel of where the objections stemmed from. As I mentioned I don't know if resource aggregation with single block syntax was explored to resolve the nesting issues. I'd also propose to avoid any assignment style syntax inside the clause, like for-of, to steer away from confusion regarding what is the resource tracked (right of of, not left).

for..of is not assignment style syntax. Each iteration produces a new per-iteration environment with a new binding for the declared variable. There are no reassignments. That said, I've debated as to whether to allow using const in for..of. Earlier iterations of the proposal did not allow it, but it is a convenient way to work with disposable resources produces by a generator. I don't have a strong feeling either way.


going to be complicated enough without wrapping everything in a for..of:

You have to wrap a disposable in a block. The question is whether a new explicit block is needed, or if re-using a regular statement's block is acceptable. My concern is that re-using existing blocks is a problem in the case of hidden async interleaving points. Aka the following should not be allowed, and I doubt there is a way to syntactically mark such block statements:

if (condition) {
  using async const res = getResource()
}

If you need to introduce a dedicated block statement, then there isn't much difference if that statement is for-of or try-with-of, or something else.

You only have to wrap the disposable in a block at the top level of a SourceFile. Every other place you could legally write using const has either an implicit or explicit block scope:

// legal:
function f() {
  using const x = ...; // ok
}
const g = () => {
  using const x = ...; // ok
};
if (condition) {
  using const x = ...; // ok
}
class C {
  static {
    using const x = ...; // ok
  }
  method() {
    using const x = ...; // ok
  }
}

// illegal:
if (condition)
  using const x = ...; // already illegal. `Statement` does not include `using const`.

@mhofman
Copy link
Member Author

mhofman commented Nov 1, 2021

Thanks for reminding me of the concerns raised by Waldemar. I agree those are workable.

for..of is not assignment style syntax. Each iteration produces a new per-iteration environment with a new binding for the declared variable. There are no reassignments.

Exactly. And my reading of the previous try using was that it used assignment syntax try using (const res = ...). What I'm saying is we should steer clear of that with something like try with (const res of ...).

Every other place you could legally write using const has either an implicit or explicit block scope

And that is my problem, as that block now may have a hidden async interleaving point when it exits.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 1, 2021

And IMO questions like #78 do show the developer ergonomics aren't quite right yet either.

Multiple members of the committee have different intuitions as to what should be disposed (the value being destructured or the destructured bindings), so I'm not sure there's a right answer aside from outright banning it. Destructuring isn't too bad if consensus is that the thing that is disposed is the result of the Initializer (i.e., the value being destructured).

However, if the destructured bindings are what is to be disposed, then destructuring would be inherently unsafe:

// with `using const`
using const { x, y } = ...;

// with `for..of`
for (const { using } of Disposable) {
  const { x, y } = ...;
  using(x);
  using(y);
}

In the above example if x is not null, undefined, or a disposable object, then y will never be disposed.

Unless we can get consensus on having the thing that is disposed be the result of evaluating Initializer, I don't think destructuring is safe. If we can get consensus on that, then we don't need using const void = since you could just write using const {} = .

Also in #78 there has been some discussion of dropping the const keyword, which would further block destructuring since using [x] = ... is already legal JavaScript.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 1, 2021

And that is my problem, as that block now may have a hidden async interleaving point when it exits.

I would be happy to add an explicit marker for the async case, if that's necessary to achieve consensus:

// postfix `await using` at end of block;
{
  // note: switch from `using await` to `using async`
  using async const x = ...;

} await using; // explicit marker for block

// postfix probably unnecessary for async function/generator/arrow/method, since function body exit 
// (or `return`) is already a potential async interleave point and is essentially unobservable
async function f() {
  using async const x = ...;

} // no explicit marker for body

While async dispose has its use cases, they are far fewer than synchronous dispose. I'm willing to incur a syntax burden for a trailing marker as long as it only affects the async case.

Regarding try using, @leobalter was adamant about not wanting try as part of this when I switched the proposal to align with Java's try-with-resources syntax. The previous try using declaration was a compromise to make the syntax more palatable. Switching to RAII-like declarations via using const was a bigger win with broader support within the committee since the syntax was more convenient and didn't use try at all.

My biggest concern with try with or try using is that we would have to rehash all of these arguments again.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 2, 2021

@mhofman I would note that, while I still think syntax is the correct direction for this proposal, I did just publish an update to @esfx/disposable with a .scope() static method inspired by your Disposable[Symbol.iterator] example (though it adds a .fail(error) method to the scope object based on the __usingScope helper in my earlier comment).

I also added DisposableStack and AsyncDisposableStack to @esfx/disposable as approximations of Python's ExitStack and AsyncExitStack:

class DisposableStack implements Disposable {
  // Based on `ExitStack.enter_context`/`ExitStack.push`/`ExitStack.callback`
  enter<T extends Disposable | (() => void) | null | undefined>(value: T): T;
  
  // Based on `ExitStack.pop_all`
  move(): DisposableStack;

  // Based on `ExitStack.close`
  [Disposable.dispose](): void; // NOTE: would be `Symbol.dispose` if shimmed  
}

Though I'm not sure if the functionality should be merged into Disposable/AsyncDisposable, as I was hoping to keep the internal state of those objects immutable (with the only exception being the ability to dispose of them). Also, while a minor point, I was hoping to avoid conflicts in TypeScript when using Disposable as an interface with only a [Symbol.dispose]() method. Adding additional methods to Disposable would result in confusing naming (a. la Promise<T> vs PromiseLike<T> today), though that might be an argument for changing the global class names in this proposal to DisposableStack and AsyncDisposableStack instead.

@leobalter
Copy link
Member

For the records I retract any of my objections to try using is that's necessary. That doesn't mean I'm +1 but I can't commit with time to support for any alternatives here.

@mhofman
Copy link
Member Author

mhofman commented Nov 2, 2021

Also, while a minor point, I was hoping to avoid conflicts in TypeScript when using Disposable as an interface with only a [Symbol.dispose]() method.

Right, I definitely overloaded the concepts of a Disposable interface with a concrete Disposable constructor which implements aggregation semantics. The type declarations my package include do make the difference, but that's because I manually typed everything.

It's still unfortunate we have to resort to this type of manual try/catch and .fail() to work around the way iterator close suppresses errors.

I would be happy to add an explicit marker for the async case

How would such a marker work with non standalone statement blocks? One interaction that comes to mind is with if-else.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 2, 2021

I would be happy to add an explicit marker for the async case

How would such a marker work with non standalone statement blocks? One interaction that comes to mind is with if-else.

The IfStatement production uses the Statement non-terminal, which includes Block, so it would be something like:

// NOTE: in [+Await]
if (...) {
  using async const x = ...;
} await using
else {
}

// or 

// NOTE: in [+Await]
if (...) {
  using async const x = ...;
} await using else {
}

Another option would be to go back to the mixed syntax with both a block form and a declaration form, but disallow the declaration form for async disposal:

// non-async block form
try using (const x = expr, y = expr) {
}
try using (expr) { // or just `try using (void = expr)`
}

// async block form
try using await (const x = expr, y = expr) {
}
try using await (expr) { // or just `try using await (void = expr)`
}

// non-async declaration form
using const x = expr, y = expr;
using const void = expr;

// no async declaration form 🙁

It has the upside of having an explicit marker at the top of the block to indicate the interleave point at the end of the block, but the downside of not having parallel syntax across both async and non-async cases.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 2, 2021

Another option would be to introduce a coupled block syntax, i.e.:

using {
  using const x = ...;
}

async using {
  using await const x = ...; 
  // or just make `using const` work with @@asyncDispose in an `async using` block?
}

Though I'm not sure I like this approach, as it complicates the majority case (synchronous dispose) to handle the minority case (asynchronous dispose). I like the convenience of writing using const x = ...; in any block, otherwise we're back to issues with scope-vs-lifetime ambiguity:

// (a)
using {
  using const x = ...; 
  // (b)
  {
    using const y = ...; // is this an error or is `y`'s lifetime scoped to (a)?
  }
}

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 2, 2021

One limitation of using for-of is that errors thrown during iterator close are shadowed by errors thrown during the iteration block (with no way to detect the iterator close is due to a throw). I would argue this is a separate problem that we should consider investigating (e.g. we could add an extra argument to iterator.return()). This is similar (but opposite) of errors thrown in a finally block shadowing an error thrown in the try block.

We've previously discussed error suppression in #19 and #63, and more recently in #74. I also sketched out a rough concept for meta properties to access suppressed exceptions in this comment, though @ljharb pointed out that meta properties wouldn't work with rejected Promises.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 2, 2021

Also, in #74 we discussed the possibility of passing the exception as an argument to @@dispose/@@asyncDispose.

@mhofman
Copy link
Member Author

mhofman commented Nov 3, 2021

In my mind the problem is not in catch blocks, but in finally blocks where even if the finally logic is defensive and catches its own errors, it cannot communicate both errors. In that sense, rejected promises have a path where promise.finally would accept an optional argument of any previous error thrown.

Basically my preference would be:

try {
  throw new Error('Initial');
} finally {
  const finallyError = finally.error;
  try {
    cleanup();
  } catch (err) {
    if (finallyError) {
      throw new AggregateError([err, finallyError]);
    } else {
      throw err;
    }
  }
}

Promise.reject(new Error('initial'))
  .finally((finallyError) => {
    try {
      cleanup();
    } catch(err) {
      if (finallyError) {
        throw new AggregateError([err, finallyError]);
      } else {
        throw err;
      }
    }
  });

And for this to be consistent, for-of would pass the iteration block error to the iterator .return and suppress the iteration block error with any iterator close error, so that the following would work too:

function * foo () {
  try {
    yield;
  } finally {
    const finallyError = finally.error;
    try {
      cleanup();
    } catch(err) {
      if (finallyError) {
        throw new AggregateError([err, finallyError]);
      } else {
        throw err;
      }
    }
  }
}

for (const _ of foo()) {
  throw new Error('Initial');
}

I'm actually wondering if we could not simply introduce:

try {
} finally (err) {

}

Anyway this is way off-topic, but I'd be happy to continue this discussion somewhere else.

@ljharb
Copy link
Member

ljharb commented Nov 3, 2021

What would finally.error be if no error was thrown? It can’t be undefined, because you can throw that.

@mhofman
Copy link
Member Author

mhofman commented Nov 3, 2021

Yeah I know there is that pesky little detail. I believe @rbuckton proposed .hasError for those who care about crazy people throwing undefined.

Even crazier:

try {
  throw undefined;
} finally (...errors) {
  if (errors.length) {
    console.log('thrown', errors[0]);
  }
}

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 3, 2021

Bubbling suppressed exceptions across try..catch..finally is a tricky proposition. It would require significant spec work to make it viable. finally isn't the only clause that would care about suppressed exceptions either:

try {
  try {
    throw new Error("a");
  }
  finally {
    finally.error; // Error("a")
    throw new Error("b");
  }
}
catch (e) {
  // e: Error("b")
  // any way to see Error("a")?
}

@mhofman
Copy link
Member Author

mhofman commented Nov 3, 2021

I don't think it needs to cross any nesting boundary. It's to each local error handling logic to decide what is best to do.

@mhofman
Copy link
Member Author

mhofman commented Nov 3, 2021

I've seen plenty of code that does:

let e;
try {
  doStuff();
  doMoreStuff();
} catch(err) {
  e = err;
} finally {
  if (e) {
   // something
  } else {
    //something else
  }
}

@ljharb
Copy link
Member

ljharb commented Nov 3, 2021

@mhofman a metaproperty for hasError wouldn't work for promises.

It's not for us to decide that throwing undefined (or null) is improper; it's something the language has always allowed and must always do.

@mhofman
Copy link
Member Author

mhofman commented Nov 3, 2021

But for promises an optional argument to the finally callback can definitely be differentiated:

promise.finally((...errors) => console.log(errors.length));

@ljharb
Copy link
Member

ljharb commented Nov 3, 2021

Sure - via arguments.length as well - but then you've got a pretty dramatic difference between how it's done with try/catch vs with Promises, and we've tried pretty hard to minimize differences there.

@mhofman
Copy link
Member Author

mhofman commented Nov 3, 2021

Which is why I suggested this:

try {
  throw undefined;
} finally (...errors) {
  if (errors.length) {
    console.log('thrown', errors[0]);
  }
}

If we make the errors expression in the finally statement optional for backwards compatibility, we're free to also allow the "rest" form for code that wants to support undefined errors

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 3, 2021

There's another issue with using for..of for this that I discovered after discussing TypeScript's control-flow analysis in #74 (comment): TypeScript's control-flow analysis for loops assumes any loop body might not be executed as an iterable might not have any elements:

let a: string | undefined;
for (const x of iterable) {
  a = "foo";
}
a; // a: string | undefined

This means that we can't do CFA accurately using the for..of approach:

let a: string | undefined;
{ // normal block
  a = "foo";
}
a; // a: string

let b: string | undefined;
for (const { using } of Disposable) {
  b = "foo";
}
b; // b: string | undefined

I'm not sure there would be a consistent way of handling CFA without new syntax. While this may seem like a TypeScript-specific issue, that's still a fairly sizable population of users we'd like to be able to support with this feature.

@bakkot
Copy link

bakkot commented Nov 3, 2021

We're getting pretty far afield here, but: as a TypeScript user, I would be quite happy if TypeScript's analysis improved to the point where it could keep track of a "definitely nonempty" bit on at least some iterables, which would solve that particular problem (since in this use the analysis would be trivial, and so even a very conservative analysis would handle it). That's currently one of the main sources of my // @ts-ignore comments.

The TypeScript team has made similar improvements to the analysis in the past, so I'm hopeful that someday it'll get there. It would be a shame to introduce new syntax for reasons which became obsolete after some improvements to TypeScript's analysis. (Not to say there aren't other reasons for new syntax; I'm just hesitant to rely on this particular one.)

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 8, 2021

Note: I've opened #80 to discuss the possibility of a more comprehensive DisposableStack/AsyncDisposableStack API.

@rbuckton
Copy link
Collaborator

Another option would be to introduce a coupled block syntax, i.e.:

I've been considering this, and an option we could consider for the async dispose case is to split the using into two declarations for the async case:

async function f() {
  // (1) change `using await const` to `using async const`
  using async const x = ...;

  // (2) require an explicit `using await;` statement at the end of a block containing `using async const`
  using await;
}

With the following rules:

  • using await; is only allowed in [+Await].
  • If present, using await; must be the last statement of the block.
    • This means it's also an error to have more than one using await in the same block (nested or containing blocks are ok though).
  • It's an error to declare a using async const in a block that does not have a using await at the end of the same block.

This results in a marker that does not affect normal using const and makes it clear where the async interleave point is.

@rbuckton
Copy link
Collaborator

or, instead of using await; we could use something equally meaningful that isn't currently valid JS, such as await finally (since await using is already legal JS).

@ljharb
Copy link
Member

ljharb commented Jan 12, 2022

await using foo isn’t valid tho.

@rbuckton
Copy link
Collaborator

But what would you put in the place of foo? This would be an interleave point for the entire block. A single keyword statement would be ideal but isn't an option (because identifiers). A two-keyword statement is palatable, but I think three keywords would be a bit much given we already would have to write using async const (or using await const) for this.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2022

oh. then "using" wouldn't make sense anyways, because that's transitive.

@rbuckton
Copy link
Collaborator

oh. then "using" wouldn't make sense anyways, because that's transitive.

I'm not sure I understand what you mean by this statement.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2022

In prose, if i see the word "using", i expect there to be a word after it that answers the question "using what?"

@mhofman
Copy link
Member Author

mhofman commented Jan 12, 2022

I've been considering this, and an option we could consider for the async dispose case is to split the using into two declarations for the async case:

That would be an acceptable approach. It does however feel like a departure from the other block syntax where the control keywords is lexically outside the block. However if your goal is to allow mixing it with other block based statements like if-else, I'm not sure I see much of an alternative.

I also do like the switch to using async as it always struck me weird that the await keyword was not in fact awaiting anuything.

FYI, I'm still not convinced the syntax cost is justified, but at least it removes some of my strong objections.

@rbuckton
Copy link
Collaborator

rbuckton commented May 23, 2022

I am strongly in favor of syntax for this proposal for a number of reasons, some of which I've outlined in the updated README.md in the root of the proposal. I am also strongly opposed to abusing for..of for this as it serves to conflate two separate capabilities: Resource Management and Iteration.

I'm also strongly in favor of a terse syntax for synchronous dispose that supports RAII-style declarations, i.e.:

using const resource = acquireResource();
using const void = acquireLock();
// etc.

Given prior discussions regarding destructuring, I've opted to err on the side of caution and forbid destructuring. This potentially leaves us the ability to make synchronous resource management even more terse:

using resource = acquireResource();
using void = acquireLock();
// etc.

However, concerns have been raised repeatedly regarding the lack of a marker to indicate the async interleaving point in asynchronous resource management:

{
  using await const resource = acquireResource();
  // ...
} // resource is *asynchronously* disposed

This arguably necessitates some kind of block form to indicate the interleaving point. To that end, we have discussed various mechanisms for achieving this, i.e.:

{
  using async const resource = acquireResource();
  // ...
} using await; // NOTE: `await using` will not work because `{} await using` is already legal JS

While this does clearly illustrate the interleaving point, it seems a bit out of place in JS with the only similar statement being do {} while (...);, and even then the while has a trailing condition.

As an alternative, I am considering reviving the original syntax of the proposal in addition to the RAII-like using declarations for synchronous resource management:

// Synchronous dispose:

// UsingStatement[~Await]:
using (expr) { ... }
using (const x = expr) { ... }

// UsingDeclaration:
using x = ...;
using void = ...;

// Asynchronous dispose:

// UsingStatement[+Await]:
using await (expr) { ... }
using await (const x = expr) { ... }

If we go that route then there will be consistency between sync and async dispose via UsingStatement, as well as a simplified RAII form for sync dispose which is often the most-used form in my experience.

We could possibly consider a future proposal to introduce an async RAII-form, such as:

// opt in to async resource management:
using await {
  using const x = ...; // implicitly uses @@asyncDispose instead of @@dispose
}

@waldemarhorwat: You raised objections to using (...) {} in the past due to the need for a cover grammar. However, it seems that the Pattern Matching proposal is also pushing for advancement with a similar requirement (i.e., match(...) { }). If a satisfactory cover grammar is crafted, would you still be opposed to using (...) {}?

@rbuckton
Copy link
Collaborator

I've created #86 with the proposed changes to syntax in my previous comment.

@bergus
Copy link

bergus commented May 24, 2022

I love going forward with the terse using x = …; syntax, even if that means no destructuring. Not having the const keyword makes it explainable as being something different.
Can you add an FAQ to the readme, please: a) why no destructuring b) why no inline operator?

@rbuckton
Copy link
Collaborator

One final alternative I'm considering is this:

// synchronous `using` declarations
{
  using x = ...; // must be null, undefined, or have a [Symbol.dispose]()
  using void = ...; // must be null, undefined, or have a [Symbol.dispose]()
}

// asynchronous `using` declarations (in a +Await context)
{
  using x = ...; // must be null, undefined, or have a [Symbol.asyncDispose]() or [Symbol.dispose]()
  using void = ...; // must be null, undefined, or have a [Symbol.asyncDispose]() or [Symbol.dispose]()
} await using;

In this version there is no using await declaration. Instead, both sync and async using just use `using` BindingList `;`. To indicate that [Symbol.asyncDispose]() should be considered (and awaited), you must terminate the block with an await using on the same line. A trailing await using is not necessary in an AsyncFunctionBody, since there is already an implicit interleaving point when exiting the function.

This more accurately models the behavior since a using await const declaration doesn't actually perform an await at the declaration site.

This also would avoid resurrecting the discussions over using a cover grammar for using (...) { } or try-with-resources.

Examples:

// normal function, no implicit `await` at body
function f1() {
  using file = createFile();
  ...
} // calls `file?.[Symbol.dispose]()`

// async function, implicit `await` at body
async function f2() {
  using stream = getReadableStream();
  ...
} // calls `stream?.[Symbol.asyncDispose]()`

async function f3() {
  // no implicit `await` at end of Block even if we're in an async function
  {
    using x = createResource1();
    ...
  } // calls `x?.[Symbol.dispose]()`
  
  // explicit `await` at end of Block
  {
    using y = createResource2();
    ..
  } await using; // calls `y?.[Symbol.asyncDispose]()`
}

@bakkot
Copy link

bakkot commented May 25, 2022

As a matter of taste,

{
  using y = createResource2();
  ...
} await using; // calls `y?.[Symbol.asyncDispose]()`

seems worse than any variation of

try async (const y = createResource2()) {
  ...
} // calls `y?.[Symbol.asyncDispose]()`

If we're going to have a special block syntax, I would prefer the specialness be visible at the top of the block.

@rbuckton
Copy link
Collaborator

If we're going to have a special block syntax, I would prefer the specialness be visible at the top of the block.

That is in direct opposition to @erights's stated preference that the interleaving point be annotated at the bottom of the block.

@bakkot
Copy link

bakkot commented May 25, 2022

Well, this comment suggests going forward with try using but seeking a better syntax; my position is that a syntax which only makes the specialness of a block visible at the end of the block is not better.

@rbuckton
Copy link
Collaborator

Well, this comment suggests going forward with try using but seeking a better syntax; my position is that a syntax which only makes the specialness of a block visible at the end of the block is not better.

That comment indicated moving forward with try using when achieving Stage 2, but seeking out alternatives during Stage 2, which is what I am investigating currently.

@rbuckton
Copy link
Collaborator

If { ... } await using is controversial, an alternative would be using await { ... } (i.e., moving the modifiers to the top of the block). In this alternative, the syntax would be:

function f() {
  using id = expr; // @@dispose
  {
    using id = expr; // @@dispose
  }
}

async function f() {
  using id = expr; // @@asyncDispose/@@dispose

  {
    using id = expr; // @@dispose only
  } // no implicit Await
  
  using await {
    using id = expr; // @@asyncDispose/@@dispose
  } // implicit Await

} // implicit Await

NOTE: Here I'm using using await as a prefix form rather than await using to hopefully minimize an potential cover grammar.

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 7, 2022

As an FYI, we've chosen to postpone async using to a follow-on proposal to focus primarily on synchronous, RAII-style using declarations.

I still strongly believe syntax is warranted here. There are runtime semantics regarding error suppression that aren't easy to replicate purely with API without also breaking TCP since leveraging for..of changes the meaning of break and continue when refactoring. I also believe that the conciseness of using x = ... is a significant improvement over nested try..finally blocks or the boilerplate needed to emulate the intended semantics using for..of.

@rbuckton
Copy link
Collaborator

rbuckton commented Oct 5, 2022

@mhofman: At this point, would you still object to the introduction of new syntax in the form of using? I'd like to think that https://github.com/tc39/proposal-explicit-resource-management#relation-to-iterator-and-forof sufficiently covers the major themes in this issue, and if so would like to close this issue for housekeeping purposes.

@mhofman
Copy link
Member Author

mhofman commented Oct 5, 2022

Given the current scope of the proposal I think we are ok with the using syntax, however I want to stress that I have reservation with the current semantics of for await(using ... of ...) and the potential conflicts with future support of AsyncDisposable as explained in tc39/proposal-async-explicit-resource-management#1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants