fix(es/minifier): Fix panic in bitwise logic and incorrect values #9258
Conversation
|
CI failed. Can you take a look? |
kdy1
changed the title
Head branch was pushed to by a user without write access
|
Not sure if that was the problem or if something else was wrong. Will take a look once it's done. |
Head branch was pushed to by a user without write access
lgtm, will update once this is merged |
CodSpeed Performance ReportMerging #9258 will not alter performanceComparing Summary
|
|
Will rebase properly shortly. Not sure how this is always so troublesome on my end lol |
|
Not sure if there's a better way to implement as_int32 and as_uint32, the current implementations are correct, test_fold_bit_shifts doesn't fold everything with the impl in #9251 as it doesn't overflow correctly in as_uint32 |
|
Some implementations are available for reference: In my opinion, leveraging the Rust standard library is optimal. Should there be any discrepancies from ECMAScript, we address those particular instances. The worst case would be to develop everything from scratch. |
I don't think Rust has a std function that behaves the same as the ECMAScript functions. I've seen the functions in V8 engine src before (like the ones you sent), the current impl follows ECMAScript spec. I guess we could implement one of those instead just to be sure. The only issue with trunc() from what I saw is that negative values don't overflow. e.g. |
| @@ -49,7 +45,8 @@ impl JsNumber { | |||
| return 0; | |||
| } | |||
|
|
|||
| self.0.trunc() as u32 | |||
| // pow(2, 32) = 4294967296 | |||
| self.0.trunc().rem_euclid(4294967296.0) as u32 | |||
There was a problem hiding this comment.
I've updated the implementation, and all the tests have passed.
Take a look to see if we need to add more test cases. @levi-nz
There was a problem hiding this comment.
lgtm, maybe we could see if v8 or another project has some test cases? https://github.com/v8/v8/blob/main/src/numbers/conversions.h
I think the current test cases are (probably) fine, as long as we're testing for overflowing, the rest should work fine
Description:
This PR fixes all issues listed in #9256 as well as the following:
1.7976931348623157e+308 << 1.7976931348623157e+308- swc currently does not transform this and leaves it as-isAnything involving low floating-point numbers, like
5e-324, such as5e-324 >> 5e-324,5e-324 >> 0etc - swc currently panics; https://play.swc.rs/?version=1.6.7&code=H4sIAAAAAAAAA0vTME3VNTYyUbCzUzDQBACkm%2Ft7DgAAAA%3D%3D&config=H4sIAAAAAAAAA32UO3LjMAyG%2B5zCozrFjostcoDtcgYOTYIyvXxoCNCxJuO7L0TJj40hdRI%2B%2FAAJgPh%2B2%2B26E5ruY%2FfNn%2Fwz6IJQ7v9swTGRvrClAxM1muIH6t5v9IQTcjogNNN1Jh3p0gM1Fe5%2F7feLogs5I9wUiy365N34nNPkOBRAfLKxlUPWCInwf%2F3CSv6aAJX6bD%2FkHECnDaI0Kp8IeihSYJND0AOCOusiRJlOqovHLKWYYCWwaih5EHmynnxOnPOVWtBWmWxBQL6AIX8GSca5WJaQryfcp2ELh9r3rc8%2F1HDWoWoScsKltYRPK0Q9Zo%2BkXE1SCWe4UoMZLsX9qfROFaBa0qvulH1a6clfAK5A0IhJR5DiNg%2FH87SmdptKnxyPLI0C5%2FmWbpmg56Iq751Q2akyUMhL3Sxgq4GpskY6zoJXyofeggLneFaE0PjlyRylpDQOkJ0AuL%2FaSVM1A3V%2FhSt8ehAb%2BA%2FfkuQBWzyipuM6xTEecthIEIGO2W44cCsor%2BPCW%2BIyrPOaLPBogBVdKjbwugT4AVBWoe3Ll9ng58ERVR%2Fy4bEmFofrfQ9HnfrHe59X8dvi0MVsa4PLkp%2F6O6%2Fm393D6baF7wfvPH7elC3p9R%2BoYzQdMAYAAA%3D%3DThis PR also fixes
shiftCountbeing incorrect and incorrect conversion betweenf64andi32/u32. A new API in swc_ecma_utils has been added,to_js_int32andto_js_uint32that allows external callers to convertf64s toi32andu32so they can perform bitwise operations themselves if they're trying to replicate JavaScript behaviour.I also believe the updated bit shifting logic should be accessible externally via swc_ecma_utils, but unsure about this.
Related issue: