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

fix(es/base): Use seperate mark for FnExpr Ident #5959

Merged
merged 1 commit into from Sep 27, 2022

Conversation

Austaras
Copy link
Member

Description:

BREAKING CHANGE:

Related issue (if exists):

(function f(f) {
f = b;
f[b] = 0;
(function f(f1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know why this is renamed -- Could you help @kdy1 ?

Copy link
Member

Choose a reason for hiding this comment

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

Because function name (f) and the parameter (f1) is now in a different scope

Copy link
Member

Choose a reason for hiding this comment

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

Previously those were treated as identical to each other

Copy link
Member

Choose a reason for hiding this comment

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

As there's ( in front of function, this is fn expr, not fn decl

Copy link
Member Author

Choose a reason for hiding this comment

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

But why do other colliding ident not got renamed?

Copy link
Member

Choose a reason for hiding this comment

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

At where? We reuse variable names in the name mangler, and we only rename declared identifiers in normal var renamer

Copy link
Member Author

Choose a reason for hiding this comment

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

function foo(foo) {}

would remain the same after renamer.

Copy link
Member

Choose a reason for hiding this comment

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

We are using same analyer for minifier and normal var renamer, and the analyzer knows that you can't refer foo (function) from in foo

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... you are right I'm not sure why is this renamed.

Copy link
Member

Choose a reason for hiding this comment

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

I think

fn visit_fn_decl(&mut self, f: &FnDecl) {
self.add_decl(f.ident.to_id());
self.with_scope(|v| {
f.function.visit_with(v);
})
}
fn visit_fn_expr(&mut self, f: &FnExpr) {
self.with_scope(|v| {
if let Some(id) = &f.ident {
v.add_decl(id.to_id());
}
f.function.visit_with(v);
})
}
is the issue

kdy1
kdy1 previously approved these changes Sep 27, 2022
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Thanks, although I think you need to update more tests.


swc-bump:

  • swc_ecma_transforms_base

@kdy1 kdy1 self-assigned this Sep 27, 2022
@kdy1 kdy1 added this to the Planned milestone Sep 27, 2022
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

I think there are room for improvement, but let's split PR.

(function f(f) {
f = b;
f[b] = 0;
(function f(f1) {
Copy link
Member

Choose a reason for hiding this comment

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

We are using same analyer for minifier and normal var renamer, and the analyzer knows that you can't refer foo (function) from in foo

(function f(f) {
f = b;
f[b] = 0;
(function f(f1) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... you are right I'm not sure why is this renamed.

(function f(f) {
f = b;
f[b] = 0;
(function f(f1) {
Copy link
Member

Choose a reason for hiding this comment

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

I think

fn visit_fn_decl(&mut self, f: &FnDecl) {
self.add_decl(f.ident.to_id());
self.with_scope(|v| {
f.function.visit_with(v);
})
}
fn visit_fn_expr(&mut self, f: &FnExpr) {
self.with_scope(|v| {
if let Some(id) = &f.ident {
v.add_decl(id.to_id());
}
f.function.visit_with(v);
})
}
is the issue

@kdy1 kdy1 merged commit 573418f into swc-project:main Sep 27, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.3.4 Sep 30, 2022
@Austaras Austaras deleted the resolver branch October 16, 2022 12:50
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants