-
-
Notifications
You must be signed in to change notification settings - Fork 720
refactor(linter): Replace crate globset with fast-glob #12825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
| // Validate the pattern by testing it against an empty string | ||
| // This helps catch invalid glob patterns early | ||
| let _ = glob_match(pattern, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation approach using glob_match(pattern, "") doesn't effectively catch invalid glob patterns since glob_match returns false for invalid patterns rather than throwing errors. This means invalid patterns will be silently accepted into the collection.
Consider implementing proper pattern validation by either:
- Using a dedicated validation function if
fast-globprovides one - Creating a simple validation helper that checks pattern syntax
- Adding a comment acknowledging this limitation if silent acceptance is acceptable
This would help maintain the same level of validation that was present in the original globset implementation.
| // Validate the pattern by testing it against an empty string | |
| // This helps catch invalid glob patterns early | |
| let _ = glob_match(pattern, ""); | |
| // TODO: This validation approach has limitations as glob_match returns false | |
| // for both non-matching valid patterns and invalid patterns without distinction. | |
| // If fast-glob provides a dedicated validation function, we should use that instead. | |
| // For now, we're accepting all patterns, including potentially invalid ones. | |
| let _ = glob_match(pattern, ""); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
CodSpeed Instrumentation Performance ReportMerging #12825 will not alter performanceComparing Summary
|
This PR replaces the
globsetcrate withfast-globthroughout the oxc codebase, addressing the requirement to use the more lightweight and efficientfast-globlibrary for pattern matching.Changes Made
Core Replacements
oxc_linter/config/overrides.rs: Replaced theGlobSetwrapper to usefast_glob::glob_matchwith compatibility logic to maintain existing behavioroxc_linter/rules/eslint/no_restricted_imports.rs: ReplacedGlobBuilderwith manual case-insensitive pattern matching usingfast_globoxc_linter/rules/import/no_unassigned_import.rs: Simplified fromGlobSetto vector of patterns with directfast_glob::glob_matchcallsoxc_linter/rules/jsx_a11y/label_has_associated_control.rs: ReplacedGlobSetwith vector of patterns and helper functionoxc_language_server/linter/server_linter.rs: ReplacedGlobSetBuilderwith direct pattern matchingKey Technical Details
Compatibility Handling: The original
globsetlibrary had permissive behavior where patterns like*.tsxwould match paths like/some/path/foo.tsx. To maintain backward compatibility, the new implementation includes fallback logic that tries patterns with**/prefix when the direct match fails.Case Sensitivity: For rules that required case-insensitive matching, the implementation now manually converts both patterns and paths to lowercase using
cow_utils::CowUtils::cow_to_lowercaseto avoid unnecessary allocations.API Simplification: The
GlobSet::new()method no longer returns aResultsincefast-globdoesn't have complex compilation that can fail, simplifying the API surface.Dependencies
globset = "0.4.16"from workspace dependenciesfast-glob = "1.0.0"was already available and is now used throughoutTesting
All existing tests continue to pass, including the glob pattern matching tests in
config::overrides::test::test_globset. The implementation maintains full compatibility with the existing behavior while using the more efficientfast-globlibrary.Benefits
globsetcrate in favor of the more lightweightfast-glob💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.