fix(linter): resolve import/extensions false positives and align with ESLint behavior#14602
Conversation
4115709 to
6bf3602
Compare
CodSpeed Performance ReportMerging #14602 will not alter performanceComparing Summary
Footnotes
|
0f575f0 to
ec5b0e7
Compare
camc314
left a comment
There was a problem hiding this comment.
Would it be better to use use the module record for this.
code like:
// Check for path aliases with single-char prefix followed by /
// Examples: @/, ~/, #/
if module_name.len() >= 2 {
let chars: Vec<char> = module_name.chars().collect();
if chars.len() >= 2 && chars[1] == '/' {
// Single character before slash - this is a path alias
return false;
}
}
feels flaky
yeah that makes sense. I'll work on refactoring this to use the module record. Update: after looking at it a bit more, I couldn't find any additional contextual information in the module record (aside from the module's import name that is already being used) that I could leverage to my benefit. I did update the helper function this snippet is in to be a little cleaner and more clear with its intentions. as far as I can tell, the linter doesn't support custom resolver configurations in however, I think adding full resolver config support deserves its own dedicated issue / PR because the filesystem lookups to resolve the maybe I'm missing something though. let me know if you have any suggestions in terms of how I can use the module record better here |
ec5b0e7 to
15eb558
Compare
|
This MR does fix various issues in the reproduction repo here. The only things it doesn't fix are
linting output on the repro repo for eslint vs main vs this branchoxlint 1.26.0 (3 errors, and 1 error missing that is expected, based on what ESLint does): This PR (after being rebased on top of main, 1 error but it's different from ESLint's error): ESLint (1 error): Aside on main5.js: I'm not sure if we should necessarily downcase the extension filename to fix Regardless, I don't think that edge case needs to be fixed in this MR. I do think we should consider adding a bunch of fixtures for this rule, to confirm various complex behaviors, but that can also be done in a separate PR imo. |
|
This definitely seems to improve the number of discrepancies between oxlint and ESLint, based on testing it on the Mastodon repo. 1.26.0 on mastodon: 169 errors However, I think more work will need to be done to fix further issues with this rule, as things like this are showing up as violations still after this PR, despite not being violations (config is See I'm wondering if this rule actually correctly has the ability to check the source file's type when it's being imported? The oxlintrc.json{
"$schema": "./node_modules/oxlint/configuration_schema.json",
"plugins": [
"import"
],
"categories": {
"correctness": "off"
},
"env": {
"builtin": true,
"es2021": true,
"browser": true,
"shared-node-browser": true
},
"rules": {
"import/extensions": [
"error",
"always",
{
"js": "never",
"jsx": "never",
"ts": "never",
"tsx": "never"
}
]
},
"ignorePatterns": [
"build/**/*",
"coverage/**/*",
"db/**/*",
"lib/**/*",
"log/**/*",
"node_modules/**/*",
"public/**/*",
"!public/embed.js",
"spec/**/*",
"tmp/**/*",
"vendor/**/*",
"streaming/**/*",
".bundle/**/*",
"storybook-static/**/*"
]
} |
Good to know! Thanks for the detailed report. I can take a look this week into this further, but I'll probably do it in a separate development branch because I suspect it will be a more significant refactor to get all those things working. I don't think the file extension parsing is working how we would expect. When I initially implemented this rule, I basically just finished once I got all the tests to pass, but there have been a few problems with this rule. I agree that we should add more testing to verify the behavior. It deserves a closer look for sure. |
|
See also #15009, not sure which is the best to go with as the solution here but I definitely think we need to prioritize fixing this rule. |
|
I'm not sure either. From the discord conversation I'm also working on a separate branch that I was planning to point to this one when it's ready that will support custom extensions (i.e., not hard coded ones). |
File extensions are now normalized to lowercase for case-insensitive matching,
resolving the edge case where `./foo.JS` was incorrectly flagged.
**Changes:**
- Normalize extensions to lowercase in `get_file_extension_from_module_name()`
- Normalize extensions to lowercase in `get_resolved_extension()`
- Add test cases for uppercase and mixed-case extensions
**Fixes:**
- ✅ `./foo.JS` now correctly matches `{ "js": "always" }` config
- ✅ `./foo.Ts` now correctly matches `{ "ts": "never" }` config
- ✅ Case variations (JS, Js, js) all treated identically
Addresses the main5.js edge case from oxc-project#14602 testing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
File extensions are now normalized to lowercase for case-insensitive matching,
resolving the edge case where `./foo.JS` was incorrectly flagged.
**Changes:**
- Normalize extensions to lowercase in `get_file_extension_from_module_name()`
- Normalize extensions to lowercase in `get_resolved_extension()`
- Add test cases for uppercase and mixed-case extensions
**Fixes:**
- ✅ `./foo.JS` now correctly matches `{ "js": "always" }` config
- ✅ `./foo.Ts` now correctly matches `{ "ts": "never" }` config
- ✅ Case variations (JS, Js, js) all treated identically
Addresses the main5.js edge case from oxc-project#14602 testing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
229407b to
6fa1ba6
Compare
File extensions are now normalized to lowercase for case-insensitive matching,
resolving the edge case where `./foo.JS` was incorrectly flagged.
**Changes:**
- Normalize extensions to lowercase in `get_file_extension_from_module_name()`
- Normalize extensions to lowercase in `get_resolved_extension()`
- Add test cases for uppercase and mixed-case extensions
**Fixes:**
- ✅ `./foo.JS` now correctly matches `{ "js": "always" }` config
- ✅ `./foo.Ts` now correctly matches `{ "ts": "never" }` config
- ✅ Case variations (JS, Js, js) all treated identically
Addresses the main5.js edge case from oxc-project#14602 testing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
I discussed and tested this rule with @connorshea on discord and pushed up some changes that will significantly improve the behavior of this rule significantly. the key highlights:
I'm planning to review it more thoroughly over the next couple of days to make sure everything looks good and that I didn't miss anything, but meanwhile I think it's in a good enough place for review. |
I'm feeling good about these changes after giving them a quick review and updating the PR description. |
|
I believe I've addressed all the feedback on this PR. I was able to simplify the implementation a decent amount and verify that it still behaves the same against the rule's testing and against mastodon's codebase. I think this is in a good place for a re-review when you get a chance @camchenry @camc314 |
|
Rebuilt the latest version of this branch and ran it on the Mastodon codebase, confirmed it still works well :) Before, with oxlint 1.32.0 (--type-aware --report-unused-disable-directives): After, with this PR (--type-aware --report-unused-disable-directives): (any differences in the starting #s vs the last comment I made are generally due to the |
connorshea
left a comment
There was a problem hiding this comment.
From a docs/"does the rule work" perspective, LGTM. Will let cam or cam review and approve the actual rust code.
camchenry
left a comment
There was a problem hiding this comment.
I think this looks pretty great! I'm not too familiar with the original rule, but I've read over the logic here and I think it makes sense.
I've left a number of comments and I would make some of these changes myself but I'm not currently at my laptop to do so. But hopefully this is also instructive for future PRs for the types of things I'm thinking about (and AI tends to miss on). I don't think any of the comments are particularly time consuming though.
I also left a few follow-up comments that are non-blocking and just notes for future PRs on top of this work.
Overall, I'm excited to get this merged in, as this is a big improvement to this rule. Since this is an existing rule with issues, I don't want to let perfection get in the way of good enough, so I'm in favor of these changes after feedback if @camc314 is good with it too.
Move ignorePackages and pathGroupOverrides documentation from the struct-level doc comment to individual field doc comments for better discoverability. Also removes the redundant default value comment since the Default derive already expresses this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add #[serde(skip)] attribute to the require_extension field since it is not user-configurable from the config object. This prevents it from appearing in the generated documentation as a supported config option. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restore the missing doc comment for the check_type_imports field that describes its purpose in enforcing extension rules for type imports. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add json to the has_any_never_rules() method for consistency with is_standard_extension() which already includes json. This ensures proper lenient behavior when json files have "never" rules configured. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the redundant `!call_expr.arguments.is_empty()` check before iterating over arguments. Iterating over an empty iterator is a no-op and this check adds unnecessary complexity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…value Move the standalone build_config function to be a static method of ExtensionsConfig named from_json_value. This is more idiomatic Rust and improves code organization by keeping related functionality together. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reorder the condition checks to put the cheapest operations first: 1. is_none() - simple option check 2. is_standard_extension() - matches! macro 3. has_rule() - hash table lookup This optimization reduces unnecessary work when the early conditions short-circuit the evaluation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Inline the is_path_group_action_enforce check into the conditional instead of computing it as a separate variable. This defers the matches! call until it's actually needed in the condition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…mentation Move require() call processing from run_once to a dedicated fn run implementation. This enables linter framework optimizations to filter nodes by type and only invoke the rule for CallExpression nodes. - Add fn run() to handle require() calls - Keep run_once() for module record iteration - Regenerate rule_runner_impls.rs with updated NODE_TYPES and RUN_FUNCTIONS 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Simplify string handling by removing unnecessary CompactStr conversions. The code was converting &str → CompactStr → &str which added overhead without benefit. - Change get_file_extension_from_module_name to take &str, return Option<String> - Change validate_extension to take Option<&str> for written_extension - Change extension_should_not_be_included_in_diagnostic to take &str - Remove unused oxc_span::CompactStr import 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add early return when global require_extension is Never - this skips up to 7 hash lookups since all extensions default to Never anyway. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ction Replace multiple bounds checks with a single slice pattern match. This reduces potential bounds checking overhead when detecting single-character path aliases like ~/ or #/. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
I appreciate your thoughtful feedback! It's helpful to get more insight into what the best practices are for these linting rules and what anti patterns you're checking for. I'll definitely keep that in mind for future contributions. I pushed up commits to address each comment of feedback. Everything still works as before, but everything should be addressed now. I'm hoping that separate commits for each improvement will make it easier to review each of the changes individually. |
|
@camc314 when you get a chance can I get a review on this? I think it's good to go. Merging this will close 3 open issues. |
Closes #12220 and #16701
This PR fixes multiple false positive scenarios in the
import/extensionsrule and adds significant improvements to make it more robust, performant, and ESLint-compatible.More thorough PR description generated by Claude Code, reviewed by me:
Summary
This PR comprehensively fixes multiple false positive scenarios in the
import/extensionslinter rule and significantly improves ESLint compatibility, performance, and extensibility.Bug Fixes
1. Unconfigured Extensions Now Ignored
Before:
After:
Impact: Eliminates false positives for framework-specific extensions (Vue, Svelte) and custom extensions (CSS, SCSS, etc.)
2. Root Package Name Detection Fixed
Before:
After:
Impact: Correctly distinguishes between package names containing dots (part of the name) vs. file paths with extensions
3. Path Alias Detection Fixed
Before:
After:
Impact: Critical fix for Vue.js, React, Next.js, and Nuxt projects using path aliases like
@/,~/, or#/4.
ignorePackagesBehavior Now Matches ESLintBefore:
trueAfter:
false(matches ESLint)New Features
Case-Insensitive Extension Matching
Before:
After:
Module Resolution Integration
Before: Only checked the string in the import statement
After: Uses
ModuleRecord::resolved_absolute_pathto validate against actual filesystem filesBenefits:
Arbitrary Extension Support
Before: Only supported hardcoded extensions:
js,jsx,ts,tsx,jsonAfter: Supports ANY extension
Path Group Overrides (Bespoke Import Specifiers)
New option for custom import protocols used by monorepo tools and custom resolvers.
{ "rules": { "import/extensions": ["error", "always", { "pathGroupOverrides": [ { "pattern": "rootverse{*,*/**}", "action": "ignore" } ] }] } }Pattern Matching:
*- Match any characters except/**- Match any characters including/(recursive){a,b}- Match alternativesActions:
"enforce"- Apply normal extension validation"ignore"- Skip all extension validation for matching importsPerformance Optimizations
Zero-Copy FxHashMap Configuration
Before:
After:
Performance Characteristics:
#[inline]attributes on hot pathsConfiguration Changes
Default Behavior
Before: Pre-populated default rules for common extensions could cause false positives
After: Unconfigured extensions are ignored - no false positives by default
ignorePackages Default
Before:
trueAfter:
false(matches ESLint)Configuration Formats Supported
Testing
Coverage Areas
@/,~/,#/)@babel/core,@types/node)pkg.js,@name/pkg.js)./foo.JS,./bar.Ts).vue,.mjs,.css)Real-World Validation
Documentation Updates
ignorePackagessemanticspathGroupOverridesdocumentationMigration Notes
Breaking Changes
ignorePackagesdefault changed fromtruetofalseIf you relied on the old default, explicitly set it:
ignorePackagesno longer affects "never" ruleThis was undefined/inconsistent behavior before. Now it only exempts packages from the "always" rule.
Non-Breaking Improvements