Stage 3 Specification Review: Shu-yu Guo

@syg

This is a placeholder task for the Stage 3 Specification Review feedback from Shu-yu Guo (@syg), per the October 2021 Meeting

@syg

@syg, please review the current specification text prior to the September meeting, if possible.

I've had a look at the new spec text. I have some questions, mostly about the new built-in constructors.

Where's the motivation for adding DisposableStack and AsyncDisposableStack?
Why is the DisposableStack.prototype.dispose auto-bound?
Do we really need species on these new constructors?
Waldemar's previously raised concern about AggregateError flattening causing refactoring hazards still stands AFAICT. I agree with his concerns.

@syg

@syg IIRC first 2 issues were explained on slides.

@zloirock

Thanks for the pointer @zloirock.

For the first bullet point, the motivations I see are "it exists in Python" and "it's useful". I'd like @rbuckton to expand on that a bit.

For the second bullet point, I'm not sure I see a reason for why it's auto-bound, just that it is auto-bound. Perhaps it's implicit in that explanation about NumberFormat. I don't recall why NumberFormat's format is auto-bound.

@ljharb

Where's the motivation for adding DisposableStack and AsyncDisposableStack?

https://github.com/tc39/proposal-explicit-resource-management#assist-in-complex-construction. Large-scale projects will often create components that consume multiple resources, the lifetimes of which are bound to the lifetime of the component. In the simplest case, DisposableStack acts as a container for disposables: you can add one or more disposables to the stack, and they will be disposed in reverse order. In the more complex case, when you define a class that acquires multiple disposable resources in the constructor, you need to ensure that:

If no errors are thrown during construction, the container can be safely stored in a field on the class to be disposed later.
If any errors are thrown during construction, resources in the container are properly disposed.

Consider the PluginHost example in the explainer. Both the NodeProcessIpcChannelAdapter and NodePluginHostIpcSocket classes are disposables. You don't want to use using to declare them in the constructor, as their lifetime is intended to outlive the constructor:

class PluginHost {
  #channel;
  #socket;
  constructor() {
    using channel = new NodeProcessIpcChannelAdapter(process);
    using socket = new NodePluginHostIpcSocket(channel);
    // these get disposed as soon as the constructor exits, so assigning them here isn't very useful
    this.#channel = channel;
    this.#socket = socket;
  }
}

If using isn't sufficient here, we might try to track the lifetime manually:

class PluginHost {
  #channel;
  #socket;
  constructor() {
    this.#channel = new NodeProcessIpcChannelAdapter(process);
    this.#socket = new NodePluginHostIpcSocket(this.#channel);
  }
  [Symbol.dispose]() {
    this.#socket[Symbol.dispose]();
    this.#channel[Symbol.dispose]();
  }
}

But there are multiple issues here. If an exception occurs when creating #socket in the constructor, #channel won't be disposed. If an exception occurs when disposing of this.#socket in our dispose method, we also won't dispose this.#channel. So now we have to further complicate our constructor to handle all of these situations:

class PluginHost {
  #channel;
  #socket;
  constructor() {
    this.#channel = new NodeProcessIpcChannelAdapter(process);
    try {
      this.#socket = new NodePluginHostIpcSocket(this.#channel);
    } catch (e) {
      this.#channel[Symbol.dispose]();
      throw e;
    }
  }
  [Symbol.dispose]() {
    try {
      this.#socket[Symbol.dispose]();
    } finally {
      this.#channel[Symbol.dispose]();
    }
  }
}

This kind of boilerplate try..catch and try..finally logic is part of what using hopes to avoid. If we couple using with DisposableStack, we can dramatically simplify this code:

class PluginHost {
  #disposables;
  #channel;
  #socket;
  constructor() {
    using stack = new DisposableStack(); // this is guaranteed to dispose at end of block
    this.#channel = stack.use(new NodeProcessIpcChannelAdapter(process));
    this.#socket = stack.use(new NodePluginHostIpcSocket(this.#channel));
    this.#disposables = stack.move(); // move out of stack that will dispose now and into a stack we can dispose later
  }
  [Symbol.dispose]() {
    this.#disposables[Symbol.dispose]();
  }
}

In this example, we create a DisposableStack that remains local to the constructor, and is guaranteed to be disposed when the constructor exits. We can add each resource we successfully create to the stack, knowing that any exception will result in the stack and its contents being disposed. At the end of the constructor, we move all of the successfully created resources out of the local stack and into a longer-lived stack. This also means we have a much simpler [Symbol.dispose]() method that will handle resource cleanup in reverse order, as well as aggregation of exceptions.

Why is the DisposableStack.prototype.dispose auto-bound?

