Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linter)!: support plugins in oxlint config files #6088

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Sep 26, 2024

Closes #5046
This PR migrates the linter crate and oxlint app to use the new LinterBuilder API. This PR has the following effects:

  1. plugins in oxlint config files are now supported
  2. irons out weirdness when using CLI args and config files together. CLI args are now always applied on top of config file settings, overriding them.

Breaking Changes

Before, config files would override rules set in CLI arguments. For example, running this command:

oxlint -A correctness -c oxlintrc.json

With this config file

// oxlintrc.json
{
  "rules": {
    "no-const-assign": "error"
  }
}

Would result in a single rule, no-const-assign being turned on at an error level with all other rules disabled (i.e. set to "allow").

Now, CLI arguments will override config files. That same command with the same config file will result with all rules being disabled.

Details

For a more in-depth explanation, assume we are running the below command using the oxlintrc.json file above:

oxlint -A all -W correctness -A all -W suspicious -c oxlintrc.json

Before

Note: GitHub doesn't seem to like deeply nested lists. Apologies for the formatting.

Here was the config resolution process before this PR:

Before Steps
  1. Start with a default set of filters (["correctness", "warn"]) if no filters were passed to the CLI. Since some were, the filter list starts out empty.
  2. Apply each filter taken from the CLI from left to right. When a filter allows a rule or category, it clears the configured set of rules. So applying those filters looks like this
    a. start with an empty list []
    b. ("all", "allow") -> []
    c. ("correctness", "warn") -> [ <correctness rules> ]
    d. ("all", "allow") -> []
    e. ("suspicious", "warn") -> [ <suspicious rules> ]. This is the final rule set for this step
  3. Apply overrides from oxlintrc.json. This is where things get a little funky, as mentioned in point 2 of what this PR does. At this point, all rules in the rules list are only from the CLI.
    a. If a rule is only set in the CLI and is not present in the config file, there's no effect
    b. If a rule is in the config file but not the CLI, it gets inserted into the list.
    c. If a rule is already in the list and in the config file
    i. If the rule is only present once (e.g. "no-loss-of-precision": "error"), unconditionally override whatever was in the CLI with what was set in the config file
    ii. If the rule is present twice (e.g. "no-loss-of-precision": "off", "@typescript-eslint/no-loss-of-precision": "error"),
    a. if all rules in the config file are set to allow, then turn the rule off
    b. If one of them is warn or deny, then update the currently-set rule's config object, but leave its severity alone.

So, for our example, the final rule set would be [<all suspicious rules: "warn">, no-const-assign: "error"]

After

Rule filters were completely re-worked in a previous PR. Now, lint filters aren't kept on hand-only the rule list is.

After Steps
  1. Start with the default rule set, which is all correctness rules for all enabled plugins ([<all correctness rules: "warn">])
  2. Apply configuration from oxlintrc.json first.
    a. If the rule is warn/deny exists in the list already, update its severity and config object. If it's not in the list, add it.
    b. If the rule is set to allow, remove it from the list.

The list is now [<all correctness rules except no-const-assign: "warn">, no-const-assign: "error"].

  1. Apply each filter taken from the CLI from left to right. This works the same way as before. So, after they're applied, the list is now [<suspicous rules: "warn">]

Copy link

graphite-app bot commented Sep 26, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link

codspeed-hq bot commented Sep 26, 2024

CodSpeed Performance Report

Merging #6088 will degrade performances by 3.36%

Comparing don/09-26-feat_linter_support_plugins_in_oxlint_config_files (80266d8) with main (454874a)

Summary

❌ 1 regressions
✅ 28 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main don/09-26-feat_linter_support_plugins_in_oxlint_config_files Change
linter[checker.ts] 2.4 s 2.5 s -3.36%

@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from d08cf2c to 51b398d Compare September 26, 2024 22:19
@DonIsaac
Copy link
Contributor Author

CodSpeed Performance Report

