Parametrized methods where type parameter is used only in return type should be removed #1846
Comments
|
it contaminates API usage from Kotlin, because type parameter couldn't be derived automatically
and should always be specified.
That's interesting. Could you give a more elaborated example?
|
|
A random part of my Kotlin code: implClass.setFormalCtTypeParameters<CtClass<*>>(collTypeFormalTypeParameters)
collClassRef.setActualTypeArguments<CtTypeReference<Any>>(collClassTypeArgs)
if (collTypeElement.kind == INTERFACE) {
implClass.addSuperInterface<Any, CtType<Any>>(collClassRef)
} else {
implClass.setSuperclass<CtClass<Any>>(collClassRef)
}All method calls are "parameterized", and those explicit parameters couldn't be removed, it's going to be compilation failure in Kotlin. Albeit they all carry zero actual meaning. |
|
Is there a way to make a static analysis and list all such useless and even counter-productive
generic parameters?
|
|
All method generic parameters that are used only in return type. I don't know if there are automated tools to find that, but they are generally found in higher-order interfaces like |
monperrus
changed the title
|
However it should be easy to make such static analysis check in checkstyle, or with spoon itself :) |
|
I agree, would you give it a try?
|
|
No, I don't plan to try to do this. |
|
Many methods are like this (see below). The goal of those type parameters is to allow method call chaining with no cast in the presence of overridden methods. Would that still work if we remove them? |
|
Hi Martin, the long list you have provided is too long - it contains also different use case - not all these methods fits to the problem described by Roman. For example: interface CtElement {
<E extends CtElement> List<E> getAnnotatedChildren(...);
}is correct, because client can assign returned list to variable s/he needs. But it is not the case of CtNamedElement#setSimpleName, where I agree with Roman. I tried this example code: CtType type = null;
//case A)
type.getTypeMembers(); //is correct
type.setSimpleName("abc").getTypeMembers(); //is a compilation error, so fluent API doesn't work
//case B)
CtType type2 = type.setSimpleName("abc"); //compiles wellSo generic return type really makes no sense in case A). It makes sense only for case B). But is it used anywhere? Note: when I add this declaration interface CtType<T> {
CtType<T> setSimpleName(String name);
}
class CtTypeImpl {
@Override
public CtTypeImpl<T> setSimpleName(String simpleName) {
return super.setSimpleName(simpleName);
}
}then code type.setSimpleName("abc").getTypeMembers(); compiles well. So if we really want to support fluent API, then we should NOT use generic return types for |
I agree What do you think of the idea #1852? |
|
I want to resurrect this discussion. When type parameters are used in the return type only, it gives an illusion of type safety that often does not exist because of type erasure. Using the type parameter directly as the return type is kind of innocuous as it's semantically equivalent to just casting. Using the type parameter inside of a generic in the return value is very bad because of type erasure. Allow me to demonstrate. Return-type-only type parameter as "pure" return type (kind if innocuous)Here's some sample code with a method public class Main {
public static void main(String[] args) {
Object obj = getObject(); // this is fine
Integer i = getObject(); // this is fine
String str = getObject(); // causes ClassCastException
}
static <T> T getObject() {
return (T) Integer.valueOf(1);
}
}The above code is semantically the equivalent to having Note that there's actually no cast inside of But why is this use kind of innocuous? Because we fail-fast on assigning to By bounding the type parameter (e.g. with Return-type-only type parameter inside generic (badness!!)This is where it gets really confusing, and really bad. Have a look at the following piece of code. import java.util.List;
public class Main {
public static void main(String[] args) {
List<Integer> intList = getList(); // this is fine
List<Double> doubleList = getList(); // this is not fine, but the JVM doesn't know that
Object shouldBeADouble = doubleList.get(0); // surely this must be a double?
System.out.println("I got this from a list of doubles: " + shouldBeADouble.getClass().getName());
}
static <T extends Number> List<T> getList() {
return (List<T>) List.of(Integer.valueOf(1));
}
}That doesn't look right, does it? But this program runs just fine, and prints this: Unless we try to retrieve an element from The problem here is that, just like before, the cast to the type parameter (here The only way we'd discover that the supposed list of doubles isn't actually a list of doubles is if we'd try to retrieve a value from Double notADouble = doubleList.get(0); // ClassCastExceptionAt best, this causes confusing exceptions. At worst, it hides errors by causing exceptions to either not occur at all, or occur well after the problematic part of the code was executed (perhaps that list of "doubles" is passed around, for example). Let's have a look at a ridiculous example in SpoonSo, unfortunately a lot of Spoon's methods suffers from these problems due to the liberal use of return-type-only type parameters. Let's take CtModel model = launcher.buildModel();
CtType<?> type = model.filterChildren(CtType.class::isInstance).first();
List<CtModule> modules = type.getAnnotatedChildren(Annotation.class);
System.out.println(modules);This code is of course absolute nonsense, there's no way that a type can have children that are of type
List<CtType<?>> types = model
.filterChildren(e -> !(e instanceof CtType))
.list();
System.out.println(types);And it runs just fine. But those examples are ridiculous!Yes, but I think they illustrate the point that using return-type-only type parameters, especially when used in generics, is pretty bad. It's both confusing and can hide problems. What to do instead?If we want to let clients avoid casting, then the way to go is to have the desired type as an argument, and call its import java.util.List;
public class Main {
public static void main(String[] args) {
List<Integer> intList = getList(Integer.class); // this is fine
List<Double> doubleList = getList(Double.class); // ClassCastException
}
static <T extends Number> List<T> getList(Class<T> type) {
return List.of(type.cast(Integer.valueOf(1)));
}
}This hides the cast allowing for fluent interface design, but at the same time causes fail-fast behavior. This would break large parts of the API, unfortunately. I just wanted to give ample evidence for why return-type-only type parameters are not good. |
|
Thanks a lot @slarse for the essay. For sake of backward compatibility, we probably won't change the existing methods. However, better documentation and new improved versions of API methods are very welcome. |
Typical example:
Type parameter
Tis used only in method's return type, not in the parameters.It doesn't make sense in Java. If this method is defined in the interface (like
CtNamedElement), it should just be declared to returnCtNamedElement. Implementations in overriding classes narrow the return type to their actual type.Why this is bad: it contaminates API usage from Kotlin, because type parameter couldn't be derived automatically and should always be specified.
The text was updated successfully, but these errors were encountered: