fix(semantic): non-exported namespace member symbols flagged as exported #4493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
> Part of #4445 Fixes a bug where non-exported functions and variables inside of exported TS namespaces were being flagged with `SymbolFlags::Export` ```ts export namespace Foo { // incorrectly flagged as exported function foo() { } } ```
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
11 tasks
CodSpeed Performance ReportMerging #4493 will not alter performanceComparing Summary
|
Merged
Dunqing
pushed a commit
that referenced
this pull request
Jul 28, 2024
## [0.22.1] - 2024-07-27 ### Features - 2477330 ast: Add `AstKind::TSExportAssignment` (#4501) (Dunqing) - aaee07e ast: Add `AstKind::AssignmentTargetPattern`, `AstKind::ArrayAssignmentTarget` and `AstKind::ObjectAssignmentTarget` (#4456) (Dunqing) - fd363d1 ast: Add AstKind::get_container_scope_id (#4450) (DonIsaac) - e2735ca span: Add `contains_inclusive` method (#4491) (DonIsaac) ### Bug Fixes - 368112c ast: Remove `#[visit(ignore)]` from `ExportDefaultDeclarationKind`'s `TSInterfaceDeclaration` (#4497) (Dunqing) - 36bb680 semantic: `TSExportAssignment` cannot reference type binding (#4502) (Dunqing) - cb2fa49 semantic: `typeof` operator cannot reference type-only import (#4500) (Dunqing) - ef0e953 semantic: Generic passed to typeof not counted as a reference (#4499) (Dunqing) - 40cafb8 semantic: Params in `export default (function() {})` flagged as `SymbolFlags::Export` (#4480) (Dunqing) - 2e01a45 semantic: Non-exported namespace member symbols flagged as exported (#4493) (Don Isaac) - e4ca06a semantic: Incorrect symbol’s scope_id after var hoisting (#4458) (Dunqing) - 77bd5f1 semantic: Use correct span for namespace symbols (#4448) (Don Isaac) - 5db7bed sourcemap: Fix pre-calculation of required segments for building JSON (#4490) (overlookmotel) - 1667491 syntax: Correct `is_reserved_keyword_or_global_object`'s incorrect function calling. (#4484) (Ethan Goh) - 82ba2a0 syntax: Fix unsound use of `NonZeroU32` (#4466) (overlookmotel) - c04b9aa transformer: Add to `SymbolTable::declarations` for all symbols (#4460) (overlookmotel) - ecdee88 transformer/typescript: Incorrect eliminate exports when the referenced symbol is both value and type (#4507) (Dunqing) ### Performance - 963a2d1 mangler: Reduce unnecessary allocation (#4498) (Dunqing) - 868fc87 parser: Optimize conditional advance on ASCII values (#4298) (lucab) - 24beaeb semantic: Give `AstNodeId` a niche (#4469) (overlookmotel) - 348c1ad semantic: Remove `span` field from `Reference` (#4464) (overlookmotel) - 6a9f4db semantic: Reduce storage size for symbol redeclarations (#4463) (overlookmotel) - 705e19f sourcemap: Reduce memory copies encoding JSON (#4489) (overlookmotel) - 4d10c6c sourcemap: Pre allocate String buf while encoding (#4476) (Brooooooklyn) ### Documentation - f5f0ba8 ast: Add doc comments to more AST nodes (#4413) (Don Isaac) - 871b3d6 semantic: Add doc comments for SymbolTester and SemanticTester (#4433) (DonIsaac) ### Refactor - 9c5d2f9 ast/builder: Use `Box::new_in` over `.into_in` (#4428) (overlookmotel) - ccb1835 semantic: Methods take `Span` as param, not `&Span` (#4470) (overlookmotel) - f17254a semantic: Populate `declarations` field in `SymbolTable::create_symbol` (#4461) (overlookmotel) - a49f491 semantic: Re-order `SymbolTable` fields (#4459) (overlookmotel) - 7cd53f3 semantic: Var hoisting (#4379) (Dunqing) - 4f5a7cb semantic: Mark SemanticTester and SymbolTester as must_use (#4430) (DonIsaac) - c958a55 sourcemap: `push_list` method for building JSON (#4486) (overlookmotel) - c99b3eb syntax: Give `ScopeId` a niche (#4468) (overlookmotel) - 96fc94f syntax: Use `NonMaxU32` for IDs (#4467) (overlookmotel) ### Testing - 4b274a8 semantic: Add more test cases for symbol references (#4429) (DonIsaac) Co-authored-by: Boshen <[email protected]>
Dunqing
pushed a commit
that referenced
this pull request
Jul 31, 2024
> Re-creation of #4427 due to rebasing issues. Original attempt: #642 ----- Third time's the charm? Each time I attempt this rule, I find a bunch of bugs in `Semantic`, and I expect this attempt to be no different. Expect sidecar issues+PRs stemming from this PR here. ## Not Supported These are cases supported in the original eslint rule, but that I'm intentionally deciding not to support - export comments in scripts ```js /* exported a */ var a; ``` - global comments ```js /* global a */ var a; ``` ## Behavior Changes These are intentional deviations from the original rule's behavior: - logical re-assignments are not considered usages ```js // passes in eslint/no-unused-vars, fails in this implementation let a = 0; a ||= 1; let b = 0; b &&= 2; let c = undefined; c ??= [] ``` ## Known Limitations - Lint rules do not have babel or tsconfig information, meaning we can't determine if `React` imports are being used or not. The relevant tsconfig settings here are `jsx`, `jsxPragma`, and `jsxFragmentName`. To accommodate this, all imports to symbols named `React` or `h` are ignored in JSX files. - References to symbols used in JSDoc `{@link}` tags are not created, so symbols that are only used in doc comments will be reported as unused. See: #4443 - `.vue` files are skipped completely, since variables can be used in templates in ways we cannot detect > note: `.d.ts` files are skipped as well. ## Todo - [x] Skip unused TS enum members on used enums - [x] Skip unused parameters followed by used variables in object/array spreads - [x] Re-assignments to array/object spreads do not respect `destructuredArrayIgnorePattern` (related to: #4435) - [x] #4493 - [x] References inside a nested scope are not considered usages (#4447) - [x] Port over typescript-eslint test cases _(wip, they've been copied and I'm slowly enabling them)_ - [x] Handle constructor properties ```ts class Foo { constructor(public a) {} // `a` should be allowed } ``` - [x] Read references in sequence expressions (that are not in the last position) should not count as a usage ```js let a = 0; let b = (a++, 0); console.log(b) ``` > Honestly, is anyone even writing code like this? - [x] function overload signatures should not be reported - [x] Named functions returned from other functions get incorrectly reported as unused (found by @camc314) ```js function foo() { return function bar() { } } Foo()() ``` - [x] false positive for TS modules within ambient modules ```ts declare global { // incorrectly marked as unused namespace jest { } } ``` ## Blockers - #4436 - #4437 - #4446 - #4447 - #4494 - #4495 ## Non-Blocking Issues - #4443 - #4475 (prevents checks on exported symbols from namespaces)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug where non-exported functions and variables inside of exported TS namespaces were being flagged with
SymbolFlags::Export