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 narrowing not preserved in closure between narrowing and usage before last assignment (incomplete "Preserved Narrowing in Closures Following Last Assignments") #57405

Closed
ghost opened this issue Feb 14, 2024 · 2 comments

Comments

@ghost
Copy link

ghost commented Feb 14, 2024

🔎 Search Terms

  • Preserved Narrowing in Closures
  • Last Assignments

Read #56908 and https://devblogs.microsoft.com/typescript/announcing-typescript-5-4-beta/#preserved-narrowing-in-closures-following-last-assignments because I feel like this related case should also be handled.

🕗 Version & Regression Information

I'm not sure if this is a bug, intended behaviour or a missing feature. To me, this looks like incomplete type narrowing in closures before the last assignment, when it should be trivial to narrow the type.

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.4.0-dev.20240214#code/GYVwdgxgLglg9mABCAzgUwHIgLYCM0BOAFAB4BciYO+BAlIgN4BQiiBaUIBSUBIaTAL5MmoSLASIoaFFADqcAgGsYYAOZFgcOLgCGBClTyFEAH2RgAJmmCq0l+s1YAbDoggALNBCX2AYtp6BpTUJubg1rZg9izuCLKIwEgAvIhE9MkAfIyxrBDxUIgwKFjGBIipnt6+lgE6+ogAhMmpETZ2lrlxYAlQ2AAOFUUloeUAZGPI6KU0RFU+-oH6tCKsrAD064gAksBSHsWIznbDcdjYaGDSlohwIFAANPtoiIQEiohqcDKIugDuugAnl1NvManUgkMtPUCKtEMJWOxONxEmAhCIxNB4DwZFAAELvXxgTRLYJGGhmCyRDqOWKuQpgxYwwyjSltKIxPIFVFDdIVbJONb5HqFYozEyVLwLWqkpotKntaKdNbdXoDIZi1kTKaYUZzKXg0krEFbXb7Q7HaKnfLnS7XW73J5QLyvAjvcpfH7-IFdRkymFQ0lwhFsDhcJBJIRAA

💻 Code

function useNumber(x: number) {
  return true
}

function testBroken(foobar: number | undefined) {
  let checkedFoobar: number | undefined
  const fn = () => {
    const isNumber = checkedFoobar !== undefined
    const tmp = isNumber && useNumber(checkedFoobar) // error; because `checkedFoobar` is `number | undefined`

    // If this line is commented out, the error goes away in 5.4.0
    // (Before 5.4.0 the error always exists, even without this assignment)
    checkedFoobar = foobar

  }
  return fn
}

🙁 Actual behavior

In the sample above:

    const isNumber = checkedFoobar !== undefined
    const tmp = isNumber && useNumber(checkedFoobar) // error; because `checkedFoobar` is `number | undefined`

🙂 Expected behavior

TypeScript should know that checkedFoobar is number (and keep that knowledge in the isNumber variable).
There's no assignment between the check and the usage, so any type narrowing should hold.

In the sample above:

    const isNumber = checkedFoobar !== undefined
    const tmp = isNumber && useNumber(checkedFoobar) // works; because `checkedFoobar` should be `number`

Additional information about the issue

An ugly workaround is to repeat the undefined check directly in the condition:

const tmp = checkedFoobar !== undefined && useNumber(checkedFoobar) // works; because `checkedFoobar` is `number`
@ghost
Copy link
Author

ghost commented Feb 14, 2024

I just found of a more concrete example; it even happens after the last assignment:

function useNumber(x: number) {
  return true
}

function testBrokenLastAssignment(foobar: number | undefined) {
  let checkedFoobar: number | undefined
  const fn = () => {

    // If this line is commented out, the error goes away in 5.4.0
    // (Before 5.4.0 the error always exists, even without this assignment)
    checkedFoobar = foobar

    const isNumber = checkedFoobar !== undefined
    const tmp = isNumber && useNumber(checkedFoobar) // error; because `checkedFoobar` is `number | undefined`

  }
  return fn
}

Also, here's a counter-example, where the same kind of narrowing works flawless in a different context:

// On global scope
const fn = (checkedFoobar: number | undefined) => {
  const isNumber = checkedFoobar !== undefined
  const tmp = isNumber && useNumber(checkedFoobar) // works; because `checkedFoobar` is `number`
}

@ghost
Copy link
Author

ghost commented Feb 14, 2024

Closing this as I misunderstood the original change. The new feature is for the scope which creates the arrow function and not inside the closure itself.

My issue seems to be about const-ness.

I actually created an even more minimal example now:

// Helper

function useNumber(x: number) {
  return true
}

// Broken cases

const fn1 = (value: number | undefined, newValue: number | undefined) => {
  value = newValue
  
  const isNumber = value !== undefined
  const tmp = isNumber && useNumber(value) // Error (incorrect)
}

const fn2 = (value: number | undefined, newValue: number | undefined) => {
    const isNumber = value !== undefined
    
    value = newValue

  const tmp = isNumber && useNumber(value) // Error (correct)
}

const fn3 = (value: number | undefined, newValue: number | undefined) => {
  const isNumber = value !== undefined
  const tmp = isNumber && useNumber(value) // Error (incorrect)

  value = newValue
}

I also noticed this is fixable by SSA (by creating a value# whenever the value is changed):

// Helper

function useNumber(x: number) {
  return true
}

// Solve the issue by SSA

const fn1Fix = (value: number | undefined, newValue: number | undefined) => {
  const value1 = value

  const value2 = newValue
  value = newValue
  
  const isNumber = value2 !== undefined
  const tmp = isNumber && useNumber(value2) // No error (correct)
}

const fn2Fix = (value: number | undefined, newValue: number | undefined) => {
  const value1 = value

  const isNumber = value !== undefined
  
  const value2 = newValue
  value = newValue

  const tmp = isNumber && useNumber(value2) // Error (correct)
}

const fn3Fix = (value: number | undefined, newValue: number | undefined) => {
  const value1 = value

  const isNumber = value1 !== undefined
  const tmp = isNumber && useNumber(value1) // No error (correct)
  
  const value2 = newValue
  value = newValue
}

However, that should be its own issue.
I'd also have to think about how SSA would interact with closures and look into why TS doesn't already do this.

@ghost ghost closed this as not planned Won't fix, can't repro, duplicate, stale Feb 14, 2024
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

0 participants