Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

pre-Stage 3 review #42

Closed
ljharb opened this issue Jul 17, 2019 · 21 comments
Closed

pre-Stage 3 review #42

ljharb opened this issue Jul 17, 2019 · 21 comments

Comments

@ljharb
Copy link
Member

ljharb commented Jul 17, 2019

(you'll also need signoff from the designated reviewers, and from @zenparsing)

  • I wonder if maybe "NullishExpression" should be "NullishORExpression", so it mirrors "LogicalORExpression"?

Otherwise it seems good.

(It may be useful to merge this and optional chaining into one stage 3 proposal, if both are ready to advance, especially since there's a lot of spec interaction between them)

@rkirsling
Copy link
Member

I wonder if maybe "NullishExpression" should be "NullishORExpression", so it mirrors "LogicalORExpression"?

Now that you mention it, I think "coalescing" is really the more important word here—even "CoalesceExpression" would likely suffice for uniqueness.

@DanielRosenwasser
Copy link
Member

Ugh, you're right but I was hoping to change it later. *gets to work*

@DanielRosenwasser
Copy link
Member

Should I change the grammar flag to Coalesced?

@rkirsling
Copy link
Member

Hmm, maybe just Coalesce? (Since I think the + is meant to be read like "has".)

@jridgewell
Copy link
Member

I don't think Coalesce is a good flag name, it's not clear what it's doing. As for the Grammar name, maybe LogicalNullishExpression?

@DanielRosenwasser
Copy link
Member

I think I'm going to run with CoalesceExpression with Coalesced as the grammar flag.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 18, 2019

#43 is merged. @rkirsling @codehag @zenparsing, would you be able to review the current spec text?

@rkirsling
Copy link
Member

LGTM (and confirmed ease of implementation using JSC 😁). Still rooting for #40!

@codehag
Copy link

codehag commented Jul 19, 2019

LGTM!

@zenparsing
Copy link
Member

Spec text 👍

@ljharb
Copy link
Member Author

ljharb commented Jul 19, 2019

Fresh LGTM

(note that we should probably find a way to make sure that document.all ?? x evaluates and returns document.all, despite that document.all == null - or, maybe it should return x? i'm not sure)

@jridgewell
Copy link
Member

jridgewell commented Jul 19, 2019

(note that we should probably find a way to make sure that document.all ?? x evaluates and returns document.all, despite that document.all == null - or, maybe it should return x? i'm not sure)

We've already done that (it returns document.all). Does the current spec text not match that?

@ljharb
Copy link
Member Author

ljharb commented Jul 19, 2019

Grammar isn't my strong suit, but I don't see where an object with an [[IsHTMLDDA]] internal slot gets special treatment - saying "is null or undefined" doesn't include document.all unless you use ToBoolean or the AbstractEqualityComparison algorithm.

Perhaps what's needed is an addition to https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot ?

@rkirsling
Copy link
Member

rkirsling commented Jul 20, 2019

Is there something that obliges us to give it special treatment here?
If Annex B.3.7 states that document.all == null is true, it seems like that's all there is to say.


Edit: Er whoops, I was trying to say that document.all ?? x should thus be x, but I think @ljharb is right that "is undefined or null" in the spec text doesn't suffice to achieve this. If we want == null semantics then it seems we do need to explicitly reference Abstract Equality Comparison (both here and in Optional Chaining).

@jridgewell
Copy link
Member

document.all ?? x is supposed to evaluate to document.all. That's why the examples and desugaring are using left === null || left === undefined.

Specifying "val is null or undefined" should be enough to distinguish that document.all is not null or undefined. Eg, Object.prototype.toString.call(document.all) (which is similarly spec'ed) and doesn't return the string "[object Undefined]" nor "[object Null]".

@ljharb
Copy link
Member Author

ljharb commented Jul 20, 2019

Got it, thanks for clarifying :-)

I think you’re right that the current spec means that document.all ?? x will be document.all, and thus no changes are needed.

@rkirsling
Copy link
Member

rkirsling commented Jul 20, 2019

Specifying "val is null or undefined" should be enough to distinguish that document.all is not null or undefined.

That is true, but ignoring Annex B is effectively creating a special case here. In particular:

  1. It does not correspond with existing ways of performing ?? or ?.:
    document.all[0] is valid on its own but none of the following return it.
    • (document.all || [])[0]
    • document.all && document.all[0]
    • document.all == null ? undefined : document.all[0]
  2. It does not correspond with the stated semantics of the Optional Chaining README.
  3. It kicks the burden to implementations instead: for instance, JSC bytecode literally has OpNeqNull.

If we're going to give document.all this kind of special treatment, we definitely need a strong reason.
(I don't think that Object.prototype.toString constitutes such a reason, because it is conceptually unrelated to nullishness and truthiness alike.)

@ljharb
Copy link
Member Author

ljharb commented Jul 20, 2019

I think that this is a lack of special treatment, since document.all can be navigated into already without throwing.

@rkirsling
Copy link
Member

So here's what WHATWG states as the goal of this bewildering "masquerade-as-undefined" behavior:

These special behaviors are motivated by a desire for compatibility with two classes of legacy content: one that uses the presence of document.all as a way to detect legacy user agents, and one that only supports those legacy user agents and uses the document.all object without testing for its presence first.

That is, the aim is to have feature checks for it always fail while having its actual functionality preserved.

Now, even if we throw out all presuppositions about desugaring and strictly focus on surface intent, I want to argue that document.all ?? [] and document.all?.[0] are nothing other than feature checks, but the situation admittedly gets rather muddy when it's reassigned to another variable.

Say we have a function third(x) which returns x[3] if possible and otherwise undefined. We surely would like to implement this as return x?.[3];. I suppose in that case having third(document.all) return undefined could be viewed as interfering with its Array-like functionality.

@ljharb
Copy link
Member Author

ljharb commented Jul 20, 2019

There's not going to be any new code written with ?? tho, that's checking for legacy user agents with document.all.

@rkirsling
Copy link
Member

Hmm, perhaps that is the best way to look at it. I guess I'm alright with this then, we just need to update the Optional Chaining README (at least, where it says "The desugaring is not exact in the sense that...").

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

No branches or pull requests

6 participants