Removed abstractmultimap

[danieldietrich]

Hi @ruslansennov, @paplorinc,@zsolt-donca,

this is an ongoing (currently dirty) draft of the bugfix of #1326. I created an early PR so that you can track the changes.

This is one of several PRs. The checklist is updated here.

Goal:

Remove all package-private super classes because method-references do not work for default implementations (JDK bug!).

What I did so far:

• Added Multimaps helper class similar to the new Maps class.
• Found another abstraction for a common Multimap Builder
• Started to move methods from AbstractMultimap to Multimaps. The generics and method arguments change.
• Changes LinkedHashMultimap to use Multimaps.

TODO:

• Move the remaining methods from AbstractMultimap to Multimaps
• Override missing methods and adjust return types in LinkedHashMap
• Do the same for HashMultimap and TreeMultimap
• Fix the problem with the emptyInstance() when an underlying Comparator is present and the generic type changes. This blows up the Multimap when the original comparator is used on a new type.
• Delete AbstractMultimap

[l0rinc]

👍

[l0rinc]

@danieldietrich, good job so far, now go get some sleep :p

[danieldietrich]

zzZZzz

[danieldietrich]

My approach looks too complicated/unintuitive to me.

Example:

public final class LinkedHashMap<K, V> implements Multimap<K, V> {
    @Override
    public LinkedHashMultimap<K, V> put(K key, V value) {
        return Multimaps.put(this, delegate, emptyContainer, containerType, builder()::ofMap, key, value);
    }
}

final class Multimaps {
    static <K, V, M extends Multimap<K, V>> M put(
            M multimap,
            Map<K, Traversable<V>> delegate,
            Traversable<V> emptyContainer,
            ContainerType containerType,
            OfMap<K, V, M> ofMap,
            K key,
            V value) {
        final Traversable<V> values = delegate.get(key).getOrElse(emptyContainer);
        final Traversable<V> newValues = containerType.add(values, value);
        return (newValues == values) ? multimap : ofMap.apply(delegate.put(key, newValues));
    }
}

E.g. we create a new LinkedHashMultimap.Builder instance to get an instance of the OfMap function, which wraps a delegate. We need an instance to get the instances emptyContainer and containerType. As an alternative we could duplicate the ofMap method - one for the Builder, one for the Multimap implementation.

You see, this starts to get cumbersome. Putting a value into a Map should be simple. I have the wrong abstraction here.

I like @ruslansennov approach to have an AbstractMultimap (btw - same applies for AbstractMap which I already removed and pushed the new Maps helper). But I don't want to make our abstract base impls public. They are not intended to be part of the user API. They just help us internally to factor out duplicate code.

Maybe there is another way to circumvent the JDK bug by just overriding all methods of the package private base class and delegating to them. I will test it.

public final class LinkedHashMap<K, V> extends AbstractMultimap<K, V, LinkedHashMap<K, V>> {
    @Override
    public LinkedHashMultimap<K, V> put(K key, V value) {
        return super.put(key, value);
    }
}

This solution is fragile in the sense that we could forget to override a method. We would need a unit test that checks overridden methods via reflection for example.

---

I'm undecided, but I think that I will finish my current refactoring. There are benefits, like fewer type casts...

Update: Actually the second ofMap method is not a duplicate. It does the same but in a different context - we will see it when I get to TreeMultimap. When an operation changes the value-type we need to adjust the Comparator... For the Builder::ofMap this is not the case, the Builder will already know the Comparator.

[danieldietrich]

What if... we just duplicate code. It would simplify things.

@Override
public LinkedHashMultimap<K, V> takeUntil(Predicate<? super Tuple2<K, V>> predicate) {
    Objects.requireNonNull(predicate, "predicate is null");
    return takeWhile(predicate.negate());
}

This is a typical code-generator case. We could generate all Multimaps and Maps. Then we have the possibility to apply better optimizations and there are fewer classes. On the other hand that would make it harder to get the hands on these classes for further changes.

---

Will sleep one night over this topic...

...but I already like the idea - we will get rid of unnecessary code and make decisions already at code generation time.

• fewer classes
• code of different impls automatically in sync because based on same generator templates
• centralized code changes, apply to all impls automatically
• no technical overhead in production code, like Map Suppliers in order to abstract over impl type
• ...

[danieldietrich]

This will still take some time. Maybe we should move this issue to the next (bugfix or minor) release in order to ship 2.0.4. In other words method references will not work for Maps/Multimaps. Workaround: use lambdas instead of method references. Good thing: It is a JDK bug, not a Javaslang bug, but should be fixed neartime.

I think we still have problems in TreeMap and/or TreeMultimap when the value type is changed by an operation (based on wrong Comparators). But I haven't investigated it yet.

Also operations of sorted collections that operate on multiple instances with different underlying Comparators make cause unexpected behavior. But this has to be checked and solved step-by-step. I'm alread on it by re-designing the RedBlackTree with some optimizations and preparation for the Navigable additions...

So much to do :) Known bugs are ok. The most important thing is to ship reliable, backward-compatible software.

[danieldietrich]

We already removed AbstractMap. I wouldn't revert that change - but stop this change here.

I will move (the rest of) #1326 to 2.0.5 and go the generator-way.

[l0rinc]

I'm trying to follow the changes, but I'm not sure I understand one thing: can't we make the abstractions public instead?

If I'm mistaken correctly the problem is that the returned value is a package-private superclass.
But can't we extract a public interface for the private superclass and return the interface instead?

(also, why isn't multimap simply a Map<K, Seq<V>>>)

[danieldietrich]

can't we make the abstractions public instead

No. They are only ballast and would pollute the user's view on the Javaslang API.

If I'm mistaken correctly the problem is that the returned value is a package-private superclass.

That's not the problem. The returned value is a generic M extends Multimap<K, V>. The M is public (e.g. LinkedHashMultimap).

But can't we extract a public interface for the private superclass and return the interface instead?

We have a public interface, it is Multimap<K, V>. But this would not solve the problem. It is a JDK bug finding the right method.

(also, why isn't multimap simply a Map<K, Seq>>)

Because Multimaps have methods that operate on V instead of Traversable<V>. (Seq<V> is not sufficient, we have also Set<V> and SortedSet<V>).

---

I like the idea of generated code. It solved the one and only problem that our AbstractMultimap also solves - factor out duplicate code into a single place.

But the generated code is better than the AbstractMultimap. We do not have the generic-hell, no type casts, fewer types within the class hierarchy, etc. There are many benefits.

There is only one drawback - the code generator is a little bit harder to write than non-generated code.

[l0rinc]

I like the idea of generated code.

Hmmm, wasn't that your counter-argument against collection specialization?

Or can we make the generator source a valid Java source? (i.e write once and forget that this is actually a base for other cases). Otherwise I think this would have a huge maintenance cost (non-standard way of programming, scaring many people away, providing more base for mistakes).

[danieldietrich]

I like the idea of generated code.

Hmmm, wasn't that your counter-argument against collection specialization?

I did not mean this statement in the very general but only considering our options:

• Making the abstract super-classes public (- this is a no-go because it is not intended to be user API)
• Staying with package-private abstract super classes and override all methods in implementing classes. These just delegate to the super-class (- this leads to maintenance hell)
• Introducing package-private helper classes with static methods (- this leads to unreadable, blown-up (internal) API, see current PR)
• generate the code (- we get rid of much technical boilerplate like generics, casts, unnecessary types and method calls, etc.)

Or can we make the generator source a valid Java source?

Interesting idea. I have already thought about self-expanding Java code. Xtend language does this with so-called Active Annotations. Scala has Macros. But these things actually hide things like the annotations of many (in)famous Java frameworks. I like to directly write the (generator) code - no hidden magic.

Btw - the AbstractQueue does not have this problem. The queues do not need to be generated.

---

I would like to experiment with it (hands-on!) and see how it does feel from the development perspective. Maybe it is not as hard to maintain as we think. The Scala code generator is really easy.

[l0rinc]

As long as the source code is not Strings, I don't mind :)

[danieldietrich]

As long as the source code is not Strings, I don't mind :)

I don't understand. We check-in the generated source. It is like hand-written code. Unit tests run against it. The unit tests will not be generated (from what I can see now), we use the existing Absrtact*MapTest classes.

Also our CI build automatically checks if a user accidentally overwrites generated code with manual changes. This breaks the build.

Generating the Maps and Multimaps should be safe regarding Java compilation and manual user-changes!

[danieldietrich]

Obsolete, will be closed. We (most probably) will go the generator way.

[l0rinc]

I don't understand.

I just meant that source code should be written in an IDE recognizable format.
In my opinion currently we have some source code in Strings that are a bit cumbersome to edit, i.e. no compiler or IDE help, only after regenerating everything can I tell if it's ok (like back in Borland C++ times).

[danieldietrich]

I just meant that source code should be written in an IDE recognizable format.

The quality of our source code and user experience are very important / priority 1. Javaslang library developer experience is also important but priority 2.

• The public API does not change, therefore user experience is not affected.
• A switch to generating Maps and Multimaps will (drastically?) increase the quality of our source code.
• Maintaining the generator templates is an additional effort, it affects the developer experience.

The right workflow will ease the pain of maintaining generator templates:

1. First we directly edit/change the generated code. We will have full IDE support. Editing may involve the introduction of new types or the erasure of existing types. As always, we write unit tests for the code. In this phase we do also refactoring until the code has a final state/seems to be in perfect shape.

   We use the IDE to test the changes. We have to take care not to run the generator (via maven build), it will overwrite our changes. It is recommended to create backups, e.g. in the form of local commits that will be squashed later.

2. If all works as expected, we adjust the generator templates in the way that they reflect the changes of the generated code we made in the first phase. We now run the code generator. In the 1st phase we committed the direct changes of the generated code. So we are able to verify if our generator adjustment is correct by looking at the git diff.

Phase 1. increases the developer experience during generator-driven development. Phase 2. is a pain in the ass, as always in stringly-driven development. But we need to do it to increase the quality of our library.

[l0rinc]

Thanks for clarifying. I like the approach.
The alternative, of generating all cases from an annotated .java file would probably be more complex :)