-
Notifications
You must be signed in to change notification settings - Fork 15
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
Method matcher should match Java primitive types in Kotlin #626
base: main
Are you sure you want to change the base?
Conversation
I've briefly explored doing a method lookup in the declared type, but it's not pretty, and not sure if such an approach will fully work: private JavaType.@Nullable Method methodInvocationType(PsiElement psi) {
JavaType.Method psiMethodType = psiElementAssociations.methodInvocationType(psi);
// Look for method in the declaring type to find the correct method parameter types, not the Kotlin equivalents
JavaType.FullyQualified declaringType = psiMethodType.getDeclaringType();
for (JavaType.Method m : declaringType.getMethods()) {
if (m.getName().equals(psiMethodType.getName())) {
List<JavaType> declParameterTypes = m.getParameterTypes();
List<JavaType> psiParameterTypes = psiMethodType.getParameterTypes();
if (declParameterTypes.size() == psiParameterTypes.size()) {
for (int i = 0; i < declParameterTypes.size(); i++) {
JavaType declType = declParameterTypes.get(i);
JavaType psiType = psiParameterTypes.get(i);
// TODO compare types JavaType.Primitive.Int and JavaType.Class kotlin.Int, and similar friends
}
}
}
}
return psiMethodType;
} |
Alternatively we can map Kotlin Primitives to JavaType.Primitive, instead of the Java.Class instances they are now; That would mean recipes written for Java that might use these primitives in their method patterns would match again. We could extend this to only do so for types coming from Java if needed / desired, but there does not appear to be an easy way to tell them apart from the conflicting signals coming back on the declared type (Java classes have super type kotlin.Any for instance). Let's agree to this approach before I update the failures in
|
Any thoughts as to the |
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:
- src/test/java/org/openrewrite/kotlin/ChangeTypeTest.java
- lines 18-18
What's changed?
Add a test to check that we can match Java primitive types in Kotlin
What's your motivation?
Saw that we do not match, as the method type uses
kotlin.Int
, not any Javaint
, which we then fail to match.The declaring type does contain methods with the correct types, so perhaps we should look up the matching method as opposed to trusting the association look up.
rewrite-kotlin/src/main/java/org/openrewrite/kotlin/internal/KotlinTreeParserVisitor.java
Lines 3652 to 3654 in 181a00d