refactor(linter): LintService and Runtime accept file_system and paths as parameters#15506
refactor(linter): LintService and Runtime accept file_system and paths as parameters#15506
file_system and paths as parameters#15506Conversation
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. |
…ameters - Remove `with_file_system` and `with_paths` mutation methods from Runtime and LintService - Update `run`, `run_source`, and `run_test_source` to accept file_system and paths as parameters - Update all call sites in lint_runner.rs and tester.rs - Export OsFileSystem for use in calling code - All tests passing Co-authored-by: Sysix <3897725+Sysix@users.noreply.github.com>
Co-authored-by: Sysix <3897725+Sysix@users.noreply.github.com>
|
@copilot fix the Lint errors |
CodSpeed Performance ReportMerging #15506 will not alter performanceComparing Summary
Footnotes
|
- Remove unused `self` parameter from get_source_type_and_text (now an associated function) - Change methods from `&mut self` to `&self` where mutation isn't needed (papaya HashMap is concurrent) - Fix needless pass by value - take reference instead of owned Box in lint_files - Remove unnecessary mut from lint_service variable in tester All clippy warnings resolved, all tests passing. Co-authored-by: Sysix <3897725+Sysix@users.noreply.github.com>
Fixed all clippy lint errors in commit d94c967:
All clippy warnings resolved, all tests passing. |
file_system and paths as parameters
|
@copilot run |
- Change LintRunner::run_source to take &self instead of &mut self - Change file_system parameter to take reference instead of owned Box - Update language server call sites to pass reference - Change IsolatedLintHandler methods to &self (lint_path, run_single) - Remove unnecessary mut from isolated_linter variable All lint errors resolved, all tests passing: - 820 oxc_linter tests pass - 136 oxlint tests pass - 92 oxc_language_server tests pass Co-authored-by: Sysix <3897725+Sysix@users.noreply.github.com>
Fixed all remaining lint errors from
All lint errors resolved. All tests passing:
|
Sysix
left a comment
There was a problem hiding this comment.
The goal of the PR is that the server side does not handle a mutable state and avoiding possible async calls for the Tool trait.
The Tool trait wants to be dynamic:
Trying to remove the
Mutex here to avoid async methods:@camc314 @overlookmotel the changes looks good to me. You know more about the fs requirements :) I think it is required for JS plugin support too (beside in memory for lsp)?
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the linting service architecture to improve thread-safety and API design by moving file_system and paths from struct fields to function parameters. This allows methods to take &self instead of &mut self, enabling better concurrency and clearer ownership semantics.
Key changes:
- Removed
pathsandfile_systemfields from theRuntimestruct, passing them as parameters to methods instead - Changed method signatures from
&mut selfto&selfacrossRuntime,LintService,LintRunner, and related components - Made
OsFileSystempublic to support the new API design where consumers must explicitly provide file system implementations - Refactored path sorting logic to use a local
sorted_pathsvariable instead of mutating a struct field
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_linter/src/service/runtime.rs |
Core refactoring: removed paths and file_system fields from Runtime, updated method signatures to take parameters instead, made OsFileSystem public, and created local sorted copy of paths |
crates/oxc_linter/src/service/mod.rs |
Removed builder methods (with_file_system, with_paths) and updated LintService method signatures to pass through parameters |
crates/oxc_linter/src/tester.rs |
Updated test code to create file system locally and pass it as a parameter to run_test_source |
crates/oxc_linter/src/lint_runner.rs |
Changed file_system parameter from owned Box to borrowed reference, and updated methods to use &self instead of &mut self |
crates/oxc_linter/src/lib.rs |
Added OsFileSystem to public exports |
crates/oxc_language_server/src/linter/server_linter.rs |
Updated to acquire non-mutable lock on isolated_linter |
crates/oxc_language_server/src/linter/isolated_lint_handler.rs |
Changed method signatures to &self and updated file system from Box to stack-allocated value |
apps/oxlint/src/lint.rs |
Updated to convert Box<dyn RuntimeFileSystem> to reference when calling lint_files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create a sorted copy of paths for processing | ||
| let mut sorted_paths: Vec<_> = paths.iter().cloned().collect(); | ||
| sorted_paths.par_sort_unstable_by(|a, b| Path::new(b).cmp(Path::new(a))); |
There was a problem hiding this comment.
While the code correctly creates a sorted copy of paths, the surrounding comments (lines 362-363, 369) still reference self.paths, but the paths field has been removed from the Runtime struct. These comments should be updated to refer to the local sorted_paths variable or the parameter paths instead.
There was a problem hiding this comment.
Fixed in commit e1237b1. Updated comments on lines 362-363 and 369 to reference paths parameter instead of the removed self.paths field.
|
@copilot rebase and fix the failing checks |
…_change` trait method (#15491) Returning `ToolRestartChanges` like `handle_configuration_change` does. The logic in `WorkspaceWorker/Backend` can be shared later for simplified usage.
- Updated comments to reference `paths` parameter instead of removed `self.paths` field - Rebased onto latest main branch (25d577e) - All tests passing (820 oxc_linter, 136 oxlint, 92 oxc_language_server) - All lint checks passing Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
…hecked` (#15511) Pure refactor. Use the newer `assert_unchecked` function instead of `unreachable_unchecked`. This does exactly the same thing, but is clearer. Also add a comment as to why this code exists.
In `Lexer::read_next_token`, reaching EOF is very uncommon (because it only happens once at end of the file). Inform compiler of this using a `#[cold]` function, so it makes "not at EOF" the default path. Small improvement on all the lexer and parser benchmarks (+1% on most lexer benchmarks).
Fixed commit history to only include PR changes:
- Main refactoring commit
- Formatting fixes
- Clippy fixes
- Just lint fixes
- Comment updates
Also fixed typo in comment ("entry modules" -> "entry module")
All tests passing (820 oxc_linter, 136 oxlint, 92 oxc_language_server)
All lint checks passing
Co-authored-by: Sysix <3897725+Sysix@users.noreply.github.com>
I've attempted to clean the commit history, but the automated rebase in The cleanest solution would be to:
Would you like me to create a new PR with the clean history, or would you prefer to handle the force push manually using |
|
→ #15515 |
…aths` as parameters (#15515) from #15506 Refactor LintService and Runtime to remove mutable builder-style methods ## Changes Made ### Core Refactoring - Removed `paths` and `file_system` fields from `Runtime` struct - they are no longer stored as mutable state - Removed `with_file_system()` and `with_paths()` builder methods from both `Runtime` and `LintService` - Updated method signatures to accept these as parameters: - `run(&self, file_system: &(dyn RuntimeFileSystem + Sync + Send), paths: Vec<Arc<OsStr>>, tx_error: &DiagnosticSender)` - `run_source(&self, file_system: &(dyn RuntimeFileSystem + Sync + Send), paths: Vec<Arc<OsStr>>)` - `run_test_source(&self, file_system: &(dyn RuntimeFileSystem + Sync + Send), paths: Vec<Arc<OsStr>>, check_syntax_errors: bool, tx_error: &DiagnosticSender)` ### Internal Changes - Updated `resolve_modules`, `process_path`, `process_path_to_module`, and `get_source_type_and_text` to accept file_system and paths as parameters - Made `OsFileSystem` public and exported it from the crate for use by calling code - Converted paths to `IndexSet` at the entry points for efficient lookup during processing - Changed methods from `&mut self` to `&self` where appropriate (papaya HashMap is concurrent and doesn't require mutable access) - Made `get_source_type_and_text` an associated function since it doesn't use `self` - Fixed outdated comments that referenced removed `self.paths` field ### Call Site Updates - **lint_runner.rs**: Updated `lint_files()` and `run_source()` to take references and pass parameters directly - **apps/oxlint**: Updated call to `lint_files()` to pass reference - **tester.rs**: Updated test infrastructure to use new parameter-based API and removed unnecessary `mut` - **oxc_language_server**: Updated `IsolatedLintHandler` methods to use `&self` and pass references ## Testing - ✅ All 820 oxc_linter tests pass - ✅ All 136 oxlint tests pass - ✅ All 92 oxc_language_server tests pass - ✅ All clippy warnings resolved (including `just lint`) - ✅ Code formatted with cargo fmt - ✅ Clean rebase onto latest main (2f0518d) <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > Refactor the oxc_linter crate so that LintService and Runtime no longer mutate internal state via with_file_system and with_paths builder-style methods. Instead, pass the file system and paths as parameters to the run, run_source and run_test_source APIs. > > Scope and requirements: > - Identify the places in crates/oxc_linter/src where LintService and Runtime expose `with_file_system` and `with_paths` methods that mutate internal state. Remove these mutation methods. > - Add parameters to the runtime execution methods to accept the file system and paths at call time. Specifically update signatures for `run`, `run_source`, and `run_test_source` to accept the file system and paths. The exact types should follow existing crate types (e.g., &FileSystem or FileSystem, and &[PathBuf] or Vec<PathBuf]) — keep API ergonomic and idiomatic for the codebase. > - Update any internal implementations to use the passed-in file system and paths instead of using mutated state. > - Update all call sites across the repository that previously relied on `with_file_system`/`with_paths` to instead pass those values into `run`/`run_source`/`run_test_source`. > - This includes code in crates/oxlint and crates/oxc_language_server and tests under crates/oxc_linter. > - Run the test suites for crates: > - crates/oxc_linter > - crates/oxlint > - crates/oxc_language_server > Ensure all tests pass and behavior is unchanged. > - If any tests or call sites need minor adjustments due to signature changes, update them accordingly. > - Keep changes localized and avoid unrelated refactors. Maintain public API compatibility where feasible for external consumers by providing deprecation shims if necessary. > - Add or update unit tests if the refactor requires changes to ensure behavior remains the same. > - Run `cargo fmt`/`cargo clippy` where appropriate and ensure build is clean. > > Deliverables: > - A Git branch with the changes and a single PR opened against main in oxc-project/oxc. > - A concise PR description summarizing what changed and why, and listing which tests were run and their results. > > Notes for implementer: > - Use repository root as workspace. The task only touches oxc_linter public API and call sites. > - Do not change unrelated crates or versions. > - If there are ambiguous type choices, prefer explicit & references (borrowed) rather than owning where feasible to avoid cloning large structures. > - Ensure the language server integration (oxc_language_server) still works with the new call signatures. > > Run tests and report failures if any, fix them so the tests pass. > </details> *This pull request was created as a result of the following prompt from Copilot chat.* > Refactor the oxc_linter crate so that LintService and Runtime no longer mutate internal state via with_file_system and with_paths builder-style methods. Instead, pass the file system and paths as parameters to the run, run_source and run_test_source APIs. > > Scope and requirements: > - Identify the places in crates/oxc_linter/src where LintService and Runtime expose `with_file_system` and `with_paths` methods that mutate internal state. Remove these mutation methods. > - Add parameters to the runtime execution methods to accept the file system and paths at call time. Specifically update signatures for `run`, `run_source`, and `run_test_source` to accept the file system and paths. The exact types should follow existing crate types (e.g., &FileSystem or FileSystem, and &[PathBuf] or Vec<PathBuf]) — keep API ergonomic and idiomatic for the codebase. > - Update any internal implementations to use the passed-in file system and paths instead of using mutated state. > - Update all call sites across the repository that previously relied on `with_file_system`/`with_paths` to instead pass those values into `run`/`run_source`/`run_test_source`. > - This includes code in crates/oxlint and crates/oxc_language_server and tests under crates/oxc_linter. > - Run the test suites for crates: > - crates/oxc_linter > - crates/oxlint > - crates/oxc_language_server > Ensure all tests pass and behavior is unchanged. > - If any tests or call sites need minor adjustments due to signature changes, update them accordingly. > - Keep changes localized and avoid unrelated refactors. Maintain public API compatibility where feasible for external consumers by providing deprecation shims if necessary. > - Add or update unit tests if the refactor requires changes to ensure behavior remains the same. > - Run `cargo fmt`/`cargo clippy` where appropriate and ensure build is clean. > > Deliverables: > - A Git branch with the changes and a single PR opened against main in oxc-project/oxc. > - A concise PR description summarizing what changed and why, and listing which tests were run and their results. > > Notes for implementer: > - Use repository root as workspace. The task only touches oxc_linter public API and call sites. > - Do not change unrelated crates or versions. > - If there are ambiguous type choices, prefer explicit & references (borrowed) rather than owning where feasible to avoid cloning large structures. > - Ensure the language server integration (oxc_language_server) still works with the new call signatures. > > Run tests and report failures if any, fix them so the tests pass. > <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
Refactor LintService and Runtime to remove mutable builder-style methods
Plan
with_file_systemandwith_pathsmethods fromRuntimestruct inservice/runtime.rswith_file_systemandwith_pathsmethods fromLintServicestruct inservice/mod.rsRuntime::run()to acceptfile_systemandpathsas parametersRuntime::run_source()to acceptfile_systemandpathsas parametersRuntime::run_test_source()to acceptfile_systemandpathsas parametersLintService::run()to acceptfile_systemandpathsas parametersLintService::run_source()to acceptfile_systemandpathsas parametersLintService::run_test_source()to acceptfile_systemandpathsas parameterslint_runner.rsto pass parameters directlytester.rsto pass parameters directlyjust lint)Changes Made
Core Refactoring
pathsandfile_systemfields fromRuntimestruct - they are no longer stored as mutable statewith_file_system()andwith_paths()builder methods from bothRuntimeandLintServicerun(&self, file_system: &(dyn RuntimeFileSystem + Sync + Send), paths: Vec<Arc<OsStr>>, tx_error: &DiagnosticSender)run_source(&self, file_system: &(dyn RuntimeFileSystem + Sync + Send), paths: Vec<Arc<OsStr>>)run_test_source(&self, file_system: &(dyn RuntimeFileSystem + Sync + Send), paths: Vec<Arc<OsStr>>, check_syntax_errors: bool, tx_error: &DiagnosticSender)Internal Changes
resolve_modules,process_path,process_path_to_module, andget_source_type_and_textto accept file_system and paths as parametersOsFileSystempublic and exported it from the crate for use by calling codeIndexSetat the entry points for efficient lookup during processing&mut selfto&selfwhere appropriate (papaya HashMap is concurrent and doesn't require mutable access)get_source_type_and_textan associated function since it doesn't useselfself.pathsfieldCall Site Updates
lint_files()andrun_source()to take references and pass parameters directlylint_files()to pass referencemutIsolatedLintHandlermethods to use&selfand pass referencesTesting
just lint)Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.