Make TypeParameterQualifier Refaster-aware#5884
Conversation
For method references, there valid use cases within Refaster `@BeforeTemplate` methods.
|
See PicnicSupermarket/error-prone-support#2274 for context. An alternative to this change is #5883, though these PRs are not mutually exclusive. |
| class Test { | ||
| @BeforeTemplate | ||
| <T extends Enum<T>> Function<T, String> rule() { | ||
| return T::name; |
There was a problem hiding this comment.
Just to be clear: This one could use Enum::name, right? It's still reasonable to test it; it's just that the case below is the one in which Refaster needs you to use T?
There was a problem hiding this comment.
Ah, sorry: Since (related to my other comment!) this is a @BeforeTemplate, then the goal is to match when people write Foo::name, even though they could write Enum::name instead. So, in a Refaster template, unlike in normal code, you very much do want to be able to write T::name.
There was a problem hiding this comment.
Thanks. I'm now grasping the other test shows that we continue not to try to support matches on Foo.valueOf(Foo.class, value) (or especially Foo.valueOf(Bar.class, value)!), which is definitely weirder than writing Enum.valueOf(Foo.class, value) :) Now I suppose that we could still want to provide a way to match the latter (to steer people away from it!), but I am happy to stop here now that I hopefully actually understand what's going on.
There was a problem hiding this comment.
Since there is (IIRC) another Error Prone rule (or is it CheckStyle?) that nudges users to using DeclaringType.staticMethod instead of SomeSubType.staticMethod, I suspect there is less need for supporting this. But I also can't come up with a strong argument for not supporting this. Sometimes those static methods are "overridden".
Note that if we do also want to allow this, the PR actually becomes simpler, and the code likely more efficient. Cause we can then simply set:
@BugPattern(
summary = "Type parameter used as type qualifier",
severity = ERROR,
suppressionAnnotations = BeforeTemplate.class)That would make the change set class with #5883, but that's not a biggie.
Let's unresolve this thread for input by @cushon.
| private static boolean isInRefasterBeforeTemplate(VisitorState state) { | ||
| MethodTree enclosingMethod = findEnclosingMethod(state); | ||
| return enclosingMethod != null | ||
| && hasAnnotation(enclosingMethod, BEFORE_TEMPLATE_ANNOTATION, state); |
There was a problem hiding this comment.
Obvious question that might have an obvious answer: Would it make sense to check @AfterTemplate, too?
There was a problem hiding this comment.
Yeah, good question. I thought I convinced myself that the answer is "no", but now that I think back to what I tested: the answer is actually "yes". T::method will be replaced with ConcreteType::method, and one may reasonably want that in case e.g. a rule wishes to rewrite some context around arbitrary T::method. I've pushed another commit. :)
There was a problem hiding this comment.
(Generalized the PR description accordingly.)
There was a problem hiding this comment.
In hindsight, I guess that my other thread implies that an @AfterTemplate could always use BoundOfTheTypeParameter::method instead of T::method.
The former might actually be safer: If BoundOfTheTypeParameter::method compiles in the template, then it's very likely to compile in the output, whereas T::method could perhaps become ambiguous when replaced with ConcreteType::method. For example, Enum::valueOf is clearly valueOf(Class, String), but ConcreteType::valueOf could be either that or the valueOf(String) method generated on the enum class ConcreteType.
The former also feels less like a spiritual violation of NonCanonicalType / NonCanonicalStaticImport. But when we discussed that very thing (in the context of List::stream vs. Collection::stream) internally a couple months ago in bug 411135416, we decided that we didn't actually care :)
Those don't seem like deal-breakers. If someone wants to produce ConcreteType::name, then sure, I guess?
It does seem believable that there would be cases in which we don't have a specific known type for T, in which case generating a literal "T::whatever" would be bad. Still... I guess it's OK-ish? Or we should catch it inside Refaster if we don't already? But I guess I could see that as a somewhat more compelling reason to have not allowed T qualifiers in the @AfterTemplate.
And finally: I am belatedly recognizing that your implementation and your test take the position that there might very well not be a good case for either a @BeforeTemplate or an @AfterTemplate that contains T.someMethod(): someMethod would have to be a static method that is probably better referred to through the actual declaring type, as in the other comment thread. Additionally, the ban of T.<anything> has the virtue of preserving the ban on T.Foo in the signature (return types / parameters / type parameters) of the Refaster template, allowing qualification on T only for method references. (So my earlier Turbine experiments were focused on a case whose behavior you haven't even been proposing to change!)
After all that, I'm probably more on the side of your original approach of omitting this from the PR :) But I don't feel confident after all the knots I've unnecessarily tied myself into here. Let's at least wait to see if @cushon has higher-level concerns about the direction of the two PRs.
There was a problem hiding this comment.
I'm totally fine either way :). You make a compelling argument, though dropping the @AfterTemplate exemption does make it harder to write a rule that matches-without-modification of that particular sub-expression. There does exist an escape hatch for that, though, through the introduction of a @Matches(IsSomeMethodReference.class)-guarded template parameter. Significantly more work for the user, but as this would be rare, it is not a strong argument for keeping the PR as-is.
If we drop the second commit, let's remember to also revert the PR description body (as Copybara transfers that to the final commit message, I've observed).
There was a problem hiding this comment.
Good call on the PR description.
I'm realizing that the bottom line here might be this: Turbine doesn't care about anything that this CL affects.
|
I also observe that:
|
My mistake: It does not run Turbine on them (in the case I tested); I just saw an error and assumed it was from Turbine when in fact it was from... I do notice that (for better or for worse) it's currently possible to disable |
|
And wait... :) If I insert the a Refaster template of... @BeforeTemplate
private <T extends java.util.Map<?, ?>> Object o(T.Entry<?, ?> t) {
return t;
}...into our |
|
For the crash, Gemini says: --- a/core/src/main/java/com/google/errorprone/refaster/Template.java 2026-01-29 08:07:06.000000000 -0500
+++ b/core/src/main/java/com/google/errorprone/refaster/Template.java 2026-06-17 14:46:11.000000000 -0400
@@ -203,6 +203,12 @@
Warner warner,
List<Type> expectedTypes,
List<Type> actualTypes) {
+ for (Type type : actualTypes) {
+ TypeTag tag = type.getTag();
+ if (tag == TypeTag.PACKAGE || tag == TypeTag.METHOD || tag == TypeTag.MODULE || tag == TypeTag.FORALL) {
+ return Optional.empty();
+ }
+ }
try {
ImmutableList<UTypeVar> freeTypeVars = freeTypeVars(unifier);
var unused =Maybe? I didn't test whether the template from PicnicSupermarket/error-prone-support#2274 would still work with that change. |
|
[edit: Wrong again; see below.] |
|
Wait, no, sorry again: What breaks the tests is my test template with |
Stephan202
left a comment
Stephan202
left a comment
There was a problem hiding this comment.
Tnx for the review @cpovirk!
Maybe? I didn't test whether the template from PicnicSupermarket/error-prone-support#2274 would still work with that change.
I can quickly try that in a bit; we have test cases for each rule, so shouldn't be hard.
| class Test { | ||
| @BeforeTemplate | ||
| <T extends Enum<T>> Function<T, String> rule() { | ||
| return T::name; |
| private static boolean isInRefasterBeforeTemplate(VisitorState state) { | ||
| MethodTree enclosingMethod = findEnclosingMethod(state); | ||
| return enclosingMethod != null | ||
| && hasAnnotation(enclosingMethod, BEFORE_TEMPLATE_ANNOTATION, state); |
There was a problem hiding this comment.
Yeah, good question. I thought I convinced myself that the answer is "no", but now that I think back to what I tested: the answer is actually "yes". T::method will be replaced with ConcreteType::method, and one may reasonably want that in case e.g. a rule wishes to rewrite some context around arbitrary T::method. I've pushed another commit. :)
That change doesn't break our build ✔️ |
cpovirk
left a comment
cpovirk
left a comment
There was a problem hiding this comment.
Thanks for testing the possible fix!
| private static boolean isInRefasterBeforeTemplate(VisitorState state) { | ||
| MethodTree enclosingMethod = findEnclosingMethod(state); | ||
| return enclosingMethod != null | ||
| && hasAnnotation(enclosingMethod, BEFORE_TEMPLATE_ANNOTATION, state); |
There was a problem hiding this comment.
In hindsight, I guess that my other thread implies that an @AfterTemplate could always use BoundOfTheTypeParameter::method instead of T::method.
The former might actually be safer: If BoundOfTheTypeParameter::method compiles in the template, then it's very likely to compile in the output, whereas T::method could perhaps become ambiguous when replaced with ConcreteType::method. For example, Enum::valueOf is clearly valueOf(Class, String), but ConcreteType::valueOf could be either that or the valueOf(String) method generated on the enum class ConcreteType.
The former also feels less like a spiritual violation of NonCanonicalType / NonCanonicalStaticImport. But when we discussed that very thing (in the context of List::stream vs. Collection::stream) internally a couple months ago in bug 411135416, we decided that we didn't actually care :)
Those don't seem like deal-breakers. If someone wants to produce ConcreteType::name, then sure, I guess?
It does seem believable that there would be cases in which we don't have a specific known type for T, in which case generating a literal "T::whatever" would be bad. Still... I guess it's OK-ish? Or we should catch it inside Refaster if we don't already? But I guess I could see that as a somewhat more compelling reason to have not allowed T qualifiers in the @AfterTemplate.
And finally: I am belatedly recognizing that your implementation and your test take the position that there might very well not be a good case for either a @BeforeTemplate or an @AfterTemplate that contains T.someMethod(): someMethod would have to be a static method that is probably better referred to through the actual declaring type, as in the other comment thread. Additionally, the ban of T.<anything> has the virtue of preserving the ban on T.Foo in the signature (return types / parameters / type parameters) of the Refaster template, allowing qualification on T only for method references. (So my earlier Turbine experiments were focused on a case whose behavior you haven't even been proposing to change!)
After all that, I'm probably more on the side of your original approach of omitting this from the PR :) But I don't feel confident after all the knots I've unnecessarily tied myself into here. Let's at least wait to see if @cushon has higher-level concerns about the direction of the two PRs.
| class Test { | ||
| @BeforeTemplate | ||
| <T extends Enum<T>> Function<T, String> rule() { | ||
| return T::name; |
There was a problem hiding this comment.
Thanks. I'm now grasping the other test shows that we continue not to try to support matches on Foo.valueOf(Foo.class, value) (or especially Foo.valueOf(Bar.class, value)!), which is definitely weirder than writing Enum.valueOf(Foo.class, value) :) Now I suppose that we could still want to provide a way to match the latter (to steer people away from it!), but I am happy to stop here now that I hopefully actually understand what's going on.
2 participants
For method references, there are valid use cases within Refaster template methods.