Skip to content

fix(noUnusedImports): edge case ambient type#9359

Merged
ematipico merged 1 commit intomainfrom
fix/no-unused-imports-v3
Mar 6, 2026
Merged

fix(noUnusedImports): edge case ambient type#9359
ematipico merged 1 commit intomainfrom
fix/no-unused-imports-v3

Conversation

@ematipico
Copy link
Member

Summary

Closes #7516

Code and tests added by Claude Code

Test Plan

Added new tests

Docs

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Mar 6, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 53002 53002 0
Passed 51782 51782 0
Failed 1178 1178 0
Panics 42 42 0
Coverage 97.70% 97.70% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 38 38 0
Passed 37 37 0
Failed 1 1 0
Panics 0 0 0
Coverage 97.37% 97.37% 0.00%

markdown/commonmark

Test result main count This PR count Difference
Total 652 652 0
Passed 652 652 0
Failed 0 0 0
Panics 0 0 0
Coverage 100.00% 100.00% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5466 5466 0
Passed 1915 1915 0
Failed 3551 3551 0
Panics 0 0 0
Coverage 35.03% 35.03% 0.00%

ts/babel

Test result main count This PR count Difference
Total 636 636 0
Passed 568 568 0
Failed 68 68 0
Panics 0 0 0
Coverage 89.31% 89.31% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18875 18875 0
Passed 13014 13014 0
Failed 5860 5860 0
Panics 1 1 0
Coverage 68.95% 68.95% 0.00%

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 6, 2026

Walkthrough

Fixes a false positive in the noUnusedImports linter rule when a local variable shadows an imported type namespace that remains in use within type annotations. The fix modifies the semantic analysis to correctly handle ambient reads for non-imported bindings that shadow type imports, and includes comprehensive test coverage for const, let, var, and nested shadowing scenarios.

Possibly related PRs

  • #7282: Modifies semantic event handling in events.rs to correct how namespace and type imports with shadowing are recorded.

Suggested labels

A-Linter, L-JavaScript, A-Core

Suggested reviewers

  • dyc3
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(noUnusedImports): edge case ambient type' directly addresses the main change—fixing a false-positive in the noUnusedImports rule for an edge case involving ambient types.
Description check ✅ Passed The description references issue #7516, mentions code and tests were added, and notes the test plan—all relevant to the changeset addressing the noUnusedImports false-positive.
Linked Issues check ✅ Passed The PR successfully addresses issue #7516 by modifying the ambient read promotion logic in events.rs to handle non-imported bindings shadowing type imports, and adds five comprehensive test cases covering const, let, var, nested, and non-type shadowing scenarios.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the noUnusedImports false-positive: semantic logic fix in events.rs, changelog entry, and five focused test cases validating the fix.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/no-unused-imports-v3

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/biome_js_semantic/src/events.rs (1)

1019-1033: Please add a dedicated parameter-shadowing regression test.

This branch explicitly handles the (stream: stream.T) shape; adding a matching valid-issue-7516-parameter.ts would lock the behaviour down and avoid future drift.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/biome_js_semantic/src/events.rs` around lines 1019 - 1033, Add a
regression test that covers the parameter-shadowing case handled by the new
logic: create a new test file (e.g., valid-issue-7516-parameter.ts) containing a
type import and a function/arrow parameter that shadows the import name but uses
the import in a type position (the `(stream: stream.T)` shape), ensuring the
analyzer accepts it (valid test). The test should exercise the code path where
reference.is_ambient_read() && !info.is_imported() leads to promoting the
reference into the parent scope (verify behavior tied to self.scopes.last_mut(),
parent.references and Reference::AmbientRead). Ensure the test asserts no error
is reported so future changes to the promotion logic are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/biome_js_semantic/src/events.rs`:
- Around line 1019-1033: Add a regression test that covers the
parameter-shadowing case handled by the new logic: create a new test file (e.g.,
valid-issue-7516-parameter.ts) containing a type import and a function/arrow
parameter that shadows the import name but uses the import in a type position
(the `(stream: stream.T)` shape), ensuring the analyzer accepts it (valid test).
The test should exercise the code path where reference.is_ambient_read() &&
!info.is_imported() leads to promoting the reference into the parent scope
(verify behavior tied to self.scopes.last_mut(), parent.references and
Reference::AmbientRead). Ensure the test asserts no error is reported so future
changes to the promotion logic are caught.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8d857a24-a300-4a5d-b20e-cc395a73cff6

📥 Commits

Reviewing files that changed from the base of the PR and between 64ce544 and d83f186.

⛔ Files ignored due to path filters (5)
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-const.ts.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-let.ts.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-nested.ts.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-non-type.ts.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-var.ts.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (7)
  • .changeset/rude-pets-enjoy.md
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-const.ts
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-let.ts
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-nested.ts
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-non-type.ts
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/valid-issue-7516-var.ts
  • crates/biome_js_semantic/src/events.rs

@ematipico ematipico requested review from a team March 6, 2026 08:46
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 6, 2026

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 156 skipped benchmarks1


Comparing fix/no-unused-imports-v3 (d83f186) with main (70c2d4e)

Open in CodSpeed

Footnotes

  1. 156 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@ematipico ematipico merged commit 701ddd3 into main Mar 6, 2026
18 checks passed
@ematipico ematipico deleted the fix/no-unused-imports-v3 branch March 6, 2026 23:45
@github-actions github-actions bot mentioned this pull request Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Linter Area: linter L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 False-positive noUnusedImports error when a variable has the same name as an imported type namespace

2 participants