Skip to content

feat(linter): Follow-up to config extends fixes #15818

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+72 −6
Open

feat(linter): Follow-up to config extends fixes #15818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/oxc_linter/fixtures/extends_config/rules_multiple/extends_order_test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"extends": [
"./allow_all.json",
"./warn_all.json",
"./deny_all.json"
]
}

49 changes: 48 additions & 1 deletion crates/oxc_linter/src/config/config_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ impl ConfigStoreBuilder {

let (extends, extends_paths) = resolve_oxlintrc_config(extends_oxlintrc)?;

oxlintrc = oxlintrc.merge(&extends);
oxlintrc = oxlintrc.merge(extends);
extended_paths.extend(extends_paths);
}

Expand Down Expand Up @@ -1278,6 +1278,53 @@ mod test {
);
}

#[test]
fn test_extends_order_precedence() {
// Test that when multiple configs are extended, the last file in the extends array
// takes precedence over earlier ones, as documented:
// "The configuration files are merged from the first to the last, with the last file
// overriding the previous ones."
//
// This test uses fixtures that set the same rule to different values:
// - allow_all.json: sets "no-console" to "off"
// - warn_all.json: sets "no-console" to "warn"
// - deny_all.json: sets "no-console" to "error"
Comment on lines +1288 to +1291
Copy link
Member

@overlookmotel overlookmotel Nov 18, 2025

Choose a reason for hiding this comment

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

The text fixture does make sure that the last wins over the first 2. But it'd be ideal to also check all the other combinations (including no clashes). Something like:

// first.json
{
  "rules": { "rule_A": "off", "rule_C": "off", "rule_D": "off", "rule_G": "off" }
}
// second.json
{
  "rules": { "rule_A": "warn", "rule_B": "warn", "rule_D": "warn", "rule_F": "warn" }
}
// third.json
{
  "rules": { "rule_A": "error", "rule_B": "error", "rule_C": "error", "rule_E": "error" }
}
// main.json
{
  "extends": [
    "./first.json",
    "./second.json",
    "./third.json"
  ]
}

Merged config should be equivalent to:

{
  "rules": {
    "rule_A": "error", // `third` wins over both `first` and `second`
    "rule_B": "error", // `third` wins over `second`
    "rule_C": "error", // `third` wins over `first`
    "rule_D": "warn",  // `second` wins over `first`
    "rule_E": "error", // `third` wins when no clash
    "rule_F": "warn",  // `second` wins when no clash
    "rule_G": "off",   // `first` wins when no clash
  },
}

That's rather convoluted, but since we're trying to fix this for once and for all, I think it's worthwhile covering all cases.

//
// When extending [allow_all, warn_all, deny_all], deny_all (last) should win.

let test_oxlintrc = Oxlintrc::from_file(&PathBuf::from(
"fixtures/extends_config/rules_multiple/extends_order_test.json",
))
.unwrap();

let mut external_plugin_store = ExternalPluginStore::default();
let builder = ConfigStoreBuilder::from_oxlintrc(
false, // start_empty = false to get default rules
test_oxlintrc,
None,
&mut external_plugin_store,
)
.unwrap();

let config = builder.build(&external_plugin_store).unwrap();

// The no-console rule should be "error" (from deny_all.json, the last in extends)
// not "off" (from allow_all.json, the first) or "warn" (from warn_all.json, the middle)
let no_console_rule = config.rules().iter().find(|(rule, _)| rule.name() == "no-console");

assert!(
no_console_rule.is_some(),
"no-console rule should be present after extending configs"
);

let (_, severity) = no_console_rule.unwrap();
assert_eq!(
*severity,
AllowWarnDeny::Deny,
"no-console should be 'error' (Deny) from deny_all.json (last in extends), not 'off' or 'warn'"
);
}

fn config_store_from_path(path: &str) -> Config {
let mut external_plugin_store = ExternalPluginStore::default();
ConfigStoreBuilder::from_oxlintrc(
Expand Down
21 changes: 16 additions & 5 deletions crates/oxc_linter/src/config/oxlintrc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,11 +215,22 @@ impl Oxlintrc {
serde_json::to_string_pretty(&schema).unwrap()
}

/// Merges two [Oxlintrc] files together
/// [Self] takes priority over `other`
/// Merges two [Oxlintrc] files together.
///
/// `self` takes priority over `other` - when both configs have the same property,
/// `self`'s value will be used in the merged result.
///
/// # Example
///
/// ```text
/// // base.json: { "rules": { "eqeqeq": "error" } }
/// // current.json: { "rules": { "eqeqeq": "warn", "no-console": "error" } }
/// // After merge: { "rules": { "eqeqeq": "warn", "no-console": "error" } }
/// // ^ current.json's "warn" wins over base.json's "error"
Comment on lines +226 to +229
Copy link
Member

@overlookmotel overlookmotel Nov 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this would clarify which is which:

Suggested change
/// // base.json: { "rules": { "eqeqeq": "error" } }
/// // current.json: { "rules": { "eqeqeq": "warn", "no-console": "error" } }
/// // After merge: { "rules": { "eqeqeq": "warn", "no-console": "error" } }
/// // ^ current.json's "warn" wins over base.json's "error"
/// // self.json: { "rules": { "eqeqeq": "error", "no-debugger": "error" } }
/// // other.json: { "rules": { "eqeqeq": "warn", "no-console": "warn" } }
/// // After merge: { "rules": { "eqeqeq": "error", "no-debugger": "error", "no-console": "warn" } }
/// // ^ self.json's "error" for `eqeqeq` wins over other.json's "warn"

/// ```
#[must_use]
pub fn merge(&self, other: &Oxlintrc) -> Oxlintrc {
let mut categories = other.categories.clone();
pub fn merge(&self, other: Oxlintrc) -> Oxlintrc {
Copy link
Member

@overlookmotel overlookmotel Nov 18, 2025

Choose a reason for hiding this comment

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

Shall we rename this method to merge_onto? self.merge_onto(other) I think makes it clearer at the call site that self takes priority.

let mut categories = other.categories;
categories.extend(self.categories.iter());

let rules = self
Expand All @@ -242,7 +253,7 @@ impl Oxlintrc {
let env = self.env.clone();
let globals = self.globals.clone();

let mut overrides = other.overrides.clone();
let mut overrides = other.overrides;
overrides.extend(self.overrides.clone());

let plugins = match (self.plugins, other.plugins) {
Expand Down
Loading