This was requested feature to better work with factory functions.

Do we really need species on these new constructors?

No. It's my understanding that many on the committee hope that species can eventually be removed, but until that occurs I felt it was best to specify it and possibly just remove it later if necessary.

Waldemar's previously raised concern about AggregateError flattening causing refactoring hazards still stands AFAICT. I agree with his concerns.

An alternative to AggregateError flattening would be to have a bunch of nested AggregateError instances with only one entry in errors:

try {
  using r1 = { [Symbol.dispose]() { throw new Error("c"); };
  using r2 = { [Symbol.dispose]() { throw new Error("b"); };
  throw new Error("a");
} catch (e) {
  e // AggregateError {
    //   cause: AggregateError {
    //     cause: Error { message: "a" },
    //     errors: [Error { message: "b" }]
    //   },
    //   errors: [Error { message: "c" }]
    // }
}

Or a new Error subclass (e.g., SuppressedError) that just has a cause and an error property. That's being discussed in #74, though IIRC @ljharb has favored just using AggregateError.

Large-scale projects will often create components that consume multiple resources [...]

VS Code has hundreds of examples of this, not to mention hundreds of extensions that use VS Code's Disposable implementation.

or the second bullet point, I'm not sure I see a reason for why it's auto-bound, just that it is auto-bound.

That's something I intended to talk more about when presenting, but I do show an example of the "factory function" use case for a bound dispose method on slide 24

Just to add a big +1 from me on having DisposableStack for the .move functionality. It completely addresses the issue I raised last year. It both reduces the amount of boilerplate and increases the locality of writing safer code.

@syg

@syg, is it your recommendation that I remove the @@species members? At present they are used in the move() method, and removing them would potentially prevent subclassing of DisposableStack/AsyncDisposableStack.

@syg

@syg, is it your recommendation that I remove the @@species members? At present they are used in the move() method, and removing them would potentially prevent subclassing of DisposableStack/AsyncDisposableStack.

That's my preference, yes. Are there compelling use cases for that subclassing? I like the move() method very much, but like, you know, subclassing...

Why would removing them prevent subclassing? It might change which methods need to be implemented on a subclass, sure, but that's not the same as preventing it.

Why would removing them prevent subclassing? It might change which methods need to be implemented on a subclass, sure, but that's not the same as preventing it.

move() creates a new instance of DisposableStack (or AsyncDisposableStack) that gets returned. Without @@species, our options are:

Use this.constructor, but that assumes the subclass constructor expects the same arguments as the superclass.
Just create a new DisposableStack regardless as to whether the instance is a subclass.

If we go with (2), since DisposableStack has an internal slot, the only way a subclass could return an instance of itself via move() would be to override move(), call the superclass method, patch its prototype and perform any other initialization necessary. And that process wouldn't work if the subclass has private fields, since you wouldn't be able to install them.

To be fair, I'm not sure how often subclassing would make sense.

Option 1 is how a lot of things already work, and that seems perfectly reasonable to me (especially if there's a fallback in the event of a non-callable constructor property to %DisposableStack%).

But yes, ime subclassing rarely makes sense anyways, especially on builtins.

Subclassing a Promise doesn't make any sense because every Promise.resolve or await will canonicalize it to the base class.

@@species was added as a scratch for Array methods called on strings, DOM collections, etc. - for the rest cases, this.constructor was enough. But @@species makes sense for consistency, usage of one subclassing pattern for all instance methods in the language. Using one pattern in one place, another in another, and in a third one completely removing support of subclassing is bringing unnecessary chaos to the language that has already become a collection of legacy and crutches - it is unrealistic for usual users to remember all these differences.

@ljharb

@ljharb may be for you, but not for many other users. For example, a half hour ago I looked at zx source - it seems they don't think so. And I see similar examples each some days.

@ljharb

Option 1 is how a lot of things already work

What means "a lot"? How many instance methods, at least some examples?

@ljharb so, maybe you will answer?

@zloirock

@zloirock You've already listed them.

The reality here is that this debate - the merits of subclassing, what constitutes "proper" subclassing, and whether Symbol.species is actually a good mechanism to achieve subclassing - is one that hasn't been resolved even in committee. However, the majority of the committee seems to consider RegExp subclassing to be a mistake (but the majority doesn't think it's worth trying to remove it, unfortunately), and the majority of the committee seems to prefer to see Symbol.species removed if it is web-compatible to do so.

New APIs shouldn't add something we're hoping to remove, even for consistency, and that includes Symbol.species.

So, by "a lot" you meant "no one"?

I agree that for RegExp subclassing was a mistake since it's very hard to optimize. However, actual usage of this on RegExp (as I mentioned above) perfectly shows the pluses of those patterns for userland.

DisposableStack is not RegExp and here it should not cause any significant problems for optimizing.

the committee seems to prefer to see Symbol.species removed

So, the committee wanna (?) degrade the language that they should improve. Clear.

if it is web-compatible to do so

Is this a call for me to make it completely incompatible with the web? It looks like a provocation.

Also, the concern is about complexity, in addition to performance performance. We'd be making the implementation of this method more complicated. Even if it were implemented with no performance overhead at all, complexity is a cost in itself.

In general the concern is like 90% complexity that has had, historically, caused security issues.

I put together #114 to address removing Symbol.species support.

Also trying to close the loop on the .dispose auto-binding behavior. I'm still not that convinced on the auto-binding behavior. I feel like it's surprising to have this one method here behave differently.

I see the justification linked above is ease of use in factories, with this example given:

function makePluginHost() {
  using const stack = new DisposableStack();
  return {
    channel: stack.use(new NodeProcessIpcChannelAdapter(process)),
    socket: stack.use(new NodePluginHostIpcSocket(this.#channel)),
    [Symbol.dispose]: stack.move().dispose,
  };
}

I am not really understanding that example. Why is the new DisposableStack bound with using at all? That is, what about the following:

function makePluginHost() {
  const stack = new DisposableStack();
  const channel = stack.use(new NodeProcessIpcChannelAdapter(process));
  const socket = stack.use(new NodePluginHostIpcSocket(this.#channel));
  return {
    channel,
    socket,
    [Symbol.dispose]: stack.dispose.bind(stack),
  };
}

Is that really that much less ergonomic as to harm adoption?

Your version is an example of faulty construction and a potential resource leak: If an exception occurs when constructing socket, then channel won't be disposed.

In my version, if construction of socket throws, then stack is disposed (because of the using decl), which in turn disposes channel. Only after construction is done do we move the resources out of stack so that they won't be disposed on exit. This is more reliable, especially when there is even more complexity in a constructor/factory than shown here.

Ah, exception safety! Thanks for the correction. It'd be something like the following then?

function makePluginHost() {
  using const stack = new DisposableStack();
  const channel = stack.use(new NodeProcessIpcChannelAdapter(process));
  const socket = stack.use(new NodePluginHostIpcSocket(this.#channel));
  // Should not throw after this point.
  const movedStack = stack.move();
  return {
    channel,
    socket,
    [Symbol.dispose]: () => { movedStack.dispose(); },
  };
}

Essentially, yes.

With the auto-binding method removed and the jagged errors behavior merged with SupressedError, the semantics here lgtm. I'm not 100% on the correctness of the grammar changes, especially the lookahead stuff, but it seems fine?

I have editorial comments about the spec draft but that can wait for later. The major editorial comment is that you define "interface" in this section and then list only the disposables as interfaces, while there are other interfaces already. If you want the spec to formally define interface, it might be better done as a pre-req step.

I have editorial comments about the spec draft but that can wait for later. The major editorial comment is that you define "interface" in this section and then list only the disposables as interfaces, while there are other interfaces already. If you want the spec to formally define interface, it might be better done as a pre-req step.

I've removed the text from this proposal. If we need to pull that text out of the Iterator section into somewhere else, I can do that when I put together the PR for ECMA262.

Navigation Menu

Repository navigation

Stage 3 Specification Review: Shu-yu Guo #93

11 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Stage 3 Specification Review: Shu-yu Guo #93

Description

Activity

rbuckton commented on Sep 6, 2022

syg commented on Sep 13, 2022

zloirock commented on Sep 13, 2022

syg commented on Sep 13, 2022

rbuckton commented on Sep 13, 2022

rbuckton commented on Sep 13, 2022

rbuckton commented on Sep 13, 2022

acutmore commented on Sep 14, 2022

rbuckton commented on Sep 16, 2022

syg commented on Sep 16, 2022

ljharb commented on Sep 16, 2022

rbuckton commented on Sep 16, 2022

ljharb commented on Sep 16, 2022

11 remaining items

ljharb commented on Sep 16, 2022

zloirock commented on Sep 16, 2022

zloirock commented on Sep 16, 2022

zloirock commented on Sep 16, 2022

ljharb commented on Sep 16, 2022

zloirock commented on Sep 16, 2022

syg commented on Sep 16, 2022

rbuckton commented on Nov 14, 2022

syg commented on Nov 14, 2022

rbuckton commented on Nov 15, 2022

syg commented on Nov 15, 2022

rbuckton commented on Nov 15, 2022

syg commented on Dec 1, 2022

rbuckton commented on Dec 1, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions