tc39 / proposal-nullish-coalescing

Nullish coalescing proposal x ?? y
https://tc39.github.io/proposal-nullish-coalescing/
1.23k stars 23 forks source link

pre-Stage 3 review #42

Closed ljharb closed 5 years ago

ljharb commented 5 years ago

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

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 commented 5 years ago

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 commented 5 years ago

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

DanielRosenwasser commented 5 years ago

Should I change the grammar flag to Coalesced?

rkirsling commented 5 years ago

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

jridgewell commented 5 years ago

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 commented 5 years ago

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

DanielRosenwasser commented 5 years ago

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

rkirsling commented 5 years ago

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

codehag commented 5 years ago

LGTM!

zenparsing commented 5 years ago

Spec text 👍

ljharb commented 5 years ago

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 commented 5 years ago

(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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

rkirsling commented 5 years ago

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 commented 5 years ago

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

rkirsling commented 5 years ago

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...").