Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type guard "in" for undefined not working when using strictNullChecks + noUncheckedIndexedAccess #54241

Closed
kkmuffme opened this issue May 13, 2023 · 7 comments
Labels
Duplicate An existing issue was already created

Comments

@kkmuffme
Copy link

Bug Report

type guard in undefined array indexed access

🔎 Search Terms

🕗 Version & Regression Information

  • This changed from version 4 with the introduction of noUncheckedIndexedAccess
  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________
  • I was unable to test this on prior versions because _______

⏯ Playground Link

https://www.typescriptlang.org/play?noUncheckedIndexedAccess=true&filetype=ts#code/FAYw9gdgzgLgBAQwEYgFxwEoFNwCcAmAPLLgJYQDmANCeRQHxwC8cA3gL4DcwwAZgK4QQMUpDhIEuABSJcFdLUpwAlG2BwNcUrzgzJFLREQoVazebjho8fAhgJmxkAG1ZBgLrdz7YD6A

💻 Code

const abc: Record<string,string> = {};

function bar( arg: string ) {
    if ( arg in abc ) {
        const data = abc[ arg ];
    }
}

🙁 Actual behavior

data is string|undefined despite it being typeguarded with "in" on a Record<string, string> which means that it cannot be undefined

🙂 Expected behavior

data should be string

The current bug causes either lots of redundant code (as we have to typeguard for undefined again separately) or disabling noUncheckedIndexAccess which would snooze tons of potential bugs though.

@jcalz
Copy link
Contributor

jcalz commented May 13, 2023

Being tracked at #10530. The issue is that arg is of type string and not a string literal type. Observe the difference:

const abc: Record<string, string> = {};
function bar() {
    const arg = "k"
    if (arg in abc) {
        const data = abc[arg];
        // const data: string
    }
}

Until and unless #10530 is fully resolved, the most ergonomic way to do what you're doing is to give up on in and just check for undefined:

function baz(arg: string) {
    const data = abc[arg];
    if (data !== undefined) {
        data;
        // const data: string
    }
}

Playground link to code

@kkmuffme
Copy link
Author

That issue is vaguely similar at best - the referenced issue refers to all (other) keys to be inferred as non-undefined and only for LITERAL strings.

However here it's about a non-literal string which has explicitly been checked with in
and not about inferring any other keys/types that might exist on the object too.

@jcalz
Copy link
Contributor

jcalz commented May 13, 2023

Sigh, no it's the right issue, it's just that the description isn't quite specific enough anymore. I'd edit the title if I could. See #53454 for example.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 16, 2023

This was the entire concern of the feature in the first place when we implemented it. People wanted a sound and convenient method to check for array bounds or key inclusion automatically. And we said, "We don't have any way to do that since both index and map/array mutation can happen at almost any time," and everyone said, "No, that's fine. We can deal with the inconvenient but sound version, having anything is better than nothing for sure." So that's what we have, and indeed it is inconvenient in some cases.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label May 16, 2023
@microsoft-github-policy-service

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

1 similar comment
@microsoft-github-policy-service

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@fatcerberus
Copy link

@RyanCavanaugh I suppose there's an argument to be made that naming a feature "no unchecked indexed access" was misleading when it errors even in cases where the index is checked (via in, e.g.), and the enforced contract is actually that you check the value you receive, not the indexed access itself (which can remain unguarded).

But yeah, this is a good case-in-point for why noUncheckedIndexedAccess isn't the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants