Linter: remove `run_on_symbol`?

[camchenry]

Currently, we only have 7 lint rules that implement the run_on_symbol function: https://github.com/search?q=repo%3Aoxc-project%2Foxc%20%22fn%20run_on_symbol%22&type=code. That's about 1.1% of all rules currently (out of 607 total rules). This made me question: do we really need to implement an entirely new type of run function just for a few rules?

Some reasons to consider removing run_on_symbol:

• Less binary code generated, since we currently have a large run_on_symbol with a match for every rule added:

  oxc/crates/oxc_macros/src/declare_all_lint_rules.rs

  Lines 153 to 157 in 9e54002

  	pub(super) fn run_on_symbol<'a>(&self, symbol_id: SymbolId, ctx: &LintContext<'a>) {
  	match self {
  	#(Self::#struct_names(rule) => rule.run_on_symbol(symbol_id, ctx)),*
  	}
  	}

• Most symbol-based rules actually end up needing to look at node type data anyway, so it can be rewritten as either a run_once that loops over symbols, or a run that looks up symbol information. Implementing only the run function has the advantage of enabling skipping running rules based on node types (see Linter: Reduce calls to Rule::run backlog#167)'
• Less code to maintain. We only need to support a few ways of running lint rules and can focus more on making it easy to work with symbols and nodes.

Some reasons to consider not removing run_on_symbol:

• The few rules that do implement run_on_symbol are more concisely expressed as being symbol-based. For example, checking for redeclarations of symbols and shadowing of variables is a natural fit for this run function.
• It could be slower if we end up iterating over the symbol table multiple times, as each rule does its own iteration over symbols, as opposed to if we keep the current single iteration of symbols and then call into each rule.
  • Note: It might also be faster! I've had unintuitive results with changing the order of iterations with respect to nested loops and rules causing performance uplifts much greater than I would expect. So this is worth benchmarking. (For example, see: perf(linter): apply small file optimization, up to 30% faster #6600)

Of the few remaining rules that do implement run_on_symbol, this is how I think we could reimplement them:

• no-class-assign: implement run and look for AstKind::Class nodes, then search for all references to that class name. (see perf(linter): run no-class-assign on class nodes instead of symbols #14578)
• no-redeclare: implement run_once and loop over symbol table
• nextjs/no-duplicate-head:
  • implement run and look for AstKind::JSXOpeningElement. If we find one called Head, then search the symbol table for any other references.
  • or, implement run_once and manually loop over symbols.
• no-unused-vars: implement run_once and manually loop over symbols
• no-shadow-restricted-names: implement run_once and manually loop over symbols.
• no-const-assign: implement run and look for VariableDeclarator, or ArrayPattern and ObjectPattern. or, implement run_once and manually loop over symbols.
• no-param-reassign: implement run and look for AstKind::FormalParameter, then search symbols for references to parameter.

[Boshen]

We can remove it. It was added by another contributor for convenience anticipating a lot of rules will use it ... apparently not.

[camchenry]

Completed in #14610.

[overlookmotel]

@camchenry I'm late to this party, but it just occurred to me:

Only BindingIdentifier has a SymbolId, so can't any rule using run_on_symbol be converted to a run rule that starts with let AstKind::BindingIdentifier(ident) = node.kind() else { return };? They'd then get skipped for all nodes except BindingIdentifiers by the infra you've already built.

The reimplementation of no-class-assign you've done is even better. It's faster to match on less common types, and Class is way less common than BindingIdentifier. But for some of the other rules, maybe matching on BindingIdentifiers is better than looping through all symbols in run_once?