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

Variables unnecessarily renamed #6233

Closed
hiroshi-yamamoto-dublr opened this issue Oct 22, 2022 · 7 comments
Closed

Variables unnecessarily renamed #6233

hiroshi-yamamoto-dublr opened this issue Oct 22, 2022 · 7 comments
Labels
Milestone

Comments

@hiroshi-yamamoto-dublr
Copy link

hiroshi-yamamoto-dublr commented Oct 22, 2022

Describe the bug

Function parameters are unnecessarily renamed when a reference to an undefined variable is made, if that variable is the name of an enclosing object's field.

Input code

const vals = {
    x: () => 0,
    y: (x) => {
        console.log(x);
    },
    z: () => {
        console.log(x);
    },
};

SWC output:

const vals = {
    x: ()=>0,
    y: (x1)=>{
        console.log(x1);
    },
    z: ()=>{
        console.log(x);
    }
};

Note that the param of y is changed from x to x1, because x is not defined in z, but it exists as a field of the object.

Output from calling the functions:

> vals
{ x: [Function: x], y: [Function: y], z: [Function: z] }
> vals.x()
0
> vals.y()
undefined
undefined
> vals.z()
Uncaught ReferenceError: x is not defined
    at Object.z (REPL9:7:21)

Config

No response

Playground link

https://play.swc.rs/?version=1.3.9&code=H4sIAAAAAAAAA0vOzysuUShLzClWsFWo5lIAggorBQ1NBVs7BQMdML8SyK8AC0DkQSAZqC0%2FJ1UvJz8dKGcNFq%2BFKK%2BCaSeoutaaCwAleDfafwAAAA%3D%3D&config=H4sIAAAAAAAAA02OsQrDMAxE%2F0VzhpKhQ%2Bau%2FQjhKsHBtoykQI3xv9cOgXSTdO%2FuVGFXB0uFjKIkY9KSDL%2BwgJVM6sRngwlM%2B2nFoNT6grKRdYR0fsxzlwOz0gVMEH3yaxlhjmMWUr0lTFugvyjBpCtLHHSgDV15kWNB4%2F6NyUGtU5E%2Fx3DV86mz%2BAntLrrCvb4v8DT%2BAEx1eRDdAAAA

Expected behavior

x should not be renamed to x1 in the parameter of y

Actual behavior

x is renamed to x1 in the parameter of y

Version

No idea, latest used with Parcel

Additional context

No response

@kdy1
Copy link
Member

kdy1 commented Oct 22, 2022

It's by design.
I may change it in the future but I don't think it's an issue

@kdy1 kdy1 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 22, 2022
@hiroshi-yamamoto-dublr
Copy link
Author

It is an issue for my code. I use the names of function parameters to build a dataflow DAG, and when parameters are renamed, it breaks the graph. Only SWC does this, other tools do not.

@hiroshi-yamamoto-dublr
Copy link
Author

(basically I am using parameter names to inject values automatically into functions -- so the names cannot change)

@kdy1 kdy1 reopened this Oct 22, 2022
@kdy1 kdy1 changed the title Parameters unnecessarily renamed Variables unnecessarily renamed Oct 22, 2022
@hiroshi-yamamoto-dublr
Copy link
Author

I keep running into another issue with variables being renamed. If I copy code from "Sources" and paste it into the Javascript console in the browser, a lot of times the code won't run, because a variable has internally renamed to have a 1 suffix. This is extremely annoying.

The Javascript variable shadowing semantics are perfectly well-defined. Variables should never be renamed in SWC if they are properly and appropriately shadowing some other definition from a parent scope.

However this seems to even happen with locals in for-loops, for no apparent reason: for (const order of orderbook) { /* ... */ } renames order to order1, and I can't see why it happens other than the fact that the same type of loop with the same loop variable name occurs twice in the same function. There is no global that is even being shadowed in this case.

@arlyon
Copy link

arlyon commented Dec 23, 2022

I have a similar issue; consider this test case:

test!(
    ignore,
    Syntax::Typescript(TsConfig {
        tsx: true,
        ..Default::default()
    }),
    |_| as_folder(test_visitor(Engine::Emotion)),
    emotion,
    // Input codes
    r#"
import { TailwindStyle } from "stailwc";
export default function App({ Component, pageProps }) {
    return <>
        <TailwindStyle />
        <Component {...pageProps} />
    </>;
}
"#,
    // Output codes after transformed with plugin
    r#"
import { css, Global } from "@emotion/react";
export default function App({ Component, pageProps }) {
    return <>
        <Global styles={css``}/>
        <Component {...pageProps} />
    </>;
}
"#
);

This test passes, however swc in a later pass renames the imported Global to Global0, and preserves the naming on the jsx component, causing a reference error. Another similar test case:

test!(
    Syntax::Typescript(TsConfig {
        tsx: true,
        ..Default::default()
    }),
    |_| as_folder(test_visitor(Engine::StyledComponents)),
    styled_components,
    // Input codes
    r#"
import { TailwindStyle } from "stailwc";
export default function App({ Component, pageProps }) {
    return <>
        <TailwindStyle />
        <Component {...pageProps} />
    </>;
}
"#,
    // Output codes after transformed with plugin
    r#"
import { createGlobalStyle } from "styled-components";
export default function App({ Component, pageProps }) {
    return <>
        <Global />
        <Component {...pageProps} />
    </>;
}
const Global = createGlobalStyle(``);
"#
);

We transform the import, call createGlobalStyle, create a var Global, and try to use it, but similarly the declaration Global is renamed while the jsx is preserves and again causes a reference error.

Given the AST transformation in my plugin is 'correct', then why does a stage outside my control go and modify the way my plugin works? If that is indeed by design, then what is the recommended approach for the above cases? Is there a way that I can have my plugin opt out of the downstream AST transform stage?

I see a comment here that may make it possible to opt-out of renaming for specific identifiers (#5068 (comment))

The first case can be fixed with the following 'fix' however the second case where we are defining a new variable altogether has no workaround. Note that we also need to 'use' the variables because I believe the SWC unused variable pass happens before my plugin is invoked.

// currently needed due to a swc bug
// ===
import { css, Global } from "@emotion/react";
css;
Global;
// ===

@kdy1
Copy link
Member

kdy1 commented Jan 10, 2023

Closing as fixed by #6670

@kdy1 kdy1 closed this as completed Jan 10, 2023
@kdy1 kdy1 modified the milestones: Planned, v1.3.26 Jan 11, 2023
@swc-bot
Copy link
Collaborator

swc-bot commented Feb 10, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

4 participants