Stage 3 Specification Review: Shu-yu Guo

[rbuckton]

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

[rbuckton]

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

[syg]

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.

[zloirock]

@syg IIRC first 2 issues were explained on slides.

[syg]

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.

[rbuckton]

• 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.

[rbuckton]

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.

[rbuckton]

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

[acutmore]

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.

[rbuckton]

@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...

[ljharb]

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.

[rbuckton]

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:

1. Use this.constructor, but that assumes the subclass constructor expects the same arguments as the superclass.
2. 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.

[ljharb]

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.

[bakkot]

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

Or it could construct a new subclass instance and store the base DisposableStack in the new subclass instance. i.e.

class DisposableStackSubclass extends DisposableStack {
  move() {
    let ret = new DisposableStackSubclass();
    ret.use(super.move());
    return ret;
  }
}

No prototype patching required, works even if the subclass has private fields, and no difference from a consumer's point of view. Plus it has the advantage that the overridden use method would get called, rather than bypassed by setting the internal slots on the new instance directly after construction.

[zloirock]

Option 1 is how a lot of things already work

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

[zloirock]

@bakkot why enforce users to wrap all methods of prototypes if they need subclasses? In some cases, like arrays, it can be dozens of methods. Why create extra instances and do extra work just for that? In case if prototypes will be extended by some methods in the future - those methods can be not wrapped by library authors that create those subclasses - but final users who think that it's a proper subclass will await subclass instances as results of those methods. Etc. Too many problems and horrible user experiences for built-ins subclassing.

[zloirock]

The proper subclassing (including built-ins) with instance methods was one of the greatest attainment of ES6. @@species pattern and methods delegation has already made it possible to realize many things that would have been unthinkable without them. I don't want to waste these opportunities.

[zloirock]

The latest example - a couple of weeks ago, in the latest Babel release, was added "RegExp duplicate named capturing groups" transform - and it properly works only because of those patterns. I don't see how such features could be properly (with all string, regexp methods, etc.) implemented without these patterns. Maybe such flexibility is a controversial moment for RegExp where it causes problems for optimization - but definitely not for DisposableStack.

[bakkot]

@zloirock I would ask the question in the other direction: why should we add overhead to the language and implementations just to make it simpler to subclass DisposableStack? It's not hard to subclass without any extra help from the language.

While I used to agree with you about the argument that new methods might break the subclass, I no longer find it particularly compelling, because new methods can break the subclass anyway by failing to uphold its invariants. It's just not practical to guarantee that new methods added to the base class will be coherent on existing subclasses before those subclasses are updated to take into account the new methods.

Plus I don't expect subclassing DisposableStack to be that common. Even Map and Set, which are the classes most suited for subclassing currently, are rarely subclassed, and frequently those subclasses are straight-up broken already by the one existing feature intended to simplify subclassing Map (i.e. the fact that the Map constructor calls this.add instead of directly manipulating the internal slot). Encapsulation is almost always a better approach than subclassing. Adding complexity to make subclassing simpler just does not seem worth it to me.

[zloirock]

@bakkot why do you think that it will cause any significant overhead? A simple check that the @@species is DisposableStack - and an optimized for native DisposableStack version of the method can be used, otherwise - a generic version. It definitely will not cause any significant overhead. I made such optimizations in many cases - and the same is done in JS engines

It's just not practical to guarantee that new methods added to the base class will be coherent on existing subclasses before those subclasses are updated to take into account the new methods.

I think that it's a strange understanding of subclassing. I (and I think many usual users too) expect that subclassing will work right out of the box everywhere - on instance methods too.

Plus I don't expect subclassing DisposableStack to be that common.

I didn't expect that RegExp subclassing will be so useful. It can be useful for many years later - in cases that we don't expect now. Even if not - it's not the case where it causes any significant problems that are the reason for disallowing it.

[bakkot]

I didn't say the overhead would be large. But it is overhead, and I don't think the overhead is justified. The benefits are very small.

I think that it's a strange understanding of subclassing. I (and I think many usual users too) expect that subclassing will work right out of the box everywhere - on instance methods too.

It "works" in the sense of "you can make a subclass". But if the base class adds a new method, the subclass's invariants might be broken if users start invoking the new methods on the subclass. That's unavoidable.

[zloirock]

I didn't say the overhead would be large. But it is overhead, and I don't think the overhead is justified.

Everything causes overhead - why do you think that exactly the proper subclassing is a feature worth sacrificing for a tiny performance improvement?

It "works" in the sense of "you can make a subclass".

If instance methods of this subclass will return the base class instances - that will mean that it "partially works".

But if the base class adds a new method, the subclass's invariants might be broken if users start invoking the new methods on the subclass.

I don't think that such collisions are related to this topic. Static methods, new constructor arguments, and many other things could cause such collisions.

[bakkot]

why do you think that the proper subclassing is a feature worth sacrificing for a tiny performance improvement?

Again, I would turn it around. Why do you think that simplifying the process of subclassing DisposableStack is worth even a tiny performance cost? Keeping in mind that the performance cost is paid by every single user of every website that uses DisposableStack.prototype.move, not just by the people doing the subclassing.

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.

If instance methods of this subclass will return the base class instances - that will mean that it "partially works".

Is your position that subclassing does not fully work in any language which doesn't have Symbol.species? Subclassing doesn't work in Python, for example?

[zloirock]

Why do you think that simplifying the process of subclassing DisposableStack is worth even a tiny performance cost?

Just for ensuring the proper subclassing everywhere where it's possible and does not cause any significant problems like significant overhead. Just for writing class MyDisposableStack extends DisposableStack {} (you could replace DisposableStack with anything else) like class MyPromise extends Promise {} and be sure that everything will work. The language already has too many cases of inconsistency and crutches - why add some more?

Is your position that subclassing does not fully work in any language which doesn't have Symbol.species?

My position is that JS by ES6 has a good subclassing system - definitely better than in many other languages. But now I see regular attempts to kill it or at least make it worse.

[ljharb]

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

[zloirock]

@@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.

[zloirock]

@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.

[zloirock]

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?

[ljharb]

@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.

[zloirock]

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.

[syg]

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.

[rbuckton]

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

[syg]

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?

[rbuckton]

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.

[syg]

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(); },
  };
}

[rbuckton]

Essentially, yes.

[syg]

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.

[rbuckton]

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.