feat(editorconfig): reenable editorconfig support, add cli flag to disable it

[dyc3]

Summary

This PR reenables editorconfig support, and additionally adds a cli flag, --use-editorconfig, to control whether or not it's enabled.

related to: #1724

• blocked by: test(parse/json): add test for bug where overrides erroneously override special parsing options #3260

Test Plan

I've reenabled the unit tests that I wrote for this.

[ematipico]

In our repository, we have command called pnpm check that runs the Biome CLI in dev mode. If you run, you'll notice that it will break:

configuration ━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Couldn't parse the **.yml, reason: recursive wildcards must form a single path component
  

 ELIFECYCLE  Command failed with exit code 1.

We should fix that

[ematipico]

After fixing the issue with editor config **.yml -> *.yml (we should make the error message a bit simpler to understand...), I run into overriding issues.. meaning that now tsconfig.json is failing because it isn't parsed as JSONC anymore:

~/packages/@biomejs/js-api/tsconfig.json:3:24 parse ━━━━━━━━━━━━━━━━━━━━━━━

  ✖ JSON standard does not allow comments.
  
    1 │ {
    2 │   "compilerOptions": {
  > 3 │     "target": "es2020" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
      │                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    4 │     "module": "commonjs" /* Specify what module code is generated. */,
    5 │     "declaration": true /* Generate .d.ts files from TypeScript and JavaScript files in your project. */,
  

tsconfig.json, like other files, are "well known", meaning that are always parsed as JSONC files

[dyc3]

I've updated it so that error will now be printed as:

/home/<absolute path>/.editorconfig configuration ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Couldn't parse the pattern "**.yml". Reason: Pattern syntax error near position 2: recursive wildcards must form a single path component

and it'll now convert that pattern into one biome can use. That tsconfig.json thing sounds like it's out of scope for editorconfig support?

[ematipico]

That tsconfig.json thing sounds like it's out of scope for editorconfig support?

But it's a regression I am experiencing in this PR. The command pnpm check doesn't work anymore. Is it expected?

[dyc3]

I think this is a bug on main. Adding the following override to this repo's biome.json produces the same result.

"overrides": [
    {
      "include": ["**/*.json"],
      "formatter": {
        "indentStyle": "space"
      }
    }
  ]

Obviously, we can't merge this without that being resolved. I'll look into it.

[dyc3]

Running cargo run --bin biome -- check packages/@biomejs/backend-jsonrpc/tsconfig.json with the override does not produce the diagnostics. I think the source of the problem has something to do with the caching being done here:

biome/crates/biome_service/src/settings.rs

Lines 1095 to 1100 in cc65fe8

	if let Ok(readonly_cache) = self.cached_json_parser_options.read() {
	if let Some(cached_options) = readonly_cache.as_ref() {
	*options = *cached_options;
	return;
	}
	}

[dyc3]

@ematipico I've submitted a draft PR to fix this in #3260, but I don't really like the solution. Details are in the PR.

[dyc3]

@ematipico This PR should be unblocked now.

I went ahead and disabled the caching for json overrides because the behavior was incorrect, as discussed in #3260, despite the potential for performance regressions. pnpm check does throw a diagnostic that wasn't there before, but its not a regression from main.

[ematipico]

This looks good to me. I am a bit worried about the default, and I would like to know what you think. Once settled on the default value, we can merge it :) Great work @dyc3, thank you for landing this feature

[ematipico]

I am a bit worried about this default. It makes sense to have to true, however this could break some formatting for users that already use Biome and have an .editorconfig somewhere. What do you think? Should we set it false for now, and set it to true for v2?

[dyc3]

That would be the most conservative. On the other hand, prettier enables editorconfig support by default, and keeping this behavior when migrating from prettier would be a better user experience.

I think we should set this to false by default, but when migrating from prettier, set it to true if it's unspecified.

ref: https://prettier.io/docs/en/configuration.html#editorconfig

[dyc3]

actually it turns out that prettier's options.editorconfig doesn't work at all, see: prettier/prettier#15255

So we should always turn it on when migrating

[ematipico]

Yeah, sounds goods me!