docs(linter): Handle rules that use tuple config options with DefaultRuleConfig #16555

connorshea · 2025-12-06T05:53:48Z

This enables DefaultRuleConfig to handle tuple structs properly so we can have an easier time resolving #16023.

This is also part of #14743.

This allows us to refactor the way we handle the config for SortKeys, so it's much simpler. And also updates func-style and arrow-body-style to use the tuple pattern and have correct config option docs. I initially tried to update the no-inner-declarations rule to use the tuple pattern, but it lead to test failures due to what I suspect is a bug in the implementation of that rule. So I split that into its own PR.

This was done with help from GitHub Copilot + their Raptor mini model, but I've added enough tests - and all the rules that currently use DefaultRuleConfig still pass their tests - that I feel pretty good about it being functional, albeit probably not elegant. Happy to take feedback on improving this in that regard especially.

Updated the eslint/func-style rule:

## Configuration

### The 1st option

type: `"expression" | "declaration"`

### The 2nd option

This option is an object with the following properties:

#### allowArrowFunctions

type: `boolean`

default: `false`

When true, arrow functions are allowed regardless of the style setting.

#### allowTypeAnnotation

type: `boolean`

default: `false`

When true, functions with type annotations are allowed regardless of the style setting.

#### overrides

type: `object`

##### overrides.namedExports

type: `string | null`

default: `null`

For eslint/arrow-body-style:

## Configuration

### The 1st option

type: `"as-needed" | "always" | "never"`

#### `"as-needed"`

`as-needed` enforces no braces where they can be omitted (default).

Examples of **incorrect** code for this rule with the `as-needed` option:

