Skip to content

Commit 41f92d1

Browse files
committed
fix(linter): Error when given config options for a lint rule that has no config options defined. (#18809)
Enforce that rules with no config struct/enum raise an error when provided with config options. For example, the `no-debugger` rule does not take any configuration options. It would silently allow them, but that was an invalid behavior. ```json { "rules": { "eslint/no-debugger": ["error", { "some": "option" }] } } ``` With this PR, we get this error: ``` Failed to parse oxlint configuration file. x Invalid configuration for rule `no-debugger`: | This rule does not accept configuration options. ``` This prevents misconfiguration from causing problems / confusing behaviors if you try to define a config option on a rule which supports no config options. This is another part of the work for #17854. There were still a handful of rules not yet migrated over to the `config` defined inside the lint macro. I've worked around that problem by giving them dummy configs for now, and added a test accordingly. AI Disclosure: Built with help from Raptor Mini + Copilot / Claude Code + Opus. Reviewed and verified by me.
1 parent dd1a653 commit 41f92d1

File tree

17 files changed

+1057
-41
lines changed

17 files changed

+1057
-41
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"rules": {
3+
// `no-debugger` does not accept options; this should error when an options object is provided
4+
"eslint/no-debugger": ["error", { "some": "option" }]
5+
}
6+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
{
2+
"plugins": ["jest", "react"],
3+
"categories": {
4+
"correctness": "off"
5+
},
6+
"rules": {
7+
// These rules have `config = Value` in their declare_oxc_lint! macro
8+
// to indicate they accept configuration options, even though they use
9+
// manual from_configuration implementations.
10+
"eslint/no-empty-function": ["error", { "allow": ["functions"] }],
11+
"eslint/no-restricted-imports": ["error", { "paths": ["lodash"] }],
12+
"eslint/no-warning-comments": ["error", { "terms": ["todo", "fixme"] }],
13+
"jest/valid-title": ["error", { "ignoreSpaces": true }],
14+
"react/forbid-dom-props": ["error", { "forbid": ["id"] }]
15+
}
16+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// This file is used to test that rules with dummy config declarations
2+
// can accept configuration options without errors.
3+
// The actual content doesn't matter - we're testing config parsing, not linting.
4+
const x = 1;

apps/oxlint/src/lint.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1362,6 +1362,13 @@ export { redundant };
13621362
.test_and_snapshot(&[]);
13631363
}
13641364

1365+
#[test]
1366+
fn test_invalid_config_rule_without_config_but_options() {
1367+
Tester::new()
1368+
.with_cwd("fixtures/invalid_config_rules_without_config".into())
1369+
.test_and_snapshot(&[]);
1370+
}
1371+
13651372
#[test]
13661373
// Ensure the config validation works with vitest/no-hooks, which
13671374
// is an alias of jest/no-hooks.
@@ -1412,6 +1419,17 @@ export { redundant };
14121419
Tester::new().with_cwd("fixtures/valid_complex_config".into()).test_and_snapshot(&[]);
14131420
}
14141421

