fix(prefer-optional-chain): avoid false positives for instanceof and redundant nullish checks#753
fix(prefer-optional-chain): avoid false positives for instanceof and redundant nullish checks#753wagenet wants to merge 1 commit intooxc-project:mainfrom
Conversation
|
@wagenet mind rebasing? thanks |
|
May want to double check main has these fixed, looks pretty similar to my other PR that was merged. AI wanted to add instanceof also but those weren't actually being triggered at least the ones I tried |
…f` and redundant nullish checks Two false positives are fixed: 1. `in`/`instanceof` operators: expressions like `(!a.b || key in a.b)` or `(a.b && foo instanceof a.b)` were incorrectly flagged because `in` and `instanceof` have no optional-chain equivalent. Add `KindInKeyword` and `KindInstanceOfKeyword` to the invalid-operator switches in both the AND-chain and OR-chain branches of `parseOperand`. 2. Redundant nullish checks: `a.b === null || a.b === undefined` was falsely reported because the chain passed `hasPropertyAccessInChain` (a.b is an access expression) even though both operands check the exact same expression with no deeper member access to convert. Add an `allOperandsCheckSameExpression` guard to `validateOrChainForReporting`, mirroring the identical guard already present in `validateAndChainForReporting`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
wagenet
force-pushed
the
fix-optional-chain
branch
from
cb78a5e to
fca5e11
Compare
wagenet
changed the title
in/instanceof and redundant nullish checks
|
|
||
| // When every operand checks the exact same expression (e.g., a.b === null || a.b === undefined), | ||
| // there is no member access to convert — skip flagging, mirroring the AND-chain guard. | ||
| if processor.allOperandsCheckSameExpression(chain) { |
There was a problem hiding this comment.
this is added already just a few lines down https://github.com/oxc-project/tsgolint/pull/753/changes#diff-0d230715551b2aebfce0eb9756db9dab5b27e4784402ce304b55bcf884976522L2560
| // so these patterns must never be reported. | ||
| validCases = append(validCases, rule_tester.ValidTestCase{ | ||
| Code: `(!data.previous_values || key in data.previous_values)`, | ||
| }, rule_tester.ValidTestCase{ |
There was a problem hiding this comment.
I cloned this file from this branch and all these test cases pass on main so I think my changes in #732 are sufficient
There was a problem hiding this comment.
Huh... that doesn't match what I was seeing. Guess we'll need to dig in more.
There was a problem hiding this comment.
Was your main up to date? As my PR was recently merged/released
There was a problem hiding this comment.
Yes, I updated it after I rebased.
There was a problem hiding this comment.
Recording.2026-02-26.210044.mp4
above shows what I am doing at least 🤷 I started with your changes locally as shown by the diff and then show that i have no other changes and I am on main. Then I run the test and they still pass meaning no changes are required to satisfy these test cases. Unless I am overlooking something
3 participants
Summary
instanceoffalse positive: patterns like(!a.b || foo instanceof a.b)and(a.b && foo instanceof a.b)were incorrectly flagged. AddedKindInstanceOfKeywordto the invalid-operator switches in both the AND-chain and OR-chain branches ofparseOperand—instanceofhas no optional-chain equivalent.Redundant nullish-check false positive:
a.b === null || a.b === undefinedwas falsely reported because both operands check the exact same expression with no deeper member access to convert. Added anallOperandsCheckSameExpressionguard tovalidateOrChainForReporting, mirroring the identical guard already present invalidateAndChainForReporting.Test plan
(!a.b || foo instanceof a.b)— no violation(a.b && foo instanceof a.b)— no violationrequest.payload === null || request.payload === undefined— no violationrequest.payload === undefined || request.payload === null— no violationgo test ./internal/rules/prefer_optional_chain/...passes🤖 Generated with Claude Code