Preserve insertion order in maps that are traversed

[fmeum]

Consumers of an Immutable{List,Map,Set} cannot know whether it has been created from a source with unspecified iteration order, such as HashMap. Since ImmutableMap itself prescribes that it preserves insertion order, it's reasonable for consumers to expect a specified, reasonable, deterministic order. While the current implementation of HashMap in the OpenJDK still results in a deterministic order, this kind of "sequenced-washing" doesn't cause non-determinism, but it still runs the risk of having the order of elements change between JDK versions or vendors.

The locations changed in this PR have been identified by instrumenting the factory methods of the Immutable{List,Map,Set} classes to track whether they 1) have been created from an unsequenced collection and 2) are iterated over. I then manually picked out those cases in which I couldn't rule out that a change in order would be observable to users.

Related to google/guava#6903 (comment)

[katre]

In general, I support this change and think this is worthwhile. However, I am worried about backsliding. What can we do to keep new uses of non-SequencedMap implementations from being added, or is this not a concern?

[fmeum]

It's a real concern and a proper fix would likely require having Immutable* implement Sequenced*, which may be far away depending on some low-level Java tooling question raised in the linked Guava issue.

[katre]

I see, thanks. I looked at the linked Guava thread and it does look difficult.

It looks like some of the current test failures are real, so I will hold off further review until those are addressed.

[Wyverald]

LGTM! Thanks for doing this.

Though I wonder if sometimes what we need is an ImmutableMap that semantically does not guarantee consistent iteration ordering, and then an ImmutableSequencedMap that does. (Several places in this PR feel like they really want an immutable map but don't care about the order at all; but they have to "care" now because they're offering an ImmutableMap.)

[fmeum]

Though I wonder if sometimes what we need is an ImmutableMap that semantically does not guarantee consistent iteration ordering, and then an ImmutableSequencedMap that does. (Several places in this PR feel like they really want an immutable map but don't care about the order at all; but they have to "care" now because they're offering an ImmutableMap.)

That could be an alternative for Guava. A new class would also resolve the concerns around adopting SequencedMap early. FYI @cpovirk

[cpovirk]

While I see the logic (and the implementation could intentionally shuffle order to prevent people from depending on it!), I think it's unlikely that we'd do it.

Inside Google, we have a JDK patch that shuffles the order of plain old HashMap (and I think ConcurrentHashMap). Those aren't the only source of unordered inputs, of course, and I think the randomization still doesn't cover every single case, but it does a pretty good job if there is some test that fails as a result of a change in ordering.

That's seemed to hit a pretty good spot in the cost-benefit space. I can definitely see how the ability to actively request randomization could be useful, especially outside Google. But I figure:

• Users have a limited amount of attention to spend on the problem. If we can get across to them that hash ordering is random and that they can fix it with types like ImmutableMap and LinkedHashMap, I think that's a good outcome, and if we drive even a small fraction of users away by saying "Or you can actively randomize it...," then that may make things worse on net.
• At this point, I assume that we have various code that "expects" ImmutableMap in various senses—APIs that require it (though that should be uncommon), static analyzers that recognize it, tools that have special handling for it. If we steer users toward ImmutableRandomizedMap, they'd probably encounter code that hasn't been updated to support it, especially if ImmutableRandomizedMap ends up being used only a fraction as often as ImmutableMap.

[fmeum]

@bazel-io flag

[iancha1992]

@fmeum Can this go to 8.2.0? Or do we have to include it in 8.1.0?

[fmeum]

This can wait for 8.2.0

[iancha1992]

@bazel-io fork 8.2.0