1422+
/// Test that rules with dummy `config = Value` declarations can accept
1423+
/// configuration options without errors. This test should be removed in
1424+
/// the future, once these rules have been updated to use proper config
1425+
/// structs.
1426+
#[test]
1427+
fn test_valid_config_rules_with_dummy_config() {
1428+
Tester::new()
1429+
.with_cwd("fixtures/valid_config_rules_with_dummy_config".into())
1430+
.test_and_snapshot(&[]);
1431+
}
1432+
14151433
#[test]
14161434
fn test_invalid_config_invalid_config_complex_enum() {
14171435
Tester::new()
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: apps/oxlint/src/tester.rs
3+
---
4+
##########
5+
arguments:
6+
working directory: fixtures/invalid_config_rules_without_config
7+
----------
8+
Failed to parse oxlint configuration file.
9+
10+
x Invalid configuration for rule `no-debugger`:
11+
| This rule does not accept configuration options.
12+
13+
----------
14+
CLI result: InvalidOptionConfig
15+
----------
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
source: apps/oxlint/src/tester.rs
3+
---
4+
##########
5+
arguments:
6+
working directory: fixtures/valid_config_rules_with_dummy_config
7+
----------
8+
Found 0 warnings and 0 errors.
9+
Finished in <variable>ms on 1 file with 5 rules using 1 threads.
10+
----------
11+
CLI result: LintSucceeded
12+
----------

crates/oxc_linter/src/config/rules.rs

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,18 @@ impl OxlintRules {
131131
.find(|r| r.name() == rule_name && r.plugin_name() == plugin_name)
132132
});
133133
if let Some(rule) = rule {
134+
// If the user provided a non-empty options array for a rule that does not
135+
// declare a `config =` type in its declaration, treat this as an invalid
136+
// configuration and report an error.
137+
if !rule_config.config.is_empty() && !rule.has_config() {
138+
errors.push(OverrideRulesError::RuleConfiguration {
139+
rule_name: rule_config.full_name().into_owned(),
140+
message: "This rule does not accept configuration options."
141+
.to_string(),
142+
});
143+
continue;
144+
}
145+
134146
// Configs are stored as `SmallVec<[Value; 1]>`, but `from_configuration` expects
135147
// a single `Value` with `Value::Null` being the equivalent of empty config
136148
let config = if rule_config.config.is_empty() {
@@ -747,6 +759,35 @@ mod test {
747759
}
748760
}
749761

762+
#[test]
763+
fn test_override_rules_errors_for_rules_without_config() {
764+
let rules_config = OxlintRules::deserialize(&json!({
765+
"eslint/no-debugger": ["error", { "some": "option" }]
766+
}))
767+
.unwrap();
768+
769+
let mut builtin_rules = RuleSet::default();
770+
let mut external_rules = FxHashMap::default();
771+
let mut store = ExternalPluginStore::default();
772+
773+
match rules_config.override_rules(
774+
&mut builtin_rules,
775+
&mut external_rules,
776+
&RULES,
777+
&mut store,
778+
) {
779+
Err(errors) => {
780+
assert!(errors.len() == 1, "expected one error, got {errors:#?}");
781+
assert!(matches!(
782+
&errors[0],
783+
super::OverrideRulesError::RuleConfiguration { rule_name, .. }
784+
if rule_name == "eslint/no-debugger" || rule_name == "no-debugger"
785+
));
786+
}
787+
Ok(()) => panic!("expected errors from invalid config"),
788+
}
789+
}
790+
750791
#[test]
751792
fn test_override_rules_errors_sorted() {
752793
let rules_config = OxlintRules::deserialize(&json!({
@@ -773,4 +814,51 @@ mod test {
773814
Ok(()) => panic!("expected errors from invalid configs"),
774815
}
775816
}
817+
818+
/// Test that rules with dummy `config = Value` declarations don't error
819+
/// when configuration options are passed to them. These rules have manual
820+
/// `from_configuration` implementations but need `config =` in their
821+
/// `declare_oxc_lint!` macro to pass the `has_config()` check.
822+
#[test]
823+
fn test_rules_with_dummy_config_accept_options() {
824+
let rules_config = OxlintRules::deserialize(&json!({
825+
"eslint/no-empty-function": ["error", { "allow": ["functions"] }],
826+
"eslint/no-restricted-imports": ["error", { "paths": ["lodash"] }],
827+
"eslint/no-warning-comments": ["error", { "terms": ["todo", "fixme"] }],
828+
"jest/valid-title": ["error", { "ignoreSpaces": true }],
829+
"react/forbid-dom-props": ["error", { "forbid": ["id"] }]
830+
}))
831+
.unwrap();
832+
833+
let mut builtin_rules = RuleSet::default();
834+
let mut external_rules = FxHashMap::default();
835+
let mut store = ExternalPluginStore::default();
836+
837+
// These rules should accept configuration without errors
838+
rules_config
839+
.override_rules(&mut builtin_rules, &mut external_rules, &RULES, &mut store)
840+
.expect("rules with dummy config should accept configuration options");
841+
842+
// Verify the rules were actually added
843+
assert!(
844+
builtin_rules.iter().any(|(r, _)| r.name() == "no-empty-function"),
845+
"no-empty-function should be in the rule set"
846+
);
847+
assert!(
848+
builtin_rules.iter().any(|(r, _)| r.name() == "no-restricted-imports"),
849+
"no-restricted-imports should be in the rule set"
850+
);
851+
assert!(
852+
builtin_rules.iter().any(|(r, _)| r.name() == "no-warning-comments"),
853+
"no-warning-comments should be in the rule set"
854+
);
855+
assert!(
856+
builtin_rules.iter().any(|(r, _)| r.name() == "valid-title"),
857+
"valid-title should be in the rule set"
858+
);
859+
assert!(
860+
builtin_rules.iter().any(|(r, _)| r.name() == "forbid-dom-props"),
861+
"forbid-dom-props should be in the rule set"
862+
);
863+
}
776864
}

0 commit comments

Comments
 (0)