refactor(napi/oxlint): diagnostics communicate which rule via rule index, not rule ID (#12482)

Related to #12437.

Diagnostics sent from JS to Rust specify which rule the diagnostic relates to via an index into `external_rules`, instead of the `ExternalRuleId`. This allows removing the loop over `external_rules` on Rust side to find the relevant rule.

Author: overlookmotel

crates/oxc_linter/src/config/config_store.rs

@@ -286,7 +286,10 @@ impl ConfigStore {
     }
 
     #[cfg_attr(not(all(feature = "oxlint2", not(feature = "disable_oxlint2"))), expect(dead_code))]
-    pub(crate) fn resolve_plugin_rule_names(&self, external_rule_id: u32) -> Option<(&str, &str)> {
+    pub(crate) fn resolve_plugin_rule_names(
+        &self,
+        external_rule_id: ExternalRuleId,
+    ) -> (/* plugin name */ &str, /* rule name */ &str) {
         self.external_plugin_store.resolve_plugin_rule_names(external_rule_id)
     }
 }

crates/oxc_linter/src/external_linter.rs

@@ -42,7 +42,7 @@ pub enum PluginLoadResult {
 #[derive(Clone, Debug, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct LintResult {
-    pub external_rule_id: u32,
+    pub rule_index: u32,
     pub message: String,
     pub loc: Loc,
 }

crates/oxc_linter/src/external_plugin_store.rs

@@ -79,11 +79,13 @@ impl ExternalPluginStore {
         })
     }
 
-    pub fn resolve_plugin_rule_names(&self, external_rule_id: u32) -> Option<(&str, &str)> {
-        let external_rule = self.rules.get(ExternalRuleId::from_raw(external_rule_id))?;
+    pub fn resolve_plugin_rule_names(
+        &self,
+        external_rule_id: ExternalRuleId,
+    ) -> (/* plugin name */ &str, /* rule name */ &str) {
+        let external_rule = &self.rules[external_rule_id];
         let plugin = &self.plugins[external_rule.plugin_id];
-
-        Some((&plugin.name, &external_rule.name))
+        (&plugin.name, &external_rule.name)
     }
 }
 

crates/oxc_linter/src/lib.rs

@@ -266,33 +266,18 @@ impl Linter {
         match result {
             Ok(diagnostics) => {
                 for diagnostic in diagnostics {
-                    match self.config.resolve_plugin_rule_names(diagnostic.external_rule_id) {
-                        Some((plugin_name, rule_name)) => {
-                            ctx_host.push_diagnostic(Message::new(
-                                // TODO: `error` isn't right, we need to get the severity from `external_rules`
-                                OxcDiagnostic::error(diagnostic.message)
-                                    .with_label(Span::new(diagnostic.loc.start, diagnostic.loc.end))
-                                    .with_error_code(plugin_name.to_string(), rule_name.to_string())
-                                    .with_severity(
-                                        (*external_rules
-                                            .iter()
-                                            .find(|(rule_id, _)| {
-                                                rule_id.raw() == diagnostic.external_rule_id
-                                            })
-                                            .map(|(_, severity)| severity)
-                                            .expect(
-                                                "external rule must exist when resolving severity",
-                                            ))
-                                        .into(),
-                                    ),
-                                PossibleFixes::None,
-                            ));
-                        }
-                        None => {
-                            // TODO: report diagnostic, this should be unreachable
-                            debug_assert!(false);
-                        }
-                    }
+                    let (external_rule_id, severity) =
+                        external_rules[diagnostic.rule_index as usize];
+                    let (plugin_name, rule_name) =
+                        self.config.resolve_plugin_rule_names(external_rule_id);
+
+                    ctx_host.push_diagnostic(Message::new(
+                        OxcDiagnostic::error(diagnostic.message)
+                            .with_label(Span::new(diagnostic.loc.start, diagnostic.loc.end))
+                            .with_error_code(plugin_name.to_string(), rule_name.to_string())
+                            .with_severity(severity.into()),
+                        PossibleFixes::None,
+                    ));
                 }
             }
             Err(_err) => {

napi/oxlint2/src-js/index.js

@@ -51,39 +51,37 @@ async function loadPluginImpl(path) {
 
   // TODO: Use a validation library to assert the shape of the plugin, and of rules
   const pluginName = plugin.meta.name;
-  let ruleId = registeredRules.length;
+  const offset = registeredRules.length;
   const ruleNames = [];
-  const ret = { name: pluginName, offset: ruleId, ruleNames };
 
   for (const [ruleName, rule] of Object.entries(plugin.rules)) {
     ruleNames.push(ruleName);
-    registeredRules.push({ rule, context: new Context(ruleId, `${pluginName}/${ruleName}`) });
-    ruleId++;
+    registeredRules.push({ rule, context: new Context(`${pluginName}/${ruleName}`) });
   }
 
-  return JSON.stringify({ Success: ret });
+  return JSON.stringify({ Success: { name: pluginName, offset, ruleNames } });
 }
 
+let setupContextForFile;
+
 /**
  * Context class.
  *
  * Each rule has its own `Context` object. It is passed to that rule's `create` function.
  */
 class Context {
-  // Rule ID. Index into `registeredRules` array.
-  #ruleId;
   // Full rule name, including plugin name e.g. `my-plugin/my-rule`.
   id;
+  // Index into `ruleIds` sent from Rust. Set before calling `rule`'s `create` method.
+  #ruleIndex;
   // Absolute path of file being linted. Set before calling `rule`'s `create` method.
   physicalFilename;
 
   /**
    * @constructor
-   * @param {number} ruleId - Rule ID
    * @param {string} fullRuleName - Rule name, in form `<plugin>/<rule>`
    */
-  constructor(ruleId, fullRuleName) {
-    this.#ruleId = ruleId;
+  constructor(fullRuleName) {
     this.id = fullRuleName;
   }
 
@@ -100,9 +98,28 @@ class Context {
     diagnostics.push({
       message: diagnostic.message,
       loc: { start: diagnostic.node.start, end: diagnostic.node.end },
-      externalRuleId: this.#ruleId,
+      ruleIndex: this.#ruleIndex,
     });
   }
+
+  static {
+    /**
+     * Update a `Context` with file-specific data.
+     *
+     * We have to define this function within class body, as it's not possible to set private property
+     * `#ruleIndex` from outside the class.
+     * We don't use a normal class method, because we don't want to expose this to user.
+     *
+     * @param {Context} context - `Context` object
+     * @param {number} ruleIndex - Index of this rule within `ruleIds` passed from Rust
+     * @param {string} filePath - Absolute path of file being linted
+     * @returns {undefined}
+     */
+    setupContextForFile = (context, ruleIndex, filePath) => {
+      context.#ruleIndex = ruleIndex;
+      context.physicalFilename = filePath;
+    };
+  }
 }
 
 // --------------------
@@ -154,9 +171,10 @@ function lintFile([filePath, bufferId, buffer, ruleIds]) {
 
   // Get visitors for this file from all rules
   initCompiledVisitor();
-  for (const ruleId of ruleIds) {
+  for (let i = 0; i < ruleIds.length; i++) {
+    const ruleId = ruleIds[i];
     const { rule: { create }, context } = registeredRules[ruleId];
-    context.physicalFilename = filePath;
+    setupContextForFile(context, i, filePath);
     const visitor = create(context);
     addVisitorToCompiled(visitor);
   }