\```js
/* arrow-body-style: ["error", "as-needed"] */

/* ✘ Bad: */
const foo = () => {
  return 0;
};
\```

Examples of **correct** code for this rule with the `as-needed` option:

\```js
/* arrow-body-style: ["error", "as-needed"] */

/* ✔ Good: */
const foo1 = () => 0;

const foo2 = (retv, name) => {
  retv[name] = true;
  return retv;
};

const foo3 = () => {
  bar();
};
\```

#### `"always"`

`always` enforces braces around the function body.

Examples of **incorrect** code for this rule with the `always` option:

\```js
/* arrow-body-style: ["error", "always"] */

/* ✘ Bad: */
const foo = () => 0;
\```

Examples of **correct** code for this rule with the `always` option:

\```js
/* arrow-body-style: ["error", "always"] */

/* ✔ Good: */
const foo = () => {
  return 0;
};
\```

#### `"never"`

`never` enforces no braces around the function body (constrains arrow functions to the role of returning an expression)

Examples of **incorrect** code for this rule with the `never` option:

\```js
/* arrow-body-style: ["error", "never"] */

/* ✘ Bad: */
const foo = () => {
  return 0;
};
\```

Examples of **correct** code for this rule with the `never` option:

\```js
/* arrow-body-style: ["error", "never"] */

/* ✔ Good: */
const foo = () => 0;
const bar = () => ({ foo: 0 });
\```

### The 2nd option

This option is an object with the following properties:

#### requireReturnForObjectLiteral

type: `boolean`

default: `false`

If `true`, requires braces and an explicit return for object literals.

Note: This option only applies when the first option is `"as-needed"`.

Examples of **incorrect** code for this rule with the `{ "requireReturnForObjectLiteral": true }` option:

\```js
/* arrow-body-style: ["error", "as-needed", { "requireReturnForObjectLiteral": true }]*/

/* ✘ Bad: */
const foo = () => ({});
const bar = () => ({ bar: 0 });
\```

Examples of **correct** code for this rule with the `{ "requireReturnForObjectLiteral": true }` option:

\```js
/* arrow-body-style: ["error", "as-needed", { "requireReturnForObjectLiteral": true }]*/

/* ✔ Good: */
const foo = () => {};
const bar = () => {
  return { bar: 0 };
};
\```

crates/oxc_linter/src/rule.rs

Copilot

Pull request overview

This PR refactors the linter's rule configuration handling by enhancing DefaultRuleConfig to properly support tuple struct configurations, making it easier to implement ESLint-style array-based rule configs. The refactoring simplifies the sort_keys rule implementation and updates no_inner_declarations to use the new pattern.

Key Changes:

Enhanced DefaultRuleConfig deserializer to handle tuple structs with smart fallback behavior for partial configurations
Refactored sort_keys rule to use the new pattern, removing ~30 lines of manual config parsing
Converted no_inner_declarations to use tuple struct configuration with DefaultRuleConfig

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
crates/oxc_linter/src/rule.rs	Enhanced DefaultRuleConfig to handle tuple configs with comprehensive test coverage; updated documentation with tuple struct examples
crates/oxc_linter/src/rules/eslint/sort_keys.rs	Simplified from_configuration using DefaultRuleConfig pattern; removed helper methods in favor of direct tuple destructuring
crates/oxc_linter/src/rules/eslint/no_inner_declarations.rs	Refactored to tuple struct config with DefaultRuleConfig; introduces breaking change in default block_scoped_functions behavior

Comments suppressed due to low confidence (1)

crates/oxc_linter/src/rules/eslint/no_inner_declarations.rs:30

This refactoring changes the default behavior for block_scoped_functions. Previously, when no second parameter was provided (e.g., ["functions"]), the old implementation defaulted to Some(BlockScopedFunctions::Allow). Now with #[serde(default)], the second field defaults to None.

This breaks tests like the one on line 218-219 which expects ["functions"] to allow inner declarations in strict mode.

To maintain backward compatibility, you could either:

Implement Default for NoInnerDeclarationsConfigObject to return Some(BlockScopedFunctions::Allow) instead of None
Update the logic in the run method to treat None as Allow (though this changes the meaning of the field)

The PR description mentions this is being left for review - a decision is needed on whether to match ESLint's behavior (where None might be the correct default) or maintain backward compatibility.

#[derive(Debug, Default, Clone, Deserialize, JsonSchema)]
#[serde(rename_all = "camelCase", default)]
struct NoInnerDeclarationsConfigObject {
    /// Controls whether function declarations in nested blocks are allowed in strict mode (ES6+ behavior).
    #[schemars(with = "BlockScopedFunctions")]
    block_scoped_functions: Option<BlockScopedFunctions>,
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/oxc_linter/src/rule.rs

codspeed-hq · 2025-12-06T05:59:12Z

CodSpeed Performance Report

Merging #16555 will not alter performance

_{Comparing handle-tuple-rules-better (48cda35) with main (8babdf9)}

Summary

✅ 4 untouched
⏩ 41 skipped¹

41 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

crates/oxc_linter/src/rule.rs

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ex config shapes. This will make it much easier to handle rules which accept tuples. I still need to fix a few things, though.

…mplify some things. This will hopefully also ensure that the most common case (a default config object for a given rule) is fast. But I still need to simplify the rest of the code here.

…imize it with this.

… now that we can do tuples in DefaultRuleConfig. This is a proof-of-concept to confirm that these updates to the DefaultRuleConfig tuple handling work as intended and simplify the code :)

…tions rule. I'm pretty sure there's a bug in this rule. It doesn't seem like it should've been returning `None` for the `block_scoped_functions` value by default, the original ESLint rule returns "allow" by default. But I left the behavior alone here, we can address that later if we want to.

I'm pretty sure the no_inner_declarations rule has a bug in it right now, so it failing here isn't really indicative of any problem with the . It's very odd to me that it was defaulting to None for the `block_scoped_functions` instead of `Allow`, I don't understand why that was done and it seems like there are behavioral differences vs. the original rule as a result.

…docs.

…-body-style` rule.

I'd rather just leave it as-is for now so that we can focus on the other changes in this PR. I'll open another PR with the code cleanup on this rule.

connorshea · 2025-12-06T22:53:34Z

Rebased to make sure the changes to sort-keys on main didn't cause any issues (I ran the tests after trying the rebase locally and they passed fine)

camc314 · 2025-12-07T12:17:31Z

crates/oxc_linter/src/rule.rs

+                if let Ok(config) = serde_json::from_value::<T>(first.clone()) {
+                    return Ok(DefaultRuleConfig(config));
+                }


is there any way we can avoid the clones here? When i originally wrote this I was super careful about the clones, as this is a hot-ish path, so avoiding them where we can is ideal.

Yeah I was a bit surprised this didn't impact the benchmarks at all, but I don't know if they're really exercising the config options much. I'll give these improvements a shot, but I may end up needing some help on getting it to work

I managed to get rid of almost all the clones, although the code is still jank.

