Guava removal

[Apanatshka]

I'm working towards removing guava as a dependency from as much stuff as possible. First step in this repo is to move the MultiTable stuff from org.metaborg.util to p_raffrayi. I've also started removing calls like Sets.newHashSet with a direct call to the constructor, and removing dependence on guava's Iterables by requiring more Collection instead of Iterable.

LMK if you have any thoughts or concerns about these (planned) changes.

[Apanatshka]

@AZWN do you know if there is shared mutable state in the concurrent solver? Since I'm replacing a few datastructures with ones I've written myself, it's relevant because of thread safety...

[AZWN]

Yes, there is a little. The framework is designed on the actor model, meaning that there is no mutable state in the actual framework. However, there is some mutable state in the implementation of the actor framework itself (package mb.p_raffaryi.actors.impl, especially the Actor and ActorSystem classes. However, I doubt it is relevant for your refactoring, as it is implemented using ThreadLocals, java.util.concurrent.ConcurrentLinkedDeque, atomic references, synchronization, and volatile variables, none of which uses Guava.

The CHM protocol implementation does not have shared data, so its data structures can be replaced as required.

Finally, there is some shared state in mb.p_raffrayi.impl.Broker. This is actually a performance hack/technical dept. I don't think it uses Guava, but if it needs to be refactored, we might as well fix it immediately.

[Apanatshka]

These are all the changes, I think. I need to push some changes in other repo's to master before this can be merged, but it can be reviewed soon.

I've broken two tests:

1. mb.p_raffrayi.IncrementalTest#testRelease_MutualDep_ChildChanged
2. mb.statix.scopegraph.diff.ScopeGraphDiffTest#testGraphAdditions_TargetEarlier

UPDATE: Both tests are fixed now ✅

[Apanatshka]

Abstract list interface: currently I cannot get Immutables to stop using guava unless I make it more specific than the abstract list.

[Virtlink]

Be careful not to replace one dependency (Guava) with another (Capsule). Arguably, Capsule is even less battle-tested and less maintained than Guava. Even if we cannot avoid picking some implementation, generally I would prefer using the Java built-in interfaces as types, to avoid making these dependencies part of the API/function signatures and painting ourselves into a corner.

[Apanatshka]

Right now we use both all over the place. The upside of Capsule is that it is a small dependencies of just those collections. Guava is huge and has many parts we don't use that get CVEs filed against. This refactoring was enough work already, and I'm not going to complicate the diff any more. We can have a separate discussion on how to tie ourselves less to particular dependencies, I'm open to the idea of abstracting them away as much as possible.

[Virtlink]

Sure, I just don't understand your comment:

Abstract list interface: currently I cannot get Immutables to stop using guava unless I make it more specific than the abstract list.

Capsule's Immutables all implement the base Java interface, right?

[Apanatshka]

Oh, the problem there is that we use a annotation processor called Immutables which by default uses guava in its generated code. This is because guava will check if one of their collections is immutable and then not do a copy, and the generated code is all about immutability so it makes sense. If you opt into using normal Java collections in the codegen, it is supposed to no longer depend on guava, but in practice this option is broken en the codegen will still (part of the time) use guava code. The only way around it (that I'm aware of) is to not use the base Java interfaces for which it knows guava implementations.

[AZWN]

I see that the annotations have the option @Style(jdkOnly = true). Documentation isn't very clear, but it seems to disable Guava. Could/should we use that instead of the very specific interfaces?

[Virtlink]

Or is there a specific issue with jdkOnly = true that you can link to?

[AZWN]

I found some more docs that support the intuition above:

• Javadoc: https://github.com/immutables/immutables/blob/28ba97e9f3075902e279c1cde3e16be01e365afb/value-annotations/src/org/immutables/value/Value.java#L801-L812

• https://immutables.github.io/style.html says:

  Force the generation of JDK-only implementation code, even if Guava is available in classpath.

  (not explicitliy tied to jdkOnly, but thats the only reasonable option).

[Apanatshka]

So last open issue (if my most recent change doesn't foul up any tests) is the immutables behaviour. Two things there:

1. Perhaps the annotation in the package-info doesn't reach into sub packages and that's the issue.
2. Regardless of whether we find a solution like that, by using normal JDK stuff instead of guava, code generated by immutables will do more list copying because it cannot tell if the normal JDK list that is given as input is immutable. But it will assume this and have you handle it for custom types like the ImList.Immutable that I'm using in this PR. So if we can get immutables to behave, it'll be a choice between a nice API and preserving the performance we had before.

[AZWN]

Last commit indeed looks like it fixes the issue, thnx!

Regarding the interface vs. performance debate: then performance is most important.

So, basically, that means I'm happy with merging this PR now.