Skip to content

feat(config): per-rule configuration with severity and options - #1534

Open
bartlomieju wants to merge 1 commit into
mainfrom
feat-rule-configuration
Open

feat(config): per-rule configuration with severity and options#1534
bartlomieju wants to merge 1 commit into
mainfrom
feat-rule-configuration

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

Introduces a per-rule configuration mechanism — severity plus rule-specific
options — and demonstrates it on three existing rules. This is a design
prototype: it establishes the machinery and proves it end-to-end, but only
three rules are wired up so far and the config-file/CLI parsing that would
populate it is intentionally out of scope.

The core idea is to stop conflating a rule's definition with a
configured, runnable instance. Today a rule is a zero-sized unit struct
registered as a pre-built Box<dyn LintRule>, so there is nowhere to put
options and no object-safe way to add a from_configuration constructor to
the trait. eslint ({ meta, create }) and oxlint (a rule descriptor plus
from_configuration) both keep the two halves apart, and this change
reintroduces that split in a new src/rules/config.rs:

  • RuleDef is the definition — &'static metadata (code, tags, default
    severity) plus a configure_options function pointer that builds a
    runnable rule from optional JSON options. The registry can eventually hold
    these instead of pre-built instances.
  • ConfiguredRule is a runnable LintRule plus the LintDiagnosticSeverity
    its diagnostics should carry.
  • RuleConfig { severity, options } models eslint's [severity, options].
    configure_rules resolves a whole registry against user data keyed by
    code, and split_configured adapts the result into the two inputs the
    linter consumes.

Severity is unified with enablement the way eslint does it: a rule resolving
to Off is never constructed, while Warn/Error map onto the existing
LintDiagnostic.severity field. LinterOptions gains a rule_severities
map, and the linter stamps each diagnostic's severity in a post-pass that is
skipped entirely when no severities are configured, so callers that don't opt
in keep the current always-error behavior.

Three rules are retrofitted to exercise three different option shapes, each
matching its eslint counterpart:

  • no-console gains allow (a string list): console.warn is permitted
    when listed, other methods are still flagged.
  • no-empty gains allowEmptyCatch (a bool): an empty catch {} is allowed
    while other empty blocks are still flagged.
  • eqeqeq gains a "always"/"smart" mode (a bare string enum): "smart"
    permits comparing against null, evaluating typeof, and comparing two
    literals.

All three default to their previous behavior, so existing rules and tests are
unchanged; the other rules keep running through the current registry
untouched. New unit tests cover each option shape, the severity resolution
(including off-excludes-the-rule and unknown-rule errors), and an end-to-end
linter test proving a rule configured to warn emits a warning rather than the
default error.

A few things are deliberately left as fast-follows, and are called out in the
module documentation: hand-writing a RuleDef per rule will not scale to the
full rule set and wants a declare_rules! macro or build-time codegen; the
registry can then become a zero-allocation &'static [RuleDef]; and the
per-run severity stamping could move to emit time. Performance and generated
code size were a stated concern, so the design keeps descriptors to static
data plus a function pointer, shares a single monomorphic path for
option-less rules, and gates the one new per-run cost behind an emptiness
check.

@bartlomieju

Copy link
Copy Markdown
Member Author

Verdict: LGTM on the design & implementation — needs maintainer sign-off on the architecture direction (it's a foundational change) + a rebase. Not auto-merging (public API/behavior + it's a design prototype). One consistency nit below.

Clean separation of rule definition (RuleDef: static metadata + ConfigureFn) from configured instance (ConfiguredRule), mirroring eslint/oxlint. I checked the machinery end-to-end:

Correctness — verified

  • RuleDef::configure resolves config.severity.unwrap_or(default_severity), returns Ok(None) for Off (rule never built), else builds the rule and pairs it with the diagnostic severity. Off = disabled matches eslint.
  • The linter post-pass is correctly cost-free when unused: gated on !rule_severities.is_empty(), then O(diagnostics) with O(1) map lookups. Callers not opting in keep the always-Error behavior — genuinely backward compatible, and the existing get_all_rules() path is untouched.
  • configure_rules reports unknown rule codes rather than silently ignoring them; empty user config → each def falls back to default_severity, so the default set = recommended-at-Error. split_configured borrows code() (&'static) into the severity map — no lifetime issues.
  • eqeqeq "always"/"smart" deserializes correctly via #[serde(rename_all="lowercase")] (bare-string enum), with "nope"InvalidOptions. no_options shares one monomorphic path for option-less rules (no per-rule codegen), and correctly treats None/null/{} as "no options".

Consistency nit (worth fixing before scaling to ~120 rules)

  • Option-taking configure fns don't normalize "empty" options the way no_options does. eqeqeq's configure does Some(value) => from_value(value.clone()), so {"eqeqeq": ["error", null]} or ["error", {}] would error ("invalid type: null, expected enum"), whereas an option-less rule treats Some(null)/Some({}) as defaults. Recommend a shared helper that maps None | Some(null) | Some({}) → defaults for all rules, so ["error", null] uniformly means "on, default options". This will matter once config-file/CLI parsing feeds real data.

Perf / scaling (already flagged in your module docs)

  • configure_rules' unknown-code check is O(user × defs); with the future &'static [RuleDef] registry, a code → def index (HashMap/phf) makes both that check and the per-def lookup O(1).
  • The declare_rules! macro / build-time codegen for RuleDef is the right call before hand-writing 120 defs — agreed with the doc.

Housekeeping: mergeStateStatus: BLOCKED (awaiting review) → rebase; this is a foundational architecture change, so the main gate is a maintainer decision on the direction (which the prototype proves well).

No blocking bugs. Recommend: align the empty-options normalization across all configure fns, then get architecture sign-off before wiring the full rule set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant