Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect symbol resolution in tree_shaking/no_side_effects_in_initialization? #5455

Closed
overlookmotel opened this issue Sep 4, 2024 · 0 comments · Fixed by #5467
Closed
Assignees
Labels
A-linter Area - Linter A-semantic Area - Semantic C-bug Category - Bug

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Sep 4, 2024

I am fairly sure that symbol resolution in this rule is not correct for ExportSpecifier:

impl<'a> ListenerMap for ExportSpecifier<'a> {
fn report_effects(&self, options: &NodeListenerOptions) {
let ctx = options.ctx;
let symbol_table = ctx.symbols();
if has_comment_about_side_effect_check(self.exported.span(), ctx) {
let Some(name) = self.exported.identifier_name() else { return };
let Some(symbol_id) = options.ctx.symbols().get_symbol_id_from_name(name.as_str())
else {
return;
};

Background

I would like to remove the method SymbolTable::get_symbol_id_from_name, as in my opinion it's a footgun (#5456). We use it to get the SymbolId for a reference, but actually that's not what it's doing - it will give you the SymbolId of the first binding with this variable name anywhere in the AST.

They are not the same. e.g. in this case:

{
    let foo;
}
let bar = foo;

If we want to know if foo in let bar = foo; refers to a local variable, get_symbol_id_from_name("foo").is_some() says "yes it does". But that's wrong.

#5446 removes a usage of get_symbol_id_from_name and adds tests which demonstrate it was incorrect.

This rule

I'm pretty sure the usage of get_symbol_id_from_name in this rule is wrong too. Isn't the question we should be asking in this code "is the local a bound identifier?", not "is the exported a bound identifier?"

In the case of export {foo}, they're the same, but in case of export {foo as bar}, they're not.

However, I must admit this rule rather bends my brain! Can someone more familiar with this code advise?

@Boshen git "blames" you for this code. Can you help? But maybe @DonIsaac or @camc314 also may have an idea what this rule is about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter A-semantic Area - Semantic C-bug Category - Bug
Projects
None yet
2 participants