Skip to content

createTempVariable() should also try to not shadowing upper lexical environment temp variables #40826

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

Closed
Jack-Works opened this issue Sep 29, 2020 · 5 comments · Fixed by #43083
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@Jack-Works
Copy link
Contributor

TypeScript Version: 3.9.3

Search Terms:

Code

I'm using createTempVariable but I found it is buggy.

The buggy version: Jack-Works/react@792a7f7#diff-6624517c3255932b3079f651161b7df0

Fixed version:

Jack-Works/react@4ae646b#diff-6624517c3255932b3079f651161b7df0

Reproduction:

const ts = require('typescript')
const t = require('./ReactFreshTypeScriptTransformer.js')

console.log(
    ts.transpileModule(
        `
import { useDefaultWallet } from '../../plugins/Wallet/hooks/useWallet'

/**
 * Get the address of the default wallet
 */
export function useAccount() {
    const wallet = useDefaultWallet()
    return wallet?.address ?? ''
}
`,
        {
            transformers: { before: [t()] },
            compilerOptions: { target: ts.ScriptTarget.ES2017 },
        }
    ).outputText
)

The buggy result using createTempVariable():

var _a;
_a = $RefreshSig$();
import { useDefaultWallet } from '../../plugins/Wallet/hooks/useWallet';
/**
 * Get the address of the default wallet
 */
export function useAccount() {
    var _a;
    _a();
    const wallet = useDefaultWallet();
    return (_a = wallet === null || wallet === void 0 ? void 0 : wallet.address) !== null && _a !== void 0 ? _a : '';        
}
_a(useAccount, "Zqcse9ORXvHAGVGDGSjWLnVdvA4=", false, () => [useDefaultWallet]);

As you can see, downleveling ?? syntax create a new variable also called "_a" which shadows my upper variable "_a", then _a() leads to an runtime error.

The correct version using createFileLevelUniqueName():

var $c0;
$c0 = $RefreshSig$();
import { useDefaultWallet } from '../../plugins/Wallet/hooks/useWallet';
/**
 * Get the address of the default wallet
 */
export function useAccount() {
    var _a;
    $c0();
    const wallet = useDefaultWallet();
    return (_a = wallet === null || wallet === void 0 ? void 0 : wallet.address) !== null && _a !== void 0 ? _a : '';
}
$c0(useAccount, "Zqcse9ORXvHAGVGDGSjWLnVdvA4=", false, () => [useDefaultWallet]);
@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Sep 29, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.2.0 milestone Sep 29, 2020
@Jack-Works
Copy link
Contributor Author

If createTempVariable can accept this enum, this problem will be resolved!

export enum GeneratedIdentifierFlags {
        None = 0,
        ReservedInNestedScopes = 8,
        Optimistic = 16,
        FileLevel = 32,
        AllowNameSubstitution = 64
    }

@Jack-Works
Copy link
Contributor Author

Exposed: (4.1.0 beta) signature

createTempVariable(recordTempVariable: ((node: Identifier) => void) | undefined): Identifier

Signature in source code:

createTempVariable(recordTempVariable: ((node: Identifier) => void) | undefined, reservedInNestedScopes?: boolean): GeneratedIdentifier

@Jack-Works
Copy link
Contributor Author

Please explosure those signatures! It's very useful when transforming code

@rbuckton
Copy link
Contributor

In the meantime, is factory.createUniqueName an option?

@Jack-Works
Copy link
Contributor Author

In the meantime, is factory.createUniqueName an option?

I don't know, but expose reservedInNestedScopes?: boolean will be very useful

@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Mar 4, 2021
@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants