feat(oxc_language_server): Support nested configs by nrayburn-tech · Pull Request #9724 · oxc-project/oxc

nrayburn-tech · 2025-03-12T23:14:21Z

Just putting this up to have the idea validated before I clean it up and separate it out into different PRs.

Left comments inline to sort of communicate what I'm doing or why. I don't do much in Rust, so I think it might be helpful to communicate what I'm trying to do in case there are better alternatives.

There are multiple places doing .unwrap that I want to review and fix.
I want to split this into at least a separate PR for the language server changes and the VS Code changes. Maybe even additional PRs that aren't explicitly required for nested config support, such as the change on what files are watched from VS Code.

graphite-app · 2025-03-12T23:14:26Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.}

codspeed-hq · 2025-03-12T23:16:43Z

CodSpeed Performance Report

Merging #9724 will create unknown performance changes

_{Comparing nrayburn-tech:lsp-nested-configs (128680c) with main (b9ab60b)}

Summary

🆕 33 new benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
🆕	`codegen[checker.ts]`	N/A	23.1 ms	N/A
🆕	`codegen_sourcemap[checker.ts]`	N/A	66.1 ms	N/A
🆕	`isolated-declarations[vue-id.ts]`	N/A	58.7 ms	N/A
🆕	`lexer[RadixUIAdoptionSection.jsx]`	N/A	20.8 µs	N/A
🆕	`lexer[antd.js]`	N/A	24.1 ms	N/A
🆕	`lexer[cal.com.tsx]`	N/A	5.7 ms	N/A
🆕	`lexer[checker.ts]`	N/A	14.5 ms	N/A
🆕	`lexer[pdf.mjs]`	N/A	3.8 ms	N/A
🆕	`linter[RadixUIAdoptionSection.jsx]`	N/A	2.7 ms	N/A
🆕	`linter[cal.com.tsx]`	N/A	1.2 s	N/A
🆕	`linter[checker.ts]`	N/A	2.9 s	N/A
🆕	`mangler[antd.js]`	N/A	16.1 ms	N/A
🆕	`mangler[react.development.js]`	N/A	294.5 µs	N/A
🆕	`mangler[typescript.js]`	N/A	39.7 ms	N/A
🆕	`minifier[antd.js]`	N/A	167.6 ms	N/A
🆕	`minifier[react.development.js]`	N/A	1.8 ms	N/A
🆕	`minifier[typescript.js]`	N/A	294.5 ms	N/A
🆕	`estree[checker.ts]`	N/A	88.4 ms	N/A
🆕	`parser[RadixUIAdoptionSection.jsx]`	N/A	88.8 µs	N/A
🆕	`parser[antd.js]`	N/A	112.1 ms	N/A
...	...	...	...	...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

nrayburn-tech · 2025-03-12T23:14:57Z

crates/oxc_language_server/src/main.rs

    diagnostics_report_map: ConcurrentHashMap<String, Vec<DiagnosticReport>>,
    options: Mutex<Options>,
    gitignore_glob: Mutex<Vec<Gitignore>>,
+    nested_configs: RwLock<FxHashMap<PathBuf, ConfigStore>>,


Store a map of directory to config for all nested configs in the workspace.

nrayburn-tech · 2025-03-12T23:15:23Z

crates/oxc_language_server/src/main.rs

    run: Run,
    enable: bool,
    config_path: String,
+    use_nested_configs: bool,


Allow opting into nested configs. If false, there shouldn't be any behavior change.

nrayburn-tech · 2025-03-12T23:16:33Z

crates/oxc_language_server/src/main.rs

