Port `?.` emit #1385

weswigham · 2025-07-10T21:18:07Z

Which is the remainder of the es2020 transform.

There are some discrepancies in parenthesis (we over-parenthesize in some cases) in the emit, seemingly caused by changes to how we parenthesize in general.

Copilot

Pull Request Overview

This PR completes the ES2020 optional chaining (?.) emit transform by updating test baseline files to switch between concise ?. syntax and the expanded null‐check form according to target settings.

Normalizes emitted optional chaining patterns across conformance tests
Reverts output to expanded nullish checks in ?. emitless contexts
Aligns test baselines for static and instance property/method access and calls

Reviewed Changes

Copilot reviewed 130 out of 130 changed files in this pull request and generated 1 comment.

File	Description
testdata/baselines/reference/submodule/conformance/typeOfThisInstanceMemberNarrowedWithLoopAntecedent.js	Fixed optional‐chain emit for `this.state.data` access
testdata/baselines/reference/submodule/conformance/thisTypeOptionalCall.js	Updated `fn?.bind` emit vs expanded form
testdata/baselines/reference/submodule/conformance/*	...many other conformance baseline updates

...elines/reference/submodule/conformance/typeOfThisInstanceMemberNarrowedWithLoopAntecedent.js

jakebailey · 2025-07-10T21:27:41Z

There are some discrepancies in parenthesis (we over-parenthesize in some cases) in the emit, seemingly caused by changes to how we parenthesize in general.

Yeah, that all is WIP and not done :(

Related is #1055 #1123

jakebailey · 2025-07-10T21:28:59Z

...ata/baselines/reference/submodule/compiler/assertionFunctionsCanNarrowByDiscriminant.js.diff

@@ -7,8 +7,4 @@
 -"use strict";


I wonder if this very common diff is an easy fix somewhere...

This is a script file by strada logic. (no exports/imports, nothing forcing module mode parsing.) In corsa, the module transforms add use strict if needed, but in strada, the ts transform does (via visitLexicalEnvironment in visitSourceFile) so it's done for script files (and I think the module transforms, redundantly, also do). Obviously the module transforms don't touch script files, so they don't add the prologue here.

jakebailey · 2025-07-10T21:30:08Z

...eference/submodule/compiler/optionalChainWithInstantiationExpression2(target=es2019).js.diff

-var _a;
-a === null || a === void 0 ? void 0 : a();
+ var _a;
+ a === null || a === void 0 ? void 0 : a();
 -(_a = (a)) === null || _a === void 0 ? void 0 : _a();


Wow, a place where Strada over-parenthesized 😄

Yeah, strada went "oh, this is a cast that became a partially emitted expression, gotta add some parens as we build it", but in corsa, with parens added on print, we're like "oh, we're in an assignment and this is a partially emitted expression of an identifier? Nah, no need for parens".

jakebailey · 2025-07-10T21:31:43Z

testdata/baselines/reference/submodule/conformance/elementAccessChain.3.js.diff

-    ;
-({ a: obj === null || obj === void 0 ? void 0 : obj["a"] } = { a: 1 });
-({ a: obj === null || obj === void 0 ? void 0 : obj.a["b"] } = { a: 1 });
+(obj === null || obj === void 0 ? void 0 : obj["a"]) = 1;


This seems to be incorrect, right? This is trying to assign 1 to the parenthesized expression?

This is the structure we make in strada for these inputs, too, just without the parenthesis. The strada output does not roundtrip - the parens are actually required. And yes, they do make it clear that the output is... questionable. But that's fine, these all have a The left-hand side of an assignment expression may not be an optional expression error, which'd be why it's not handled sensibly in the transform - it's completely invalid.

jakebailey · 2025-07-10T21:33:17Z

...eference/submodule/conformance/optionalChainingInParameterBindingPattern(target=es5).js.diff

This seems funky. (But maybe still equivalent?)

This is just the >es5 output, since there's no es5 transform. In <=es5, we used to just emit a var _a in the body following the parameter list, which, pre-es6, was valid.

DanielRosenwasser · 2025-07-11T22:10:22Z

internal/transformers/estransforms/optionalchain.go

+func (ch *optionalChainTransformer) visitCallExpression(node *ast.CallExpression, captureThisArg bool) *ast.Node {
+	if node.Flags&ast.NodeFlagsOptionalChain != 0 {
+		// If `node` is an optional chain, then it is the outermost chain of an optional expression.
+		return ch.visitOptionalExpression(node.AsNode(), captureThisArg, false)


Can you restore the isDelete comments?

I can... but the go formatting for it is weird af, placing the comment before the preceding comma. I've been avoiding most of those comments since I noticed it. With only two callsites that aren't named variables (both of which pass false), I'm not too worried about this one being too confusing.

DanielRosenwasser · 2025-07-11T22:22:31Z

internal/transformers/estransforms/optionalchain.go

+		return ch.visitOptionalExpression(node.AsNode(), captureThisArg, isDelete)
+	}
+	expression := ch.Visitor().VisitNode(node.Expression())
+	// Debug.assertNotNode(expression, isSyntheticReference); // !!!


Does anything go wrong if you panic here?

These asserts were to satisfy the TS typechecker and remove a type from a union type. Technically, they're not actually required anymore, but if they're added back, they should be in the same systematic debug-build-specific style, not as ad-hoc panics.

DanielRosenwasser · 2025-07-11T23:06:08Z

internal/transformers/estransforms/optionalchain.go

+	if unwrapped.Flags&ast.NodeFlagsOptionalChain != 0 {
+		return ch.visitNonOptionalExpression(node.Expression, false, true)
+	}
+	return ch.Visitor().VisitEachChild(node.AsNode())


Any reason you do this instead of the original-ish

Suggested change

return ch.Visitor().VisitEachChild(node.AsNode())

deletedExpr := ch.Visitor().VisitNode(node.Expression)

return ch.Factory().UpdateDeleteExpression(node, deletedExpr)

Because they're equivalent (as best I can tell). Why manually pull out and update the child of the node, when visitEachChild does that already.

True, though it's technically doing a bit more work under the hood.

DanielRosenwasser · 2025-07-11T23:17:28Z

internal/transformers/estransforms/optionalchain.go

+		links = append([]*ast.Node{chain}, links...)
+	}


I know that this was the original but it seems so not-ideal - why didn't we just keep appending and reverse?

Suggested change

links = append([]*ast.Node{chain}, links...)

}

links = append(links, chain)

}

slices.Reverse(links)

More often than not, these flattened chains are single element - it's not often people do a a?.b?.c?.d deal. Given that, I think the original is fine for the single or even two element cases. It's not something I'd think needs optimization, especially since reversing a slice isn't great. If anything, you'd do a quick decent to count the chain length, preallocate the slice (probably from a pool, if this was actually hot code), and then insert directly at target position as you descend.

DanielRosenwasser · 2025-07-11T23:22:04Z

internal/transformers/estransforms/optionalchain.go

+	if ast.IsSyntheticReferenceExpression(left) {
+		leftThisArg = left.AsSyntheticReferenceExpression().ThisArg
+		capturedLeft = left.AsSyntheticReferenceExpression().Expression


Suggested change

if ast.IsSyntheticReferenceExpression(left) {

leftThisArg = left.AsSyntheticReferenceExpression().ThisArg

capturedLeft = left.AsSyntheticReferenceExpression().Expression

if ast.IsSyntheticReferenceExpression(left) {

left := left.AsSyntheticReferenceExpression()

leftThisArg = left.ThisArg

capturedLeft = left.Expression

I don't think that kind of shadowing is a great idea (left the node being shadowed by left the node data) - don't we have a lint rule forbidding it? Two uses like this kinda aren't enough to bother, anyway, since it's not clarifying or simplifying much of anything. Unless someone shows me the casts having an outsized impact on perf, which is going to make me scream, since old casts were free-and-erased so never really needed perf scrutiny.

All of the calls and casts have cost, I'm afraid

(but really, don't think about it)

It just gets a bit unwieldy, and I end up thinking about it :D

DanielRosenwasser · 2025-07-11T23:24:00Z

internal/transformers/estransforms/optionalchain.go

+					thisArg = rightExpression
+				}
+			}
+			if segment.Kind == ast.KindElementAccessExpression {


nit - the check is inverted compared to the original code

As are the bodies in the branches of the check. E comes before P, so E comes first in the case clause list, so handling the first case explicitly is a bit more natural looking.

DanielRosenwasser · 2025-07-11T23:30:38Z

internal/transformers/estransforms/optionalchain.go

+			rightExpression,
+		)
+	}
+	target.Loc = node.Loc


Any reason you have to do this? I see you run SetOriginal farther below. The ordering is a little bit different here though. I would have thought you'd run SetOriginal here based on the original code.

.Loc = .Loc is equivalent to SetTextRange, which is independent from SetOriginalNode (text ranges control comment and source map spans, original nodes control emit resolver API nodes and in some cases fallback location spans) - and the original nodes are set in new places now because we now have an assert if we try and set original multiple times (to different things), so all these codepaths now reliably set originals when they're done constructing the new node, rather than sometimes doing it and sometimes letting the caller handle it.

DanielRosenwasser

I haven't looked closely at the test cases, but the porting looks precise enough apart from comments I left.

Port optional chain emit

9ca86e3

weswigham requested review from sandersn, jakebailey and Copilot July 10, 2025 21:18

Copilot AI reviewed Jul 10, 2025

View reviewed changes

...elines/reference/submodule/conformance/typeOfThisInstanceMemberNarrowedWithLoopAntecedent.js Show resolved Hide resolved

jakebailey reviewed Jul 10, 2025

View reviewed changes

DanielRosenwasser reviewed Jul 11, 2025

View reviewed changes

Merge branch 'main' into optchain

42e9301

DanielRosenwasser approved these changes Jul 14, 2025

View reviewed changes

weswigham added this pull request to the merge queue Jul 14, 2025

Merged via the queue into microsoft:main with commit 6c17bea Jul 14, 2025
22 checks passed

weswigham deleted the optchain branch July 14, 2025 20:46

	return ch.Visitor().VisitEachChild(node.AsNode())
	deletedExpr := ch.Visitor().VisitNode(node.Expression)
	return ch.Factory().UpdateDeleteExpression(node, deletedExpr)

-		links = append([]*ast.Node{chain}, links...)
-	}
+		links = append(links, chain)
+	}
+    slices.Reverse(links)

Port ?. emit #1385

Port ?. emit #1385

Uh oh!

Conversation

weswigham commented Jul 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

jakebailey commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Port `?.` emit #1385

Port `?.` emit #1385

weswigham Jul 14, 2025 •

edited

Loading

weswigham Jul 14, 2025 •

edited

Loading

jakebailey Jul 10, 2025 •

edited

Loading

DanielRosenwasser Jul 11, 2025 •

edited

Loading

weswigham Jul 14, 2025 •

edited

Loading