From fc748490e99346230d8208a1fd1c64aabd160382 Mon Sep 17 00:00:00 2001 From: camchenry <1514176+camchenry@users.noreply.github.com> Date: Tue, 4 Mar 2025 02:37:03 +0000 Subject: [PATCH] feat(linter): inherit `rules` via the extended config files (#9308) - part of https://github.com/oxc-project/oxc/issues/9307 This gives the `extends` keyword some functionality: specifically to allow inheriting rules from other configuration files. Rules that are in the extended configuration will be used as the base, and then files in the config doing the extending will override the extended rules. So, you can, for example, specify a base configuration that defines rules that you generally want to be enabled. Then, in each nested configuration file, you could disable rules depending on the directory: ```text .oxlintrc.json // { "rules: { "no-useless-escape": "error" } } package1/ .oxlintrc.json // { "extends": ["../.oxlintrc.json"], "rules": { "no-useless-escape": "off" } } package2/ src // uses ../.oxlintrc.json with `no-useless-escape` enabled ``` As a side effect of this, building a config from a `.oxlintrc.json` file can result in config errors, even if all of the syntax and rules are configured correctly, because an extended configuration file might be incorrect or unparseable. For now, we simply raise an error if this occurs and stop execution, but in the future we could make this more graceful if we wanted and just ignore that file. We will also need to revisit performance here, as I think a global cache config might be necessary so that we never load a file more than once and parse it more than once. --- .../oxlint/fixtures/extends_config/console.js | 1 + .../fixtures/extends_config/empty_config.json | 1 + .../extends_invalid_config.json | 3 + .../extends_config/extends_rules_config.json | 3 + .../extends_config/invalid_config.json | 3 + .../fixtures/extends_config/rules_config.json | 7 + .../rules_multiple/allow_all.json | 9 + .../rules_multiple/deny_all.json | 9 + .../rules_multiple/warn_all.json | 9 + apps/oxlint/src/lint.rs | 50 +++- ...s_rules_config.json console.js@oxlint.snap | 20 ++ crates/oxc_language_server/src/main.rs | 1 + .../oxc_linter/src/config/config_builder.rs | 222 +++++++++++++++++- crates/oxc_linter/src/tester.rs | 1 + 14 files changed, 324 insertions(+), 15 deletions(-) create mode 100644 apps/oxlint/fixtures/extends_config/console.js create mode 100644 apps/oxlint/fixtures/extends_config/empty_config.json create mode 100644 apps/oxlint/fixtures/extends_config/extends_invalid_config.json create mode 100644 apps/oxlint/fixtures/extends_config/extends_rules_config.json create mode 100644 apps/oxlint/fixtures/extends_config/invalid_config.json create mode 100644 apps/oxlint/fixtures/extends_config/rules_config.json create mode 100644 apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json create mode 100644 apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json create mode 100644 apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json create mode 100644 apps/oxlint/src/snapshots/fixtures__extends_config_--config extends_rules_config.json console.js@oxlint.snap diff --git a/apps/oxlint/fixtures/extends_config/console.js b/apps/oxlint/fixtures/extends_config/console.js new file mode 100644 index 0000000000000..14c198cadb60d --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/console.js @@ -0,0 +1 @@ +console.log("test"); diff --git a/apps/oxlint/fixtures/extends_config/empty_config.json b/apps/oxlint/fixtures/extends_config/empty_config.json new file mode 100644 index 0000000000000..0967ef424bce6 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/empty_config.json @@ -0,0 +1 @@ +{} diff --git a/apps/oxlint/fixtures/extends_config/extends_invalid_config.json b/apps/oxlint/fixtures/extends_config/extends_invalid_config.json new file mode 100644 index 0000000000000..54e2f6da0c630 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/extends_invalid_config.json @@ -0,0 +1,3 @@ +{ + "extends": ["./invalid_config.json"] +} diff --git a/apps/oxlint/fixtures/extends_config/extends_rules_config.json b/apps/oxlint/fixtures/extends_config/extends_rules_config.json new file mode 100644 index 0000000000000..c0d39c617ccd5 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/extends_rules_config.json @@ -0,0 +1,3 @@ +{ + "extends": ["./rules_config.json"] +} diff --git a/apps/oxlint/fixtures/extends_config/invalid_config.json b/apps/oxlint/fixtures/extends_config/invalid_config.json new file mode 100644 index 0000000000000..c39e892ecc601 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/invalid_config.json @@ -0,0 +1,3 @@ +{ + invalid +} diff --git a/apps/oxlint/fixtures/extends_config/rules_config.json b/apps/oxlint/fixtures/extends_config/rules_config.json new file mode 100644 index 0000000000000..b9e3db9d4e252 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/rules_config.json @@ -0,0 +1,7 @@ +{ + "rules": { + "no-debugger": "off", + "no-console": "error", + "unicorn/no-null": "error" + } +} diff --git a/apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json b/apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json new file mode 100644 index 0000000000000..a8e0ac59da760 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json @@ -0,0 +1,9 @@ +{ + "rules": { + "no-console": "off", + "no-var": "off", + "oxc/approx-constant": "off", + "unicorn/no-null": "off", + "unicorn/no-array-for-each": "off" + } +} diff --git a/apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json b/apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json new file mode 100644 index 0000000000000..45ab733b9a0e9 --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json @@ -0,0 +1,9 @@ +{ + "rules": { + "no-console": "error", + "no-var": "error", + "oxc/approx-constant": "error", + "unicorn/no-null": "error", + "unicorn/no-array-for-each": "error" + } +} diff --git a/apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json b/apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json new file mode 100644 index 0000000000000..60c544e273a9e --- /dev/null +++ b/apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json @@ -0,0 +1,9 @@ +{ + "rules": { + "no-console": "warn", + "no-var": "warn", + "oxc/approx-constant": "warn", + "unicorn/no-null": "warn", + "unicorn/no-array-for-each": "warn" + } +} diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 866506833a615..9c7547e8f2727 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -7,7 +7,7 @@ use std::{ use cow_utils::CowUtils; use ignore::{gitignore::Gitignore, overrides::OverrideBuilder}; -use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler}; +use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler, OxcDiagnostic}; use oxc_linter::{ AllowWarnDeny, ConfigStore, ConfigStoreBuilder, InvalidFilterKind, LintFilter, LintOptions, LintService, LintServiceOptions, Linter, Oxlintrc, loader::LINT_PARTIAL_LOADER_EXT, @@ -200,9 +200,28 @@ impl Runner for LintRunner { // iterate over each config and build the ConfigStore for (dir, oxlintrc) in nested_oxlintrc { + // TODO(refactor): clean up all of the error handling in this function + let builder = match ConfigStoreBuilder::from_oxlintrc(false, oxlintrc) { + Ok(builder) => builder, + Err(e) => { + let handler = GraphicalReportHandler::new(); + let mut err = String::new(); + handler + .render_report(&mut err, &OxcDiagnostic::error(e.to_string())) + .unwrap(); + stdout + .write_all( + format!("Failed to parse configuration file.\n{err}\n").as_bytes(), + ) + .or_else(Self::check_for_writer_error) + .unwrap(); + stdout.flush().unwrap(); + + return CliRunResult::InvalidOptionConfig; + } + } // TODO(perf): figure out if we can avoid cloning `filter` - let builder = - ConfigStoreBuilder::from_oxlintrc(false, oxlintrc).with_filters(filter.clone()); + .with_filters(filter.clone()); match builder.build() { Ok(config) => nested_configs.insert(dir.to_path_buf(), config), Err(diagnostic) => { @@ -230,8 +249,22 @@ impl Runner for LintRunner { } else { None }; - let config_builder = - ConfigStoreBuilder::from_oxlintrc(false, oxlintrc).with_filters(filter); + let config_builder = match ConfigStoreBuilder::from_oxlintrc(false, oxlintrc) { + Ok(builder) => builder, + Err(e) => { + let handler = GraphicalReportHandler::new(); + let mut err = String::new(); + handler.render_report(&mut err, &OxcDiagnostic::error(e.to_string())).unwrap(); + stdout + .write_all(format!("Failed to parse configuration file.\n{err}\n").as_bytes()) + .or_else(Self::check_for_writer_error) + .unwrap(); + stdout.flush().unwrap(); + + return CliRunResult::InvalidOptionConfig; + } + } + .with_filters(filter); if let Some(basic_config_file) = oxlintrc_for_print { let config_file = config_builder.resolve_final_config_file(basic_config_file); @@ -982,4 +1015,11 @@ mod test { ]; Tester::new().with_cwd("fixtures/nested_config".into()).test_and_snapshot(args); } + + #[test] + fn test_extends_explicit_config() { + // Check that referencing a config file that extends other config files works as expected + let args = &["--config", "extends_rules_config.json", "console.js"]; + Tester::new().with_cwd("fixtures/extends_config".into()).test_and_snapshot(args); + } } diff --git a/apps/oxlint/src/snapshots/fixtures__extends_config_--config extends_rules_config.json console.js@oxlint.snap b/apps/oxlint/src/snapshots/fixtures__extends_config_--config extends_rules_config.json console.js@oxlint.snap new file mode 100644 index 0000000000000..9b9e35119c848 --- /dev/null +++ b/apps/oxlint/src/snapshots/fixtures__extends_config_--config extends_rules_config.json console.js@oxlint.snap @@ -0,0 +1,20 @@ +--- +source: apps/oxlint/src/tester.rs +--- +########## +arguments: --config extends_rules_config.json console.js +working directory: fixtures/extends_config +---------- + + x ]8;;https://oxc.rs/docs/guide/usage/linter/rules/eslint/no-console.html\eslint(no-console)]8;;\: eslint(no-console): Unexpected console statement. + ,-[console.js:1:1] + 1 | console.log("test"); + : ^^^^^^^^^^^ + `---- + help: Delete this console statement. + +Found 0 warnings and 1 error. +Finished in ms on 1 file with 101 rules using 1 threads. +---------- +CLI result: LintFoundErrors +---------- diff --git a/crates/oxc_language_server/src/main.rs b/crates/oxc_language_server/src/main.rs index 53670b87a8b63..d902d4faa60b8 100644 --- a/crates/oxc_language_server/src/main.rs +++ b/crates/oxc_language_server/src/main.rs @@ -497,6 +497,7 @@ impl Backend { let config = Oxlintrc::from_file(&config_path) .expect("should have initialized linter with new options"); let config_store = ConfigStoreBuilder::from_oxlintrc(true, config.clone()) + .expect("failed to build config") .build() .expect("failed to build config"); *linter = ServerLinter::new_with_linter( diff --git a/crates/oxc_linter/src/config/config_builder.rs b/crates/oxc_linter/src/config/config_builder.rs index c691becd91e33..7a3b92d4a7344 100644 --- a/crates/oxc_linter/src/config/config_builder.rs +++ b/crates/oxc_linter/src/config/config_builder.rs @@ -78,11 +78,13 @@ impl ConfigStoreBuilder { /// /// # Errors /// - /// Will return a [`ConfigBuilderError::UnknownRules`] if there are unknown rules in the - /// config. This can happen if the plugin for a rule is not enabled, or the rule name doesn't - /// match any recognized rules. - pub fn from_oxlintrc(start_empty: bool, oxlintrc: Oxlintrc) -> Self { - // TODO: monorepo config merging, plugin-based extends, etc. + /// Returns [`ConfigBuilderError::InvalidConfigFile`] if a referenced config file is not valid. + pub fn from_oxlintrc( + start_empty: bool, + oxlintrc: Oxlintrc, + ) -> Result { + // TODO(refactor); can we make this function infallible, and move all the error handling to + // the `build` method? let Oxlintrc { plugins, settings, @@ -93,7 +95,7 @@ impl ConfigStoreBuilder { overrides, path, ignore_patterns: _, - extends: _, + extends, } = oxlintrc; let config = LintConfig { plugins, settings, env, globals, path: Some(path) }; @@ -108,10 +110,37 @@ impl ConfigStoreBuilder { { let all_rules = builder.cache.borrow(); + + if !extends.is_empty() { + let config_file_path = builder.config.path.as_ref().and_then(|p| p.parent()); + for path in &extends { + // resolve path relative to config path + let path = match config_file_path { + Some(config_file_path) => &config_file_path.join(path), + None => path, + }; + // TODO: throw an error if this is a self-referential extend + // TODO(perf): use a global config cache to avoid re-parsing the same file multiple times + match Oxlintrc::from_file(path) { + Ok(extended_config) => { + extended_config + .rules + .override_rules(&mut builder.rules, all_rules.as_slice()); + } + Err(err) => { + return Err(ConfigBuilderError::InvalidConfigFile { + file: path.display().to_string(), + reason: err.to_string(), + }); + } + } + } + } + oxlintrc_rules.override_rules(&mut builder.rules, all_rules.as_slice()); } - builder + Ok(builder) } /// Configure what linter plugins are enabled. @@ -295,7 +324,7 @@ impl TryFrom for ConfigStoreBuilder { #[inline] fn try_from(oxlintrc: Oxlintrc) -> Result { - Ok(Self::from_oxlintrc(false, oxlintrc)) + Self::from_oxlintrc(false, oxlintrc) } } @@ -309,10 +338,12 @@ impl fmt::Debug for ConfigStoreBuilder { } /// An error that can occur while building a [`ConfigStore`] from an [`Oxlintrc`]. -#[derive(Debug, Clone)] +#[derive(Eq, PartialEq, Debug, Clone)] pub enum ConfigBuilderError { /// There were unknown rules that could not be matched to any known plugins/rules. UnknownRules { rules: Vec }, + /// A configuration file was referenced which was not valid for some reason. + InvalidConfigFile { file: String, reason: String }, } impl std::fmt::Display for ConfigBuilderError { @@ -325,6 +356,9 @@ impl std::fmt::Display for ConfigBuilderError { } Ok(()) } + ConfigBuilderError::InvalidConfigFile { file, reason } => { + write!(f, "invalid config file {file}: {reason}") + } } } } @@ -421,6 +455,8 @@ impl RulesCache { #[cfg(test)] mod test { + use std::path::PathBuf; + use super::*; #[test] @@ -641,7 +677,7 @@ mod test { "#, ) .unwrap(); - let builder = ConfigStoreBuilder::from_oxlintrc(false, oxlintrc); + let builder = ConfigStoreBuilder::from_oxlintrc(false, oxlintrc).unwrap(); for rule in &builder.rules { let name = rule.name(); let plugin = rule.plugin_name(); @@ -675,4 +711,170 @@ mod test { } } } + + #[test] + fn test_extends_rules_single() { + let base_config = + config_store_from_path("../../apps/oxlint/fixtures/extends_config/rules_config.json"); + let derived_config = config_store_from_str( + r#" + { + "extends": [ + "../../apps/oxlint/fixtures/extends_config/rules_config.json" + ] + } + "#, + ); + + assert_eq!(base_config.rules(), derived_config.rules()); + + let update_rules_config = config_store_from_str( + r#" + { + "extends": [ + "../../apps/oxlint/fixtures/extends_config/rules_config.json" + ], + "rules": { + "no-debugger": "warn", + "no-console": "warn", + "unicorn/no-null": "off", + "typescript/prefer-as-const": "warn" + } + } + "#, + ); + + assert!( + update_rules_config + .rules() + .iter() + .any(|r| r.name() == "no-debugger" && r.severity == AllowWarnDeny::Warn) + ); + assert!( + update_rules_config + .rules() + .iter() + .any(|r| r.name() == "no-console" && r.severity == AllowWarnDeny::Warn) + ); + assert!( + !update_rules_config + .rules() + .iter() + .any(|r| r.name() == "no-null" && r.severity == AllowWarnDeny::Allow) + ); + assert!( + update_rules_config + .rules() + .iter() + .any(|r| r.name() == "prefer-as-const" && r.severity == AllowWarnDeny::Warn) + ); + } + + #[test] + fn test_extends_rules_multiple() { + let warn_all = config_store_from_str( + r#" + { + "extends": [ + "../../apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json" + ] + } + "#, + ); + assert!(warn_all.rules().iter().all(|r| r.severity == AllowWarnDeny::Warn)); + + let deny_all = config_store_from_str( + r#" + { + "extends": [ + "../../apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json" + ] + } + "#, + ); + assert!(deny_all.rules().iter().all(|r| r.severity == AllowWarnDeny::Deny)); + + let allow_all = config_store_from_str( + r#" + { + "extends": [ + "../../apps/oxlint/fixtures/extends_config/rules_multiple/warn_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json" + ] + } + "#, + ); + assert!(allow_all.rules().iter().all(|r| r.severity == AllowWarnDeny::Allow)); + assert_eq!(allow_all.number_of_rules(), 0); + + let allow_and_override_config = config_store_from_str( + r#" + { + "extends": [ + "../../apps/oxlint/fixtures/extends_config/rules_multiple/deny_all.json", + "../../apps/oxlint/fixtures/extends_config/rules_multiple/allow_all.json" + ], + "rules": { + "no-var": "warn", + "oxc/approx-constant": "error", + "unicorn/no-null": "error" + } + } + "#, + ); + assert!( + allow_and_override_config + .rules() + .iter() + .any(|r| r.name() == "no-var" && r.severity == AllowWarnDeny::Warn) + ); + assert!( + allow_and_override_config + .rules() + .iter() + .any(|r| r.name() == "approx-constant" && r.severity == AllowWarnDeny::Deny) + ); + assert!( + allow_and_override_config + .rules() + .iter() + .any(|r| r.name() == "no-null" && r.severity == AllowWarnDeny::Deny) + ); + } + + #[test] + fn test_extends_invalid() { + let invalid_config = ConfigStoreBuilder::from_oxlintrc( + true, + Oxlintrc::from_file(&PathBuf::from( + "../../apps/oxlint/fixtures/extends_config/extends_invalid_config.json", + )) + .unwrap(), + ); + let err = invalid_config.unwrap_err(); + assert!(matches!(err, ConfigBuilderError::InvalidConfigFile { .. })); + if let ConfigBuilderError::InvalidConfigFile { file, reason } = err { + assert!(file.ends_with("invalid_config.json")); + assert!(reason.contains("Failed to parse")); + } + } + + fn config_store_from_path(path: &str) -> ConfigStore { + ConfigStoreBuilder::from_oxlintrc(true, Oxlintrc::from_file(&PathBuf::from(path)).unwrap()) + .unwrap() + .build() + .unwrap() + } + + fn config_store_from_str(s: &str) -> ConfigStore { + ConfigStoreBuilder::from_oxlintrc(true, serde_json::from_str(s).unwrap()) + .unwrap() + .build() + .unwrap() + } } diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 0d98b9591630c..2858a9a497916 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -448,6 +448,7 @@ impl Tester { .as_ref() .map_or_else(ConfigStoreBuilder::empty, |v| { ConfigStoreBuilder::from_oxlintrc(true, Oxlintrc::deserialize(v).unwrap()) + .unwrap() }) .with_plugins(self.plugins) .with_rule(RuleWithSeverity::new(rule, AllowWarnDeny::Warn))