JS-1257 Fix S4325 false positive: suppress type assertions on generic DOM API calls#6576
Conversation
Tests cover the scenario where a type assertion wraps a non-null assertion on a generic DOM method call (e.g., `querySelector()! as HTMLElement`). TypeScript infers the generic type parameter from the `as` assertion target, making the assertion appear redundant, but it is necessary to drive inference. Also adds an upstream sentinel block to verify the upstream ESLint rule (`no-unnecessary-type-assertion`) still raises on the suppressed patterns. Relates to JS-1257
The decorator's shouldSuppressTypeAssertion now handles two patterns: 1. Direct generic call: querySelector() as T 2. Non-null then assert: querySelector()! as T Without strictNullChecks, the upstream no-unnecessary-type-assertion rule flags BOTH the `!` and `as T` in `querySelector()! as T` as unnecessary (since null is erased from types, making both operations no-ops). The sentinel test correctly expects 2 errors. Because the `!` and `as T` fixes have overlapping ranges, ESLint only applies the `!` fix per pass, leaving `as HTMLElement` in the output — the test now reflects this actual behavior. Relates to JS-1257.
Ruling Report✅ No changes to ruling expected issues in this PR |
|
Three type-casting improvements to consider in 1. The function only uses its parameter via 2. Introduce
3. Remove the After fixing |
Comment: Three type-casting improvements to consider in `decorator.ts`: **1. `isCalleeGeneric` parameter: `estree.Node` → `TSESTree.Node`** The function only uses its parameter via `services.esTreeNodeToTSNodeMap.get(callExpression as TSESTree.Node)` — it immediately casts away the declared type. Declaring the parameter as `TSESTree.Node` makes the contract honest and removes the internal cast. It also eliminates the `as unknown as estree.Node` forced cast at the call site on line 114 — a forced `as unknown` cast is usually a signal that the declared type of a variable can be improved. **2. Introduce `const tsExpression = node.expression` in `shouldSuppressTypeAssertion`** `expression` must stay `estree.Node` for the `getTypeFromTreeNode` calls. But the `.type` discriminant checks and `isCalleeGeneric` calls all work in TSESTree space. Rather than repeating `expression as TSESTree.Node` at each of those sites, declare one variable of the right type and use it throughout. This also collapses `expression as TSESTree.Node as TSESTree.TSNonNullExpression` to a single cast. **3. Remove the `expression` variable in `shouldSuppressNonNullAssertion`** After fixing `isCalleeGeneric`'s parameter type, both uses of `expression` in that function can be replaced with `node.expression` directly — `node.expression` is already `TSESTree.Expression`, which is accepted by both `isCalleeGeneric` and `esTreeNodeToTSNodeMap.get()`. The intermediate variable and its cast are no longer needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
All three improvements applied:
|
README Freshness Check❌ The rules README is out of date. A fix PR has been created: #6587 Please review and merge it into your branch. |
Removed an unnecessary type assertion in `shouldSuppressTypeAssertion` in `decorator.ts`. After the `tsExpression.type === 'TSNonNullExpression'` type guard, TypeScript already narrows `tsExpression` to `TSESTree.TSNonNullExpression`, so the explicit cast `tsExpression as TSESTree.TSNonNullExpression` was redundant. Replaced the intermediate `nonNull` variable with direct use of the narrowed `tsExpression`, eliminating the unnecessary type assertion (CODE_SMELL reported by typescript:S4325).
Fixed a CODE_SMELL (S1066) in decorator.ts: merged two nested if statements into a single condition. The check for `tsExpression.type === 'TSNonNullExpression'` and its inner check `tsExpression.expression.type === 'CallExpression'` were combined into one if statement with `&&`, eliminating unnecessary nesting.
|
1 participant
Fixes a false positive in S4325 where type assertions on generic DOM API method calls (e.g.,
querySelector()! as HTMLElement) were incorrectly flagged as unnecessary.Problem
When TypeScript infers the generic type parameter from an
asassertion target, the assertion is necessary to drive type inference — even though it may appear redundant. The rule was flagging these as unnecessary type assertions.Two patterns were affected:
querySelector() as TquerySelector()! as TChanges
shouldSuppressTypeAssertionin the S4325 decorator to recognize and suppress assertions on generic DOM API calls for both patterns abovequerySelector()! as HTMLElementscenario, including an upstream sentinel block verifying the underlying ESLint rule (no-unnecessary-type-assertion) still raises on suppressed patternsstrictNullChecks, the upstream rule flags both!andas Tas unnecessary (2 errors), and due to overlapping fix ranges ESLint only applies the!fix per passRelates to JS-1257.