fix(semantic): get_symbol_id_from_span not searching symbol redeclarations

[valeneiko]

Fixes #6689

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[codspeed-hq]

CodSpeed Performance Report

Merging #6701 will not alter performance

_{Comparing valeneiko:fix-6689 (38a22e5) with main (01a35bb)}

Summary

✅ 30 untouched benchmarks

[Dunqing]

Are there any situations where we need to use this method?

The method looks a little complicated and not very efficient. I have checked our codebase only three places used this method and it looks like we can replace them.

To remove this method, we need to add SymbolId to ModuleRecord's ImportEntry struct. Is it possible? @Boshen

[overlookmotel]

Agreed. We generally consider it an anti-pattern to use Spans as unique identifiers. It's possible for 2 nodes to have the same Span (e.g. ExpressionStatement and the Expression it wraps), and after AST has been through transformer, even 2 nodes of the same type can have the same Span, and freshly-created nodes have no real Span at all (start 0, end 0).

@valeneiko Can I ask: what is your use case for get_symbol_id_from_span?

[valeneiko]

@overlookmotel
I am building a whole-program dependency graph between symbols E.g.:

• const a = b+c; - Symbol a depends on b and c
• function foo() { return bar(); } - Symbol foo depends on bar

I am doing it by iterating over symbol references, for each reference finding the ancestor declaration in root scope and resolving that declaration's AstNode into a Symbol.

get_symbol_id_from_span only works with spans of names already. So I could just do span.source_text(src) and use get_symbol_by_name. Or if you know of a better way to do all this, I am open to suggestions.

For now the goal was to just explore what can be done with such a graph.

@Dunqing as for the perf of this specific PR. I think it is fine, because symbol redeclarations are rare, so in reality that whole method will most of the time be an empty iterator and occasionally iterate over very few items. As for the get_symbol_id_from_span itself, yes it could be improved, or removed if a better alternative exists.