Type inference broken in 6.9.0

[manusa]

Description

(From internal conversation with @iocanel and @metacosm)

PR #5138, in scope of #5135, provided changes to make Resources Editable.

While this improvement allows for one line edit operations similar to what Lombok's toBuilder does, it has somehow broken type inference in some scenarios.

One such scenario is where you have elements that are parameterized as such: A<R extends HasMetadata, P>. If you have several such objects which all have the same value for P but different values for R and you want to process them as a list.

Before 6.9.0, the list was correctly inferred to be a list containing elements of type A<? extends HasMetadata, P>.
With 6.9.0, the list is now inferred to contain elements of type A<? extends Editable<? extends BaseFluent<? extends BaseFluent<?>>>, P>.

With type erasure, this results in hard-to-diagnose errors because they occur outside of the user's code, similar to:

accept(java.util.List<java.util.Map.Entry<java.lang.String,java.lang.Object>>,java.lang.String,io.fabric8.kubernetes.api.builder.Visitor...) in io.fabric8.kubernetes.api.builder.BaseFluent cannot implement accept(java.util.List<java.util.Map.Entry<java.lang.String,java.lang.Object>>,java.lang.String,io.fabric8.kubernetes.api.builder.Visitor...) in io.fabric8.kubernetes.api.builder.Visitable
[ERROR]   return type capture#1 of ? is not compatible with capture#2 of ?

You can see such an error here: https://github.com/operator-framework/java-operator-sdk/actions/runs/6456675029/job/17526638395#step:5:2605
If you look at the code, you'll see that none of the classes or method mentioned in the error are used, which makes for a very unpleasant diagnosing experience.

[metacosm]

Note that the workaround this issue is to either not process related elements generically or relax typing which isn't desirable and goes against the philosophy of the client.

[shawkins]

@manusa @metacosm two thoughts. It could be unrelated but it could be worth seeing the split package that was introduced - #5520 - could be contributing. Could you try to move at least Pluralize moved to common and see if that helps. The other thought is that the resource does not actually need to implement Editable to expose these methods - that was done for the sundrio logic that is creating Builders, but that can just fall through to the lookup tables.

[manusa]

I'm trying to but I'm unable to reproduce this.

#5520 needs to get solved anyway, could you please create the PR with the fix? I'll try to release snapshots after merging.

[shawkins]

#5520 needs to get solved anyway, could you please create the PR with the fix? I'll try to release snapshots after merging.

#5525 - should just be a tweak to the pom. It was copying the builder stuff into both modules.

[iocanel]

@manusa @metacosm two thoughts. It could be unrelated but it could be worth seeing the split package that was introduced - #5520 - could be contributing.

Yeah, that does look suspicious. The Editable part of the theory explains how the BaseFluent gets dragged in. This might explain why inference actually breaks.

[metacosm]

Added a test that should exhibit the issue.

[shawkins]

@metacosm observations based upon the test:

• the fix: should not export the builder package #5525 fix does not resolve the issue
• running the test in the ide is fine, but not with the command line
• Explicitly adding the typing works fine:

Arrays.<KubernetesDependentResource> asList(cdr, ddr).forEach(dr -> dr.reconcile());

or

Arrays.asList(cdr, ddr).forEach(KubernetesDependentResource::reconcile);

Let me try backing out the change that added the edit method.

[manusa]

Added a test that should exhibit the issue.

I can't really figure out what makes this test different to the one I proposed in the internal chat, hence why one fails and the other doesn't.
It might be the extra-nesting provided by DependentResourceConfigurator.

Arrays.asList(cdr, ddr).forEach(KubernetesDependentResource::reconcile);

It's interesting that the method reference works.

With Visitors we had the problem that lambdas didn't work and that anonymous inner classes had to be used instead.

[manusa]

In my case, running from the command-line (Java 8 from kubernetes-client-api directory) is still not failing.

Update It does fail with Java11 and Java17, so it definitely might have something to do with modules.

[metacosm]

@metacosm observations based upon the test:

