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

{} block can change a program's control flow when using is used #197

Closed
rixtox opened this issue Jul 22, 2023 · 9 comments
Closed

{} block can change a program's control flow when using is used #197

rixtox opened this issue Jul 22, 2023 · 9 comments

Comments

@rixtox
Copy link

rixtox commented Jul 22, 2023

Observe the following two programs:

{
    using x = X();
    using y = Y();
    console.log("done");
}

{
    using x = X();
    {
        using y = Y();
    }
    console.log("done");
}

function X(): Disposable {
    return { [Symbol.dispose]() {} };
}

function Y(): Disposable {
    return { [Symbol.dispose]() { throw new Error(); } };
}

Both programs would throw the same error, but the first program would print the log while the second won't.

This behavior may be surprising to many people as we are used to the fact that a {} block should not affect the control flow, but with using, {} can change your program's control flow.

@bakkot
Copy link

bakkot commented Jul 22, 2023

Yes, that's the point of it. We were aware of this when designing the feature and consider it to be acceptable.

@rixtox
Copy link
Author

rixtox commented Jul 22, 2023

@bakkot
Yes, that's the point of it. We were aware of this when designing the feature and consider it to be acceptable.

I don't know how to justify this control flow implication on {} block to be acceptable. The bahvior is very implicit and subtle. Even knowing how using works, reasoning about the behavior differences in the above simple examples can still take a few seconds for me.

What should I tell my colleagues who don't know about using to understand the control flow implications in a code review? I sent the example to a few colleagues, and none of them can imagine how the behavior difference can happen. IMO this behavior creates more problems than it solves.

@mhofman
Copy link
Member

mhofman commented Jul 22, 2023

What should I tell my colleagues who don't know about using to understand the control flow implications in a code review?

Any using statement in a block implies code executed when exiting the block. As with any code execution, it can throw.

I believe this is the trade-off that has to be made for allowing inline RAII style. The alternative requires declaring all resources when opening the block, which has poorer ergonomics, and still implies code execution when exiting the block, but is maybe more "balanced" and thus more noticeable.

@bakkot
Copy link

bakkot commented Jul 22, 2023

What should I tell my colleagues who don't know about using to understand the control flow implications in a code review?

You should send them a link to the MDN page on using, or any other such tutorial or overview, the same way you'd tell them about any other new syntax. All new syntax requires that people to learn about the syntax to understand what it does.

@mhofman
Copy link
Member

mhofman commented Jul 22, 2023

FWIW, It would have been great if we could have marked the block exit more clearly as an execution / interleaving point. Maybe someone can come up with a code editor tool that automatically adds a comment to these block exits.

@rixtox
Copy link
Author

rixtox commented Jul 22, 2023

I really don't see how using is better than DisposableStack + try ... finally in solving the issues in the Motivations section of the proposal.

const stack = new DisposableStack();
try {
    const x = stack.use(X());
    const y = stack.use(Y());
    console.log("done");
} finally {
    stack.dispose();
}

It solves all issues in the Motivations, with only the cost of creating a stack and adding a single try ... finally block. And it doesn't have any implication on control flow of {} blocks:

const stack = new DisposableStack();
try {
    const x = stack.use(X());
    {
        const y = stack.use(Y());
    }
    console.log("done");
} finally {
    stack.dispose();
}

@mhofman
Copy link
Member

mhofman commented Jul 22, 2023

In my original suggestion against adding syntax, I suggested abusing the for of construct, which didn't suffer as much from this gotcha:

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

You can still write an adapter today on top of DisposableStack that does this.

@rbuckton
Copy link
Collaborator

When this proposal was first introduced, the syntax was similar to C#'s earliest version of using:

using (const x = y) {
}

Over the many years that this proposal has been discussed, there was more and more interest within the committee to switch to an RAII-style form, and the consensus was to switch entirely to the RAII form. Even C# has adopted the RAII form for using and await using in recent editions. The implications of this syntax are well known to the committee at this time.

@rixtox
Copy link
Author

rixtox commented Jul 22, 2023

@mhofman
In my original suggestion against adding syntax, I suggested abusing the for of construct, which didn't suffer as much from this gotcha:

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

You can still write an adapter today on top of DisposableStack that does this.

Though I can't say if I like the idea of abusing for of contract for the effect, I think the problem for using is trying to confuse execution scope with resource scope, as an implication of adopting RAII. I've discussed this in length in #159.

Any solution that creates a seperate scope for resource binding can avoid this issue.

@rbuckton
Even C# has adopted the RAII form for using and await using in recent editions. The implications of this syntax are well known to the committee at this time.

I don't know having the committee knowing the implication is a good justification to impose this behavior for all JavaScript developers. Many JavaScript developers don't have the experience with RAII like C# or C++, and having this new syntax breaking a well established instinct on how {} block should behave is problematic for the developer community.

I think we should raise this behavior change on {} block very public and explicit and make sure we fully understand how the wider community feels about the change. This is exactly what Stage 3 was meant for.

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

4 participants