Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notate abstract ops that can and cannot throw #253

Closed
domenic opened this issue Dec 15, 2015 · 24 comments · Fixed by #2547
Closed

Notate abstract ops that can and cannot throw #253

domenic opened this issue Dec 15, 2015 · 24 comments · Fixed by #2547
Labels
completion records Relates to completion records, and ? / ! notation.

Comments

@domenic
Copy link
Member

domenic commented Dec 15, 2015

In streams, which is heavily promise-based, there are a lot of abstract operations that cannot throw. The ES spec has these too, in smaller number.

Of course, this kind of nothrow vs. throw characteristic can be contagious. If you only use nothrow abstract ops in your abstract op, then your abstract op is nothrow. Whereas if you use any throwing ops, you are now throwing. Unless you are certain for external reasons that the given throw operation will not throw in this context, in which case you might want to assert that it is a non-abrupt-completion.

There are a couple action items here, some of which can be done independently:

  • Come up with a convention for asserting that the return value is never an abrupt completion, e.g. Let _result_ be ! AbstractOp(). Use it throughout. A lot of places in the spec currently use "perform" for this purpose, which is not defined anywhere.
  • Introduce EMU support for notating with throws/nothrow. (Or perhaps, abrupt/noabrupt.) You can see this in action in Streams, e.g. https://streams.spec.whatwg.org/#rs-abstract-ops. For the source format, I'd suggest either <emu-clause nothrow> or <emu-clause flair="nothrow">.
  • Go through the ES spec and update all abstract ops with whether or not they can throw. This could be done incrementally.
  • Build automated checker tools to enforce the contagion rules, based on throw/nothrow and ! or ?.
@ljharb
Copy link
Member

ljharb commented Dec 15, 2015

