Editorial: add PromiseCapability Record assertions #2050
Conversation
|
We should also add a Doesn't need to be done in this PR if you're not inclined to, though. |
|
all updated |
spec.html
Outdated
| @@ -39528,6 +39528,7 @@ <h1>AsyncGeneratorResolve ( _generator_, _value_, _done_ )</h1> | |||
| 1. Assert: _queue_ is not an empty List. | |||
| 1. Remove the first element from _queue_ and let _next_ be the value of that element. | |||
| 1. Let _promiseCapability_ be _next_.[[Capability]]. | |||
| 1. Assert: _promiseCapability_ is a PromiseCapability Record. | |||
There was a problem hiding this comment.
I am not convinced it's good to assert on the type of an internal slot which is described as holding a value of the type in question. From reading this assertion I would assume that it's possible for the slot to hold some other type, so that it's meaningful to assert that it does not happen to in this particular case - but that's not so; the slot only ever holds a PromiseCapability.
There was a problem hiding this comment.
Without these assertions tho, that's only the case "accidentally" - iow, there's nothing anywhere else that ensures it's always one in the event of a spec authoring error.
If that's not a concern, I can remove these, but I'm not sure what the downside would be of keeping them?
There was a problem hiding this comment.
An assertion also would not ensure the type is correct. It just asserts it, exactly the same way the description of the field does. I don't see what the value of the assertion is given that the field is described as containing a PromiseCapability Record.
I'm not sure what the downside would be of keeping them?
The downside is as I said in the previous comment: from reading this assertion I would assume that it's possible for the slot to hold some other type, so that it's meaningful to assert that it does not happen to in this particular case. Which would be incorrect.
There was a problem hiding this comment.
an implementation can alter internal slots at any time; without the assertion somewhere then it already can be any possible type.
There was a problem hiding this comment.
an implementation can alter internal slots at any time
... Why do you believe this to be true? I don't think this is true.
without the assertion somewhere then it already can be any possible type.
Again, assertions have no normative force. They are just comments for clarity to the reader.
There was a problem hiding this comment.
They're normative in the sense that an implementation doesn't have any observable action to take, and can thus ignore them, but if the assertion is violated, it's either a spec bug or an implementation bug.
There was a problem hiding this comment.
if the assertion is violated, it's either a spec bug or an implementation bug.
If an assertion is violated, it's a spec bug, yes. (I'm not really sure what it would mean for an implementation to violate an assertion.) But it would be exactly as much of a bug to put a non-PromiseCapability Record value in a slot which is described as holding a PromiseCapability Record. And it would similarly be a bug to treat a non-PromiseCapability Record value as a PromiseCapability Record. It does not somehow become more of a spec bug just because there's an assertion.
Assertions in the context of the spec are just comments. I think it is a mistake to add a comment for any reason other than because we think it will aid clarity for readers.
(I don't really know what you mean by "normative" in this context.)
There was a problem hiding this comment.
Fair - I do think this change adds clarity for readers, because I don't have to go elsewhere to figure out what kind of thing is held in the slot.
There was a problem hiding this comment.
Fair - I do think this change adds clarity for readers, because I don't have to go elsewhere to figure out what kind of thing is held in the slot.
We could achieve that by naming the slot more precisely, e.g. [[PromiseCapability]] (or, if that's still not enough, [[PromiseCapabilityRecord]]).
There was a problem hiding this comment.
That still wouldn't make it auto-link to "PromiseCapability Record", though.
|
Removed the assertions immediately following a slot extraction. |
I didn't bother to add an assertion after every callsite of
NewPromiseCapability.