Skip to content

Commit

Permalink
feat(linter)!: support plugins in oxlint config files (#6088)
Browse files Browse the repository at this point in the history
> 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>
  • Loading branch information
DonIsaac committed Oct 10, 2024
1 parent 454874a commit 80266d8
Show file tree
Hide file tree
Showing 16 changed files with 317 additions and 81 deletions.
7 changes: 7 additions & 0 deletions apps/oxlint/fixtures/import/.oxlintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"plugins": ["import"],
"rules": {
"import/no-default-export": "error",
"import/namespace": "allow"
}
}
9 changes: 9 additions & 0 deletions apps/oxlint/fixtures/import/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import * as foo from './foo';
// ^ import/namespace

console.log(foo);

// import/no-default-export
export default function foo() {}


2 changes: 2 additions & 0 deletions apps/oxlint/fixtures/typescript_eslint/eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
{
// NOTE: enabled by default
"plugins": ["@typescript-eslint"],
"rules": {
"no-loss-of-precision": "off",
"@typescript-eslint/no-loss-of-precision": "error",
Expand Down
199 changes: 169 additions & 30 deletions apps/oxlint/src/command/lint.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{path::PathBuf, str::FromStr};

use bpaf::Bpaf;
use oxc_linter::{AllowWarnDeny, FixKind};
use oxc_linter::{AllowWarnDeny, FixKind, LintPlugins};

use super::{
expand_glob,
Expand Down Expand Up @@ -211,64 +211,203 @@ impl FromStr for OutputFormat {

/// Enable Plugins
#[allow(clippy::struct_field_names)]
#[derive(Debug, Clone, Bpaf)]
#[derive(Debug, Default, Clone, Bpaf)]
pub struct EnablePlugins {
/// Disable react plugin, which is turned on by default
#[bpaf(long("disable-react-plugin"), flag(false, true), hide_usage)]
pub react_plugin: bool,
#[bpaf(
long("disable-react-plugin"),
flag(OverrideToggle::Disable, OverrideToggle::NotSet),
hide_usage
)]
pub react_plugin: OverrideToggle,

/// Disable unicorn plugin, which is turned on by default
#[bpaf(long("disable-unicorn-plugin"), flag(false, true), hide_usage)]
pub unicorn_plugin: bool,
#[bpaf(
long("disable-unicorn-plugin"),
flag(OverrideToggle::Disable, OverrideToggle::NotSet),
hide_usage
)]
pub unicorn_plugin: OverrideToggle,

/// Disable oxc unique rules, which is turned on by default
#[bpaf(long("disable-oxc-plugin"), flag(false, true), hide_usage)]
pub oxc_plugin: bool,
#[bpaf(
long("disable-oxc-plugin"),
flag(OverrideToggle::Disable, OverrideToggle::NotSet),
hide_usage
)]
pub oxc_plugin: OverrideToggle,

/// Disable TypeScript plugin, which is turned on by default
#[bpaf(long("disable-typescript-plugin"), flag(false, true), hide_usage)]
pub typescript_plugin: bool,
#[bpaf(
long("disable-typescript-plugin"),
flag(OverrideToggle::Disable, OverrideToggle::NotSet),
hide_usage
)]
pub typescript_plugin: OverrideToggle,

/// Enable the experimental import plugin and detect ESM problems.
/// It is recommended to use along side with the `--tsconfig` option.
#[bpaf(switch, hide_usage)]
pub import_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub import_plugin: OverrideToggle,

/// Enable the experimental jsdoc plugin and detect JSDoc problems
#[bpaf(switch, hide_usage)]
pub jsdoc_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub jsdoc_plugin: OverrideToggle,

/// Enable the Jest plugin and detect test problems
#[bpaf(switch, hide_usage)]
pub jest_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub jest_plugin: OverrideToggle,

/// Enable the Vitest plugin and detect test problems
#[bpaf(switch, hide_usage)]
pub vitest_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub vitest_plugin: OverrideToggle,

/// Enable the JSX-a11y plugin and detect accessibility problems
#[bpaf(switch, hide_usage)]
pub jsx_a11y_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub jsx_a11y_plugin: OverrideToggle,

/// Enable the Next.js plugin and detect Next.js problems
#[bpaf(switch, hide_usage)]
pub nextjs_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub nextjs_plugin: OverrideToggle,

/// Enable the React performance plugin and detect rendering performance problems
#[bpaf(switch, hide_usage)]
pub react_perf_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub react_perf_plugin: OverrideToggle,