camc314 · 2025-12-07T12:18:09Z

crates/oxc_linter/src/rule.rs

+                // Attempt the tuple-of-objects case by appending an empty object
+                // (so `[obj]` -> `[obj, {}]`). If that deserializes to T, accept it.
+                let arr_two = serde_json::Value::Array(vec![
+                    first.clone(),
+                    serde_json::Value::Object(serde_json::Map::new()),
+                ]);
+                if let Ok(config) = serde_json::from_value::<T>(arr_two) {
+                    return Ok(DefaultRuleConfig(config));
+                }


is this the same behaviour as eslint? I'm wondering whether we can just fallback to default if they only provide one of the config options

I guess that depends on what you mean by this question. We can fallback to default for the second option if they only pass the first, but if you're asking to fallback to default for both tuple values when only one is passed, that'd be different from ESLint's implementation.

The three main shapes different rules take (as you know) are generally:

[{ "bar": "baz" }]

["foo"]

["foo", { "bar": "baz" }]

But a small handful of rules (literally 3 in core ESLint as far as I can tell, and one is stylistic/deprecated) take a tuple of objects, e.g. no-restricted-syntax:

"no-restricted-syntax": [ "error", { "selector": "FunctionExpression", "message": "Function expressions are not allowed." }, { "selector": "CallExpression[callee.name='setTimeout'][arguments.length!=2]", "message": "setTimeout must always be invoked with two arguments." } ]

For no-restricted-syntax, you can just pass the first object value and it'll use the default for the second value (you can also technically pass an object for the first argument and a string for the second, which I despise). We need to somehow determine whether the shape of T matches this pattern in order to optimize what we do here, which I had trouble doing because I'm a Rust noob lol. So Copilot and I ended up with that current implementation which does wacky serialization attempts.

Since this pattern is only used in a small number of rules, maybe for optimization reasons we should assume that - if the first element in the config array is an object - this should just be treated as an object-only config like [{ "bar": "baz" }]. No need to bother with the case of a two-object tuple rule for now. Almost all the tuple rules are in the shape of ["foo", { "bar": "baz" }] anyway.

And then any rules that need the more complex handling can be dealt with later, or just won't be able to use DefaultRuleConfig/will need some alternative implementation like DefaultRuleConfigWithDoubleObject.

Does skipping the ability to handle two-object config tuples sound fine as a solution here?

Removed the logic for two-object tuple support in 332f8ea and that simplified things a ton.

camc314 · 2025-12-07T12:20:13Z

crates/oxc_linter/src/rule.rs

+                let first = arr.first().cloned();
+
+                if let Ok(t) = serde_json::from_value::<T>(serde_json::Value::Array(arr)) {
+                    return Ok(DefaultRuleConfig(t));
+                }
+
+                // Parsing the whole array failed; parse first element if present.
+                let config =
+                    first.and_then(|v| serde_json::from_value(v).ok()).unwrap_or_else(T::default);
+
+                Ok(DefaultRuleConfig(config))


this feels wierd, you go from arr which is an array, clone it, just to make a new serde array?

I think we can be a bit stricter with what we accept here, if it improves performance.

Rewrote this part to mostly fix this problem.

This simplifies the config parsing logic *a ton*.

Maybe we should consider adding a panic if the input array has more than one object passed to it, maybe only in debug mode?

connorshea · 2025-12-07T20:33:57Z

crates/oxc_linter/src/rule.rs

+        let t = serde_json::from_value::<T>(serde_json::Value::Array(arr))
+            .unwrap_or_else(|_| T::default());


Could alternatively do this, but I'm not really sure that's any better/different:

let t = T::deserialize(serde_json::Value::Array(arr)) .unwrap_or_else(|_| T::default());

connorshea · 2025-12-07T21:17:37Z

Ok, this is all ready for re-review.

