whatwg / streams

Streams Standard
https://streams.spec.whatwg.org/
Other
1.34k stars 157 forks source link

Editorial: update use of, or stop using, completion values? #1224

Open domenic opened 2 years ago

domenic commented 2 years ago

https://github.com/tc39/ecma262/pull/2547 has changed how completion records work. Now, if an operation is infallible, the recommended practice is have it not return a completion record. Also, everything has a header saying what it returns.

We could update to follow, similar to https://github.com/whatwg/html/pull/7661 .

Or, we could try to further harmonize with the rest of the web spec ecosystem, and stop doing completion records entirely. (Except where necessary to interface with ECMAScript.) I think I'd kind of prefer that. Concretely, we'd:

ricea commented 2 years ago

I like the ? and ! but there's no doubt they're super confusing to people coming to the spec for the first time.

Would we have to explicitly write "Assert: This does not throw an exception" everywhere?

domenic commented 2 years ago

Would we have to explicitly write "Assert: This does not throw an exception" everywhere?

We wouldn't have to. We could if it's helpful. I'd suggest only doing it in cases where the not-throwing is potentially surprising. (Which is of course subjective.)

This is related to the question of whether/how we should introduce headers to the abstract operations, like ECMAScript now has.

If we keep !/? and just follow ECMAScript, then the headers could look something like:

AcquireWritableStreamDefaultWriter(stream) returns either a normal completion containing a WritableStreamDefaultWriter or an abrupt completion. It performs the following steps:

IsReadableStreamLocked(stream) returns a boolean. It performs the following steps:

ReadableStreamAddReadRequest(stream, readRequest) returns unused. It performs the following steps:

Or they could be omitted if we think they're not worthwhile.

Whereas if we do web spec style, then I think omitting is probably more conventional, since most specs are not very explicit about whether their operations can throw or not; it's just kind of assumed things will flow throughout. But if we did want headers they would look something like:

AcquireWritableStreamDefaultWriter(stream) either returns a a WritableStreamDefaultWriter or throws. It performs the following steps:

IsReadableStreamLocked(stream) returns a boolean. It performs the following steps:

ReadableStreamAddReadRequest(stream, readRequest) performs the following steps:

ricea commented 2 years ago

For Blink's implementation, knowing whether something can throw is useful, because it changes the calling convention.

However, in practice there's stuff that can throw in Blink that never throws in the spec, so we can't use the spec as the ultimate source of truth. For example, stack overflow and Worker termination can cause many V8 operations to throw.

mgaudet commented 2 years ago

Gecko has the same story: Calling convention changes for throwing algorithms, and places where we will throw that the specification says has throwing impossible.

Minimizing these cases is on my mind at the moment, and so I've been using completion records in the specification to help figure out what's expected to throw and what's not expected.

In my opinion they're valuable information and I'd be sad to see them go.

domenic commented 2 years ago

Thanks, this is valuable feedback. Would something like my "But if we did want headers they would look something like:" from the OP work, or is seeing the !/? at call sites especially valuable?

mgaudet commented 2 years ago

I guess one thing I don't quite understand is the structure of the proposed headers. You're suggesting something like

SetUpWritableStreamDefaultController(stream, controller, startAlgorithm, writeAlgorithm, closeAlgorithm, abortAlgorithm, highWaterMark, sizeAlgorithm) performs the following steps, returning a completion value, which may be an abrupt completion

?

domenic commented 2 years ago

In the last part of https://github.com/whatwg/streams/issues/1224#issuecomment-1060818793, where I am talking about avoiding completion values but keeping some of the same information, I'm specifically suggesting:

SetUpWritableStreamDefaultController(stream, controller, startAlgorithm, writeAlgorithm, closeAlgorithm, abortAlgorithm, highWaterMark, sizeAlgorithm) performs the following steps, which can throw an exception:

ricea commented 2 years ago

What happens when we call an operation that could throw, but which is annotated with a "!" to indicate that it doesn't throw in this case, is that we turn it into an assert in the implementation.

So one option would be to replace the "!" with asserts, but only when the operation is defined as "can throw".