Merging #6088 will degrade performances by 3.72%

Comparing don/09-26-feat_linter_support_plugins_in_oxlint_config_files (d08cf2c) with don/09-26-fix_linter_rule_and_generic_filters_do_not_re-configure_existing_rules (6edcd5c)

Summary

❌ 1 regressions ✅ 28 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark don/09-26-fix_linter_rule_and_generic_filters_do_not_re-configure_existing_rules don/09-26-feat_linter_support_plugins_in_oxlint_config_files Change
linter[checker.ts] 2.4 s 2.5 s -3.72%

It looks like some rules are not running that weren't before. This either means this PR fixed a bug that caused them not to run, or ...run() is now taking longer, so those spans are showing up?

TBH I'm not quite sure what to make of this.

@DonIsaac DonIsaac force-pushed the don/09-26-fix_linter_rule_and_generic_filters_do_not_re-configure_existing_rules branch 2 times, most recently from b1a12f2 to 29c00c5 Compare September 27, 2024 02:28
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from 51b398d to 27c99ab Compare September 27, 2024 02:28
@DonIsaac DonIsaac changed the base branch from don/09-26-fix_linter_rule_and_generic_filters_do_not_re-configure_existing_rules to graphite-base/6088 September 27, 2024 03:39
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from 27c99ab to 8592ba8 Compare September 27, 2024 03:43
@DonIsaac DonIsaac changed the base branch from graphite-base/6088 to main September 27, 2024 03:44
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from 8592ba8 to d5a088e Compare September 27, 2024 03:44
apps/oxlint/src/command/lint.rs Outdated Show resolved Hide resolved
apps/oxlint/src/command/lint.rs Show resolved Hide resolved
apps/oxlint/src/command/lint.rs Show resolved Hide resolved
crates/oxc_linter/src/config/oxlintrc.rs Outdated Show resolved Hide resolved
crates/oxc_linter/src/options/plugins.rs Outdated Show resolved Hide resolved
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from d5a088e to b197bff Compare September 27, 2024 03:54
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from b197bff to 2be7add Compare September 27, 2024 03:57
Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest manually test this setup on some of the oxlint-ecosystem-ci projects and see if the rule numbers match.

@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch 2 times, most recently from dd0946f to 8ea51af Compare September 27, 2024 14:56
@DonIsaac DonIsaac changed the title feat(linter): support plugins in oxlint config files feat(linter)!: support plugins in oxlint config files Sep 27, 2024
@DonIsaac
Copy link
Contributor Author

We'll need to allow for category-based rule configuration in oxlintrc.json files before our next release in order for this PR to be viable. Otherwise there's no way to get that broad-level configuration, followed by rule-by-rule tweaking, that people were getting before using the CLI. Working on a PR now.

DonIsaac added a commit that referenced this pull request Sep 27, 2024
> closes #5454

Adds a `categories` property to config files, where each key is a `RuleCategory` and each value is `"allow"/"off"`, `"warn"`, or `"deny"/"error"`.

Note that this change won't come into effect until after #6088 is merged.
DonIsaac added a commit that referenced this pull request Sep 27, 2024
> closes #5454

Adds a `categories` property to config files, where each key is a `RuleCategory` and each value is `"allow"/"off"`, `"warn"`, or `"deny"/"error"`.

Note that this change won't come into effect until after #6088 is merged.
@DonIsaac DonIsaac requested a review from camchenry September 27, 2024 17:34
@DonIsaac
Copy link
Contributor Author

Waiting on #6120 to land before merging this.

DonIsaac added a commit that referenced this pull request Oct 8, 2024
> closes #5454

Adds a `categories` property to config files, where each key is a `RuleCategory` and each value is `"allow"/"off"`, `"warn"`, or `"deny"/"error"`.

