Skip to content

feat(linter): PoC to add deny_unknown_fields and error propagation for ~140 rules#17614

Closed
connorshea wants to merge 43 commits intomainfrom
add-deny-unknown-fields-across-app
Closed

feat(linter): PoC to add deny_unknown_fields and error propagation for ~140 rules#17614
connorshea wants to merge 43 commits intomainfrom
add-deny-unknown-fields-across-app

Conversation

@connorshea
Copy link
Member

@connorshea connorshea commented Jan 3, 2026

JUST A TEST, DO NOT MERGE

Includes/builds off of #17600 as a test for the changes in that PR. In this PR, only the last 5 commits really matter, as the rest come from #17600.

I ran this against ecosystem-ci, and honestly I'm pretty surprised that this doesn't error for a single repo: https://github.com/oxc-project/oxc-ecosystem-ci/actions/runs/20683517161

Just an experiment for the moment, but this converts almost all of the straight-forward rules to this pattern.

AI Disclosure: I used Copilot to write a script to add the deny_unknown_fields attribute on relevant rules with struct-based configuration options. I've included the script below.

I then manually did a find+replace on the common pattern for the from_configuration function, to change that for all the rules where it was reasonably simple to do so.

#!/usr/bin/env node
/*
Add `deny_unknown_fields` to `#[serde(...)]` attributes in files under `rules/`.

Usage:
  node scripts/add_deny_unknown_fields.js [--apply] [--base PATH]

- Scans for Rust files under any `rules/` directory.
- For files that contain the token `DefaultRuleConfig` and a `#[serde(...)]`
  attribute containing `rename_all = "camelCase"` and `default`, it will
  add `deny_unknown_fields` to that serde attribute if missing.

This is conservative and only modifies attributes that match those
conditions and currently do not include `deny_unknown_fields`.

Examples:
  # Preview changes (dry-run):
  node scripts/add_deny_unknown_fields.js

  # Apply changes in-place:
  node scripts/add_deny_unknown_fields.js --apply
*/

const fs = require("fs").promises;
const path = require("path");

