Skip to content

Narrow by property assignment in an object literal #22006

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

Closed
wants to merge 12 commits into from

Conversation

sandersn
Copy link
Member

This PR narrows the type of variables that (1) have a type annotation and (2) are initialized with an object literal:

// @strict: true
type A = {
    x?: string[]
    y?: number[]
}
let a: A = {
    x: [],
}
a.x.push('hi') // ok!
a.y.push(4) // error, object is possibly undefined

This works recursively, and for shorthand property assignments:

type A = {
    x?: string[]
    y?: number[]
    z?: {
        ka?: boolean
        ki?: boolean
    }
}
const y = [1, 2, 3]
let a: A = {
    x: [],
    y,
    z: {
        ka: false
    },
}
a.x.push('hi')
a.y.push(4)
let b = a.z.ka // ok
b = a.z.ki // error, object is possibly undefined

But not for spread assignments — it's too complicated to track reference in the control flow graph — or string-literal/number-literal/computed properties — they are accessed via element access, which doesn't participate in control flow.

Note that FlowInitializer looks very similar to FlowAssignment. I tried re-using the existing structures, but the code size to constantly decide between the two uses was actually quite a bit bigger. Handling FlowInitializer is still very similar to FlowAssignment, but mostly simpler.

Fixes #20219

Not sure why, so the code is currently scratched up with investigation.
1. Requires its own FlowFlag because isMatchingReference assumes
that it's operating only on references or variable declarations, not
initializers.
2. Has a bunch of ugly plumbing in the binder's
bindAssignmentTargetFlow; probably it could be simplified.
3. Doesn't add or check control flow nodes recursively, so only
top-level property initialisers narrow.
They are accessed via element access, which doesn't participate in
control flow for performance reasons.
@sandersn
Copy link
Member Author

@RyanCavanaugh I think you were interested in this.

@weswigham
Copy link
Member

Just because it's non-obvious if it works, can we add a test with a binding pattern, ie:

// @strict: true
type A = {
    x?: string[]
    y?: number[]
    z?: {
        ka?: boolean
        ki?: boolean
    }
    extra?: string
    0?: string
    'two words'?: string
}
const y = [1, 2, 3]
const wat = { extra: "life" }
let {x, y, z, extra, 0: zero, ["two words"]: words}: A = {
    x: [],
    y,
    z: {
        ka: false
    },
    ...wat,
    0: 'hi',
    'two words': 'ho'
}
x.push('hi')
y.push(4)
let b = z.ka
b = z.ki // error, object is possibly undefined
extra.length // error, reference doesn't match the spread
zero.length
words.length

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@RyanCavanaugh RyanCavanaugh deleted the narrow-by-property-assignment branch June 16, 2022 22:07
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

Successfully merging this pull request may close these issues.

3 participants