fix(linter/plugins): do not call before hook if empty visitor (#14401)

If rule is defined using `createOnce`, and it returns an empty visitor (`before` / `after` hooks only), then don't run the hooks - the rule is a no-op.

Reason this is important is forward-compatibility with an optimization we could make where we calculate on Rust side whether AST contains any nodes that rules act on, and if it doesn't, skip calling into JS entirely. In that case, the `before` hook wouldn't run on those files.

We can't emulate that behavior entirely, but we can at least avoid users creating rules which *only* contain a `before` hook, and then finding that when we apply that optimization later on, it stops behaving as expected.

i.e. we make the `before` hook never be called for a rule defined like this:

```js
export default {
  createOnce(context) {
    return {
      before() {
        if (content.filename.endsWith('.tsx') {
          context.report({ message: 'No TSX!', node: context.sourceCode.ast });
        }
      },
    };
  },
};
```

Author: overlookmotel

apps/oxlint/src-js/plugins/load.ts

@@ -60,6 +60,9 @@ const registeredPluginPaths = new Set<string>();
 // Indexed by `ruleId`, which is passed to `lintFile`.
 export const registeredRules: RuleAndContext[] = [];
 
+// `before` hook which makes rule never run.
+const neverRunBeforeHook: BeforeHook = () => false;
+
 // Plugin details returned to Rust
 interface PluginDetails {
   // Plugin name
@@ -146,6 +149,19 @@ async function loadPluginImpl(path: string): Promise<PluginDetails> {
       beforeHook = conformHookFn(beforeHook, 'before');
       afterHook = conformHookFn(afterHook, 'after');
 
+      // If empty visitor, make this rule never run by substituting a `before` hook which always returns `false`.
+      // This means the original `before` hook won't run either.
+      //
+      // Reason for doing this is:
+      // In future, we may do a check on Rust side whether AST contains any nodes which rules act on,
+      // and if not, skip calling into JS entirely. In that case, the `before` hook won't get called.
+      // We can't emulate that behavior exactly, but we can at least emulate it in this simple case,
+      // and prevent users defining rules with *only* a `before` hook, which they expect to run on every file.
+      if (ObjectKeys(visitor).length === 0) {
+        beforeHook = neverRunBeforeHook;
+        afterHook = null;
+      }
+
       ruleAndContext = { rule, context, visitor, beforeHook, afterHook };
     } else {
       ruleAndContext = { rule, context, visitor: null, beforeHook: null, afterHook: null };

apps/oxlint/test/fixtures/createOnce/.oxlintrc.json

@@ -6,6 +6,7 @@
     "create-once-plugin/skip-run": "error",
     "create-once-plugin/before-only": "error",
     "create-once-plugin/after-only": "error",
+    "create-once-plugin/only-hooks": "error",
     "create-once-plugin/no-hooks": "error"
   }
 }

apps/oxlint/test/fixtures/createOnce/plugin.ts

@@ -117,6 +117,20 @@ const afterOnlyRule: Rule = {
   },
 };
 
