Different behavior for `a?.b<c>.d` depending on the compilation target

[nicolo-ribaudo]

Bug Report

🔎 Search Terms

instantiation expression optional chaining target

🕗 Version & Regression Information

This feature has been introduced in 4.7. I tested 4.7.0-dev.20220528

⏯ Playground Link

Playground link with relevant code

💻 Code

a?.b<c>.d

🙁 Actual behavior

When targeting <=ES2019 it generates (a === null || a === void 0 ? void 0 : a.b).d, while when targeting >=ES2020 it generates a?.b.d.

They have different behaviors: in the ES2019 output it always reads .d, while in the ES2020 output it only reads .d if a is not nullish.

🙂 Expected behavior

They should have the same behavior.

The ES2020 behavior feels more useful, but "adding <c> to an optional chain ends the optional context" (as wrapping it in parentheses does) would be ok too as long as it's consistent. You would then have to write a?.b<c>?.d.

(Babel's implementation has the same different behavior depending on the target, so this is apparently something very easy to overlook while implementing instantiation expressions 😬)

[RyanCavanaugh]

@jakebailey good luck 🍀

[jakebailey]

The parser parses this the same as (a?.b)<c>.d.

Comparing this to a?.b.d, the trees look pretty much the same in that that's parsed like (a?.b).d, just that the new instantiation expression means that a?.b (a PropertyAccessExpression) is wrapped in a new ExpressionWithTypeArguments node.

See: Playground Link

My gut is telling me that this is just a downleveling bug and we just aren't skipping past the type arguments somewhere (since the type args won't be emitted).

[jakebailey]

That being said, I think there's also a checker bug here that's related. In writing a test case, I get a confusing Object is possibly 'undefined' error if there's an instantiation expression present.

declare namespace A {
    export class b<T> {
        static d: number;
        constructor(x: T);
    }
}

type c = unknown;

declare const a: typeof A | undefined;

a?.b<c>.d; // Error

a?.b.d // No error

Playground Link

[jakebailey]

Confusingly, I don't always get this error in my test, but I see it in the editor and on the playground.

[jakebailey]

I think there's some problematic AST API surface that was missed when we added instantiation expressions; specifically, we have the notion of "access chain" branded types (PropertyAccessChain | ElementAccessChain | CallChain | NonNullChain, see isOptionalChain) which rely on the fact that we only see ProperyAccess, ElementAccess, etc, but now there's a new node (ExpressionWithTypeArguments) that can appear in these chains and get in the way of those checks.

Because we parse a?.b<c>.d like (a?.b)<c>.d and not (a?.(b<c>)).d, the optional chain helpers don't work until whatever's using them recurses down into the ExpressionWithTypeArguments, hence this transform/emit bug, as well as the possibly undefined error I'm seeing above.

@ahejlsberg @rbuckton may have thoughts here.

[jakebailey]

Hm, scratch that; given it's a PropertyAccessExpression with a ExpressionWithTypeArguments inside of it, those branded types should be fine. I think it's probably more that skipPartiallyEmittedExpressions doesn't skip those arguments.

I'll keep digging and stop pinging people until I have a real view.

[jakebailey]

It seems to be a mix of everything I listed above; for example isOptionalChain and other functions like it make assumptions about parents and such, including code like:

export function isExpressionOfOptionalChainRoot(node: Node): node is Expression & { parent: OptionalChainRoot } {
    return isOptionalChainRoot(node.parent) && node.parent.expression === node;
}

Which use a type guard to say that a parent is a chain root, when for example, parent.expression may be a ExpressionWithTypeArguments that then contains the current node. These helpers are used all over the checker, and I am guessing are part of my error problem.

Then in es2020.ts, the extra node breaks chain detection because it doesn't know to keep looking upward. This one seems more approachable, but also uses those same branded types and helpers.

[jakebailey]

We talked about this in today's design meeting. What we're going to do is to disallow a . (probably also ?.) after the type arguments of an ExpressionWithTypeArguments in the parser, rather than trying to fix everything else (the helpers, the checker, the emitter, etc).

The rationale here is that if I write code that looks like this:

a.b<c>.d

That because I haven't actually instantiated anything yet (new'd the thing I instantiated, called, whatever), any property of b<c> cannot depend on the type arguments given. For example, in my test case:

declare namespace A {
    export class b<T> {
        static d: number;
        constructor(x: T);
    }
}

type c = 'c type';

declare const a: typeof A | undefined;

a?.b<c>.d;

d is a static class property, which is not allowed to depend on T. Therefore, the "right" thing would have been to write a?.b.d, because <c> does nothing.

[nicolo-ribaudo]

That sounds reasonable!

[nicolo-ribaudo]

Q: If you will disallow a<b>?.c, did you also talk about a<b>?.()? (I don't think that ?.( should be disallowed, since it's a function call and not a property access).