Add support for reading `air.toml` in the CLI and LSP #122

DavisVaughan · 2024-12-20T20:17:46Z

Closes #83

The point of this PR is to allow air to read air.toml configuration files in your project and pass information from them on to format document / selection requests. There are 3 main parts:

Finding and reading air.toml files. This is all contained in a new workspace crate. We read air.toml files with the toml crate. The "spec" is the TomlOptions struct itself.
Utilizing the workspace tools in the CLI. This is pretty easy and small since each CLI call is a "one shot" discovery of air.toml configuration files (i.e. there is no state to maintain between CLI calls).
Utilizing the workspace tools in the LSP. This is much more involved because we want to:
- Parse and cache the air.tomls we find and associate them correctly with R files in a workspace
- Refresh the air.tomls when the user adds a new workspace, or if the user changes the air.toml

DavisVaughan · 2024-12-20T21:37:32Z

crates/lsp/src/capabilities.rs

+pub(crate) struct ResolvedClientCapabilities {
+    pub(crate) position_encodings: Vec<PositionEncodingKind>,
+    pub(crate) dynamic_registration_for_did_change_configuration: bool,
+    pub(crate) dynamic_registration_for_did_change_watched_files: bool,
+}


Collecting client capabilities we care about here. We get them in Initialize but use them in other places

DavisVaughan · 2024-12-20T21:38:11Z

crates/lsp/src/handlers.rs

+    if lsp_state
+        .capabilities
+        .dynamic_registration_for_did_change_watched_files
+    {
+        // Watch for changes in `air.toml` files so we can react dynamically
+        let watch_air_toml_registration = lsp_types::Registration {
+            id: uuid::Uuid::new_v4().to_string(),
+            method: "workspace/didChangeWatchedFiles".into(),
+            register_options: Some(
+                serde_json::to_value(DidChangeWatchedFilesRegistrationOptions {
+                    watchers: vec![FileSystemWatcher {
+                        glob_pattern: lsp_types::GlobPattern::String("**/air.toml".into()),
+                        kind: None,
+                    }],
+                })
+                .unwrap(),
+            ),
+        };
+
+        registrations.push(watch_air_toml_registration);


Watching for changes in air.toml files anywhere within a workspace (creation, deletion, modify)

DavisVaughan · 2024-12-20T21:39:30Z

crates/lsp/src/handlers_format.rs

 pub(crate) fn document_formatting(
    params: lsp_types::DocumentFormattingParams,
+    lsp_state: &LspState,
    state: &WorldState,
 ) -> anyhow::Result<Option<Vec<lsp_types::TextEdit>>> {
    let doc = state.get_document(&params.text_document.uri)?;

-    let line_width = LineWidth::try_from(80).map_err(|err| anyhow::anyhow!("{err}"))?;
-
-    // TODO: Handle FormattingOptions
-    let options = RFormatOptions::default()
-        .with_indent_style(IndentStyle::Space)
-        .with_line_width(line_width);
+    let settings = lsp_state.document_settings(&params.text_document.uri);
+    let format_options = settings.format.to_format_options(&doc.contents);


After all was said and done this is the main change to the FormatDocument request code, pretty nice right now

Note that we are currently ignoring anything related to the document settings provided by the editor

DavisVaughan · 2024-12-20T21:40:38Z

crates/lsp/src/handlers_state.rs

+pub(crate) fn did_change_workspace_folders(
+    params: DidChangeWorkspaceFoldersParams,
+    lsp_state: &mut LspState,
+) -> anyhow::Result<()> {
+    // Collect all `errors` to ensure we don't drop events after a first error
+    let mut errors = ErrorVec::new();
+
+    for lsp_types::WorkspaceFolder { uri, .. } in params.event.added {
+        errors.push_err(lsp_state.open_workspace_folder(&uri, Settings::default()));
+    }
+    for lsp_types::WorkspaceFolder { uri, .. } in params.event.removed {
+        errors.push_err(lsp_state.close_workspace_folder(&uri));
+    }
+
+    errors.into_result()
+}


Updating our per-workspace settings resolver when a workspace is opened / closed

DavisVaughan · 2024-12-20T21:41:07Z

crates/lsp/src/handlers_state.rs

+pub(crate) fn did_change_watched_files(
+    params: DidChangeWatchedFilesParams,
+    lsp_state: &mut LspState,
+) -> anyhow::Result<()> {
+    for change in &params.changes {
+        lsp_state
+            .workspace_settings_resolver
+            .reload_workspaces_matched_by_url(&change.uri);
+    }
+
+    Ok(())
+}


Updating our per-workspace settings resolver when an air.toml is changed

DavisVaughan · 2024-12-20T21:41:58Z

crates/lsp/src/snapshots/lsp__handlers_format__tests__format_range_logical_lines-4.snap

-  2 + 2
+	2 + 2


The LSP was hardcoded to Space before but we are going to get the default settings now, which are currently Tab

DavisVaughan · 2024-12-20T21:44:48Z

crates/lsp/src/workspaces.rs

+    /// Return the appropriate [`Settings`] for a given document [`Url`].
+    pub(crate) fn settings_for_url(&self, url: &Url) -> &Settings {
+        if let Ok(Some(path)) = Self::url_to_path(url) {
+            return self.settings_for_path(&path);
+        }
+
+        // For `untitled` schemes, we have special behavior.
+        // If there is exactly 1 workspace, we resolve using a path of
+        // `{workspace_path}/untitled` to provide relevant settings for this workspace.
+        if url.scheme() == "untitled" && self.path_to_settings_resolver.len() == 1 {
+            tracing::trace!("Using workspace settings for 'untitled' URL: {url}");
+            let workspace_path = self.path_to_settings_resolver.keys().next().unwrap();
+            let path = workspace_path.join("untitled");
+            return self.settings_for_path(&path);
+        }
+
+        tracing::trace!("Using default settings for non-file URL: {url}");
+        self.path_to_settings_resolver.fallback().fallback()
+    }


Tricky little behavior here with untitled files

If there aren't any workspaces open, then the untitled file can't be associated with any air.toml

If there are >1 workspaces open, we can't make a reasonable guess about which workspace's air.toml to try and associate the untitled file with

If there is exactly 1 workspace open (the 95% case), then we can make a reasonable guess that we should use that workspace's air.toml with untitled files

DavisVaughan · 2024-12-20T21:46:56Z

crates/lsp/src/workspaces.rs

+        for (workspace_path, settings_resolver) in self.path_to_settings_resolver.matches_mut(&path)
+        {
+            tracing::trace!("Reloading workspace settings: {}", workspace_path.display());
+
+            settings_resolver.clear();
+
+            if let Err(error) = settings_resolver.load_from_paths(&[workspace_path]) {
+                tracing::error!(
+                    "Failed to reload workspace settings for {path}:\n{error}",
+                    path = workspace_path.display(),
+                    error = error
+                );
+            }
+        }


I find this "refresh" behavior kind of awesome. We mutably iterate over the workspace specific resolvers that are associated with the updated air.toml and update them in place by clearing them and then rediscovering the air.tomls within them

DavisVaughan · 2024-12-20T21:48:10Z

crates/workspace/src/resolve.rs

+    /// Resolve a [`Path`] to its associated `T`
+    ///
+    /// This resolver works by finding the closest directory to the `path` to search for.
+    ///
+    /// The [`BTreeMap`] is an ordered map, so if you do:
+    ///
+    /// ```text
+    /// resolver.add("a/b", value1)
+    /// resolver.add("a/b/c", value2)
+    /// resolver.add("a/b/d", value3)
+    /// resolver.resolve("a/b/c/test.R")
+    /// ```
+    ///
+    /// Then it detects both `"a/b"` and `"a/b/c"` as being "less than" the path of
+    /// `"a/b/c/test.R"`, and then chooses `"a/b/c"` because it is at the back of
+    /// that returned sorted list (i.e. the "closest" match).
+    pub fn resolve(&self, path: &Path) -> Option<&T> {
+        self.resolve_entry(path).map(|(_, value)| value)
+    }


This is the main thing to read about how path/to/file.R -> Settings resolution works

DavisVaughan · 2024-12-20T21:53:17Z

crates/workspace/src/resolve.rs

+    // TODO: Make these configurable options (possibly just one?)
+    // Right now we explicitly call them even though they are `true` by default
+    // to remind us to expose them.
+    //
+    // "This toggles, as a group, all the filters that are enabled by default"
+    // builder.standard_filters(true)
+    builder.hidden(true);
+    builder.parents(true);
+    builder.ignore(false);
+    builder.git_ignore(true);
+    builder.git_global(true);
+    builder.git_exclude(true);


It's nice that we will get gitignore support out of the box so we don't format any directories marked in a .gitignore

DavisVaughan · 2024-12-20T21:53:45Z

crates/workspace/src/settings.rs

+impl FormatSettings {
+    // Finalize `RFormatOptions` in preparation for a formatting operation on `source`
+    pub fn to_format_options(&self, source: &str) -> RFormatOptions {
+        let line_ending = match self.line_ending {
+            LineEnding::Lf => biome_formatter::LineEnding::Lf,
+            LineEnding::Crlf => biome_formatter::LineEnding::Crlf,
+            #[cfg(target_os = "windows")]
+            LineEnding::Native => biome_formatter::LineEnding::Crlf,
+            #[cfg(not(target_os = "windows"))]
+            LineEnding::Native => biome_formatter::LineEnding::Lf,
+            LineEnding::Auto => match line_ending::infer(source) {
+                line_ending::LineEnding::Lf => biome_formatter::LineEnding::Lf,
+                line_ending::LineEnding::Crlf => biome_formatter::LineEnding::Crlf,
+            },
+        };
+
+        RFormatOptions::new()
+            .with_indent_style(self.indent_style.into())
+            .with_indent_width(self.indent_width.into())
+            .with_line_ending(line_ending)
+            .with_line_width(self.line_length.into())
+            .with_magic_line_break(self.magic_line_break.into())
+    }


Finalized FormatSettings are really "mostly" finalized. Things like LineEnding::Auto get truly finalized when converting to RFormatOptions, because that's when we are most likely to have the source code to infer line endings for lying around.

DavisVaughan · 2024-12-20T21:54:03Z

crates/workspace/src/settings/line_ending.rs

+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Default, serde::Deserialize)]
+#[serde(rename_all = "kebab-case")]
+pub enum LineEnding {


I think it is very reasonable / inevitable that we end up with our own copies of these settings in addition to the ones from biome.

For each one we need a Deserialize method that works nicely with the toml crate and uses kebab-case

For some, like here, we add additional user facing options like Auto that don't make it into the underlying formatter option

For some, like IndentWidth, we may choose to have our own IndentWidth::default() that is different from what biome's is

DavisVaughan · 2024-12-20T21:54:43Z

crates/workspace/src/toml_options.rs

+    /// The line length at which the formatter prefers to wrap lines.
+    ///
+    /// The value must be greater than or equal to `1` and less than or equal to `320`.
+    ///
+    /// Note: While the formatter will attempt to format lines such that they remain
+    /// within the `line-length`, it isn't a hard upper bound, and formatted lines may
+    /// exceed the `line-length`.
+    pub line_length: Option<LineLength>,


ruff has some very cool macros that use TomlOptions as its "source of truth" for documentation.

It extracts out each table name (like format) and each option under the table (like line_length) along with all documentation comments and makes a really nice markdown page for it that ends up here:

https://docs.astral.sh/ruff/settings/

Without a page like this its hard to know what the spec is for what you can supply in air.toml

DavisVaughan · 2024-12-20T21:54:57Z

crates/workspace/src/toml_options.rs

+    /// This option changes the number of spaces the formatter inserts when
+    /// using `indent-style = "space"`. It also represents the width of a tab when
+    /// `indent-style = "tab"` for the purposes of computing the `line-length`.
+    pub indent_width: Option<IndentWidth>,


Both line_length and indent_width are top-level global options used by "all of air" because if we consider a linter then it will need access to these options to determine if a line exceeds the line length.

Compare that with indent_style, line_ending, and ignore_magic_line_break which are more clearly format specific and live under the [format] table header.

DavisVaughan · 2024-12-20T21:56:49Z

editors/code/src/lsp.ts

-			synchronize: {
-				// Notify the server about file changes to R files contained in the workspace
-				fileEvents:
-					vscode.workspace.createFileSystemWatcher("**/*.[Rr]"),
-			},


This causes the middleware to automatically start sending DidChangeWatchedFiles notifications to the Client

It was super confusing for me when I was registering the air.toml watched files notification because it looked like I had somehow done it wrong and was getting .R files too!

This was referenced Dec 20, 2024

Read air.toml and use in air format #109

Closed

Blockers for alpha release #56

Open

DavisVaughan force-pushed the feature/air-toml branch from f345350 to 8045f6c Compare December 20, 2024 20:21

DavisVaughan commented Dec 20, 2024

View reviewed changes

Add support for reading air.toml in the CLI and LSP

ba8d566

DavisVaughan force-pushed the feature/air-toml branch from 8045f6c to ba8d566 Compare December 20, 2024 21:52

DavisVaughan commented Dec 20, 2024

View reviewed changes

This was referenced Dec 20, 2024

Send structured InitializationOptions to the server #121

Open

Log a warning on parse errors, but don't return an LSP error #120

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reading `air.toml` in the CLI and LSP #122

Add support for reading `air.toml` in the CLI and LSP #122

DavisVaughan commented Dec 20, 2024 •

edited

Loading

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

DavisVaughan Dec 20, 2024

Add support for reading air.toml in the CLI and LSP #122

Are you sure you want to change the base?

Add support for reading air.toml in the CLI and LSP #122

Conversation

DavisVaughan commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for reading `air.toml` in the CLI and LSP #122

Add support for reading `air.toml` in the CLI and LSP #122

DavisVaughan commented Dec 20, 2024 •

edited

Loading