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

Map get fails on BigInt key #577

Closed
dckc opened this issue Feb 23, 2021 · 2 comments
Closed

Map get fails on BigInt key #577

dckc opened this issue Feb 23, 2021 · 2 comments

Comments

@dckc
Copy link
Contributor

dckc commented Feb 23, 2021

Steps to Reproduce

  1. xst -e 'const store = new Map([[1n, "abc"]]); print(store.get(1n))'
  2. See undefined

Expected behavior
print abc

Other information

I thought maybe a Map's key had to be an Object, but that's WeakMap. "A Map's keys can be any value (including functions, objects, or any primitive)." -- MDN

I checked the XS conformance docs and didn't find anything about this. That suggests test262 doesn't cover this. odd, that.

node and browser consoles both show...

> const store = new Map([[1n, "abc"]]); store.get(1n)
'abc'

context:

As part of Agoric/agoric-sdk#2486 , I'm running zoe/test/unitTests/test-fakePriceAuthority.js and the assertion at store.js line 35 is failing.

cc @erights @katelynsills

Build environment: linux
Target device: xst, xsnap, agoric

@phoddie
Copy link
Collaborator

phoddie commented Feb 24, 2021

Nice find. It looks like test262 doesn't have a case for this.

It looks pretty easy to add. In fxTestEntry add this after the reference case:

		else if ((XS_BIGINT_KIND == a->kind) || (XS_BIGINT_X_KIND == a->kind))
			result = gxTypeBigInt.compare(the, 0, 1, 0, a, b);

That's enough to make it work, but all the BigInt values will share a hash table entry which is eventually slow. So, in fxSumEntry add this after the reference case:

		else if ((XS_BIGINT_KIND == kind) || (XS_BIGINT_X_KIND == kind)) {
			address = (txU1*)slot->value.bigint.data;
			size = slot->value.bigint.size;
		}

That seems to work in quick testing. I'll ask Patrick to review and recheck with with test262 (just in case) before we integrate this.

mkellner pushed a commit that referenced this issue Mar 2, 2021
@dckc
Copy link
Contributor Author

dckc commented Mar 2, 2021

fix confirmed.

@dckc dckc closed this as completed Mar 2, 2021
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

2 participants