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

[jakebailey]

"Disallow" is probably too strong of a term; what I mean is that we'll bail out of parsing the PropertyAccessExpression (not eating the . or ?.), and then do the "right thing" when getting to the call because that's outside of that node.

In your example, I believe the ?. token is a part of the CallExpression, not the PropertyAccessExpression, so it should work as expected. But that's a great test case I'll be sure to include.

[nicolo-ribaudo]

Actually, replying to myself: what would be the difference between a?.<b>() and a<b>?.()?

(sorry, I'm just dumping my thoughts here in case they are interesting for you 😅)

[jakebailey]

I had a silly typo:

declare interface A {
    c: number;
    <T>(): T;
}

type b = 'b type';

declare const a: A | undefined;


a?.<b>();

a<b>?.();

Playground Link

We emit:

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

Which is... weird, but functionally equivalent (same as if you do (a)?.(); the expression with type arguments node is like a paren). I'll have to see if one the latter one breaks when I fix this bug.

[ExE-Boss]

We emit:

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

Which is... weird, but functionally equivalent (same as if you do (a)?.(); the expression with type arguments node is like a paren). I'll have to see if one the latter one breaks when I fix this bug.

The second one is correct, because if a is a getter on the global object, then the former will invoke it each time, which is incorrect.

[jakebailey]

That sounds unrelated; the top one is how we emit a?.() (and I think we always have). If that's a problem, I'd suggest filing a new issue.

EDIT: I checked, and that has been the behavior since TS v3.8. In v3.7, TS did emit _a, and v3.6 and earlier didn't support that syntax at all. Seems like a tradeoff made for performance a long time ago (predating me).

[DanielRosenwasser]

In practice it's rare to trigger that behavior. We do guard against situations like these with isSimpleInlineableExpression vs. isSimpleCopiableExpression, and we do generate temp variables instead of identifiers regardless of those checks - but often that's also to guard against mutation of the identifier within the rest of the transformed construct (which cannot happen here¹). If it's a thing you're really impacted by, consider opening an issue.

^[1] Ugh with the exception of x?.[x = y]

[nicolo-ribaudo]

@DanielRosenwasser In x?.[x = y], x is reassigned after referencing it so it's safe to avoid a temporary variable.

[nicolo-ribaudo]

Hey, I realized that there is at least one case where a property access after an instantiation expression is meaningful:

function f<T extends 0 | 1 | 2 | 3>(this: T, a: T) {
   return this + a;
}

let fn1 = f<0 | 1>.bind(1);
let fn2 = f<0 | 1 | 2>.bind(1);

fn1(2); // error
fn2(2); // ok

[jakebailey]

That's an interesting case. Another example would be something like this.method<number>.bind(this).

We talked about this in the design meeting and while it feels like the true problem is the optional chaining, we're not certain enough about this syntax to allow it. We'd rather be conservative here about the syntax, given this is a hole we didn't see when the feature was merged. We can always revisit and allow this syntax, or some forms, but the main goal right now is to make sure people aren't writing spuriously incorrect code like the one in the original issue without knowing.

A workaround is to instantiate this and save it in a variable:

const fn = f<0 | 1>;
fn.bind(...)