Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GROOVY-11192: Diamond inference should work on subclasses with less generic parameters #1970

Closed
wants to merge 3 commits into from

Conversation

yuhengfdada
Copy link
Contributor

Bug Description

Something like

            interface A<K, V> {
            }
            class B<V> implements A<Long, V>{
            }
            A<Long, String> a = new B<>()

would cause groovyc to fail.

Analysis

https://issues.apache.org/jira/browse/GROOVY-11192

There is a bug in org.codehaus.groovy.transform.stc.StaticTypeCheckingVisitor#inferDiamondType.

The inferredType is java.util.Map<java.lang.Long, java.lang.String> -> java.util.Map<K, V> and cceType is MapLong<> -> MapLong<V>.

After adjustGenerics() on line 1124, cceType becomes MapLong<java.lang.Long, java.lang.String> -> MapLong<V>, which is incorrect. It should be MapLong<java.lang.String> -> MapLong<V>.

Fix

If we see (# of generic types for A) > (# of generic types for B's redirect), then only keep the generic types that correspond to B's redirect generic types.

A.genericTypes = [Long, String]
B.genericTypes = []
A.redirect().genericTypes = [K, V]
B.redirect().genericTypes = [V]

In this case, find the index of V in A.redirect().genericTypes, then find the corresponding type in A.genericTypes, which is String.

@blackdrag
Copy link
Contributor

could you please add a test case:

        interface A<G1, G2> {
        }
        class B<G3> implements A<Long, G3>{
        }
        A<Long, String> a = new B<>()

oh yes.. using shouldCompile would be better than assertScript where possible. We have to test we repeating names for generic types because of mistakes, that have been don in the past where generic types leaked to other classes with the wrong name, and then solutions that worked only because of the just mentioned reason and not if the names are different.

@eric-milles
Copy link
Member

Generics is a very tricky area of the compiler. The diamond inference may assume that the target type's generics exactly correspond to the constructor type's generics. I don't think the > check is sufficient. You may have a type like "class C<V,K> implements Map<K,V>".

inferDiamondType should be walking from 'Map' to MapLong calling parameterizeType each time to fill in the type parameters. So, I would expect a fix in inferDiamondType not adjustGenerics if something is amiss.

@yuhengfdada
Copy link
Contributor Author

Thank you so much @blackdrag @eric-milles for your comments . Now I understand the problem is more complicated than I thought. Closing the PR for now.

@yuhengfdada
Copy link
Contributor Author

yuhengfdada commented Oct 20, 2023

Generics is a very tricky area of the compiler. The diamond inference may assume that the target type's generics exactly correspond to the constructor type's generics. I don't think the > check is sufficient. You may have a type like "class C<V,K> implements Map<K,V>".

@eric-milles Can you please share a bit more on You may have a type like "class C<V,K> implements Map<K,V>"? This PR is about when subclasses have less type parameters.
Are you suggesting the compiler might wrongly inference class C as <K,V> instead of <V, K>?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants