isi-vista / immutablecollections

A library for immutable collections, in the spirit of Guava's Immutable Collections.
MIT License
3 stars 2 forks source link

ImmutableDict may fail to maintain iteration order on non-CPython implementations of Python 3.6 #52

Open gabbard opened 5 years ago

gabbard commented 5 years ago

51 made me notice our current code loses the ImmutableDict ordering guarantee on any 3.6 implementations which do not guarantee deterministic dict ordering in general because if we initialize from an Iterable of key-value pairs, we eventualy just pass the iterable directly to dict.

My inclination is to crash on module load in the same circumstances we currently crash when trying to initialize from a dict. That will prevent inadvertent bugs. If someone needs to run on a non-CPython 3.6 implementation, they can add the alternate implementation which maintains ordering. @ConstantineLignos ?

tag @berquist

berquist commented 5 years ago

Is it as simple as changing https://github.com/isi-vista/immutablecollections/blob/6985a25ffcd34bccc59ed423aceef257cdc681f5/immutablecollections/_immutabledict.py#L59 to

if not DICT_ITERATION_IS_DETERMINISTIC

? There should probably be a keyword argument to bypass this check (like for ImmutableSet).

gabbard commented 5 years ago

@berquist : Aha, yes, you are correct (on both counts)

ConstantineLignos commented 5 years ago

This all sounds good, just make sure that if that if is changed the error messages change accordingly.

ConstantineLignos commented 5 years ago

Wait, maybe I'm missing something, isn't this entire package supposed to be 3.6+? From setup.py:

      # 3.6 and up, but not Python 4
      python_requires='~=3.6',
ConstantineLignos commented 5 years ago

Sorry, I think was mixed up by the title. @rgabbard am I right that the issue is not about "Python < 3.6" as the title says, but only for non-CPython 3.6 where dicts are not guaranteed to have deterministic iteration order?

gabbard commented 5 years ago

@ConstantineLignos : Correct - apologies for the incorrect title