Skip to content

fix(semantic): Set all exported declaration variables with the SymbolFlags::Export flag #7344

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix(semantic): Set all exported declaration variables with the SymbolFlags::Export flag #7344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64204d4
fix(semantic): Set all exported declaration variables with the `Symbo…
magic-akari Nov 18, 2024
c24f9a6
chore: update test case
magic-akari Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions crates/oxc_semantic/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::{
sync::Arc,
};

use oxc_ecmascript::BoundNames;
use rustc_hash::FxHashMap;

use oxc_ast::{ast::*, AstKind, Visit};
Expand Down Expand Up @@ -580,6 +581,12 @@ impl<'a> SemanticBuilder<'a> {
}
}
}

if let Some(decl) = &decl.declaration {
decl.bound_names(&mut |ident| {
self.add_export_flag_to_identifier(ident.name.as_str());
});
}
Comment on lines +585 to +589
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this way, not only handle VariableDeclaration but also handle FunctionDeclaration and other declarations, so we may need to remove the repeated logic we had written to handle other declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you prefer to handle VariableDeclarations here only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you prefer to handle all exports here (leave program), removing other code logic for handling exports?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to handle all export declarations here because the previous way we need to avoid adding SymbolFlags::Export to the wrong symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote: Permission to oxc-project/oxc.git denied to magic-akari.
fatal: unable to access 'https://github.com/oxc-project/oxc.git/': The requested URL returned error: 403

😂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dunqing I attempted to relocate some code but encountered strange problems.
Directly moving the code from add_export_flag_to_export_identifiers (the AstKind::Program case of leave_kind) to leave_kind is not equivalent.

I successfully moved AstKind::ExportNamedDeclaration, but when I tried to move AstKind::ExportDefaultDeclaration, the test cases failed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been a long time since I last modified this part code, and I need to get reacquainted with it. BTW, our team wants to fix this issue as soon as possible, I will make edits directly in this PR to avoid communication costs. I hope you don’t mind!

}
}
}
Expand Down
13 changes: 13 additions & 0 deletions crates/oxc_semantic/tests/integration/modules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,16 @@ fn test_import_type() {
.contains_flags(SymbolFlags::TypeImport)
.test();
}

#[test]
fn test_multiple_export_var() {
let test = SemanticTester::js(
"
export var x = 1, y = 2, [z] = [3], { a: b } = { a: 4 };
",
);
test.has_some_symbol("x").contains_flags(SymbolFlags::Export).test();
test.has_some_symbol("y").contains_flags(SymbolFlags::Export).test();
test.has_some_symbol("z").contains_flags(SymbolFlags::Export).test();
test.has_some_symbol("b").contains_flags(SymbolFlags::Export).test();
}
Loading