perf(semantic)!: remove SymbolTable::spans field

[overlookmotel]

Remove spans field from SymbolTable. It's unnecessary because you can get the Span from the AstNodeId, and these lookups only happen in the linter when generating diagnostics (i.e. cold path).

Lots of tests failing. I believe this isn't due to this PR being incorrect, but that it's surfacing existing bugs where AstNodeId for a lot of symbols is set incorrectly.

@DonIsaac Are these the same bugs you've found already? Or new ones?

[overlookmotel]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• perf(semantic)!: remove SymbolTable::spans field #4473 👈
• perf(linter): no_shadow_restricted_names only look up name in hashmap once #4472
• refactor(semantic): methods take Span as param, not &Span #4470
• perf(semantic): give AstNodeId a niche #4469
• refactor(syntax): give ScopeId a niche #4468
• refactor(syntax): use NonMaxU32 for IDs #4467
• fix(syntax): fix unsound use of NonZeroU32 #4466
• perf(linter): avoid unnecessary work in nextjs:no_duplicate_head rule #4465
• perf(semantic): remove span field from Reference #4464
• perf(semantic): reduce storage size for symbol redeclarations #4463
• refactor(semantic): populate declarations field in SymbolTable::create_symbol #4461
• fix(transformer): add to SymbolTable::declarations for all symbols #4460
• refactor(semantic): re-order SymbolTable fields #4459
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite

[DonIsaac]

I need this API for no no-unused-vars for reporting the locations of unused symbols. I believe other rules rely on it as well. It provides a much higher-quality Span then the one provided by the declaration AstNode. Please do not remove it.

[codspeed-hq]

CodSpeed Performance Report

Merging #4473 will not alter performance

_{Comparing 07-25-perf_semantic_remove_symboltable_spans_field (16c2ec4) with main (81384f5)}

Summary

✅ 32 untouched benchmarks

[overlookmotel]

@DonIsaac Shouldn't the AST node that a symbol's AstNodeId points to be the BindingIdentifier? In which case, the 2 spans should be equivalent. Intent here was not to remove any functionality or information, just to use a more efficient means of storing/accessing that information.

You can still get the Span of a symbol via semantic.symbol_span(symbol_id) (previously semantic.symbols. get_span(symbol_id)).

I had assumed the problem here causing the test failures was that the AstNodeIds set for symbols were incorrect, and that if that was fixed, nothing would change. Have I misunderstood?

[DonIsaac]

@overlookmotel I'm not sure, I just trust getting spans from AST node declarations less than symbol spans. There's some funkiness going on with getting declaration nodes for function expressions. Mind if we wait until #4445 is merged first, then see how this PR affects spans it reports?

[Dunqing]

Same as #4464 (comment)

[Boshen]

Closing due to unsure if this is going to complicate other features. And the benchmark shows no performance improvement.

[overlookmotel]

Well 1-2% improvement on semantic benchmarks isn't nothing. But, yes, I agree we should close for now.

@Boshen can you answer one question though? What AST nodes are SymbolTable::declarations meant to point to? The BindingIdentifier for the binding? Or its parent? Currently it appears to be the latter, and I was assuming that's a bug that's crept in (and is what caused all the test failures on this PR).

[overlookmotel]

Ignore that question. #4739 explains the problem.