…, I have. (#16562) Uses DefaultRuleConfig now, this rule does. Updated to use a proper tuple config, it has been. Continue to pass, the tests do. Part of #14743 and dependent on #16555, this PR is. Generated docs: ```md ## Configuration ### The 1st option type: `"never" | "always"` #### `"never"` The default `"never"` option can have exception options in an object literal, via `exceptRange` and `onlyEquality`. #### `"always"` The `"always"` option requires that literal values must always come first in comparisons. ### The 2nd option This option is an object with the following properties: #### exceptRange type: `boolean` default: `false` If the `"exceptRange"` property is `true`, the rule _allows_ yoda conditions in range comparisons which are wrapped directly in parentheses, including the parentheses of an `if` or `while` condition. A _range_ comparison tests whether a variable is inside or outside the range between two literal values. #### onlyEquality type: `boolean` default: `false` If the `"onlyEquality"` property is `true`, the rule reports yoda conditions _only_ for the equality operators `==` and `===`. The `onlyEquality` option allows a superset of the exceptions which `exceptRange` allows, thus both options are not useful together. ```

…so the config option docs for it (#16560) This is part of #16023. See [the tests for the original rule](https://github.com/eslint/eslint/blob/b017f094d4e53728f8d335b9cf8b16dc074afda3/tests/lib/rules/eqeqeq.js). We have a test in [the eqeqeq.rs implementation](https://github.com/oxc-project/oxc/blob/e24aabdfa65044a7223e4ea7b294ad3bf5dfb1ec/crates/oxc_linter/src/rules/eslint/eqeqeq.rs) like so: ```rs // Issue: <#8773> ("href != null", Some(json!([{"null": "ignore"}]))), ``` The problem is that this test has an incorrect shape for the config object, see here: ```jsonc // Should always be in one of these three formats, all three work in the original rule as well: "eslint/eqeqeq": ["error", "always", { "null": "never" }], "eslint/eqeqeq": ["error", "always"], "eslint/eqeqeq": ["error"], // But right now the tests have a case where the string arg is skipped, while the ESLint rule does not allow this: "eslint/eqeqeq": ["error", { "null": "ignore" }], ``` The problem is that the code _did_ previously handle this config array as invalid. However, because the implementation of `from` on NullType would fall back to `ignore` if it received bad data, it looked like it worked: ```rs impl NullType { pub fn from(raw: &str) -> Self { match raw { "always" => Self::Always, "never" => Self::Never, _ => Self::Ignore, } } } ``` Because `always` is marked as the default value (and is also the default value in the original ESLint rule), and so should be the default case. The test was just hitting the fallback value, so it looked like it worked, but really the fallback value was incorrect previously and did not match the docs _or_ the ESLint behavior. This fixes that issue by correcting the fallback value, and also fixes the auto-generated config shape/docs, so it correctly represents itself as taking a tuple. Generated docs: ```md ## Configuration ### The 1st option type: `"always" | "smart"` #### `"always"` Always require triple-equal comparisons, `===`/`!==`. This is the default. #### `"smart"` Allow certain safe comparisons to use `==`/`!=` (`typeof`, literals, nullish). ### The 2nd option This option is an object with the following properties: #### null type: `"always" | "never" | "ignore"` ##### `"always"` Always require triple-equals when comparing with null, `=== null`/`!== null`. This is the default. ##### `"never"` Never require triple-equals when comparing with null, always use `== null`/`!= null` ##### `"ignore"` Ignore null comparisons, allow either `== null`/`!= null` and `=== null`/`!== null` ```

connorshea requested a review from camc314 as a code owner December 6, 2025 05:53

Copilot AI review requested due to automatic review settings December 6, 2025 05:53

github-actions bot added A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 6, 2025

Copilot started reviewing on behalf of connorshea December 6, 2025 05:54 View session

graphite-app bot reviewed Dec 6, 2025

View reviewed changes

crates/oxc_linter/src/rule.rs Outdated Show resolved Hide resolved

Copilot AI reviewed Dec 6, 2025

View reviewed changes

crates/oxc_linter/src/rule.rs Outdated Show resolved Hide resolved

crates/oxc_linter/src/rule.rs Outdated Show resolved Hide resolved

crates/oxc_linter/src/rule.rs Outdated Show resolved Hide resolved

crates/oxc_linter/src/rule.rs Outdated Show resolved Hide resolved

connorshea changed the title ~~refactor(linter): Handle tuple rules more effectively with DefaultRuleConfig~~ refactor(linter): Handle rules that use tuple config options with DefaultRuleConfig Dec 6, 2025

connorshea changed the title ~~refactor(linter): Handle rules that use tuple config options with DefaultRuleConfig~~ docs(linter): Handle rules that use tuple config options with DefaultRuleConfig Dec 6, 2025

github-actions bot added the C-docs Category - Documentation. Related to user-facing or internal documentation label Dec 6, 2025

graphite-app bot reviewed Dec 6, 2025

View reviewed changes

crates/oxc_linter/src/rule.rs Outdated Show resolved Hide resolved

connorshea requested a review from Copilot December 6, 2025 20:34

Copilot started reviewing on behalf of connorshea December 6, 2025 20:34 View session

Copilot AI reviewed Dec 6, 2025

View reviewed changes

connorshea mentioned this pull request Dec 6, 2025

docs(linter): Updated eslint/yoda rule with auto-gen config options, I have. #16562

Merged

camc314 self-assigned this Dec 6, 2025

connorshea added 13 commits December 6, 2025 15:52

docs(linter): Update DefaultRuleConfig deserialization for more compl…

484c41e

…ex config shapes. This will make it much easier to handle rules which accept tuples. I still need to fix a few things, though.

refactor(linter): Refactor the DefaultRuleConfig implementation to si…

f79c1aa

…mplify some things. This will hopefully also ensure that the most common case (a default config object for a given rule) is fast. But I still need to simplify the rest of the code here.

Add more tests for the DefaultRuleConfig behavior.

2054095

Add more tests, simply code a bit hopefully.

8c59173

Updates for tests.

986d18c

I'm sure this can be done better, but I've done my best trying to opt…

766f7c7

…imize it with this.

refactor(linter): Rewrite SortKeys config options to be a lot simpler…

0e5ecc9

… now that we can do tuples in DefaultRuleConfig. This is a proof-of-concept to confirm that these updates to the DefaultRuleConfig tuple handling work as intended and simplify the code :)

fmt

6b7ccaa

Resolve lint warning and fix feedback.

bbfd138

Resolve feedback.

4eeccd3

Resolve lint warnings.

d71a82b

connorshea added 5 commits December 6, 2025 15:52

doctests lol

96cf9ae

docs(linter): Update eslint/func-style rule to fix the config option …

5c771ef

…docs.

docs(linter): Add auto-generated config option docs for `eslint/arrow…

414a7f5

…-body-style` rule.

Revert no_inner_declarations.rs back to its state on main.

536781c

I'd rather just leave it as-is for now so that we can focus on the other changes in this PR. I'll open another PR with the code cleanup on this rule.

Appease the linter.

962bfe2

connorshea force-pushed the handle-tuple-rules-better branch from ca59885 to 962bfe2 Compare December 6, 2025 22:52

camc314 reviewed Dec 7, 2025

View reviewed changes

connorshea added 8 commits December 7, 2025 12:05

Remove all but one usage of clone in DefaultRuleConfig deserialization.

fdc4795

refactor(linter): Remove support for double-object tuples.

332f8ea

This simplifies the config parsing logic *a ton*.

Add another test case.

948e173

Fix tuple test to use an actual tuple struct.

3fea757

Remove test for handling of extra elements, no need to support that.

8375a6d

refactor: Avoid converting into an array and then back for no reason.

43c982a

Maybe we should consider adding a panic if the input array has more than one object passed to it, maybe only in debug mode?

Flatten the code a bit.

200d057

Add a few comments.

19b5d40

connorshea commented Dec 7, 2025

View reviewed changes

connorshea requested a review from camc314 December 7, 2025 20:34

connorshea added 3 commits December 7, 2025 13:58

Create an assert function to simplify testing the DefaultRuleConfig.

5d40c28

Update one more test to use the custom assert fn

a6a0eb1

Resolve lint violations.

5f2ee80

connorshea added 2 commits December 9, 2025 23:59

		let t = serde_json::from_value::<T>(serde_json::Value::Array(arr))
		.unwrap_or_else(\|_\| T::default());

Uh oh!

docs(linter): Handle rules that use tuple config options with DefaultRuleConfig #16555

Are you sure you want to change the base?

docs(linter): Handle rules that use tuple config options with DefaultRuleConfig #16555

Uh oh!

Conversation

connorshea commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codspeed-hq bot commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #16555 will not alter performance

Summary

Footnotes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

connorshea commented Dec 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorshea Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorshea commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

connorshea commented Dec 6, 2025 •

edited

Loading

codspeed-hq bot commented Dec 6, 2025 •

edited

Loading

connorshea Dec 7, 2025 •

edited

Loading

connorshea commented Dec 7, 2025 •

edited

Loading