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

error in ** => Math.pow(bigInt, bigInt) #2626

Closed
LongTengDao opened this issue Jan 2, 2019 · 4 comments · Fixed by #2640
Closed

error in ** => Math.pow(bigInt, bigInt) #2626

LongTengDao opened this issue Jan 2, 2019 · 4 comments · Fixed by #2640

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented Jan 2, 2019

  • Rollup Version: 1.0.0
  • Operating System (or Browser): No relationship.
  • Node Version: 10.9.0

How Do We Reproduce?

input.js:

const max = 2n**64n;
const min = -max;
export default function isSafe (int) {
  return min<=int && int<=max;
};

Expected Behavior

Actual Behavior

TypeError: Cannot convert a BigInt value to a number
    at Math.pow (<anonymous>)
    at ** (/rollup/dist/rollup.js:11055:90)
    at BinaryExpression.getLiteralValueAtPath (/rollup/dist/rollup.js:11076:16)
    at LocalVariable.getLiteralValueAtPath (/rollup/dist/rollup.js:2626:26)
    at Identifier$$1.getLiteralValueAtPath (/rollup/dist/rollup.js:11185:34)
    at UnaryExpression.getLiteralValueAtPath (/rollup/dist/rollup.js:13362:43)
    at LocalVariable.getLiteralValueAtPath (/rollup/dist/rollup.js:2626:26)
    at Identifier$$1.getLiteralValueAtPath (/rollup/dist/rollup.js:11185:34)
    at BinaryExpression.getLiteralValueAtPath (/rollup/dist/rollup.js:11067:35)
    at LogicalExpression.analyseBranchResolution (/rollup/dist/rollup.js:12208:35)

The problem code is in repo/src/ast/nodes/BinaryExpression.ts:

31	'**': (left: any, right: any) => Math.pow(left, right),

I tried to fix that and make a PR, but with difficult:

(1) Change Math.pow(left, right) to left ** right is best, but I have no idea about why here is Math.pow, maybe for old version Node.js.

(2) Replacing with this can fix the error too:

31	'**': (left: any, right: any) => {
32		if ( typeof left==='bigint' && typeof right==='bigint' ) {
33			if ( right<0 ) { throw new RangeError('Exponent must be positive'); }
34			if ( !right ) { return ++right; }
35			let result: any = left;
36			while ( --right ) result *= left;
37			return result;
38		}
39		return Math.pow(left, right);
40	},

But I don't know how to add test for this bugfix in repo rollup.

(3) I tried **=, that will not be computed so there is no problem now, but maybe in somewhere it need to be fix as the same. I'm not sure.

So it still need other one who can manage these to fix that. Thanks!

@lukastaegert
Copy link
Member

Yes, Math.pow is used for compatibility with older versions. My feeling is we should just return UNKNOWN_VALUE, shouldn't be a big loss in tree-shaking quality unless someone explicitly compares results involving the ** operator. Once we raise the minimum node version of rollup high enough so that the ** operator is supported on all platforms, we can switch to using the actual implementation.

I am not a fan of adding a bigint specific polyfill at this point until we have official bigint support in acorn and even then maintaining this hand-crafted algorithm would require quite a few tests.

@lukastaegert
Copy link
Member

Also note that your algorithm still relies on bigint support on the system where rollup is run which is definitely not guaranteed.

@lukastaegert
Copy link
Member

Fix at #2640

@LongTengDao
Copy link
Contributor Author

I am not a fan of adding a bigint specific polyfill at this point until we have official bigint support in acorn and even then maintaining this hand-crafted algorithm would require quite a few tests.

Also note that your algorithm still relies on bigint support on the system where rollup is run which is definitely not guaranteed.

Sure. Thank you! I will wait the fix version release and try.

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

Successfully merging a pull request may close this issue.

2 participants