-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Fix "never nullish" diagnostic missing expressions wrapped in parentheses #62789
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: jakebailey <[email protected]>
Co-authored-by: jakebailey <[email protected]>
|
@typescript-bot test this |
|
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Hey @RyanCavanaugh, the results of running the DT tests are ready. Everything looks the same! |
|
@RyanCavanaugh Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
RyanCavanaugh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. For clarity, this code does go through casts, which it sounds like we're at least tentatively OK with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where the "never nullish" diagnostic (TS2881) was not reported when the left operand of a nullish coalescing operator (??) was wrapped in parentheses, causing valid type errors to be silently ignored.
Key Changes:
- Modified
isNotWithinNullishCoalesceExpressioninchecker.tsto usewalkUpOuterExpressions()instead of directly checkingnode.parent, allowing it to traverse through parentheses and other outer expressions - Added comprehensive test case
neverNullishThroughParentheses.tscovering various levels of parenthesis nesting - Updated
predicateSemantics.tsbaseline to reflect additional errors now correctly detected in existing test cases
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/compiler/checker.ts |
Fixed isNotWithinNullishCoalesceExpression to walk up through outer expressions (like parentheses) to find the actual parent binary expression |
tests/cases/compiler/neverNullishThroughParentheses.ts |
Added new test case demonstrating the bug fix with various levels of parenthesis nesting |
tests/baselines/reference/neverNullishThroughParentheses.errors.txt |
Baseline showing 8 errors correctly detected in the new test |
tests/baselines/reference/neverNullishThroughParentheses.types |
Type baseline for new test |
tests/baselines/reference/neverNullishThroughParentheses.symbols |
Symbol baseline for new test |
tests/baselines/reference/neverNullishThroughParentheses.js |
JS output baseline for new test |
tests/baselines/reference/predicateSemantics.errors.txt |
Updated baseline showing 3 additional errors now correctly caught in existing tests (lines with parenthesized expressions) |
The "never nullish" diagnostic (TS2881) was not reported when the left operand of a
??operator was wrapped in parentheses, causing valid errors to be silently ignored.Changes
checker.ts: ModifiedisNotWithinNullishCoalesceExpressionto usewalkUpOuterExpressions()instead of directly checkingnode.parent, allowing it to traverse through parentheses and other outer expressions to find the actual parent binary expressionTests: Added
neverNullishThroughParentheses.tstest case and updatedpredicateSemantics.tsbaseline to include additional errors now correctly detectedImpact
The fix catches previously missed errors in cases like:
(expr ?? value) ?? fallback- now detects never-nullishvalueopt ?? (null ?? 1)- now detects never-nullish1Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.