+const hooksOnlyRule: Rule = {
+  createOnce(context) {
+    return {
+      // Neither hook should be called, because no AST node visitor functions
+      before() {
+        context.report({ message: 'before hook: should not be output', node: SPAN });
+      },
+      after() {
+        context.report({ message: 'after hook: should not be output', node: SPAN });
+      },
+    };
+  },
+};
+
 const noHooksRule: Rule = {
   createOnce(context) {
     return {
@@ -139,6 +153,7 @@ const plugin: Plugin = {
     'skip-run': skipRunRule,
     'before-only': beforeOnlyRule,
     'after-only': afterOnlyRule,
+    'only-hooks': hooksOnlyRule,
     'no-hooks': noHooksRule,
   },
 };

apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/output.snap.md

@@ -6,7 +6,7 @@
   x Error running JS plugin.
   | File path: <root>/apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/files/index.js
   | Error: Whoops!
-  |     at after (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/plugin.ts:12:19)
+  |     at after (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/plugin.ts:13:19)
 
 Found 0 warnings and 1 error.
 Finished in Xms on 1 file using X threads.

apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/plugin.ts

@@ -8,6 +8,7 @@ const plugin: Plugin = {
     error: {
       createOnce(_context) {
         return {
+          Program(_program) {},
           after() {
             throw new Error('Whoops!');
           },

apps/oxlint/test/fixtures/custom_plugin_lint_before_hook_error/plugin.ts

@@ -11,6 +11,7 @@ const plugin: Plugin = {
           before() {
             throw new Error('Whoops!');
           },
+          Program(_program) {},
         };
       },
     },

apps/oxlint/test/fixtures/definePlugin/output.snap.md

@@ -48,20 +48,6 @@
    : ^
    `----
 
-  x define-plugin-plugin(create-once-hooks-only): before hook:
-  | filename: files/1.js
-   ,-[files/1.js:1:1]
- 1 | let a, b;
-   : ^
-   `----
-
-  x define-plugin-plugin(create-once-hooks-only): after hook:
-  | filename: files/1.js
-   ,-[files/1.js:1:1]
- 1 | let a, b;
-   : ^
-   `----
-
   x define-plugin-plugin(create): ident visit fn "a":
   | filename: files/1.js
    ,-[files/1.js:1:5]
@@ -186,20 +172,6 @@
    : ^
    `----
 
-  x define-plugin-plugin(create-once-hooks-only): before hook:
-  | filename: files/2.js
-   ,-[files/2.js:1:1]
- 1 | let c, d;
-   : ^
-   `----
-
-  x define-plugin-plugin(create-once-hooks-only): after hook:
-  | filename: files/2.js
-   ,-[files/2.js:1:1]
- 1 | let c, d;
-   : ^
-   `----
-
   x define-plugin-plugin(create): ident visit fn "c":
   | filename: files/2.js
    ,-[files/2.js:1:5]
@@ -286,7 +258,7 @@
    :        ^
    `----
 
-Found 0 warnings and 39 errors.
+Found 0 warnings and 35 errors.
 Finished in Xms on 2 files using X threads.
 ```
 

apps/oxlint/test/fixtures/definePlugin/plugin.ts

@@ -187,6 +187,7 @@ const createOnceAfterOnlyRule: Rule = {
 const createOnceHooksOnlyRule: Rule = {
   createOnce(context) {
     return {
+      // Neither hook should be called, because no AST node visitor functions
       before() {
         context.report({
           message: 'before hook:\n' +

apps/oxlint/test/fixtures/definePlugin_and_defineRule/output.snap.md

@@ -48,20 +48,6 @@
    : ^
    `----
 
-  x define-plugin-and-rule-plugin(create-once-hooks-only): before hook:
-  | filename: files/1.js
-   ,-[files/1.js:1:1]
- 1 | let a, b;
-   : ^
-   `----
-
-  x define-plugin-and-rule-plugin(create-once-hooks-only): after hook:
-  | filename: files/1.js
-   ,-[files/1.js:1:1]
- 1 | let a, b;
-   : ^
-   `----
-
   x define-plugin-and-rule-plugin(create): ident visit fn "a":
   | filename: files/1.js
    ,-[files/1.js:1:5]
@@ -186,20 +172,6 @@
    : ^
    `----
 
-  x define-plugin-and-rule-plugin(create-once-hooks-only): before hook:
-  | filename: files/2.js
-   ,-[files/2.js:1:1]
- 1 | let c, d;
-   : ^
-   `----
-
-  x define-plugin-and-rule-plugin(create-once-hooks-only): after hook:
-  | filename: files/2.js
-   ,-[files/2.js:1:1]
- 1 | let c, d;
-   : ^
-   `----
-
   x define-plugin-and-rule-plugin(create): ident visit fn "c":
   | filename: files/2.js
    ,-[files/2.js:1:5]
@@ -286,7 +258,7 @@
    :        ^
    `----
 
-Found 0 warnings and 39 errors.
+Found 0 warnings and 35 errors.
 Finished in Xms on 2 files using X threads.
 ```
 

apps/oxlint/test/fixtures/definePlugin_and_defineRule/plugin.ts

@@ -186,6 +186,7 @@ const createOnceAfterOnlyRule = defineRule({
 const createOnceHooksOnlyRule = defineRule({
   createOnce(context) {
     return {
+      // Neither hook should be called, because no AST node visitor functions
       before() {
         context.report({
           message: 'before hook:\n' +