Editorial: various fixes

[MattiasBuelens]

• Fix typo in Close/Enqueue checks for readable byte streams. The internal slot is called [[closeRequested]] instead of [[closeRequest]]. Looks like we missed this in Add state checks to Close() and Enqueue() #1029. 😅
• Remove redundant ReadableStreamDefaultControllerCanCloseOrEnqueue check in TransformStreamDefaultSinkCloseAlgorithm.
• Add missing link to ReadableStreamDefaultControllerEnqueue inside ReadableStreamTee. Was already fixed in 63b4407.
• Fix link IDs for TransformStream abstract ops, which were presumable broken in the WebIDL rewrite.

---

Preview | Diff

[domenic]

Interesting. The fact that all the tests passed with this typo indicates a lack of test coverage. Would you be up for fixing that too? (Not a requirement for merging.)

[MattiasBuelens]

Added a drive-by fix for a related check that is now redundant. (I could also move this to a separate PR, if you prefer.)

[MattiasBuelens]

The ReadableStreamDefaultControllerCanCloseOrEnqueue check has become redundant, similar to this change in TransformStreamDefaultControllerTerminate.

[MattiasBuelens]

The fact that all the tests passed with this typo indicates a lack of test coverage. Would you be up for fixing that too? (Not a requirement for merging.)

I think the same problems arise as in the original PR. These changes are inside abstract operations that are not directly accessible through the API. The API will throw a TypeError when these checks fail, so it's not possible to reach the abstract op with a problematic state.

You'd need to find some other abstract operation which doesn't perform these checks on its own in advance. For ReadableStreamDefaultController, we could look at ReadableStreamTee or TransformStreamDefaultControllerEnqueue. However, for ReadableByteStreamController, the abstract ops are currently only used through the API, which always performs these checks. So I'm afraid there's currently no way to really test the changes from this PR. (Unless we would somehow export these abstract ops only for testing purposes, which would be weird...)

[MattiasBuelens]

I might be going on a bit of a tangent here, but... Now that ReadableStreamDefaultControllerEnqueue no longer asserts CanCloseOrEnqueue() but instead early-returns, shouldn't we do the same thing for TransformStreamDefaultControllerEnqueue? That means:

• Move what is currently step 3 of TransformStreamDefaultControllerEnqueue into TransformStreamDefaultController.enqueue().
• Add a check + early-return at the start of TransformStreamDefaultControllerEnqueue.

(Even more of a tangent: I just noticed that the anchor links for these transform stream abstract ops are weird. For example, TransformStreamDefaultControllerEnqueue is linked as #set-up-transform-stream-default-controller-enqueue. The set-up- bit looks like a copy-paste error? 😅)

[ricea]

lgtm

[ricea]

I might be going on a bit of a tangent here, but... Now that ReadableStreamDefaultControllerEnqueue no longer asserts CanCloseOrEnqueue() but instead early-returns, shouldn't we do the same thing for TransformStreamDefaultControllerEnqueue?

I think it's less clear-cut for TransformStreamDefaultControllerEnqueue because it is already handling the exceptional case, so there is no danger of state confusion. I don't feel strongly that we need to be consistent here.

[domenic]

The API will throw a TypeError when these checks fail, so it's not possible to reach the abstract op with a problematic state.

In that case, can we change them to asserts?

shouldn't we do the same thing for TransformStreamDefaultControllerEnqueue? That means:

It looks like this might have a normative effect on SetUpTransformStreamDefaultControllerFromTransformer, where 2.2 would no longer trigger. I'm not sure though if the branch is ever reached though...

(Even more of a tangent: I just noticed that the anchor links for these transform stream abstract ops are weird. For example, TransformStreamDefaultControllerEnqueue is linked as #set-up-transform-stream-default-controller-enqueue. The set-up- bit looks like a copy-paste error? 😅)

This was presumably broken in the Web IDL rewrite; good catch! https://streams.spec.whatwg.org/commit-snapshots/cfcd303a38158e4ec3837d1028158c88d505c35d/#transform-stream-default-controller-enqueue has the more-correct abstract operation IDs. A fix, perhaps in this PR, would be lovely.

[MattiasBuelens]

In that case, can we change them to asserts?

Uhm, I thought the whole point of #1029 was to change the asserts into early-exits, so that if other specs want to use the abstract op, they don't have to do the checks themselves? Now you want to change them back to asserts? I'm not sure I'm following... 😕

It looks like this might have a normative effect on SetUpTransformStreamDefaultControllerFromTransformer, where 2.2 would no longer trigger. I'm not sure though if the branch is ever reached though...

Hmm, good point, I seem to have missed that usage of TransformStreamDefaultControllerEnqueue. That would mean we have a "default" TransformStream where the readable side has been cancelled, but the writable side is still trying to send a chunk.

However, even in that case, I think we won't reach TransformStreamDefaultControllerPerformTransform: we would have already turned the readable's cancellation into a writable error in TransformStreamErrorWritableAndUnblockWrite, so TransformStreamDefaultSinkWriteAlgorithm should not get past the "If state is erroring, throw writable.[[storedError]]." step.

...I think. I'm not confident enough about it to actually change TransformStreamDefaultControllerEnqueue. 😅

This was presumably broken in the Web IDL rewrite; good catch! https://streams.spec.whatwg.org/commit-snapshots/cfcd303a38158e4ec3837d1028158c88d505c35d/#transform-stream-default-controller-enqueue has the more-correct abstract operation IDs. A fix, perhaps in this PR, would be lovely.

Will do. 🙂

[domenic]

Uhm, I thought the whole point of #1029 was to change the asserts into early-exits, so that if other specs want to use the abstract op, they don't have to do the checks themselves? Now you want to change them back to asserts? I'm not sure I'm following... 😕

Yeah, OK, fair... I guess I'm still kind of uneasy with the resolution of #1029, based on the fact that it's untestable and the comment in #1029 (comment). I think our best path forward is to just finish up this PR's uncontroversial aspects, and then maaaaybe come back to the early-exit vs. throw vs. assert discussion separately. Maybe after I eventually create a better interface for other specs to use streams abstract ops (#372).

In which case, this LGTM. I'll let @ricea have another look though just in case, before merging.

[ricea]

lgtm