Note that this change won't come into effect until after #6088 is merged.
@DonIsaac DonIsaac linked an issue Oct 10, 2024 that may be closed by this pull request
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from 8ea51af to ba4dd95 Compare October 10, 2024 19:15
@DonIsaac DonIsaac added the 0-merge Merge with Graphite Merge Queue label Oct 10, 2024
Copy link
Contributor Author

DonIsaac commented Oct 10, 2024

Merge activity

  • Oct 10, 3:16 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Oct 10, 3:21 PM EDT: A user added this pull request to the Graphite merge queue.
  • Oct 10, 3:26 PM EDT: A user merged this pull request with the Graphite merge queue.

> Closes #5046
This PR migrates the linter crate and oxlint app to use the new `LinterBuilder` API. This PR has the following effects:

1. `plugins` in oxlint config files are now supported
2. irons out weirdness when using CLI args and config files together. CLI args are now always applied on top of config file settings, overriding them.

# Breaking Changes
Before, config files would override rules set in CLI arguments. For example, running this command:
```sh
oxlint -A correctness -c oxlintrc.json
```
With this config file
```jsonc
// oxlintrc.json
{
  "rules": {
    "no-const-assign": "error"
  }
}
```
Would result in a single rule, `no-const-assign` being turned on at an error level with all other rules disabled (i.e. set to "allow").

Now, **CLI arguments will override config files**. That same command with the same config file will result with **all rules being disabled**.

## Details

For a more in-depth explanation, assume we are running the below command using the `oxlintrc.json` file above:
```sh
oxlint -A all -W correctness -A all -W suspicious -c oxlintrc.json
```

### Before
> Note: GitHub doesn't seem to like deeply nested lists. Apologies for the formatting.

Here was the config resolution process _before_ this PR:
<details><summary>Before Steps</summary>

1. Start with a default set of filters (["correctness", "warn"]) if no filters were passed to the CLI. Since some were, the filter list starts out empty.
2. Apply each filter taken from the CLI from left to right. When a filter allows a rule or category, it clears the configured set of rules. So applying those filters looks like this
  a. start with an empty list `[]`
  b. `("all", "allow")` -> `[]`
  c. `("correctness", "warn")` -> `[ <correctness rules> ]`
  d. `("all", "allow")` -> `[]`
  e. `("suspicious", "warn")` -> `[ <suspicious rules> ]`. This is the final rule set for this step
3. Apply overrides from `oxlintrc.json`. This is where things get a little funky, as mentioned in point 2 of what this PR does. At this point, all rules in the rules list are only from the CLI.
  a. If a rule is only set in the CLI and is not present in the config file, there's no effect
  b. If a rule is in the config file but not the CLI, it gets inserted into the list.
  c. If a rule is already in the list and in the config file
    i. If the rule is only present once (e.g. `"no-loss-of-precision": "error"`), unconditionally override whatever was in the CLI with what was set in the config file
    ii. If the rule is present twice (e.g. `"no-loss-of-precision": "off", "@typescript-eslint/no-loss-of-precision": "error"`),
      a. if all rules in the config file are set to `allow`, then turn the rule off
      b. If one of them is `warn` or `deny`, then update the currently-set rule's config object, but _leave its severity alone_.

  So, for our example, the final rule set would be `[<all suspicious rules: "warn">, no-const-assign: "error"]`

</details>

### After
Rule filters were completely re-worked in a previous PR. Now, lint filters aren't kept on hand-only the rule list is.

<details><summary>After Steps</summary>

1. Start with the default rule set, which is all correctness rules for all enabled plugins (`[<all correctness rules: "warn">]`)
2. Apply configuration from `oxlintrc.json` _first_.
  a. If the rule is warn/deny exists in the list already, update its severity and config object. If it's not in the list, add it.
  b. If the rule is set to allow, remove it from the list.

  The list is now `[<all correctness rules except no-const-assign: "warn">, no-const-assign: "error"]`.

3. Apply each filter taken from the CLI from left to right. This works the same way as before. So, after they're applied, the list is now `[<suspicous rules: "warn">]`