* the [fix: should not export the builder package #5525](https://github.com/fabric8io/kubernetes-client/pull/5525) fix does not resolve the issue

Doesn't indeed appear to change anything.

* running the test in the ide is fine, but not with the command line

It doesn't work in my IDE here, or at least, the original issue in JOSDK showed up in my IDE.

* Explicitly adding the typing works fine:

Arrays.<KubernetesDependentResource> asList(cdr, ddr).forEach(dr -> dr.reconcile());

or

Arrays.asList(cdr, ddr).forEach(KubernetesDependentResource::reconcile);

In both cases, though, this is a wider type than what would be expected. The more specific common type should be KubernetesDependentResource<? extends HasMetadata, Service>, which is what was previously correctly inferred. I should be able to resolve operations / objects that use Service as a parameter in my lambda without first having to cast.

Let me try backing out the change that added the edit method.

[metacosm]

Added a test that should exhibit the issue.

I can't really figure out what makes this test different to the one I proposed in the internal chat, hence why one fails and the other doesn't. It might be the extra-nesting provided by DependentResourceConfigurator.

Yes, the thing that makes it fail is DependentResourceConfigurator, I think it introduces a different parameter that is not resolved properly anymore.

Arrays.asList(cdr, ddr).forEach(KubernetesDependentResource::reconcile);

It's interesting that the method reference works.

I think that's expected because it's basically telling Java to consider all elements as KubernetesDependentResource, which they are but which is a wider type than what should be resolvable.

With Visitors we had the problem that lambdas didn't work and that anonymous inner classes had to be used instead.

[shawkins]

In both cases, though, this is a wider type than what would be expected. The more specific common type should be KubernetesDependentResource<? extends HasMetadata, Service>, which is what was previously correctly inferred. I should be able to resolve operations / objects that use Service as a parameter in my lambda without first having to cast.

I wasn't trying to be overly narrow, just using the class where the resolve method is introduced. I understand that it should work without the typing.

I expanded the test to inline definitions of an AltConfigMap and AltDeployment along with dummy builders. Based upon that the next observations are:

• the compile error goes away if the resources are not Editable
• the compile error also goes away if BaseFluent is not used for the Builders

Further inlining all of the builder package with the test shows:

• the compile error still persists, so it's not related to modules nor classloading

So I think there's a viable workaround by removing the Editable interface from the resources (but the edit / toBuilder methods can stay) - however there just seems to be something pretty funky about the sundrio typing that is confusing the compiler. I'll poke around a little more and see if there are any changes that could be made to BaseFluent to resolve this.

[shawkins]

@manusa @metacosm I've opened a sundrio pr for this. The short form is that implementing Visitable twice - even if it has the same bound from a high-level perspective is causing confusion.

[metacosm]

I was thinking that indeed the error might have come from an interface being implemented twice with different type constraints…

[manusa]

Similar problem is still ongoing in 6.9.3 (Sundrio 0.101.3)

Quarkus build reports:

Error: ]: Build step io.quarkus.kubernetes.deployment.KubernetesProcessor#build threw an exception: java.lang.NoSuchMethodError: 'io.fabric8.kubernetes.api.builder.Fluent io.fabric8.kubernetes.api.model.KubernetesListBuilder.accept(io.fabric8.kubernetes.api.builder.Visitor[])'
	at io.dekorate.ResourceRegistry.lambda$generate$2(ResourceRegistry.java:181)
	at java.base/java.util.HashMap.forEach(HashMap.java:1421)
	at io.dekorate.ResourceRegistry.generate(ResourceRegistry.java:173)
	at io.dekorate.Session.generate(Session.java:293)
	at io.dekorate.Session.close(Session.java:256)
	at io.quarkus.kubernetes.deployment.KubernetesProcessor.lambda$build$2(KubernetesProcessor.java:194)
	at java.base/java.util.Optional.ifPresent(Optional.java:178)
	at io.quarkus.kubernetes.deployment.KubernetesProcessor.build(KubernetesProcessor.java:142)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:849)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
	at java.base/java.lang.Thread.run(Thread.java:840)
	at org.jboss.threads.JBossThread.run(JBossThread.java:501)

	at io.quarkus.test.QuarkusProdModeTest.beforeAll(QuarkusProdModeTest.java:512)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Triggered at:

https://github.com/dekorateio/dekorate/blob/5a9d246bdd0ecbcbfabbbbc3070f757605366a21/core/src/main/java/io/dekorate/ResourceRegistry.java#L181

        for (Decorator d : sortDecorators(union)) {
          log("Applying decorator '%s'", d.getClass().getName());
          groups.get(group).accept(d);
        }
      }

[metacosm]

Indeed seeing this type of errors as well in QOSDK.