immutable-js-oss / immutable-js

Immutable persistent data collections for Javascript which increase efficiency and simplicity.
https://immutable-js-oss.github.io/immutable-js/
MIT License
37 stars 6 forks source link

Incorrect OrderedSet.subtract result when 32+ elements #139

Closed Methuselah96 closed 3 years ago

Methuselah96 commented 3 years ago

From @pixmaster on Thu, 13 Jun 2019 11:46:48 GMT

What happened

Incorrect subtract result when Set contains 32+ elements and subtracted elements is n/2+1

How to reproduce

Check 2 cases when capacity is less than 32 and equal or more than 32

const { OrderedSet } = require('immutable@4.0.0-rc.12');
const capacity = 32;
const allItems = OrderedSet(Array(capacity).fill(1).map((el, i) => i + 1));
const someOfThem = Array(Math.ceil(capacity / 2) + 1).fill(1).map((el, i) => i + 1);
const existingItems = OrderedSet(someOfThem).intersect(allItems);

expect(allItems.subtract(existingItems).size+someOfThem.length).toBe(allItems.size);

Copied from original issue: https://github.com/immutable-js/immutable-js/issues/1716

Methuselah96 commented 3 years ago

Possibly related: https://github.com/immutable-js-oss/immutable-js/pull/4

jdeniau commented 3 years ago

I am willing to investigate on that, but I don't understand the original issue.

I tried to copy the code and "it works". I do not understand what "Check 2 cases" is in the original issue. 🤔

@Methuselah96 Do you have an hint or is this bug fixed ?

Methuselah96 commented 3 years ago

@jdeniau Here's a reproduction of the issue: https://codesandbox.io/s/elegant-saha-edmb7?file=/src/index.js

Notice that the the two console.log statements are outputting different numbers. They should both be 32.

Methuselah96 commented 3 years ago

@jdeniau It's also worth keeping https://github.com/immutable-js-oss/immutable-js/pull/4 in mind because they might be related or a similar issue.

jdeniau commented 3 years ago

@Methuselah96 Well in this case, this issue has somehow been fixed. I added only tests cases in #198

In your codesandbox, allItems.subtract(existingItems).size is 32, but in the tests on master, it is 15 (which is expected).