Vavr: ClassCastException during pattern matching

[danieldietrich]

Thx @goerge for reporting this bug on Gitter!

[danieldietrich]

Luckily the Match generics are correct.

The pattern matcher $(List.empty()) compares the _prototype_ instance (here: List.empty()) which the actual object that is matched ($() is of type Pattern0):

The error is located in the implementation of List.Nil.equals()

It uses the internal Collections.equals() method:

Which does a check _by contents_.

Therefore Match matches (because Pattern0.isDefinedAt returns equals == true), then the case is performed, then -----> 💥

I will fix that and check all other locations which could be affected by Collections.equals!

Many thanks! 😊

[danieldietrich]

A fix could look like this:

    static <V> boolean equals(Seq<V> source, Object object) {
        if (object == source) {
            return true;
-       } else if (source != null && object instanceof Seq) {
+       } else if (source != null && object != null && source.getClass() == object.getClass()) {
            final Seq<V> seq = (Seq<V>) object;
            return seq.size() == source.size() && areEqual(source, seq);
        } else {
            return false;
        }
    }

Other Collections.equals() overloads are also affected...

[danieldietrich]

Looks like changing the Collections.equals() implementatios has a bigger impact on the code-base than I thought 😅. I will get it right!

[goerge]

@danieldietrich I’m really impressed how fast you react on this issue. Thanks for the good work on vavr.

[danieldietrich]

Our notion of _collection_ equality is aligned to that of Scala. All collections that share the same parent type (Seq / Set / Map) are equal (by definition), if they contain the same elements (modulo order).

However, I'm aware of the fact that might be unexpected to Java developers.

Changing List.empty() == Stream.empty() to List.empty() != Stream.empty() might have a big impact to users of VAVR. We can do that in VAVR 1.0.0.

But fixing pattern matching is a _must_. In order to fix this bug, I will change the implementation of Pattern0 the way, that isDefinedAt also checks _assignability_ in addition to _equality_.

[goerge]

I think your / vavrs / scalas notion of collection equality is right.
To be honest I found this issue by accident when using List.empty() to check for emptiness where Seq::isEmpty would have been appropriate.
So with your mentioned fix in isDefinedAt already being available my naive approach would have been right.

[goerge]

@danieldietrich Wow. Thanks for the fast fix!

[goerge]

I'm not sure if the result of this fix was intended.

The following documents the difference between "match" and "equality check":

  @Test
  public void no_match() {
    final Seq empty = Stream.empty();
    final Option<String> result = API.Match(empty).option(
      API.Case(API.$(List.empty()), x -> "Stream.empty() matches List.empty()")
    );

    assertThat(result).isEqualTo(none());
  }

  @Test
  public void eq() {
    final Option<String> result = Option.when(Stream.empty().equals(List.empty()), () -> "Stream.empty() equals List.empty()");

    assertThat(result).isEqualTo(some("Stream.empty() equals List.empty()"));
  }

I'm aware of the fact that this is not a breaking change due to the raised ClassCastException.
Though I doubt that this intuitive.
Is this the same behaviour in Scala?

[danieldietrich]

In fact Scala behaves the same... however, I think we should implement collection equality like it is expected by Java developers. I will think about it for VAVR 1.0

[chrylis]

Thanks for your work, Daniel! This bug (I think) is blocking me in 0.10.0 because it causes $(not(instanceOf())) to blow up every time; do you have an estimate for 0.10.1 availability?

[danieldietrich]

@chrylis I will release a backward compatible 1.0 within this or the next week.