What about the case where the result of AbstractOp is not needed? That's usually the cases where "Perform" appears. Having a useless _result_ doesn't seem ideal. Do you have an alternative suggestion for an abstract op that solely causes side effects (some of which can throw, and some of which can't)?

In general I love this idea - being explicit about what can throw and what can't seems like an overall improvement.

@domenic
Copy link
Member Author

domenic commented Dec 15, 2015

Do you have an alternative suggestion for an abstract op that solely causes side effects (some of which can throw, and some of which can't)?

1. ? AbstractOp() to bubble errors, or 1. ! AbstractOp() to assert it never errors. You could introduce the "perform" verb for that but I'd like it to be defined this time.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2015

Sounds good - I like having an explicit "this is for side effects" mechanism like "Perform".

@jmdyck
Copy link
Collaborator

jmdyck commented Dec 15, 2015

Here's what I said when this was raised in the ecmarkup repo:

Presumably "Perform AbstractOp(x)" implies not just that AbstractOp doesn't throw, but also that it doesn't return anything useful (via normal completion). (Or at least, that these things are known to be true for the particular arguments being passed.)

I think it would be ideal if Ecmarkup supported this, e.g. <h1 aoid="..." nothrow>...</h1>.

To generalize your suggestion, I think it would be useful (though even more work) if every operation had a "header" that systematically listed/described the operation's inputs (parameters) and outputs (returns). (Currently, preambles give some of this info, but not consistently or completely.) E.g., outputs/returns might say something like:

normal returns: Boolean
abrupt returns: TypeError

So the throw/nothrow distinction would be captured by whether any abrupt returns are listed.

@bterlson
Copy link
Member

@jmdyck: that would be amazing, but much work... I think we can probably get there eventually by going down the path of incrementally adding !.

Note that Perform X() is not about implying that AbstractOp doesn't throw (note further the many instances of Perform ? Op()). It's simply ecmaspeak for "the result of X()" where x doesn't have a useful result. In other words, with bang, if an abstract op doesn't have a useful result, we'd want Perform ! Op().

I fully support ! as a short-hand for "Assert: previous thing is not an abrupt completion". We can probably just start the incremental addition by doing a similar regexp replace as we did to kick off the ? conversion.

@zenparsing
Copy link
Member

I'm a little worried about extending the ECMASpeak programming language too fast. The RIA shorthand was a good buy in terms of readability/writability, but I'm not so sure about "!".

@jmdyck
Copy link
Collaborator

jmdyck commented Dec 16, 2015

@jmdyck: that would be amazing, but much work...

I'm prepared to do the work if we can agree on a "header" syntax.

@domenic
Copy link
Member Author

domenic commented Dec 16, 2015

I'm a fan of !. It fixes two important problems with the current spec:

  • There are many cases where a completion value v is used, but v.[[value]] is expected. For example, CreateDataProperty(array, ToString(n), e). ToString returns a completion value, but CreateDataProperty does not accept completion values as its second argument. This is just a bug in the current spec really. (Or if you prefer, an intentional type mismatch for the sake of brevity.)
  • There are several (39) places in the spec where we assert something is not an abrupt completion. This is a bit distracting, but is honestly probably something we should be doing more! It should be easy to say something is expected to never throw.

I am less of a fan of the header syntax, but some mock-up screenshots could possibly convince me.

@anba
Copy link
Contributor

anba commented Dec 16, 2015

ToString returns a completion value, but CreateDataProperty does not accept completion values as its second argument. This is just a bug in the current spec really. (Or if you prefer, an intentional type mismatch for the sake of brevity.)

No, it is a specified and documented feature of completion records to allow implicit access to the [[value]] field (cf. 6.2.2.2).

How about doing it the other way around: Explicitly state with ! when a caller needs to receive a completion record. I think that should be more ergonomic than adding ! after every infallible operation.

@rossberg
Copy link
Member

@anba, please no. The spec should not be optimised for maximum brevity but for maximum clarity. I think the implicit conversion from completion records to their values is actively harmful in that regard. There are already too many hidden conventions going on in the spec (a criticism I keep hearing from fellow V8 implementers trying to make sense of it every other day).

@domenic
Copy link
Member Author

domenic commented Dec 16, 2015

No, it is a specified and documented feature of completion records to allow implicit access to the [[value]] field (cf. 6.2.2.2).

Yes, that is what I meant in the parenthetical.

@jmdyck
Copy link
Collaborator

jmdyck commented Dec 16, 2015

I think the implicit conversion from completion records to their values is actively harmful in that regard.

I agree. (And also the reverse implicit conversion.)

I've thought of an approach that I think would eliminate most of these conversions. I'll raise it in another issue when I have some time.

@allenwb
Copy link
Member

allenwb commented Dec 16, 2015

I want to support the general position expressed by @rossberg-chromium above.

Maximizing clarity (and particularly for occasional readers should be among the highest priorities. Most readers of the spec. are not spending 8 hours a day emerged within it. More likely they are occasionally looking up some specific details. They may or may not have read the clause 5 conversions and even if they have they may not remember them what they read.

@domenic things like CreateDataProperty(array, ToString(n), e) is used in situations where n is known to have a value that will convert to a string, without error. Most typically this is an integer valued index variable of a algorithmic loop. I considered making the conversion from completion to value explicit but found that formulation to be less readable.

@bterlson
Copy link
Member

Going to close this issue as I believe we have satisfied the original request.

@domenic
Copy link
Member Author

domenic commented Feb 18, 2016

I don't believe we have. The first bullet point is mostly implemented, but the overall request motivating it (as outlined in the subsequent ones) remains open.

@domenic domenic reopened this Feb 18, 2016
@bterlson
Copy link
Member

Oh I see, sorry about that.

@domenic
Copy link
Member Author

domenic commented Feb 24, 2022

Is this really fixed?

@ljharb
Copy link
Member

ljharb commented Feb 24, 2022

The only abstract operations that can throw now return a completion record; those that can't, don't. It's possible it's not being noted in the rendering just yet? cc @bakkot @michaelficarra to confirm

@domenic
Copy link
Member Author

domenic commented Feb 24, 2022

I think I see. Wow, that is really unfortunate, for the reasons I argued long ago in #486 (comment) and elsewhere. I wonder how many places in HTML/Web IDL will need updating to remove the ! prefix they've been using. Oh well.

I'll note that it seems like this idea doesn't hold for abstract/internal methods, e.g. https://tc39.es/ecma262/#sec-ordinary-object-internal-methods-and-internal-slots-getprototypeof returns a completion but cannot throw, and similarly for https://tc39.es/ecma262/#sec-declarative-environment-records-hasbinding-n . But I can understand why that would happen.

@ljharb
Copy link
Member

ljharb commented Feb 24, 2022

It's true that for internal method overloads where some of them throw, the ones that don't still need to return a completion.

@bakkot
Copy link
Contributor

bakkot commented Feb 24, 2022

@domenic re

Eliminating the ! from the call site reduces clarity, making me jump to the definition of the abstract operation to find out if we're assuming it returns a non-completion record, or a completion record.

We've adopted the convention (enforced by tooling) that every call of an operation which returns a completion record is of the form ? A(), ! A(), or Completion(A()), and no call of an operation which does not return a completion record is of that form. So you don't have to jump to the definition to know if an operation can throw; that information is present at every call site.

(The Completion form is rare; it only comes up when explicitly manipulating completion records, usually for promises or iterators.)

@syg
Copy link
Contributor

syg commented Feb 24, 2022

I wonder how many places in HTML/Web IDL will need updating to remove the ! prefix they've been using.

Does HTML / WebIDL call many AOs that don't actually return completions? My intuition would be that it doesn't actually, so relatively few sites would need changing.

Edit: Upon looking in HTML the answer is actually yes, because there are several calls to predicates that now just return a boolean. I can prepare a PR for that.

@domenic
Copy link
Member Author

domenic commented Feb 24, 2022

Just skimming https://webidl.spec.whatwg.org/#index-defined-elsewhere: MakeBasicObject, CanonicalNumericIndexString, GetFunctionRealm, IsConstructor, ... I'd guess maybe 1/4 of all abstract ops?

@bakkot
Copy link
Contributor

bakkot commented Feb 24, 2022

Once the updated ecma262 biblio is published (for the first time since 2016...) it would probably be pretty easy to check those with tooling.

I note that ! isn't currently used consistently in HTML specs. In some cases, for example the call to OrdinaryGetOwnProperty in step 2.1.2 of CrossOriginGetOwnPropertyDescriptor; that's because the operation actually doesn't throw. In others, for example the call to GetFunctionRealm in step 9.a of HTML element constructors; it's not clear to me if it actually can throw in that position and the authors failed to account for it, or if there's some reason it definitely can't in this case. (Did you know GetFunctionRealm can throw? Fun fact! Revoked proxies are fun!)

In an ideal world, tooling would do the same checks ecma262 does, so that authors actually have to deal with possible exceptions for operations which can sometimes throw.

That's the main reason we did it this way: rather than just sprinkling ! on every call, you can skip it for AOs which can never throw, and for those which sometimes throw you have to actually think about that particular call site.

domenic pushed a commit to whatwg/html that referenced this issue Feb 27, 2022
As part of the tc39/ecma262#2547 the JavaScript specification has reformed how it deals with Completion Records, and thus what abstract operations can return. See tc39/ecma262#253 (comment) for a brief description of when to use ? vs. ! vs. nothing at all.

Additional minor fixes:

* Replace uses of ObjectCreate (which no longer exists) with OrdinaryObjectCreate.
* Replace "Rethrow any exceptions" with the ? syntax.
* Use ? with GetFunctionRealm, which can throw due to revoked proxies.
domenic pushed a commit to whatwg/webidl that referenced this issue Mar 7, 2022
As part of the tc39/ecma262#2547 ecma262 has reformed how it deals with Completion Records. See tc39/ecma262#253 (comment) for a brief description of when to use ? vs. ! vs. nothing at all.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
As part of the tc39/ecma262#2547 the JavaScript specification has reformed how it deals with Completion Records, and thus what abstract operations can return. See tc39/ecma262#253 (comment) for a brief description of when to use ? vs. ! vs. nothing at all.

Additional minor fixes:

* Replace uses of ObjectCreate (which no longer exists) with OrdinaryObjectCreate.
* Replace "Rethrow any exceptions" with the ? syntax.
* Use ? with GetFunctionRealm, which can throw due to revoked proxies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion records Relates to completion records, and ? / ! notation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants