Narrow on element access of known property

[sandersn]

Fixes #10530

Narrowing now happens for x['knownProperty'] the same way that it does for x.knownProperty

[sandersn]

@RyanCavanaugh @ahejlsberg do you want to take a look?

[mhegazy]

the expr.expression should be, an identifier, this, a PopertyAccessExpression or ElementAccessExpression , so we should recursively call the function on the expression.

[mhegazy]

we should also add tests to multiple levels of elementAccess. a[0][1]

[sandersn]

Actually, multiple levels of property access don't work either. So I think this function should actually be even simpler until we fix that bug.

interface Square {
  sub: { kind: "square; };
  size: number;
}
// etc ...
function area(s: Shape) {
    switch(s.sub.kind) {
        case "square": return s.size * s.size;
        case "rectangle": return s.width * s.height;
        case "circle": return Math.PI * s.radius * s.radius;
    }
}

[sandersn]

Nested element accesses are now supported.

[sandersn]

@mhegazy you started reviewing this change. Can you look at the new commits so it can go in?

[sandersn]

I ran a performance benchmark on the code compared to master. The standout difference was that Monaco (the online component that powers the playground) has a 6.85% slowdown in the checker, or 0.41 second. TFS has a small slowdown in the checker but it's not statistically significant.

I'm trying to get github formatting to co-operate so I can show the actual table but haven't had any luck yet.

[sandersn]

As @mhegazy suggested, I tried the benchmark again, this time only narrowing string element accesses, not number accesses. The results are about the same.

[mhegazy]

given the performance penalty vs. utility of this change, we will not be able to take it.

[kachkaev]

Would it be possible to revise the decision on this PR 1.5 years since the original implementation? Will the performance impact be so critical now too?

When a property has spaces or other no-standard characters, it is impossible to either define it or read its value via x.knownProperty. This makes it impossible to validate dictionaries configs., e.g.:

const config = {
  param: 42,
  "param 2": 43,
}

console.log(config.parammmmm); // <--- typo caught by the compiler
console.log(config["parammmmm 2"]); // <--- typo not caught

[mhegazy]

i do not think the error you listed above has do with this PR. The issue here is that it is legal to index into any property using a string/numeric name. the result is an implicit any. This should be an error under --noImplicitAny.

[kachkaev]

I'm not sure that --noImplicitAny would work for me here, because I'll have to add type annotation when defining config and the code will become too verbose. --noImplicitAny also affects function argument definition, which I'd prefer to avoid.

[m1gu3l]

could this perhaps land behind a flag? or maybe just for numeric properties accessed via brackets?

interface X {
    0: "xx",
    1: number
}

interface Y {
    0: "yy",
    1: string
}

type A = ["aa", number];
type B = ["bb", string];

type Z = X | Y;

type C = A | B;

function check(z: Z, c: C) {
    z[0] // fine, typescript sees "xx" | "yy"
    switch (z[0]) {
        case "xx":
            z[1] // should be number, typescript sees string | number
            break;
        case "yy":
            z[1] // should be string, typescript sees string | number
            break;
    }
    c[0] // fine, typescript sees "xx" | "yy"
    switch (c[0]) {
        case "aa":
            c[1] // should be number, typescript sees string | number
            break;
        case "bb":
            c[1] // should be string, typescript sees string | number
            break;
    }
}

[sandersn]

I reconstituted the PR and found (1) a probably performance bug that it introduced in isMatchingReference and friends (2) no significant difference in performance after fixing the bug. I'll open a new PR with the details and some tests.

[BalaM314]

Can we have this behind a flag? I've had to add dozens of non-null assertions to fix this and would gladly take the 6.5% performance hit.

[thelinuxlich]

I agree, the current behavior is unintuitive and should be fixed.

[phaseOne]

For what it's worth, I've verified that noImplicitAny/strict addresses the case in #10565 (comment).

For my use case, strict is fine to enable. The only reason I'm making this post is that it's unintuitive that noImplicitAny controls type checking for property accessors (bracket notation) in common cases like the aforementioned, so hopefully, this post enables the algorithm to help other users find the workaround and latest news.

It's purported that #10530 is tracking the issue and #48335 is tracking new documentation for the current behavior and workarounds.

[Zzzen]

Would it be possible for us to simply rebase this branch and rerun the benchmarks to get a clearer picture of the real-world performance?

[DamianGlowala]

@sandersn, what do you think about the above question? Could we give this another try, please?

[RyanCavanaugh]

One does not "simply" rebase a 7-year-old PR 😅

The example in the linked issue, and the example in this comment #10565 (comment), both now work. Can you clarify what you're looking for?

[DamianGlowala]

Now as I am reading the purpose of this PR again, I certainly commented on the wrong one. I was trying to figure out why TS doesn't narrow down the type in this specific scenario.