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

update contextual discrimination to include omitted members #43633

Merged
merged 1 commit into from
Apr 24, 2021
Merged

update contextual discrimination to include omitted members #43633

merged 1 commit into from
Apr 24, 2021

Conversation

erikbrinkman
Copy link
Contributor

This diff extends the types checked by discriminateContextualTypeByObjectMembers and discriminateContextualTypeByJSXAttributes to also include any optional components in the type union.

fixes #41759 although it doesn't address the better error reporting for their last repro, which I'm not sure how to address.

@ghost
Copy link

ghost commented Apr 10, 2021

CLA assistant check
All CLA requirements met.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Apr 10, 2021
This diff extends the types checked by
discriminateContextualTypeByObjectMembers and
discriminateContextualTypeByJSXAttributes to also include any optional
components in the type union.

fixes #41759 although it doesn't address the better error reporting for
their last repro, which I'm not sure how to address.
@erikbrinkman
Copy link
Contributor Author

@weswigham that check makes sense. It should be resolved, although I had trouble accepting the changes through gh. Thanks for the pointers of where to start btw, probably wouldn't have thrown this together without them.

@sandersn sandersn added this to Not started in PR Backlog Apr 23, 2021
PR Backlog automation moved this from Not started to Needs merge Apr 24, 2021
@weswigham weswigham merged commit 4d4ea66 into microsoft:master Apr 24, 2021
PR Backlog automation moved this from Needs merge to Done Apr 24, 2021
@erikbrinkman erikbrinkman deleted the omitted-discriminated-union branch May 3, 2021 01:03
@schmod
Copy link

schmod commented May 4, 2021

Under the TS 4.3 beta, this change appears to have introduced a stack overflow in my codebase:

RangeError: Maximum call stack size exceeded
    at checkIdentifier (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:55295:26)
    at checkExpressionWorker (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:61948:28)
    at checkExpression (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:61903:38)
    at checkNonNullExpression (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:57527:37)
    at checkPropertyAccessExpression (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:57572:85)
    at checkExpressionWorker (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:61977:28)
    at checkExpression (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:61903:38)
    at getTypeOfExpression (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:61857:24)
    at getMatchingUnionConstituentForObjectLiteral (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:53649:40)
    at discriminateContextualTypeByObjectMembers (/Users/andrew/code/repo_name/node_modules/typescript/lib/tsc.js:56277:20)

@erikbrinkman
Copy link
Contributor Author

@schmod can you provide more context? Ideally a minimum working example, and even more ideally a bisect showing it was this commit? The call stack seems to indicate that it's causing a stack overflow at getMatchingUnionConstituentForObjectLiteral which is called before this commit's change to discriminateContextualTypeByObjectMembers .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Omitted property does not narrow discriminated union contextual type
4 participants