JS-1475 Fix false positives in S7763 for local exports flagged as re-export candidates#6644
Conversation
Tests cover the scenario where locally-defined exports using the `export const alias = importedThing` pattern are incorrectly flagged as re-export candidates. The tests verify that ExportNamedDeclaration nodes with a declaration (single and multiple aliases, adapter/facade patterns) are not flagged, while genuine named re-exports still raise. An upstream sentinel test confirms the unicorn rule still raises on these patterns so the decorator fix remains valid. Relates to JS-1475
Local exports are incorrectly flagged as re-export candidates when identifiers are defined in the same file. The decorator now suppresses two additional cases: (1) ExportNamedDeclaration with a declaration property (e.g. `export const alias = importedThing`), where the binding is a locally defined variable; and (2) ExportSpecifier nodes where the local identifier name does not appear in any import specifier, meaning the identifier is locally defined rather than imported. A defensive suppression is also added when the identifier name cannot be extracted. Relates to JS-1475
No implementation changes were needed: ruling analysis confirmed that all 73 entries currently raising are genuine re-export patterns (import then export) and the 4 entries not raising are `export const alias = imported` patterns correctly suppressed by the decorator. Added one new test case covering the namespace import alias pattern found in ruling data: `import * as _AllIcons from './svgs'; export const AllIcons = _AllIcons` should not be flagged, consistent with the existing suppression of ExportNamedDeclaration nodes that carry a declaration.
Ruling ReportCode no longer flagged (4 issues)S7763eigen/src/palette/index.tsx:15 13 |
14 | /** Icons used as a map for Palette docs */
> 15 | export const AllIcons = _AllIcons
16 | rxjs/src/observable/concat.ts:3 1 | import { concatStatic } from '../operator/concat';
2 |
> 3 | export const concat = concatStatic;rxjs/src/observable/merge.ts:3 1 | import { mergeStatic } from '../operator/merge';
2 |
> 3 | export const merge = mergeStatic; 1 | import { zipStatic } from '../operator/zip';
2 |
> 3 | export const zip = zipStatic; |
Remove unnecessary type assertion in decorator.ts (typescript:S4325). After the `node.type === 'ExportNamedDeclaration'` discriminated union check, TypeScript already narrows `node` to `estree.ExportNamedDeclaration`, making the explicit `(node as estree.ExportNamedDeclaration)` cast redundant.
SummaryThis PR eliminates false positives in rule S7763 (prefer-export-from) where locally-defined exports were incorrectly flagged as re-export candidates. Key distinction: What changed:
What reviewers should knowStart here: Core logic: The Watch for: The sentinel test at lines 23-44 of the unit file. It verifies that the upstream ESLint rule ( False positives fixed: The ruling file deletions (rxjs-S7763.json completely removed, one entry removed from eigen) represent real codebases where this pattern legitimately appears. The patterns (adapter/facade exports, namespace aliases) are documented in the test cases.
|
SummaryFixes false positives in rule S7763 where locally-defined exports (like The core issue: the rule was treating all export statements as potential re-exports, when only genuine re-exports (import X, then export X) should be flagged. The fix adds two layers of suppression in the decorator:
The helper function was refactored from What reviewers should knowStart with The unit tests are instructive: the "upstream sentinel" verifies that Ruling files show 4 false positives removed (from eigen and rxjs) and all 73 remaining entries are genuine re-exports. The refactored helper function is cleaner and more reliable than the original — it catches the case where an identifier is simply not imported, which was the missing piece.
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
|
Thanks for the summary — the description accurately captures the intent and approach of this fix. |
|
Thanks for the summary — the description accurately captures the three suppression conditions added to the decorator and the net effect on ruling results. |
…ules/S7763/decorator.ts Comment: **Logic duplication:** `isFromImport` and `isDefaultImport` both walk `sourceCode.ast.body`, enter each `ImportDeclaration`, and scan specifiers by `specifier.local.name`. Since `isDefaultImport` is only ever called when `isFromImport` already confirmed the identifier exists in an import, the AST is traversed twice for that name. A single helper avoids the double pass and keeps the detection logic in one place: ```ts function getImportKind( sourceCode: SourceCode, identifierName: string, ): 'default' | 'named' | null { for (const node of sourceCode.ast.body) { if (node.type !== 'ImportDeclaration') continue; for (const specifier of node.specifiers) { if (specifier.local.name !== identifierName) continue; if ( specifier.type === 'ImportDefaultSpecifier' || (specifier.type === 'ImportSpecifier' && (specifier.imported.type === 'Identifier' ? specifier.imported.name : String(specifier.imported.value)) === 'default') ) { return 'default'; } return 'named'; } } return null; } ``` The call site becomes: ```ts const importKind = getImportKind(context.sourceCode, identifierName); if (!importKind || importKind === 'default') return; context.report(reportDescriptor); ``` If traversal logic ever needs to change (e.g. handling `export … from` re-declarations), there is only one place to update. - [ ] Mark as noise
…ules/S7763/decorator.ts Comment: Is suppressing all `ExportNamedDeclaration` nodes with a `declaration` intentional for *every* possible declared type, or only `VariableDeclaration`? The unicorn rule can also report on `export function foo() {}` and `export class Foo {}` where the function/class body references an imported value. Those nodes also have `declaration !== null`. Suppressing them all is conservative and avoids FPs, but it means the rule goes silent on cases it might legitimately catch — worth a comment confirming the intended scope. - [ ] Mark as noise
|
Thanks for the review! Both questions have been addressed in the latest commits:
|
|
|
Thank you for the review! |
Fix false positives in rule S7763 where locally-defined exports were incorrectly flagged as re-export candidates (JS-1475).
What changed
ExportNamedDeclarationnodes with adeclarationproperty (e.g.export const alias = importedThing), where the binding is locally defined rather than re-exportedExportSpecifiernodes where the local identifier does not appear in any import specifier, meaning it is locally definedWhy
The rule was raising on patterns like
export const alias = importedThingeven whenaliasis defined locally in the same file. Only genuine re-export patterns (import then export the same binding) should be flagged.Validation
import * as _X from '...'; export const X = _X)