-    async fn did_change_watched_files(&self, _params: DidChangeWatchedFilesParams) {
+    async fn did_change_watched_files(&self, params: DidChangeWatchedFilesParams) {
        debug!("watched file did change");
+        if self.options.lock().await.use_nested_configs {


If using nested configs, check the watched files to determine if they are .oxlintrc.json files (which are the only nested config files that are supported to my knowledge).

This just keeps the nested config map up to date with any workspace changes.

nrayburn-tech · 2025-03-12T23:17:30Z

crates/oxc_language_server/src/main.rs

+            );
+        }
+
+        if self.options.lock().await.use_nested_configs {


If using nested configs, use Linter::new_with_nested_configs and pass in the nested config files. Otherwise, should be no behavior change. However, I did do some refactoring here to make this easier for me to read through.

nrayburn-tech · 2025-03-12T23:19:20Z

editors/vscode/client/extension.ts

+        workspace.createFileSystemWatcher(oxlintConfigFileGlob),
+        ...workspaceConfigPatterns.map((workspaceConfigPattern) => {
+          return workspace.createFileSystemWatcher(workspaceConfigPattern);
+        }),


Watch any .oxlintrc.json file in the workspace.
Watch the file that the user has configured within their configuration.

This stops watching all variations of oxlint config files that were previously watched. Which, I think is safe.

nrayburn-tech · 2025-03-12T23:19:59Z

editors/vscode/client/extension.ts

+    changes: initialConfigFiles.map(file => {
+      return { uri: file.toString(), type: FileChangeType.Created };
+    }),
+  });


On startup, send all found config files to the language server as "created", so that they get added to the nested config map.

nrayburn-tech · 2025-03-12T23:23:53Z

crates/oxc_language_server/src/main.rs

    diagnostics_report_map: ConcurrentHashMap<String, Vec<DiagnosticReport>>,
    options: Mutex<Options>,
    gitignore_glob: Mutex<Vec<Gitignore>>,
+    nested_configs: RwLock<FxHashMap<PathBuf, ConfigStore>>,


I don't know if this is the correct type. RwLock<FxHashMap<PathBuf, ConfigStore>>. Maybe something else should be used, but this is the only one I could figure out how to make work.

Seems ok to me, but I'm not an expert in the async data structures area 😄 Might be worth looking at https://docs.rs/papaya/latest/papaya/, as I know we have been using that recently?

nrayburn-tech · 2025-03-12T23:25:04Z

crates/oxc_language_server/src/main.rs

+            let existing_nested_configs = (*self.nested_configs.read().await).clone();
+            for (key, value) in existing_nested_configs {
+                config_files.insert(key.clone(), value.clone());
+            }


I am copying the map here, so I can mutate it later, before setting the value back into self.nested_configs. There might be a better way to do this that I don't know about.

Might be worth looking at https://doc.rust-lang.org/std/mem/fn.take.html. It still requires allocating an empty hash map, but that is much cheaper compared to cloning a full hash map. Then after you've taken the hash map, you have full ownership over it and can insert into as needed.

nrayburn-tech · 2025-03-12T23:25:46Z

crates/oxc_language_server/src/main.rs

+            let existing_nested_configs = (*self.nested_configs.read().await).clone();
+            for (key, value) in existing_nested_configs {
+                config_files.insert(key.clone(), value.clone());
+            }


Copying the map so that I can pass it into Linter::new_with_nested_configs. Might be a better way to do the copy that I'm not aware of.

maybe std::mem::take? https://doc.rust-lang.org/std/mem/fn.take.html but not sure if that works in async context like this

nrayburn-tech · 2025-03-12T23:26:13Z

crates/oxc_linter/src/config/config_store.rs

 }

-#[derive(Debug)]
+#[derive(Debug, Clone)]


Needed so that I can clone my map that contains this struct.

nrayburn-tech · 2025-03-12T23:26:18Z

crates/oxc_linter/src/config/config_store.rs


 /// Resolves a lint configuration for a given file, by applying overrides based on the file's path.
-#[derive(Debug)]
+#[derive(Debug, Clone)]


Needed so that I can clone my map that contains this struct.

nrayburn-tech · 2025-03-12T23:28:54Z

@camchenry can you review this to make sure the logic lines up with how the CLI works? (And any assistance with the code that looks odd would be helpful to, like if I should be doing something different with the maps instead of cloning.)

nrayburn-tech · 2025-03-12T23:30:55Z

@Sysix can you make sure this functionality makes sense to you from the language server perspective? (And any feedback with how the code is actually written would be appreciated too.)

camchenry

I think this all looks right so far. I guess one advantage of having the actual config resolution in just oxc_linter is that it should "just work". As long as the config files are found correctly and passed in, the config files should just get resolved as expected, same as in the CLI (hopefully).

camchenry · 2025-03-13T00:09:43Z