/// Enable the promise plugin and detect promise usage problems
#[bpaf(switch, hide_usage)]
pub promise_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub promise_plugin: OverrideToggle,

/// Enable the node plugin and detect node usage problems
#[bpaf(switch, hide_usage)]
pub node_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub node_plugin: OverrideToggle,

/// Enable the security plugin and detect security problems
#[bpaf(switch, hide_usage)]
pub security_plugin: bool,
#[bpaf(flag(OverrideToggle::Enable, OverrideToggle::NotSet), hide_usage)]
pub security_plugin: OverrideToggle,
}

/// Enables or disables a boolean option, or leaves it unset.
///
/// We want CLI flags to modify whatever's set in the user's config file, but we don't want them
/// changing default behavior if they're not explicitly passed by the user. This scheme is a bit
/// convoluted, but needed due to architectural constraints imposed by `bpaf`.
#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)]
#[allow(clippy::enum_variant_names)]
pub enum OverrideToggle {
/// Override the option to enabled
Enable,
/// Override the option to disabled
Disable,
/// Do not override.
#[default]
NotSet,
}

impl From<Option<bool>> for OverrideToggle {
fn from(value: Option<bool>) -> Self {
match value {
Some(true) => Self::Enable,
Some(false) => Self::Disable,
None => Self::NotSet,
}
}
}

impl From<OverrideToggle> for Option<bool> {
fn from(value: OverrideToggle) -> Self {
match value {
OverrideToggle::Enable => Some(true),
OverrideToggle::Disable => Some(false),
OverrideToggle::NotSet => None,
}
}
}

impl OverrideToggle {
#[inline]
pub fn is_enabled(self) -> bool {
matches!(self, Self::Enable)
}

#[inline]
pub fn is_not_set(self) -> bool {
matches!(self, Self::NotSet)
}

pub fn inspect<F>(self, f: F)
where
F: FnOnce(bool),
{
if let Some(v) = self.into() {
f(v);
}
}
}

impl EnablePlugins {
pub fn apply_overrides(&self, plugins: &mut LintPlugins) {
self.react_plugin.inspect(|yes| plugins.set(LintPlugins::REACT, yes));
self.unicorn_plugin.inspect(|yes| plugins.set(LintPlugins::UNICORN, yes));
self.oxc_plugin.inspect(|yes| plugins.set(LintPlugins::OXC, yes));
self.typescript_plugin.inspect(|yes| plugins.set(LintPlugins::TYPESCRIPT, yes));
self.import_plugin.inspect(|yes| plugins.set(LintPlugins::IMPORT, yes));
self.jsdoc_plugin.inspect(|yes| plugins.set(LintPlugins::JSDOC, yes));
self.jest_plugin.inspect(|yes| plugins.set(LintPlugins::JEST, yes));
self.vitest_plugin.inspect(|yes| plugins.set(LintPlugins::VITEST, yes));
self.jsx_a11y_plugin.inspect(|yes| plugins.set(LintPlugins::JSX_A11Y, yes));
self.nextjs_plugin.inspect(|yes| plugins.set(LintPlugins::NEXTJS, yes));
self.react_perf_plugin.inspect(|yes| plugins.set(LintPlugins::REACT_PERF, yes));
self.promise_plugin.inspect(|yes| plugins.set(LintPlugins::PROMISE, yes));
self.node_plugin.inspect(|yes| plugins.set(LintPlugins::NODE, yes));
self.security_plugin.inspect(|yes| plugins.set(LintPlugins::SECURITY, yes));

// Without this, jest plugins adapted to vitest will not be enabled.
if self.vitest_plugin.is_enabled() && self.jest_plugin.is_not_set() {
plugins.set(LintPlugins::JEST, true);
}
}
}

#[cfg(test)]
mod plugins {
use super::{EnablePlugins, OverrideToggle};
use oxc_linter::LintPlugins;

#[test]
fn test_override_default() {
let mut plugins = LintPlugins::default();
let enable = EnablePlugins::default();

enable.apply_overrides(&mut plugins);
assert_eq!(plugins, LintPlugins::default());
}

#[test]
fn test_overrides() {
let mut plugins = LintPlugins::default();
let enable = EnablePlugins {
react_plugin: OverrideToggle::Enable,
unicorn_plugin: OverrideToggle::Disable,
..EnablePlugins::default()
};
let expected =
LintPlugins::default().union(LintPlugins::REACT).difference(LintPlugins::UNICORN);

enable.apply_overrides(&mut plugins);
assert_eq!(plugins, expected);
}

#[test]
fn test_override_vitest() {
let mut plugins = LintPlugins::default();
let enable =
EnablePlugins { vitest_plugin: OverrideToggle::Enable, ..EnablePlugins::default() };
let expected = LintPlugins::default() | LintPlugins::VITEST | LintPlugins::JEST;

enable.apply_overrides(&mut plugins);
assert_eq!(plugins, expected);
}
}

