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

let indexes using noUncheckedIndexedAccess in TS 5.5.2 #58972

Closed
ex0ns opened this issue Jun 22, 2024 · 6 comments
Closed

let indexes using noUncheckedIndexedAccess in TS 5.5.2 #58972

ex0ns opened this issue Jun 22, 2024 · 6 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@ex0ns
Copy link

ex0ns commented Jun 22, 2024

πŸ”Ž Search Terms

"array", "noUncheckedIndexedAccess", "5.5"

πŸ•— Version & Regression Information

  • This changed between versions 5.4.5 and 5.5.2

⏯ Playground Link

https://www.typescriptlang.org/play/?noUncheckedIndexedAccess=true&noUnusedLocals=true&noUnusedParameters=true&ts=5.5.2#code/MYewdgzgLgBAhgLhmArgWwEYFMBOBtAXRgF4Y8BGAGhgCZqBmAgbgCgAbLWASyVU1xIxyrFlwBmACigBPAA5YQY+Hi5Fi6mACI+2HJoCULGMeWqA1GaA

πŸ’» Code

const a: number[] = [1, 2, 3];
let i: number = 1;

if(typeof a[i] === "number")
    a[i]++

πŸ™ Actual behavior

Typescript reports the following error:

Object is possibly 'undefined'.(2532)
const a: number[]

πŸ™‚ Expected behavior

It should not report an error for such cases

Additional information about the issue

This obviously requires noUncheckedIndexedAccess to be enabled in the tsconfig.

This only happens when the variable used to access the array is non const (let in this case).

I think that TS 5.5 actually fixed the original behavior because in 5.4 if we remove the check:

const a: number[] = [1, 2, 3];
let i: number = 1;
a[i]++

TS does not raise the same error (which would be expected as a[i] could be undefined).
The current workaround:

const j = i;
if (typeof digits[j] === "number") {
  digits[j]++;
}
@ex0ns ex0ns changed the title noUncheckedIndexedAccess in TS 5.5.2 let indexes using noUncheckedIndexedAccess in TS 5.5.2 Jun 22, 2024
@MartinJohns
Copy link
Contributor

I think that TS 5.5 actually fixed the original behavior

See: https://devblogs.microsoft.com/typescript/announcing-typescript-5-5/#control-flow-narrowing-for-constant-indexed-accesses

And also the PR: #57847

This intentionally is only working for constant-like variables.

@fatcerberus
Copy link

fatcerberus commented Jun 22, 2024

This does seem to meet the criteria in #57847 though:

With this PR we perform control flow analysis for element access expressions obj[key] where key is a const variable, or a let variable or parameter that is never targeted in an assignment.

If the initializer counts as an "assignment" in this context then it effectively doesn't work for let variables at all and that clause shouldn't be there.

@Andarist
Copy link
Contributor

Heh, this is a fun one... in a sense this is surprising. TS thinks i is global here. If we just add export {} to the end of this file then it works like expected: TS playground

Even though it doesn't exactly look like it... this feels like a duplicate of #58345 . Obviously, this repro is wildly different from the other one but the implementation source of the problem is very likely the same

@ahejlsberg
Copy link
Member

ahejlsberg commented Jun 23, 2024

This is working as intended. We specifically exclude global variables from assignment analysis as the control flow analyzer can't see all references (there might be references in other files). Adding an export {} limits the scope to the current file at which point CFA can determinate the location of the last assignment and treat the variable as const for the remainder of the file.

@ahejlsberg ahejlsberg added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 23, 2024
@ex0ns
Copy link
Author

ex0ns commented Jun 23, 2024

Thanks, for the clarifications, I was simply surprised because the behavior in a very "hello world" code is now a bit confusing for non-experienced people:

function goThroughArray() {
  const a: number[] = [1, 2, 3];
  for(let i = 0; i < a.length; i++) {
    if (typeof a[i] === "number") {
        a[i]++; // a[i] can be undefined
      }
  }
}

Which is not exactly the reproducer I have send in the original issue, as I was trying to narrow this down.

However given that the compiler can't know if i was modified in the loop or not, I guess it's the best we can do for now.

@Andarist
Copy link
Contributor

Ah, shoot. I forgot that those are still global variables (even if not available on globalThis) and can leak across scripts 😒

However given that the compiler can't know if I was modified in the loop or not, I guess it's the best we can do for now.

This would be a duplicate of #58803 . CFA graph walk could likely check if an assignment happens between those 2 points but that could cost some perf.

@ex0ns ex0ns closed this as completed Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants