• 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

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

@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?

@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?

Same as #4464 (comment)

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

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

Ignore that question. #4739 explains the problem.