-
Notifications
You must be signed in to change notification settings - Fork 29
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
Comments
While novel, there are several things that concern me with this approach:
Abusing
|
Its not so simple to just add another argument to |
@rbuckton While I totally agree with what you wrote in "Abusing 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 {
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 |
Your example using
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 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 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
With {
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 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"). |
While I agree the
Like @bergus, I disagree with the premise that it's unacceptable to call I have however realized that I forgot to invalidate the
Very true, added the shorthand in mhofman/disposator@120c2ec for (const res of Disposable.using(getResource())) {
}
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
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 (const res of getResource()) {
} try with (const { using } of Disposable) {
const res = using(getResource());
} |
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 shared struct class State {
ready = false;
...
}
const mut = new Mutex();
const cv = new ConditionVariable();
...
{
using const lck = mut.lock();
cv.wait(lck, () => state.ready);
...
}
I'm not clear on where the complexity is. Are you talking about spec complexity or the developer experience?
We just got rid of |
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 A strawman I have in mind:
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 |
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.
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
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 |
Waldemar brought up three cases he considered surprising:
Block refactoringI'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 callsTail recursing with // 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 If you wanted prevent case 2 from breaking tail recursion, you would need to move the // 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
}
|
Thanks for reminding me of the concerns raised by Waldemar. I agree those are workable.
Exactly. And my reading of the previous
And that is my problem, as that block now may have a hidden async interleaving point when it exits. |
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 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 Also in #78 there has been some discussion of dropping the |
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 My biggest concern with |
@mhofman I would note that, while I still think syntax is the correct direction for this proposal, I did just publish an update to I also added 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 |
For the records I retract any of my objections to |
Right, I definitely overloaded the concepts of a It's still unfortunate we have to resort to this type of manual try/catch and
How would such a marker work with non standalone statement blocks? One interaction that comes to mind is with |
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. |
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 // (a)
using {
using const x = ...;
// (b)
{
using const y = ...; // is this an error or is `y`'s lifetime scoped to (a)?
}
} |
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. |
In my mind the problem is not in 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, 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. |
What would finally.error be if no error was thrown? It can’t be undefined, because you can throw that. |
Yeah I know there is that pesky little detail. I believe @rbuckton proposed Even crazier: try {
throw undefined;
} finally (...errors) {
if (errors.length) {
console.log('thrown', errors[0]);
}
} |
Bubbling suppressed exceptions across 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")?
} |
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. |
I've seen plenty of code that does: let e;
try {
doStuff();
doMoreStuff();
} catch(err) {
e = err;
} finally {
if (e) {
// something
} else {
//something else
}
} |
@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. |
But for promises an optional argument to the finally callback can definitely be differentiated: promise.finally((...errors) => console.log(errors.length)); |
Sure - via |
Which is why I suggested this:
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 |
There's another issue with using 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 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. |
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 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.) |
Note: I've opened #80 to discuss the possibility of a more comprehensive |
I've been considering this, and an option we could consider for the async dispose case is to split the 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:
This results in a marker that does not affect normal |
or, instead of |
|
But what would you put in the place of |
oh. then "using" wouldn't make sense anyways, because that's transitive. |
I'm not sure I understand what you mean by this statement. |
In prose, if i see the word "using", i expect there to be a word after it that answers the question "using what?" |
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 I also do like the switch to FYI, I'm still not convinced the syntax cost is justified, but at least it removes some of my strong objections. |
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 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 As an alternative, I am considering reviving the original syntax of the proposal in addition to the RAII-like // 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 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 |
I've created #86 with the proposed changes to syntax in my previous comment. |
I love going forward with the terse |
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 This more accurately models the behavior since a This also would avoid resurrecting the discussions over using a cover grammar for 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]()`
} |
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. |
That is in direct opposition to @erights's stated preference that the interleaving point be annotated at the bottom of the block. |
Well, this comment suggests going forward with |
That comment indicated moving forward with |
If 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 |
As an FYI, we've chosen to postpone async 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 |
@mhofman: At this point, would you still object to the introduction of new syntax in the form of |
Given the current scope of the proposal I think we are ok with the |
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: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 toiterator.return()
). This is similar (but opposite) of errors thrown in afinally
block shadowing an error thrown in thetry
block.The text was updated successfully, but these errors were encountered: