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

Unstable names when alwaysStrict is true #2537

Closed
eelco opened this issue Sep 10, 2022 · 15 comments
Closed

Unstable names when alwaysStrict is true #2537

eelco opened this issue Sep 10, 2022 · 15 comments

Comments

@eelco
Copy link
Contributor

eelco commented Sep 10, 2022

We’re experiencing unstable names (resulting in different hashes) when we enable alwaysStrict in the TypeScript configuration.

We’re currently using version 0.14.45, but I’ve also reproduced it with the latest (0.15.7). The oldest version I can reproduce it with is 0.14.44. (Not unexpectedly, because that was the version that introduced the parsing of the option.)

I have not reproduced the issue in a minimal example, but I’m happy to provide access to the codebase or get instructions on how to help debugging this.

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

I’ve scanned the diff of the release, but can’t figure out how this could happen.

One more interesting note perhaps is that I’m only able to reproduce the issue on a Linux (x86) system, not on macOS (arm).

@evanw
Copy link
Owner

evanw commented Sep 10, 2022

That's unfortunate. Sorry that it's happening. I'm not already aware of the issue you're describing and nothing comes to mind for what might be causing this, so it would be helpful to provide a way to reproduce this. Let me know how you want to provide access (if that's something you can do).

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

Thanks! One more odd detail: it is not fully deterministic. We found it because we had a bug in our build process doing double builds, and some of them did have equal hashes. Likely it is happening in a parallelized part of the process?

I will spent some time reducing the code involved to see if I can get to a smaller reproduction that I can share.

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

I found a (pretty) minimal reproduction! It seems it’s due to an odd pattern in our code, using conditional inner functions.

This code

export function aap(noot: boolean, wim: number) {

    let mies = "teun"

    if (noot) {
        function vuur(v: number) {
            return v * 2
        }

        function schaap(s: number) {
            return s / 2
        }

        mies = vuur(wim) + schaap(wim)
    }

    return mies
}

Compiles to

"use strict";function b(o,e){let r="teun";if(o){let u=function(n){return n*2},t=function(n){return n/2};var a=u,i=t;r=u(e)+t(e)}return r}export{b as aap};

OR

"use strict";function b(o,e){let r="teun";if(o){let u=function(n){return n*2},t=function(n){return n/2};var i=u,a=t;r=u(e)+t(e)}return r}export{b as aap};

Note it’s the naming of the inner functions that is different (a=u,i=t vs i=u,a=t)

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

Also verified it’s not something else in our config. Assuming the file about is called test.ts and I can reproduce with a tsconfig.json file that has

{
    "compilerOptions": {
	    "alwaysStrict": true
    }
}

Using the command:

esbuild --tsconfig=tsconfig.json --minify test.ts

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

I can now also reproduce on macOS btw, but it seems that one naming is favored slightly over the other.

The reason that this does not happen with alwaysStrict off, is that the additional vars are not created in that case:

export function aap(r,u){let e="teun";if(r){let t=function(n){return n*2},o=function(n){return n/2};e=t(u)+o(u)}return e}

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

Oh, I’m only noticing now that the additional vars are actually fully unused! They get assigned, but the functions are called by their original name: var i=u,a=t;r=u(e)+t(e)

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

Assuming the bug can be fully fixed by removing the superfluous var generation, the reproduction can be made smaller

function aap(noot) {
    if (noot) {
        function mies() {}
        mies()
    }
}

@eelco
Copy link
Contributor Author

eelco commented Sep 10, 2022

Ah, seems this is related to (the behavior as explained in) #1552

@evanw
Copy link
Owner

evanw commented Sep 11, 2022

Thank you for this excellent reproduction test case. I wasn't expecting such a small one, which makes my job much easier. I can reproduce the issue and have found the problem (iteration over an unordered map). The fix will be in the next release.

@evanw evanw closed this as completed in 3e2374c Sep 11, 2022
@eelco
Copy link
Contributor Author

eelco commented Sep 11, 2022

Also was not expecting such a small case, but is was a nice puzzle to find it! 😊

Thank you so much for the fast turnaround on this issue, looking forward to the release.

@evanw
Copy link
Owner

evanw commented Sep 12, 2022

looking forward to the release

Heads up that I'm currently traveling and it may be a week or two before I have internet that's stable enough to publish a new release.

@eelco
Copy link
Contributor Author

eelco commented Sep 12, 2022

No worries, enjoy your travels!

@eoghanmurray
Copy link

Was this fixed in a released version? Which version?
With thanks!

@evanw
Copy link
Owner

evanw commented Jul 13, 2023

If I click on the commit that closed this issue, GitHub tells me that this was released in version 0.15.8:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants