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

Remove redundant ReturnIfAbrupt shorthands #1225

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Mar 4, 2022

See #1224

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …
    • Deno: …
    • Node.js: …

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@saschanaz
Copy link
Member Author

I'm doing this since I guess this shouldn't collide with #1224 (comment):

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.

Just want to make sure this is in the right direction since I'm not super familiar with ReturnIfAbrupt. Especially:

Also, everything has a header saying what it returns.

Is this needed to do this cleanup?

@domenic

@domenic
Copy link
Member

domenic commented Mar 7, 2022

Yes, I think this is in in the right direction. Basically:

  • Find all never-fail operations in the streams spec, and remove all !s from calls to them.

  • Remove all !s from calls to ECMAScript operations that do not return abstract completions anymore. I believe the following list is comprehensive: CopyDataBlockBytes, CreateArrayFromList, CreateBuiltinFunction, IsDetachedBuffer, IsIntegralNumber (we need to update we're using IsInteger), OrdinaryObjectCreate, SameValue, Type.

I think we should be sure to do all of these at once, as otherwise it's a bit confusing.

Also, everything has a header saying what it returns.

Is this needed to do this cleanup?

I'm not sure; I welcome your input or that of the other editors.

Without the header, the situation is a bit confusing, because you can only know whether an abstract op returns a completion or a bare value by inspecting the call sites. (I.e., if all call sites use ?/!, it's a completion. If all do not, it's a bare value. If it's a mix, that's a spec bug.) It seems relatively easy to get confused.

However, it might be worth thinking about the world post-#1224, to avoid wasted effort. I'll comment over there with some ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants