refactor(core): configuration, settings and capabilities #4845

ematipico · 2025-01-06T18:07:53Z

Summary

Supersedes #3317

Configuration

The Partial macro has been removed. The macro was hiding some important information, and that caused problems where some fields were Option<T>, and others were T.
All fields are Option<T>, and all fields must be Option. As a consequence, the macro #[serde(skip_serializing_if = "Option::is_none")] needs to be implemented for all fields. This was already done for rules and actions. Failing to implement the macro will result in some snapshot regressions, so we're good here and can catch bugs early.
Consistent naming. All structs are suffixed with *Configuration
Added a new Bool type that holds at build time the default value of the bool assigned. This value is then assigned to the relative setting.
Due to how bool values functioned in our toolchain, we were forced to use methods called is_disabled. This is no longer an issue thanks to the new Bool type, so now we can use is_enabled everywhere.

Settings

The most significant change is how the workspace computes when whether a file is enabled for a certain feature. There are four occurrences: top-level configuration, language configuration, override top-level configuration, override language configuration. The priority is (first has most priority):
1. overrides.langauges.css.formatter.enabled
2. overrides.formatter.enabled
3. langauges.css.formatter.enabled
4. css.formatter.enabled
The main problem was that every time we needed to add a new language to the toolchain, it was very easy to miss updating the new functionality. Now, this functionality has been moved to the ServiceLangauge level. The ServiceLanguage is used by the workspace to implement common functionality that must be supported by all languages. This means that we need to update ServiceLanguage only when we add a new capability. When adding a new language, the code won't compile until all the functions of the ServiceLanguage are implemented.

As you might have guessed, we would need to update the ServiceLanguage trait in case we add a new feature (like we did with the search). We add languages more often than features, so that's the more maintainable solution.
A new method called unwrap_settings was created, which is used instead of get_settings. Why? Because the absence of Settings inside a project should be an error, not a possibility. Biome always creates Settings, even when biome.json isn't present. In fact, we have already raised an error (only in one occurrence) in case settings aren't found. However, I left many method signatures to Option<&Settings> because the PR was becoming too big. I plan to that change that later.
There were a lot of missing features, like assist, that weren't implemented. One more reason why implementing things at ServiceLanguage level should improve the internal DX of the codebase.

Capabilities

I introduced a new set of capabilities. The capability is called enabled_for_path, and each member of the type EnabledForPath checks if a certain feature (formatter, linter, assist and search) is enabled for the given path. The functions are very simple, and they are meant to call the ServiceLanguage functions via WorkspaceSettingsHandle. See ### Settings, first bullet point.

Missing things that I will implement later

Change all Option<&Settings> to &Settings
Pass WorkspaceSettingsHandle to methods instead of &Settings. WorkspaceSettingsHandle is what needs to be used all over, because it's the type that is meant to hold the settings coming from the IDE.
Add search to the configuration (and relative sub configurations, per language), so users can configure which files can be searched or not. This isn't implemented nowadays

Test Plan

The majority of CLI tests should pass.
The rage snapshots changed, and they are fixed (you'll more unset).
The help snapshots have been updated too.

…-configuration

codspeed-hq · 2025-01-06T23:08:28Z

CodSpeed Performance Report

Merging #4845 will improve performances by 6.12%

_{Comparing refactor/settings-and-configuration (a54c8a8) with next (01044aa)}

Summary

⚡ 1 improvements
✅ 96 untouched benchmarks

Benchmarks breakdown

	Benchmark	`next`	`refactor/settings-and-configuration`	Change
⚡	`pure_9395922602181450299.css[cached]`	3.6 ms	3.4 ms	+6.12%

crates/biome_cli/tests/snapshots/main_commands_rage/with_configuration.snap

arendjr

Thanks a ton for this! This is a great cleanup! ❤️

Quite relieved to see the Partial prefixes are gone now. They never sat well with me, even when I didn't see a better alternative back then. Glad to see we have a better alternative now :)

The Bool type is also a big improvement 💪

I left a bunch of comments, mostly nits, but there's a few places where I think we should clean up a little bit more.

arendjr · 2025-01-07T09:07:07Z

crates/biome_cli/src/commands/rage.rs

-                            .map(|s| s.to_string())
-                            .collect::<Vec<_>>()
-                            .join(", ");
+                        let ignore = formatter_configuration.ignore.map(|s| {


Can we use more descriptive names instead of s? This makes it hard to understand this code just by reading it, especially in GitHub where I can't hover over the type.

arendjr · 2025-01-07T09:09:39Z

crates/biome_cli/tests/snapshots/main_commands_check/check_help.snap

        --grit-formatter-enabled=<true|false>  Control the formatter for Grit files.
        --grit-formatter-indent-style=<tab|space>  The indent style applied to Grit files.
        --grit-formatter-indent-width=NUMBER  The size of the indentation applied to Grit files.
                              Default to 2.
        --grit-formatter-line-ending=<lf|crlf|cr>  The type of line ending applied to Grit files.
        --grit-formatter-line-width=NUMBER  What's the max width of a line applied to Grit files.
                              Defaults to 80.
+        --grit-linter-enabled=<true|false>  Control the linter for Grit files.


Do we even have any lint rules for Grit? :) Either way I don't mind, it's good to have it for completeness so we can't forget it should we add any.

arendjr · 2025-01-07T09:12:31Z

crates/biome_cli/tests/snapshots/main_commands_rage/with_configuration.snap

-  Linter disabled:              false
-  Assist disabled:              false
+  Linter disabled:              true
+  Assist disabled:              true


Not really a feedback for this PR, but I think it would be better if we inverted the phrasing here. "Assist enabled" is more friendly than "Assist disabled", since it avoids the negation.

arendjr · 2025-01-07T09:15:07Z

crates/biome_cli/tests/snapshots/main_commands_rage/with_formatter_configuration.snap

@@ -85,20 +85,20 @@ Formatter:
  Line ending:                  LF
  Line width:                   120
  Attribute position:           Multiline
-  Bracket spacing:              true
+  Bracket spacing:              unset


I think "Not set" is better phrasing than "unset". Even better would be "true (inherited)" or, in this case, "true (default)", so you can see the effective value while it's also clear where this value comes from.

crates/biome_configuration/src/analyzer/linter/mod.rs

arendjr · 2025-01-07T10:00:10Z

crates/biome_service/src/settings.rs

+///
+/// - If `is_override` is `true`, we don't care if a feature is meant to be opt-in, so we fall back
+///   to the top-level configuration.
+pub(crate) fn check_feature_activity<const LANG: bool, const TOP: bool>(


I think we better split this function into two: One for overrides and one for non-overrides. The boolean doesn't make for great readability at the call sites (I had to jump to the implementation to understand what it even meant), and as the function documentation already indicates, the actual implementation is quite different between the two versions.

crates/biome_service/src/workspace.rs

arendjr · 2025-01-07T10:04:50Z

crates/biome_service/src/workspace/server.rs

-    /// global settings object for this workspace
-    settings: Projects,
+    /// projects held by the workspace
+    projects: Projects,


Hah, I did the exact same rename in my PR. No worries, it should be a very minor conflict for the last one to merge :)

crates/biome_service/src/projects.rs

arendjr · 2025-01-07T10:18:13Z

crates/biome_service/src/file_handlers/graphql.rs

+    fn formatter_enabled_for_this_file_path(settings: Option<&Settings>, path: &Utf8Path) -> bool {
+        settings
+            .and_then(|settings| {
+                let overrides_activity =
+                    settings
+                        .override_settings
+                        .patterns
+                        .iter()
+                        .rev()
+                        .find_map(|pattern| {
+                            check_feature_activity(
+                                pattern.languages.graphql.formatter.enabled,
+                                pattern.formatter.enabled,
+                                true,
+                            )
+                            .and_then(|enabled| {
+                                // Then check whether the path satisfies
+                                if pattern.include.matches_path(path)
+                                    && !pattern.exclude.matches_path(path)
+                                {
+                                    Some(enabled)
+                                } else {
+                                    None
+                                }
+                            })
+                        });
+
+                overrides_activity.or(check_feature_activity(
+                    settings.languages.graphql.formatter.enabled,
+                    settings.formatter.enabled,
+                    false,
+                ))
+            })
+            .unwrap_or_default()
+            .into()
+    }


These functions are quite a bit of a repeated pattern. Every language has three of these and it seems the only things that really changes are the pattern.languages.graphql.formatter.enabled and pattern.formatter.enabled fields. I think it would be good to create a helper for this, so they don't need to be copy-pasted as much.

I suspect we can even achieve this by moving the bodies of these functions fully onto the ServiceLanguage trait, and instead let the language-specific implementations implement getters that return fields such as settings.languages.graphql.formatter.enabled.

I already explored this solution, but unfortunately, the compiler gets in the way because we would need to define tree const generics bool, and eventually, the last check_feature_activity doesn't compile because the compiler expects a different type.

I don't have enough knowledge and energies to go back to the previous solution and fix the compiler errors.

I'd prefer to keep this here now, and maybe explore a solution later.

arendjr · 2025-01-07T10:26:29Z

Add search to the configuration (and relative sub configurations, per language), so users can configure which files can be searched or not. This isn't implemented nowadays.

I wouldn't do this, to be honest. Search is very much an ad-hoc action, and it's near impossible to predict how someone wants to use it. Configuring it means configuring the scope for a search while you don't yet know what you'll be searching for. I'd just leave the configuration to the CLI/LSP instead.

ematipico · 2025-01-07T10:28:37Z

Configuring it means configuring the scope for a search while you don't yet know what you'll be searching for

True. However, some users would like not to search (or search!) some paths, like generated files, minified files, etc.

arendjr · 2025-01-07T10:30:48Z

However, some users would like not to search (or search!) some paths, like generated files, minified files, etc.

Yeah, but I'd expect things like generated files to be part of the top-level files.exclude already. We don't need search-specific settings for that. At least I wouldn't spend energy on it until someone specifically asks for it :)

Co-authored-by: Arend van Beelen jr. <[email protected]>

ematipico · 2025-01-07T18:10:41Z

Regarding #4845 (comment), I did more exploration. Even though const D: bool is convenient, if forces us to pass any generic inside the function that uses Bool.

In order to make things generic, we can't determine the exact types, and the compiler isn't able to infer them, so we are kind stuck.

In order to make things generic, we need to kill const D: bool. I will consider removing it, and make Bool using the Either utility from rayon

With Either, we might be able to generically compute the overrides.

…-configuration

arendjr · 2025-01-07T19:07:23Z

In order to make things generic, we need to kill const D: bool. I will consider removing it, and make Bool using the Either utility from rayon

Can I have a try at it before you do that? I do like the elegance of the current solution, maybe there's another way to get it to work...

arendjr · 2025-01-07T19:20:16Z

crates/biome_configuration/src/max_size.rs

+
+#[derive(Clone, Copy, Eq, Merge, PartialEq, serde::Serialize, serde::Deserialize)]
+#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
+pub struct MazSize(pub NonZeroU64);


Suggested change

pub struct MazSize(pub NonZeroU64);

pub struct MaxSize(pub NonZeroU64);

ematipico · 2025-01-07T21:43:11Z

Sure! I'll merge the PR and then we can follow up :)

refactor(core): configuration, settings and capabilities

a5c6527

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools A-LSP Area: language server protocol L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS labels Jan 6, 2025

arendjr mentioned this pull request Jan 6, 2025

refactor: initial project layout #4846

Merged

ematipico added 4 commits January 6, 2025 21:47

Merge remote-tracking branch 'origin/next' into refactor/settings-and…

cb765e1

…-configuration

merge

58e6191

fix merge

e508d94

rename

fbf6942

fix tests and bindings

6163ed9

ematipico marked this pull request as ready for review January 7, 2025 08:42

ematipico requested review from a team January 7, 2025 08:42

lint

6e376a9

ematipico commented Jan 7, 2025

View reviewed changes

crates/biome_cli/tests/snapshots/main_commands_rage/with_configuration.snap Outdated Show resolved Hide resolved

ematipico added 3 commits January 7, 2025 09:41

use settings default to cal

6fa2358

fix type

8fc117e

clippy

0de9488

arendjr reviewed Jan 7, 2025

View reviewed changes

ematipico and others added 2 commits January 7, 2025 15:17

Update crates/biome_configuration/src/analyzer/linter/mod.rs

a442d98

Co-authored-by: Arend van Beelen jr. <[email protected]>

Apply suggestions from code review

117c959

Co-authored-by: Arend van Beelen jr. <[email protected]>

ematipico and others added 5 commits January 7, 2025 15:25

Apply suggestions from code review

7040799

Co-authored-by: Arend van Beelen jr. <[email protected]>

Update crates/biome_service/src/workspace.rs

1d93398

Co-authored-by: Arend van Beelen jr. <[email protected]>

Update crates/biome_service/src/settings.rs

b1f3bb7

Co-authored-by: Arend van Beelen jr. <[email protected]>

Update crates/biome_service/src/settings.rs

33fab5a

Co-authored-by: Arend van Beelen jr. <[email protected]>

feedback applied

76d74a3

github-actions bot added the A-Core Area: core label Jan 7, 2025

ematipico added 2 commits January 7, 2025 18:12

Merge remote-tracking branch 'origin/next' into refactor/settings-and…

a96b26a

…-configuration

clippy

a54c8a8

arendjr reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): configuration, settings and capabilities #4845

refactor(core): configuration, settings and capabilities #4845

ematipico commented Jan 6, 2025 •

edited

Loading

codspeed-hq bot commented Jan 6, 2025 •

edited

Loading

arendjr left a comment

arendjr Jan 7, 2025

arendjr Jan 7, 2025

arendjr Jan 7, 2025

arendjr Jan 7, 2025

arendjr Jan 7, 2025

arendjr Jan 7, 2025

arendjr Jan 7, 2025

ematipico Jan 7, 2025

arendjr commented Jan 7, 2025

ematipico commented Jan 7, 2025

arendjr commented Jan 7, 2025

ematipico commented Jan 7, 2025 •

edited

Loading

arendjr commented Jan 7, 2025

arendjr Jan 7, 2025

ematipico commented Jan 7, 2025

	pub struct MazSize(pub NonZeroU64);
	pub struct MaxSize(pub NonZeroU64);

refactor(core): configuration, settings and capabilities #4845

Are you sure you want to change the base?

refactor(core): configuration, settings and capabilities #4845

Conversation

ematipico commented Jan 6, 2025 • edited Loading

Summary

Configuration

Settings

Capabilities

Missing things that I will implement later

Test Plan

codspeed-hq bot commented Jan 6, 2025 • edited Loading

Merging #4845 will improve performances by 6.12%

Summary

Benchmarks breakdown

arendjr left a 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

arendjr commented Jan 7, 2025

ematipico commented Jan 7, 2025

arendjr commented Jan 7, 2025

ematipico commented Jan 7, 2025 • edited Loading

arendjr commented Jan 7, 2025

Choose a reason for hiding this comment

ematipico commented Jan 7, 2025

ematipico commented Jan 6, 2025 •

edited

Loading

codspeed-hq bot commented Jan 6, 2025 •

edited

Loading

ematipico commented Jan 7, 2025 •

edited

Loading