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

Narrowing for constant indexed access fails on array getter #58803

Open
grant-d opened this issue Jun 7, 2024 · 7 comments
Open

Narrowing for constant indexed access fails on array getter #58803

grant-d opened this issue Jun 7, 2024 · 7 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@grant-d
Copy link

grant-d commented Jun 7, 2024

πŸ”Ž Search Terms

constant indexed access, array

πŸ•— Version & Regression Information

  • This changed between versions 5.4.5 and 5.5.beta
  • This changed in commit or PR

⏯ Playground Link

I could not repro this in playground, due to the lib import

πŸ’» Code

import crypto from 'crypto-js'

const bar = "abc"
const foo: crypto.lib.WordArray = crypto.enc.Utf8.parse(bar) // Returns type: { words: number[], ... }
for (let i = 0; i < foo.words.length; i++) {
  foo.words[i] += 1
//~~~~~~~~~~~~ error TS2532: Object is possibly 'undefined'.

  // Alternatively:
  foo.words[i] = foo.words[i] + 1
//               ~~~~~~~~~~~~ <same error>
}

Adding suitable checks in the loop-body for falsey values does not fix it, eg:

if (foo && foo.words && foo.words[i]) {
 // Same code as above, same error
}

However the following does fix it:

const word = foo.words[i]
if (word) {
  foo.words[i] = word + 1 // OK
}

πŸ™ Actual behavior

error TS2532: Object is possibly 'undefined'.

πŸ™‚ Expected behavior

No error - this type-checked fine previously

Additional information about the issue

This issue is new since the new Control Flow Narrowing for Constant Indexed Accesses feature. Rolling back to the previous typescript version 'fixes' it

@grant-d grant-d changed the title Narrowing for constant indexed access fails on array Narrowing for constant indexed access fails on array getter Jun 7, 2024
@Andarist
Copy link
Contributor

Andarist commented Jun 8, 2024

You can often import npm packages in the playground just fine. It's capable of fetching their types.

I don't see any error here with the nightly version: TS playground. foo.words has number[] type so I'm not sure how undefined has crawled into all of this in your tests. Perhaps you are using noUncheckedIndexedAccess?

This would indeed show the difference in the behavior:
TS 5.4
TS nightly

This, in turn, looks like a correctness improvement.

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Jun 14, 2024
@RyanCavanaugh
Copy link
Member

If this is just an array, I don't see why you need crypto-js as part of the repro. Please provide a minimal example, with config, that shows what's happening

@grant-d
Copy link
Author

grant-d commented Jun 15, 2024

@RyanCavanaugh yes I am aware of that, but I was unable to find a repro without the package.

@Andarist I think you nailed it - thanks so much, that must be the issue. Not sure what the action is here, I am closing but please re-open as necessary.

@grant-d grant-d closed this as completed Jun 15, 2024
@grant-d
Copy link
Author

grant-d commented Jun 20, 2024

@RyanCavanaugh here is a minimal repro in TS Playground (based on @Andarist example)

Including code here too for ease of tracking:

// @noUncheckedIndexedAccess: true
interface Foo { words: number[] }
const foo: Foo = { words: [1, 2, 3] }

for (let i = 0; i < foo.words.length; i++) {
  foo.words[i] += 1 // Alternative 1) Same error as below

  foo.words[i] = foo.words[i] + 1 // Alternative 2) Same error as below

  // Alternative 3) Explicit falsey check still fails
  if (foo.words[i]) {
    foo.words[i] = foo.words[i] + 1
    //             ^Object is possibly 'undefined'.(2532)^
  }
}
image

@grant-d grant-d reopened this Jun 20, 2024
@Andarist
Copy link
Contributor

The problem here is that i is assigned to so it doesn't pass this check:
https://github.com/microsoft/TypeScript/pull/57847/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R26613

i is special in loops. We know that it's only assigned to after this loop block. So perhaps some extra fine-tuning for this case would be possible.

@Andarist
Copy link
Contributor

Oh, this just never works for variables that are reassigned:

function f1(obj: Record<string, unknown>) {
  let key: string = "";
  if (typeof obj[key] === "string") {
    obj[key].toUpperCase();
  }
  key = ""; // toggle to see the error to go away and come back
}

This is kinda spooky to see an error pop up on a previous line that seemingly has no real correlation with the line that just got added.

@grant-d
Copy link
Author

grant-d commented Jul 9, 2024

Possibly related
#59185

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

3 participants