crates/oxc_language_server/src/main.rs

+            let mut config_files = FxHashMap::default();
+            let existing_nested_configs = (*self.nested_configs.read().await).clone();
+            for (key, value) in existing_nested_configs {
+                config_files.insert(key.clone(), value.clone());


instead of inserting one-by-one, maybe we can try extending the hash map all at once? this should be more efficient because it allocates the needed memory upfront: https://doc.rust-lang.org/nightly/std/collections/struct.HashMap.html#impl-Extend%3C(K,+V)%3E-for-HashMap%3CK,+V,+S%3E

camchenry · 2025-03-13T00:10:42Z

crates/oxc_language_server/src/main.rs

+            let existing_nested_configs = (*self.nested_configs.read().await).clone();
+            for (key, value) in existing_nested_configs {
+                config_files.insert(key.clone(), value.clone());
+            }


Might be worth looking at https://doc.rust-lang.org/std/mem/fn.take.html. It still requires allocating an empty hash map, but that is much cheaper compared to cloning a full hash map. Then after you've taken the hash map, you have full ownership over it and can insert into as needed.

camchenry · 2025-03-13T00:13:47Z

crates/oxc_language_server/src/main.rs

+                if x.uri.to_file_path().unwrap().file_name().unwrap() != OXC_CONFIG_FILE {
+                    return;
+                }
+                // spellchecker:off -- "typ" is accurate
+                if x.typ == FileChangeType::CREATED || x.typ == FileChangeType::CHANGED {
+                    // spellchecker:on
+                    let file_path = x.uri.to_file_path().unwrap();


we could avoid a duplicate unwrap here and some work by saving to_file_path():

Suggested change

if x.uri.to_file_path().unwrap().file_name().unwrap() != OXC_CONFIG_FILE {

return;

}

// spellchecker:off -- "typ" is accurate

if x.typ == FileChangeType::CREATED || x.typ == FileChangeType::CHANGED {

// spellchecker:on

let file_path = x.uri.to_file_path().unwrap();

let Some(file_path) = x.uri.to_file_path() else {

return;

};

if file_path.file_name().unwrap() != OXC_CONFIG_FILE {

return;

}

// spellchecker:off -- "typ" is accurate

if x.typ == FileChangeType::CREATED || x.typ == FileChangeType::CHANGED {

// spellchecker:on

camchenry · 2025-03-13T00:16:58Z

crates/oxc_language_server/src/main.rs

+            let existing_nested_configs = (*self.nested_configs.read().await).clone();
+            for (key, value) in existing_nested_configs {
+                config_files.insert(key.clone(), value.clone());
+            }


maybe std::mem::take? https://doc.rust-lang.org/std/mem/fn.take.html but not sure if that works in async context like this

nrayburn-tech · 2025-03-13T08:11:38Z

Closing this as it has been replaced with smaller PRs along with some of the feedback from here addressed.

#9731 - Only watch relevant config files
#9739 - Language server nested config support.
TBD - VS Code send all config files to the Language server on extension startup.

feat(oxc_language_server): Support nested configs

128680c

github-actions bot added A-linter Area - Linter A-editor Area - Editor and Language Server C-enhancement Category - New feature or request labels Mar 12, 2025

nrayburn-tech commented Mar 12, 2025

View reviewed changes

nrayburn-tech requested review from Sysix and camchenry March 12, 2025 23:27

camchenry reviewed Mar 13, 2025

View reviewed changes

nrayburn-tech mentioned this pull request Mar 13, 2025

feat(oxc_language_server): Support nested configs #9739

Merged

nrayburn-tech closed this Mar 13, 2025

nrayburn-tech deleted the lsp-nested-configs branch July 10, 2025 23:54

Uh oh!

Conversation

nrayburn-tech commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot commented Mar 12, 2025

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #9724 will create unknown performance changes

Summary

Benchmarks breakdown

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrayburn-tech commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrayburn-tech commented Mar 12, 2025

Uh oh!

camchenry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrayburn-tech commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nrayburn-tech commented Mar 12, 2025 •

edited

Loading

codspeed-hq bot commented Mar 12, 2025 •

edited

Loading

nrayburn-tech commented Mar 12, 2025 •

edited

Loading