pre-Stage 3 review

[ljharb]

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

although it looks OK, could you add <ins>/<del> tags around all individual changes in existing algorithms, like https://tc39.es/proposal-optional-chaining/#sec-function-calls-runtime-semantics-evaluation? It's tricky to review without the markup.

[DanielRosenwasser]

I'm not necessarily as familiar with the spec text as @claudepache and @jridgewell. Would either of you be able to help? If not, I I will try to take some time to do so later tonight or tomorrow. @zenparsing @rkirsling @bcoe, let me know when we can get explicit signoff.

[jridgewell]

I don't believe there are any changes in

• https://tc39.es/proposal-optional-chaining/#sec-property-accessors
• https://tc39.es/proposal-optional-chaining/#sec-function-calls

They're leftovers from when we had nil values.

[ljharb]

In that case perhaps those sections could be removed?

[DanielRosenwasser]

103 has the appropriate diff tags for the requested sections, and a few more. Hope that is helpful!

[DanielRosenwasser]

Okay, #103 is merged. Awaiting signoff from our fabulous reviewers. 😃

[rkirsling]

Wow, that PR made things way easier to read!

Spec content looks good, though I see two minor editorial concerns:

1. We probably ought to resync 3.5 with the current spec, as it seems rather out of alignment.
2. 3.1.1 is not highlighted on the sidebar while 3.2.2 and 3.2.3 are, due to the difference in <ins> usage.

---

Incidentally, I noticed while checking (1) that single-line versus multi-line productions are getting mixed for some pretty yucky results over there. 😮 We should track that separately.

[DanielRosenwasser]

I'm actually going to be traveling today. Would you be able to list out specific differences in alignment? I'll try to take care of it by Monday if I can. Alternatively, you can send a direct PR for anything that looks a little off.

[ljharb]

Fresh LGTM.

Would it make sense to upstream the EvaluateStaticPropertyAccess and EvaluateDynamicPropertyAccess operations to the main spec now, so the resulting diff for this proposal is smaller? It seems like it'd be a useful refactoring as-is. (also the editorial change here)

[bcoe]

:+1: on removing any sections left over from the nil reference proposal.

re: we probably ought to resync section 3.5

Agreed (figured we'd do this as late as possible) but it would be great to make sure we're in alignment with the current version of the spec.

Would it make sense to upstream the EvaluateStaticPropertyAccess and EvaluateDynamicPropertyAccess operations to the main spec now

@ljharb I like the idea of potentially landing EvaluateStaticPropertyAccess and EvaluateDynamicPropertyAccess upstream, in the name of simplifying the spec text :+1: (it seems like doing so adds clarity).

re: sign off

This is looking good to me :+1: I'm excited to give it a final read through as we address some of the comments in this thread; but nothing is looking out of place to me.

[zenparsing]

The spec has some cases where syntax-directed runtime operations are invoked like x.op(...) that will need to be updated but:

👍

[DanielRosenwasser]

It sounds like we have the appropriate signoffs! 🚀

While it also appears that the recommendations here are not blockers, can I get a few concrete examples of the suggestions here to ensure that this proposal is in good shape for the meeting? For example

• places 3.5 is out of sync
• where the x.op(...) syntax is currently used

[DanielRosenwasser]

(also, I will give the upstream changes a shot, but it sounds like that is something that can happen regardless of stage 2/3, so it is not an immediate priority)

[rkirsling]

For 3.5, I just mean that while all the content is there, it's shaped as "here are the three chunks that need to be inserted in the right places of the appropriate section" instead of actually copying over said appropriate section and performing the insertions. :smile:

[rkirsling]

Also, about the cosmetic discrepancy of 3.1.1 (and also 3.4) vs. 3.2.2 and 3.2.3: I think it's probably 3.2.2 and 3.2.3 that are doing the odd thing by not using <ins class="block">, but if we upstream those two then it'll be moot anyway.

[Mouvedia]

What are the exceptions?

e.g. I suppose that new?.target is not supported since new "is not really an object". It seems kind of weird to have new.target?.prototype—even if it seems useless—but not new?.target?.prototype.

[ljharb]

new can’t ever be null or undefined, so I’m not sure how valuable that’d be. Similarly there wouldn’t be import?.meta or super?.foo

[Mouvedia]

I just find it visually inconsistent. It's clearly justified though, but for the newcomer it sure looks like an object.

or super?.foo

super?.foo may be useful if you have a function that is shared and behave differently when you don't have a parent. That would require to check what currently happens when super is used in a parent-less class.

Are the exceptions import, super and new?

[ljharb]

When would super sometimes work and sometimes not? I believe super is a syntax error when not inside a derived class method.

[Mouvedia]

I believe super is a syntax error when not inside a derived class method.

Wouldn't surprise me; never tried though. Anyway my remark was more about consistency than usefulness. What about a mention in the README?

[bcoe]

@Mouvedia @ljharb

> class Foo { constructor () {super()} }
Thrown:
class Foo { constructor () {super()} }
                            ^^^^^

SyntaxError: 'super' keyword unexpected here

I think this case, and similar cases like new.target in the wrong context, are covered by the FAQ section:

The feature looks like an error suppression operator, right?

No. Optional Chaining just checks whether some value is undefined or null. It does not catch or suppress errors that are thrown by evaluating the surrounding code. For example:

---

Perhaps it's worthwhile to add a couple more examples in this section though?

[claudepache]

Optional new as in new Foo?.() is already mentioned in paragraph “Not supported”.

Or are you thinking about new?.target? Uh, well, I don’t think it’s worth saying “We don’t support optional chaining in context where it doesn’t make sense”. Ditto for import?.("...").

For super, maybe one can give some theoretical meaning? but unless you have a reasonable use case, I won’t think hard about that.

[claudepache]

@bcoe The FAQ item “The feature looks like an error suppression operator, right?” was alluding to runtime errors, not syntax errors.

[rkirsling]

Are the exceptions import, super and new?

The thing is though, that none of these three are PrimaryExpressions—the valid productions which contain them are listed off one by one, which kind of makes them more like operators than identifiers.

I don't think this should be surprising for import or new, but it may be surprising for super, if folks are thinking that super is more like this than it actually is. In that regard, it may be worth calling out as not supported (with a link to #4, which appears to be whether the relevant discussion took place).

[claudepache]

I’ve open PR #106 in order to address the issue raised in the last comments. Please, stop hijacking this thread, and continue the discussion there.

[claudepache]

Closing as obsolete