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))