#[cfg(test)]
Expand Down
64 changes: 33 additions & 31 deletions apps/oxlint/src/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use ignore::gitignore::Gitignore;
use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler};
use oxc_linter::{
loader::LINT_PARTIAL_LOADER_EXT, AllowWarnDeny, InvalidFilterKind, LintFilter, LintService,
LintServiceOptions, Linter, OxlintOptions,
LintServiceOptions, Linter, LinterBuilder, Oxlintrc,
};
use oxc_span::VALID_EXTENSIONS;

Expand Down Expand Up @@ -98,39 +98,32 @@ impl Runner for LintRunner {
let number_of_files = paths.len();

let cwd = std::env::current_dir().unwrap();
let mut options =
LintServiceOptions::new(cwd, paths).with_cross_module(enable_plugins.import_plugin);
let lint_options = OxlintOptions::default()
.with_filter(filter)
.with_config_path(basic_options.config)
.with_fix(fix_options.fix_kind())
.with_react_plugin(enable_plugins.react_plugin)
.with_unicorn_plugin(enable_plugins.unicorn_plugin)
.with_typescript_plugin(enable_plugins.typescript_plugin)
.with_oxc_plugin(enable_plugins.oxc_plugin)
.with_import_plugin(enable_plugins.import_plugin)
.with_jsdoc_plugin(enable_plugins.jsdoc_plugin)
.with_jest_plugin(enable_plugins.jest_plugin)
.with_vitest_plugin(enable_plugins.vitest_plugin)
.with_jsx_a11y_plugin(enable_plugins.jsx_a11y_plugin)
.with_nextjs_plugin(enable_plugins.nextjs_plugin)
.with_react_perf_plugin(enable_plugins.react_perf_plugin)
.with_promise_plugin(enable_plugins.promise_plugin)
.with_node_plugin(enable_plugins.node_plugin)
.with_security_plugin(enable_plugins.security_plugin);

let linter = match Linter::from_options(lint_options) {
Ok(lint_service) => lint_service,
Err(diagnostic) => {
let handler = GraphicalReportHandler::new();
let mut err = String::new();
handler.render_report(&mut err, diagnostic.as_ref()).unwrap();
return CliRunResult::InvalidOptions {
message: format!("Failed to parse configuration file.\n{err}"),
};

let mut oxlintrc = if let Some(config_path) = basic_options.config.as_ref() {
match Oxlintrc::from_file(config_path) {
Ok(config) => config,
Err(diagnostic) => {
let handler = GraphicalReportHandler::new();
let mut err = String::new();
handler.render_report(&mut err, &diagnostic).unwrap();
return CliRunResult::InvalidOptions {
message: format!("Failed to parse configuration file.\n{err}"),
};
}
}
} else {
Oxlintrc::default()
};

enable_plugins.apply_overrides(&mut oxlintrc.plugins);
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc)
.with_filters(filter)
.with_fix(fix_options.fix_kind());

let mut options =
LintServiceOptions::new(cwd, paths).with_cross_module(builder.plugins().has_import());
let linter = builder.build();

let tsconfig = basic_options.tsconfig;
if let Some(path) = tsconfig.as_ref() {
if path.is_file() {
Expand Down Expand Up @@ -562,4 +555,13 @@ mod test {
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_errors, 1);
}

#[test]
fn test_import_plugin_enabled_in_config() {
let args = &["-c", "fixtures/import/.oxlintrc.json", "fixtures/import/test.js"];
let result = test(args);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_warnings, 0);
assert_eq!(result.number_of_errors, 1);
}
}
1 change: 1 addition & 0 deletions crates/oxc_linter/src/config/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize};
/// environments](https://eslint.org/docs/v8.x/use/configure/language-options#specifying-environments)
/// for what environments are available and what each one provides.
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
#[cfg_attr(test, derive(PartialEq))]
pub struct OxlintEnv(FxHashMap<String, bool>);

impl OxlintEnv {
Expand Down
Loading

0 comments on commit 80266d8

Please sign in to comment.