const ATTR_RE = /(#\s*\[\s*serde\(([^)]*)\)\s*\])/gs; // s flag to allow newlines (node 12+)
const RENAME_RE = /rename_all\s*=\s*"camelCase"/;

async function findRuleFiles(base) {
  const results = [];
  const rulesDir = path.join(base, "crates", "oxc_linter", "src", "rules");
  try {
    const stat = await fs.stat(rulesDir);
    if (!stat.isDirectory()) return results;
  } catch (e) {
    // No rules directory found; return empty list
    return results;
  }

  async function walk(dir) {
    const entries = await fs.readdir(dir, { withFileTypes: true });
    for (const ent of entries) {
      const res = path.join(dir, ent.name);
      if (ent.isDirectory()) {
        // skip node_modules and target for speed
        if (ent.name === "node_modules" || ent.name === "target") continue;
        await walk(res);
      } else if (ent.isFile() && res.endsWith(".rs")) {
        results.push(res);
      }
    }
  }
  await walk(rulesDir);
  return results;
}

function updateSerdeAttribute(full, inner) {
  // inner is the content inside serde(...)
  const hasRename = RENAME_RE.test(inner);
  const hasDefault = /\bdefault\b/.test(inner);
  const hasDeny = /deny_unknown_fields/.test(inner);
  if (hasRename && hasDefault && !hasDeny) {
    // normalize tokens by splitting on commas
    const tokens = inner
      .split(",")
      .map((t) => t.trim())
      .filter(Boolean);
    if (!tokens.includes("deny_unknown_fields")) tokens.push("deny_unknown_fields");
    const newInner = tokens.join(", ");
    const newFull = full.replace("(" + inner + ")", "(" + newInner + ")");
    return { newFull, newInner };
  }
  return null;
}

async function processFile(filePath, apply) {
  const text = await fs.readFile(filePath, "utf8");

  const FROM_CONF_SNIPPET = `fn from_configuration(value: serde_json::Value) -> Result<Self, serde_json::error::Error> {
        Ok(serde_json::from_value::<DefaultRuleConfig<Self>>(value)
            .unwrap_or_default()
            .into_inner())
    }`;

  const FROM_CONF_SNIPPET_ALT = `fn from_configuration(value: Value) -> Result<Self, serde_json::error::Error> {
        Ok(serde_json::from_value::<DefaultRuleConfig<Self>>(value)
            .unwrap_or_default()
            .into_inner())
    }`;

  // Only process files that contain the exact from_configuration method above
  if (!text.includes(FROM_CONF_SNIPPET) && !text.includes(FROM_CONF_SNIPPET_ALT)) return false;

  let changed = false;
  let out = text;
  const edits = [];
  let m;
  while ((m = ATTR_RE.exec(text)) !== null) {
    const full = m[1];
    const inner = m[2];
    const upd = updateSerdeAttribute(full, inner);
    if (upd) {
      edits.push({ full, newFull: upd.newFull });
    }
  }

  if (edits.length > 0) {
    for (const e of edits) {
      out = out.replace(e.full, e.newFull);
      changed = true;
      console.log(`Will update: ${filePath}:`);
      console.log(`  - ${e.full.trim()}`);
      console.log(`  + ${e.newFull.trim()}`);
      console.log("");
    }
    if (apply) {
      await fs.writeFile(filePath, out, "utf8");
      console.log(`Applied changes to ${filePath}\n`);
    }
  }
  return changed;
}

async function main() {
  const args = process.argv.slice(2);
  const apply = args.includes("--apply");
  const quiet = args.includes("--quiet");
  const baseIndex = args.findIndex((a) => a === "--base");
  let base = process.cwd();
  if (baseIndex !== -1 && args.length > baseIndex + 1) base = path.resolve(args[baseIndex + 1]);

  try {
    const files = await findRuleFiles(base);
    if (!files.length) {
      if (!quiet) console.log("No files found in crates/oxc_linter/src/rules/.");
      return;
    }

    let changedAny = false;
    const editedFiles = [];
    for (const f of files) {
      try {
        const changed = await processFile(f, apply);
        if (changed) {
          changedAny = true;
          if (apply) editedFiles.push(f);
        }
      } catch (err) {
        console.error(`Error processing ${f}: ${err}`);
      }
    }

    if (!changedAny && !quiet)
      console.log("No matching serde attributes found that needed changes.");
    if (changedAny && !apply) console.log("\nRun with --apply to make the changes.");

    if (apply && editedFiles.length > 0) {
      if (!quiet) {
        console.log("\nEdited files:");
        for (const ef of editedFiles) console.log(`  - ${ef}`);
      }
    } else if (apply && !quiet) {
      console.log("\nNo files were changed.");
    }
  } catch (err) {
    console.error(err);
    process.exit(2);
  }
}

if (require.main === module) main();

connorshea and others added 26 commits January 2, 2026 20:44
…he jest/no-hooks rule.

The upstream no-hooks rule does allow "undefined" and "null" values based on their tests, but I don't really see any value in supporting that explicitly, quite frankly. So I've removed the tests that allowed those particular cases.

I also added some tests to ensure that the "allow" array can be empty, and also that the config itself can be empty and not cause a panic.

Written with help from Copilot + Raptor mini.
`diagnostic` was misspelled.
…nderstand.

Needed to make it pretty and split on whitespace to avoid multi-line error messages.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Connor Shea <connor.james.shea@gmail.com>
It seems like it may be necessary? Not totally clear on that.
…DefaultRuleConfig.

Used a script to determine which to convert, then manually removed a few problematic ones I know of (namely the max_* rules).
…n failing to deserialize.

It is the exact same change for each of these rules.
…when an invalid config is hit.

This converts most remaining DefaultRuleConfig usages to this pattern. Some rules have customization that prevent this conversion for now.
@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI A-linter-plugins Area - Linter JS plugins C-enhancement Category - New feature or request labels Jan 3, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 3, 2026

CodSpeed Performance Report

Merging #17614 will not alter performance

Comparing add-deny-unknown-fields-across-app (f56c61a) with main (3a0c782)1

Summary

✅ 4 untouched
⏩ 41 skipped2

Footnotes

  1. No successful run was found on main (2e8f469) during the generation of this report, so 3a0c782 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

  2. 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.

@camc314 camc314 self-assigned this Jan 5, 2026
/// This helper also performs an integration check by embedding the config into an
/// `.oxlintrc` `rules` entry and running `ConfigStoreBuilder::from_oxlintrc` to ensure
/// that overrides parsing surface the same errors when applicable.
pub fn expect_configs(self, valid: Vec<Value>, invalid: Vec<Value>) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should consider adding this change to main by cherry-picking it/copying it over, but otherwise this PR can probably just be closed. I can redo the deny_unknown_fields/from_configuration change to the full ruleset easily enough.

@connorshea
Copy link
Member Author

Closing since this is being handled properly by #18104 (and a few other small PRs) instead.

@connorshea connorshea closed this Jan 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-linter Area - Linter A-linter-plugins Area - Linter JS plugins C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants