fix(oxlint): resolve false positives for non-PascalCase JSX components in no-unused-vars #13817
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes false positives in the no-unused-vars rule for JSX components that use non-PascalCase naming conventions. The issue occurred because the parser only promotes JSX tags starting with uppercase letters, _, or $ to IdentifierReference, leaving multilingual (CJK) and camelCase component names as plain Identifier nodes without semantic references.
- Added fallback logic to detect JSX usage for components with no semantic references
- Maintained existing behavior for HTML intrinsics (pure lowercase ASCII) which should still trigger unused warnings
- Added comprehensive test coverage for both CJK and camelCase component scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/oxc_linter/src/rules/eslint/no_unused_vars/usage.rs |
Added JSX plain identifier fallback logic to detect usage of non-PascalCase components |
crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs |
Added test cases covering CJK and camelCase components with proper pass/fail scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for node in self.nodes().iter() { | ||
| match node.kind() { | ||
| AstKind::JSXOpeningElement(open) => { | ||
| if let JSXElementName::Identifier(ident) = &open.name { | ||
| if ident.name == target { | ||
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { | ||
| continue; | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| AstKind::JSXClosingElement(close) => { | ||
| if let JSXElementName::Identifier(ident) = &close.name { | ||
| if ident.name == target { | ||
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { | ||
| continue; | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The logic for checking JSX element names is duplicated between opening and closing elements. Consider extracting this into a helper function to reduce code duplication.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { | ||
| continue; | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| AstKind::JSXClosingElement(close) => { | ||
| if let JSXElementName::Identifier(ident) = &close.name { | ||
| if ident.name == target { | ||
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { |
There was a problem hiding this comment.
Logic inconsistency bug: The code checks ident.name.chars().all(|c| c.is_ascii_lowercase()) in both JSXOpeningElement and JSXClosingElement branches, but this creates inconsistent behavior. If an opening tag like <foo> is skipped due to being all lowercase ASCII (treated as HTML intrinsic), but the closing tag </foo> doesn't exist or has different casing, the function could return true inappropriately. The logic should be consistent - either both opening and closing elements should use the same lowercase check, or the check should be done once before processing both element types to avoid inconsistent treatment of the same component name.
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { | |
| continue; | |
| } | |
| return true; | |
| } | |
| } | |
| } | |
| AstKind::JSXClosingElement(close) => { | |
| if let JSXElementName::Identifier(ident) = &close.name { | |
| if ident.name == target { | |
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { | |
| if ident.name.chars().all(|c| c.is_ascii_lowercase()) { | |
| continue; | |
| } | |
| return true; | |
| } | |
| } | |
| } | |
| AstKind::JSXClosingElement(close) => { | |
| if let JSXElementName::Identifier(ident) = &close.name { | |
| if ident.name == target && !ident.name.chars().all(|c| c.is_ascii_lowercase()) { |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
…s in no-unused-vars
ikki-kki
force-pushed
the
fix/no-unused-vars-jsx-non-pascalcase-components
branch
from
ae4bbbd to
2e4f620
Compare
camc314
left a comment
camc314
left a comment
There was a problem hiding this comment.
Thanks for your work on this, and highlighting this issue.
However this isn't a bug in the linter, but rather the parser:
If we consider the following input to TS:
const myComponent = () => <div>Hello</div>;
const anotherComponent = () => <myComponent />;
It is transformed to:
const myComponent = () => React.createElement("div", null, "Hello");
const anotherComponent = () => React.createElement("myComponent", null);
Since both div and myComponent start with a lowercase letter, they are treated as intrinsic elements (HTML/SVG) and passed as string literals to React.createElement. Therefore, myComponent is not considered used in this context.
However, if we consider:
const 테스트 = () => <div>Hello</div>;
const anotherComponent = () => <테스트 />;
It is transformed to:
const 테스트 = () => React.createElement("div", null, "Hello");
const anotherComponent = () => React.createElement(테스트, null);
Here, 테스트 starts with a non-ASCII character, so it is treated as a custom component and references the JavaScript variable 테스트. Thus, 테스트 is considered used.
I've put up a pr #13819 to fix this.
Thanks
| // ``` | ||
| if self.is_in_jsx() && self.references().next().is_none() { | ||
| let target = self.name(); | ||
| for node in self.nodes().iter() { |
There was a problem hiding this comment.
performing this check is really slow as it requires looping over all all nodes in the ast. Are there other solutions?
https://github.com/microsoft/typescript/blob/1c4083f4edc71cb1744a62013732e3c95d05a035/src/compiler/utilities.ts#L6189-L6192 This directly copies TS's implementation - non-ascii characters are always references (vs intrinsic elements) closes #13817
|
@camc314 I saw that you fixed the parser issue in the PR you mentioned #13819 Glad the problem is resolved. I'll submit a PR if I encounter any bugs while using it. Thanks |
|
no worries! thanks for contributing and using oxlint! |
2 participants
Fixes false positives in
no-unused-varsfor non-PascalCase JSX components like CJK and camelCase identifiers.Problem
The
no-unused-varsrule incorrectly marks JSX components as "unused" when they use non-PascalCase naming, even when actually rendered in JSX. While variables and constants with multilingual characters or mixed casing are correctly recognized, component names likefooBarand테스트(CJK) trigger false positives.This occurs because the parser only promotes JSX tags starting with uppercase letters,
_, or$toIdentifierReference, leaving others as plainIdentifiernodes without generating semantic references. If this is intended to enforce the convention that "JSX components should start with uppercase letters," this should be handled by separate naming/JSX rules rather than havingno-unused-vars, which is responsible for determining usage, issue warnings inappropriately.Solution
Added a local fallback within the rule logic that checks for JSX usage when no semantic references exist
This maintains the existing heuristic for HTML intrinsics (pure lowercase ASCII) while correctly detecting usage for multilingual and mixed-case components.
Test:
Added
test_jsx_plain_identifier_fallbackcovering CJK (테스트) and camelCase (fooBar) components, ensuring HTML intrinsics like<foo />still trigger unused warnings.Note: ESLint currently exhibits the same false positive behavior that this PR addresses for oxlint.