-
Notifications
You must be signed in to change notification settings - Fork 421
Add expression type in JavaTemplate #5384
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
base: main
Are you sure you want to change the base?
Conversation
rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateGenericsTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateGenericsTest.java
Outdated
Show resolved
Hide resolved
Thanks again! Looking at the overlap in changes I think this PR should go first, right? |
Yes, that should go first. It adds some of the functionality removed in #5394. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions could not be made:
- rewrite-java-tck/src/main/java/org/openrewrite/java/tree/TypeUtilsTest.java
- lines 18-18
rewrite-java-tck/src/main/java/org/openrewrite/java/tree/TypeUtilsTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-tck/src/main/java/org/openrewrite/java/tree/TypeUtilsTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-tck/src/main/java/org/openrewrite/java/tree/TypeUtilsTest.java
Outdated
Show resolved
Hide resolved
@@ -974,4 +1186,54 @@ public static String toGenericTypeString(JavaType.GenericTypeVariable type) { | |||
return type.getName() + " extends " + bounds; | |||
} | |||
} | |||
|
|||
public static class TypeMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering how we can improve the naming here of TypeMode
, TypeMode.Mode
, TypeMode#get()
and possibly also the push()
and contains()
methods. I think that might help when trying to understand the code. Without having thought much about it, should we rename TypeMode.Mode
to TypeMode.Direction
and then possibly also get()
to direction()
? Or what ideas do you have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, naming things is hard 😅. I've pushed the changes to the other branch: #5420
I hope the new names make things a bit clearer.
The ComparisonContext has two main responsibilities:
-
It tracks the current comparisons using a stack. Since parameterized types can be recursive, it checks whether a type pair is already being compared to avoid infinite loops.
-
It tracks which side (if any) we're inferring types from during the comparison. Normally, the inference side is TO, but in some cases it might flip to FROM—for example, in
Comparator<? super Comparable<T>>
, thesuper
bound causes the sides to be swapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, naming is one of the 2 hard problems 😄 Let's then also add a test to make sure it works correctly for more complicated cases like Comparator<? super Comparable<List<? extends T>>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I merged the other PR to make it a bit easier to continue with this.
@@ -1046,4 +1046,62 @@ Predicate<Object> test() { | |||
) | |||
); | |||
} | |||
|
|||
@Test | |||
void matchMemberReferenceAndLambda() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to see a test which verifies that we can also use expressionType()
for JavaTemplate#apply()
to e.g. replace a lambda (or anonymous class) with a method reference as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of match I have use apply with the other template
* </ul> | ||
* In such cases, the type must be specified manually. | ||
*/ | ||
public Builder expressionType(String expressionType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if any of these names would be better: expectedType()
, bindType()
, or typeConstraint()
?
@@ -206,4 +289,118 @@ Stream<Integer> test() { | |||
) | |||
); | |||
} | |||
|
|||
@Test | |||
@ExpectedToFail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is failing because T::toString
, matches correctly the memberReference but then when visiting the containing visit(memberRef.getContaining(), compareTo.getContaining());
is visiting two identifiers T and Object and it fails to match equal names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can add an example without generics that will already be useful as a test.
} | ||
|
||
@Test | ||
@ExpectedToFail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one it's more to showcase a scenario we are not handling, even if the apply had arguments, T is not really a parameter, and #{type:any(T)}
would not match a JavaType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have some other kind of argument. #{type(T)}
where instead of using __P__.<T>any()
we could add the TypeUtils.toString()
that would also make the test above pass, being an argument the containing would match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered doing exactly this before, but I didn't yet see a use case for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have try this templates in error-prone refaster and they work. But I haven't found any use on the Picnic tests.
static final class RefToLambda<T> {
@BeforeTemplate
Function<T, String> before() {
return T::toString;
}
@AfterTemplate
Function<T, String> after() {
return e -> e.toString();
}
}
static final class LambdaToRef<T> {
@BeforeTemplate
Function<T, String> before() {
return e -> e.toString();
}
@AfterTemplate
Function<T, String> after() {
return T::toString;
}
}
---
ImmutableSet<Function<Object, String>> testRefToLambda() {
return ImmutableSet.of(
Object::toString);
}
ImmutableSet<Function<Object, String>> testLambdaToRef() {
return ImmutableSet.of(
e -> e.toString());
}
rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateGenericsTest.java
Outdated
Show resolved
Hide resolved
…ateGenericsTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
What's changed?
Added support for explicit setting
expressionType
inJavaTemplate
Have you considered any alternatives or workarounds?
It is possible to write code that doesn't require setting the expression type.
However, this approach presents challenges:
Checklist