</details>
@DonIsaac DonIsaac force-pushed the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch from ba4dd95 to 80266d8 Compare October 10, 2024 19:21
@graphite-app graphite-app bot merged commit 80266d8 into main Oct 10, 2024
26 checks passed
@graphite-app graphite-app bot deleted the don/09-26-feat_linter_support_plugins_in_oxlint_config_files branch October 10, 2024 19:26
DonIsaac added a commit that referenced this pull request Oct 19, 2024
## [0.10.0] - 2024-10-18

- 7f6b219 editor/vscode: [**BREAKING**] Unify configuration logic
(#6630) (DonIsaac)

- 782f0a7 codegen: [**BREAKING**] Rename `print_char` method to
`print_ascii_byte` (#6512) (overlookmotel)

- 7645e5c codegen: [**BREAKING**] Remove CommentOptions API (#6451)
(Boshen)

- 5200960 oxc: [**BREAKING**] Remove passing `Trivias` around (#6446)
(Boshen)

- 80266d8 linter: [**BREAKING**] Support plugins in oxlint config files
(#6088) (DonIsaac)

### Features

- 6f22538 ecmascript: Add `ToBoolean`, `ToNumber`, `ToString` (#6502)
(Boshen)
- 1e7fab3 linter: Implement `no-callback-in-promise` (#6157) (dalaoshu)
- c56343d linter: Promote `no_unsafe_optional_chaining` to correctness
(#6491) (Boshen)
- 454874a linter: Implement `react/iframe-missing-sandbox` (#6383) (Radu
Baston)
- c8174e2 linter: Add suggestions for `no-plusplus` (#6376) (camchenry)
- 6e3224d linter: Configure by category in config files (#6120)
(DonIsaac)
- c5e66e1 linter/no-unused-vars: Report own type references within
class, interface, and type alias declarations (#6557) (DonIsaac)
- 8c78f97 linter/node: Implement no-new-require (#6165) (Jelle van der
Waa)

### Bug Fixes

- cf92730 editor: Use human-readable output channel names (#6629)
(DonIsaac)
- d9159a2 editor: Misaligned command prefixes (#6628) (DonIsaac)
- b9c94bb editors/vscode: Temporarily solve oxc_language_server issue on
windows (#6384) (dalaoshu)
- e340424 linter: Support import type with namespaced import in
`import/no-duplicates` (#6650) (Dmitry Zakharov)
- a668397 linter: Panic in `no-else-return` (#6648) (dalaoshu)
- 41dc8e3 linter: Stack overflow in `oxc/no-async-endpoint-handlers`
(#6614) (DonIsaac)
- d07a9b0 linter: Panic in `no-zero-fractions` (#6607) (dalaoshu)
- d6a0d2e linter: Fix file name checking behavior of
`unicorn/filename-case` (#6463) (camchenry)
- 0784e74 linter: Error fixer of `switch-case-braces` (#6474) (dalaoshu)
- e811812 linter: Error diagnostic message based on parameter length of
valid-expect (#6455) (dalaoshu)
- f71c91e linter: Move `eslint/sort-keys` to `style` category (#6377)
(DonIsaac)
- 2b86de9 linter/no-control-regex: False negative for flags in template
literals (#6531) (DonIsaac)
- 685a590 linter/no-control-regex: Better diagnostic messages (#6530)
(DonIsaac)
- 6d5a9f2 linter/no-control-regex: Allow capture group references
(#6529) (DonIsaac)
- ba53bc9 linter/no-unused-vars: False positives in TS type assertions
(#6397) (DonIsaac)
- d3e59c6 linter/no-unused-vars: False positive in some default export
cases (#6395) (DonIsaac)
- e08f956 linter/no-unused-vars: False positive for functions and
classes in arrays (#6394) (DonIsaac)
- b9d7c5f no-unused-vars: Consider functions within conditional
expressions usable (#6553) (Brian Donovan)

### Performance

- 0cbd4d0 linter: Avoid megamorphism in `RuleFixer` methods (#6606)
(DonIsaac)
- 725f9f6 linter: Get fewer parent nodes in
`unicorn/prefer-dom-node-text-content` (#6467) (camchenry)
- c00f669 linter: Use NonZeroUsize for pending module cache entries
(#6439) (DonIsaac)
- a1a2721 linter: Replace `ToString::to_string` with `CompactStr` in
remaining rules (#6407) (camchenry)
- c5c69d6 linter: Use `CompactStr` in `valid-title` (#6406) (camchenry)
- d66e826 linter: Use `CompactStr` in `prefer-lowercase-title` (#6405)
(camchenry)
- 889400c linter: Use `CompactStr` for `get_node_name` in Jest rules
(#6403) (camchenry)
- 9906849 linter: Use `CompactStr` in `no-large-snapshots` (#6402)
(camchenry)
- c382ec4 linter: Use `CompactStr` in `no-hooks` (#6401) (camchenry)
- 24a5d9b linter: Use `CompactStr` in `expect-expect` (#6400)
(camchenry)
- 71dbdad linter: Use `CompactStr` in `no-console` (#6399) (camchenry)
- f5f00a1 linter: Use `CompactStr` in `no-bitwise` (#6398) (camchenry)
- 62afaa9 linter/jsx-no-comment-textnodes: Remove regex for checking
comment patterns (#6534) (camchenry)
- b3d0cce linter/no-unescaped-entities: Add fast path to check if char
should be replaced (#6594) (camchenry)
- ee73f56 linter/no-unused-vars: Do not construct `Regex` for default
ignore pattern (#6590) (camchenry)
- 77ddab8 linter/numeric-separators-style: Replace regex with number
parser (#6546) (camchenry)
- 8f47cd0 linter/react: Remove regex patterns in `no-unknown-property`
(#6536) (camchenry)

### Documentation

- 557f941 linter: Add docs to no-unused-vars and Tester (#6558)
(DonIsaac)

### Refactor

- ecce5c5 linter: Improve recursive argument handling and diagnostics
creation (#6513) (no-yan)
- f960e9e linter: Add suggested file names for `unicorn/filename-case`
(#6465) (camchenry)
- 7240ee2 linter: Make advertised fix kinds consistent (#6461)
(Alexander S.)
- b48c368 linter: `no_global_assign` rule: reduce name lookups (#6460)
(overlookmotel)
- 2566ce7 linter: Remove OxlintOptions (#6098) (DonIsaac)
- 002078a linter: Make Runtime's members private (#6440) (DonIsaac)
- 6a0a533 linter: Move module cache logic out of Runtime (#6438)
(DonIsaac)
- c18c6e9 linter: Split service code into separate modules (#6437)
(DonIsaac)
- 5ea9ef7 linter: Improve labels and help message for
`eslint/no-useless-constructor` (#6389) (DonIsaac)
- 2c32dac linter/no-control-regex: Remove duplicate code (#6527)
(DonIsaac)
- 435a89c oxc: Remove useless `allocator.alloc(program)` calls (#6571)
(Boshen)
- f70e93b oxc: Ban index methods on std::str::Chars (#6075) (dalaoshu)

### Testing

- a6cae98 linter: Make sure all auto-fixing rules have fixer test
(#6378) (DonIsaac)
- 06b09b2 linter/no-unused-vars: Enable now-passing tests (#6556)
(DonIsaac)
- badd11c linter/no-unused-vars: Ignored catch parameters (#6555)
(DonIsaac)
- 84aa2a2 linter/no-useless-constructor: Add cases for initializers in
subclass constructors (#6390) (DonIsaac)

---------

Co-authored-by: DonIsaac <22823424+DonIsaac@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-cli Area - CLI A-linter Area - Linter
Projects
None yet
3 participants