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

Editorial: Fixed error for already unwrapped completion records #1664

Open
wants to merge 1 commit into
base: master
from

Conversation

@jhnaldo
Copy link

commented Aug 9, 2019

The Get and Call function always return abrupt completion or non-completion values
because the ReturnIfAbrupt(?) is applied right before return the result in Get and Call.
Thus, in the following cases, mappedValue in Array.from and k and v in AddEntriesFromIterable are not completion but just non-completion values because the abrupt completion cases are already filtered previous conditions. Thus, I suggest to remove access of [[Value]] for mappedValue, k, and v.

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors Aug 9, 2019

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

Personally, I'm fine with these edits. However, I feel obliged to point out that they go against the current editorial direction (and so I'm puzzled that @ljharb approves).

In more detail:

  1. These changes are fine (in fact, they're necessary corrections) in an interpretation where Call and Get can return both abrupt completions and non-completion values (as mentioned in the initial message above). While this is my preferred interpretation (see issue #497), I don't think it's widely accepted. (But I'll be happy to hear otherwise.)

  2. The changes are also valid (though not necessary) in an interpretation where Call and Get always return a completion record (sometimes abrupt, sometime normal), and you're willing to make use of the rule whereby a normal completion can be implicitly coerced to its [[Value]] (see 5.2.3.1). However, there seems to be a general preference not to use that rule (or at least, no more than the spec already is).

  3. So, if Call and Get always return a completion record, and there's a preference to avoid implicit coercion-to-[[Value]], you instead have to either explicitly 'unwrap' a normal completion via '?' and '!', or explicitly reference its [[Value]] field. The latter is what the existing steps do, and this PR is moving away from that.

So @ljharb, is 3 not the current editorial direction, and you're actually okay with 1 or 2?

@anba

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

I don't think these changes should be applied, because in addition to the reasons outlined above, it'll also create inconsistencies with the rest of the spec. For example compare IteratorClose, where step 8 will be ambiguous, because it's not clear if innerResult.[[Type]] means innerResult is an unwrapped normal completion whose [[Type]] field is accessed or if innerResult is a completion record.

@ljharb

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@jmdyck I stamped based on number 1; it seemed like it's a bugfix. I looked at Call, and it appears to indeed return either an abrupt completion, or a non-completion-record. However, it's entirely possible I made a mistake, and the non-completion-record gets implicitly wrapped in a NormalCompletion, which would make master correct as-is.

I agree, option 2 isn't what I'd want to see; now that you state it that way, option 3 seems preferable to me, which would mean this PR shouldn't land.

@allenwb

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Note that as originally specified in ES6 (plus requirement that internal methods return completion records) the Call abstract operation always returned a completion record. So, if it now may return both completion records and non-completion values, that is surely a bug that potentially impacts all uses of Call.

@ljharb

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Sounds like this should be closed, then?

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

For example compare IteratorClose, where step 8 will be ambiguous, because it's not clear if innerResult.[[Type]] means innerResult is an unwrapped normal completion whose [[Type]] field is accessed or if innerResult is a completion record.

It could only be ambiguous if a completion record's [[Value]] could be a record (with a [[Type]] field), which is supposedly not allowed. (6.2.3 says that [[Value]] must be an ES language value or empty.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.