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.

Sign up for GitHub

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

top-level function declaration type fixes. #339

Merged
merged 5 commits into from
Oct 11, 2023
Merged

top-level function declaration type fixes. #339

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adacc44
Fixed signature and type mapping for return types of ConeClassLikeType.
traceyyoshima Oct 10, 2023
fdd57b0
Fixed GenericTypeVariable name for ConeKotlinTypeProjectIn and ConeKo…
traceyyoshima Oct 10, 2023
2cb2623
Fixed type mapping for ConeIntersectionType.
traceyyoshima Oct 10, 2023
e9af625
Fixed GTV name of kotlin intersection type.
traceyyoshima Oct 10, 2023
fb9104a
Updated generic type tests with names.
traceyyoshima Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/kotlin/KotlinTypeGoat.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ package org.openrewrite.kotlin
import java.lang.Object

const val field = 10
fun function() {}
fun function(arg: C) {}

@AnnotationWithRuntimeRetention
@AnnotationWithSourceRetention
Expand Down
34 changes: 25 additions & 9 deletions src/main/kotlin/org/openrewrite/kotlin/KotlinTypeMapping.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,17 @@ import org.openrewrite.kotlin.KotlinTypeSignatureBuilder.Companion.convertFileNa
import org.openrewrite.kotlin.KotlinTypeSignatureBuilder.Companion.convertKotlinFqToJavaFq

@Incubating(since = "0.0")
class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : JavaTypeMapping<Any> {
class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession, firFileSymbol: FirFileSymbol) : JavaTypeMapping<Any> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ownerStack will be wrong without access to the file when a SimpleFunction is on the top of the stack for the parameters in a top-level method declaration.

private val signatureBuilder: KotlinTypeSignatureBuilder
private val typeCache: JavaTypeCache
private val firSession: FirSession
private val firFileSymbol: FirFileSymbol

init {
signatureBuilder = KotlinTypeSignatureBuilder(firSession)
signatureBuilder = KotlinTypeSignatureBuilder(firSession, firFileSymbol)
this.typeCache = typeCache
this.firSession = firSession
this.firFileSymbol = firFileSymbol
}

override fun type(type: Any?): JavaType {
Expand Down Expand Up @@ -184,6 +186,8 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
if (classifierSymbol != null && classifierSymbol.fir is FirTypeParameter) {
return resolveConeTypeProjection(classifierSymbol.fir as FirTypeParameter, signature)
}
} else if (coneKotlinType is ConeClassLikeType) {
return type(coneKotlinType, ownerFallBack)
}
return classType(type, signature, ownerFallBack)
}
Expand Down Expand Up @@ -546,7 +550,7 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
}
}
if (resolvedDeclaringType == null) {
return null
resolvedDeclaringType = TypeUtils.asFullyQualified(type(firFileSymbol.fir))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knutwannheden I needed to figure out where we should fall back to the FirFileSymbol. This "could" technically set the incorrect owner if we don't assign the owner appropriately, but it also isolates incorrect types to each file.

Curious, what do you think about falling back onto the FirFileSymbol by default?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, what do you think about falling back onto the FirFileSymbol by default?

I think that sounds reasonable. I wanted to do something similar myself, but first wanted to discuss that with you. So instead I added this owner stack, which is easy enough to remove again.

}
val returnType =
if (function is FirJavaMethod) type(methodSymbol.fir.returnTypeRef) else type(methodSymbol.resolvedReturnTypeRef)
Expand Down Expand Up @@ -844,7 +848,7 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
}
}
if (resolvedDeclaringType == null) {
return null
resolvedDeclaringType = TypeUtils.asFullyQualified(type(firFileSymbol.fir))
}
val returnType = type(functionCall.typeRef, ownerSymbol)
method.unsafeSet(
Expand Down Expand Up @@ -882,6 +886,8 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
// TODO: fix type attribution for anonymous objects. Currently, returns JavaType.Unknown.
if (symbol.dispatchReceiverType != null) {
resolvedOwner = type(symbol.dispatchReceiverType)
} else if (ownerFallBack is FirFunctionSymbol<*>) {
resolvedOwner = methodDeclarationType(ownerFallBack.fir, null, firFileSymbol)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature is set correctly to the method declaration signature. This change fixes the JavaType returned by the signature.

} else if (symbol.getContainingClassSymbol(firSession) != null) {
if (symbol.getContainingClassSymbol(firSession) !is FirAnonymousObjectSymbol) {
resolvedOwner = type(symbol.getContainingClassSymbol(firSession)!!.fir)
Expand All @@ -895,11 +901,11 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
resolvedOwner = type(ownerFallBack.fir)
}
if (resolvedOwner == null) {
resolvedOwner = JavaType.Unknown.getInstance()
resolvedOwner = type(firFileSymbol)
}
val typeRef =
if (symbol.fir is FirJavaField || symbol.fir is FirEnumEntry) symbol.fir.returnTypeRef else symbol.resolvedReturnTypeRef
variable.unsafeSet(resolvedOwner!!, type(typeRef), annotations)
variable.unsafeSet(resolvedOwner, type(typeRef), annotations)
return variable
}

Expand Down Expand Up @@ -999,19 +1005,24 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
val isGeneric = type is ConeKotlinTypeProjectionIn ||
type is ConeKotlinTypeProjectionOut ||
type is ConeStarProjection ||
type is ConeTypeParameterType
type is ConeTypeParameterType ||
type is ConeIntersectionType ||
type is ConeCapturedType
if (isGeneric) {
var variance: JavaType.GenericTypeVariable.Variance = JavaType.GenericTypeVariable.Variance.INVARIANT
var bounds: MutableList<JavaType>? = null
val name: String = when (type) {
is ConeKotlinTypeProjectionIn, is ConeKotlinTypeProjectionOut -> {
""
"?"
}

is ConeStarProjection -> {
is ConeStarProjection, is ConeCapturedType -> {
"*"
}

is ConeIntersectionType -> {
""
}
else -> {
type.toString()
}
Expand Down Expand Up @@ -1049,6 +1060,11 @@ class KotlinTypeMapping(typeCache: JavaTypeCache, firSession: FirSession) : Java
}
}
}
} else if (type is ConeIntersectionType) {
bounds = ArrayList(type.intersectedTypes.size)
for (t: ConeTypeProjection in type.intersectedTypes) {
bounds.add(type(t))
}
}
gtv.unsafeSet(name, variance, bounds)
resolvedType = gtv
Expand Down
11 changes: 9 additions & 2 deletions src/main/kotlin/org/openrewrite/kotlin/KotlinTypeSignatureBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import org.openrewrite.internal.lang.Nullable
import org.openrewrite.java.JavaTypeSignatureBuilder
import java.util.*

class KotlinTypeSignatureBuilder(private val firSession: FirSession) : JavaTypeSignatureBuilder {
class KotlinTypeSignatureBuilder(private val firSession: FirSession, private val firFileSymbol: FirFileSymbol) : JavaTypeSignatureBuilder {
private var typeVariableNameStack: MutableSet<String>? = null
override fun signature(type: @Nullable Any?): String {
return signature(type, null)
Expand Down Expand Up @@ -167,6 +167,8 @@ class KotlinTypeSignatureBuilder(private val firSession: FirSession) : JavaTypeS
} else {
parameterizedTypeRef(coneKotlinType.lowerBound)
}
} else if (coneKotlinType is ConeClassLikeType) {
return signature(coneKotlinType)
}
return if (coneKotlinType.typeArguments.isNotEmpty()) {
parameterizedTypeRef(coneKotlinType)
Expand Down Expand Up @@ -576,6 +578,8 @@ class KotlinTypeSignatureBuilder(private val firSession: FirSession) : JavaTypeS
owner = convertFileNameToFqn(ownerSymbol.fir.name)
} else if (ownerSymbol != null) {
owner = classSignature(ownerSymbol.fir)
} else {
owner = convertFileNameToFqn(firFileSymbol.fir.name)
}
val typeSig =
if (symbol.fir is FirJavaField || symbol.fir is FirEnumEntry) {
Expand Down Expand Up @@ -642,6 +646,9 @@ class KotlinTypeSignatureBuilder(private val firSession: FirSession) : JavaTypeS
}
}
}
if (owner == "{undefined}") {
owner = convertFileNameToFqn(firFileSymbol.fir.name)
}
var s = owner
val namedReference = functionCall.calleeReference
s += if (namedReference is FirResolvedNamedReference &&
Expand Down Expand Up @@ -676,7 +683,7 @@ class KotlinTypeSignatureBuilder(private val firSession: FirSession) : JavaTypeS
signature(ownerSymbol.fir)
}
else -> {
"{undefined}"
convertFileNameToFqn(firFileSymbol.fir.name)
}
}
s += if (symbol is FirConstructorSymbol) {
Expand Down
3 changes: 1 addition & 2 deletions src/main/kotlin/org/openrewrite/kotlin/internal/KotlinParserVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import org.jetbrains.kotlin.fir.expressions.impl.FirSingleExpressionBlock
import org.jetbrains.kotlin.fir.expressions.impl.FirUnitExpression
import org.jetbrains.kotlin.fir.references.*
import org.jetbrains.kotlin.fir.resolve.toFirRegularClassSymbol
import org.jetbrains.kotlin.fir.resolve.toSymbol
import org.jetbrains.kotlin.fir.symbols.ConeClassLikeLookupTag
import org.jetbrains.kotlin.fir.symbols.FirBasedSymbol
import org.jetbrains.kotlin.fir.symbols.SymbolInternals
Expand Down Expand Up @@ -123,7 +122,7 @@ class KotlinParserVisitor(
charset = `is`.charset
charsetBomMarked = `is`.isCharsetBomMarked
this.styles = styles
typeMapping = KotlinTypeMapping(typeCache, firSession)
typeMapping = KotlinTypeMapping(typeCache, firSession, kotlinSource.firFile!!.symbol)
this.data = data
this.firSession = firSession
this.nodes = kotlinSource.nodes
Expand Down
36 changes: 33 additions & 3 deletions src/test/java/org/openrewrite/kotlin/KotlinTypeMappingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
*/
package org.openrewrite.kotlin;

import org.jetbrains.kotlin.fir.declarations.FirProperty;
import org.jetbrains.kotlin.fir.declarations.FirSimpleFunction;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.openrewrite.InMemoryExecutionContext;
Expand Down Expand Up @@ -157,7 +155,13 @@ void fileFunction() {

assertThat(md.getName().getType()).isEqualTo(md.getMethodType());
assertThat(md.getMethodType().toString())
.isEqualTo("KotlinTypeGoatKt{name=function,return=kotlin.Unit,parameters=[]}");
.isEqualTo("KotlinTypeGoatKt{name=function,return=kotlin.Unit,parameters=[org.openrewrite.kotlin.C]}");

J.VariableDeclarations.NamedVariable nv = ((J.VariableDeclarations) md.getParameters().get(0)).getVariables().get(0);
assertThat(nv.getVariableType()).isEqualTo(nv.getName().getFieldType());
assertThat(nv.getType().toString()).isEqualTo("org.openrewrite.kotlin.C");
assertThat(nv.getVariableType().toString())
.isEqualTo("KotlinTypeGoatKt{name=function,return=kotlin.Unit,parameters=[org.openrewrite.kotlin.C]}{name=arg,type=org.openrewrite.kotlin.C}");
}

@Test
Expand Down Expand Up @@ -365,6 +369,32 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Atomi
);
}

@Test
void genericIntersectionType() {
rewriteRun(
kotlin(
"""
val l = listOf ( "foo" to "1" , "bar" to 2 )
""", spec -> spec.afterRecipe(cu -> {
MethodMatcher methodMatcher = new MethodMatcher("kotlin.collections.CollectionsKt listOf(..)");
AtomicBoolean found = new AtomicBoolean(false);
new KotlinIsoVisitor<AtomicBoolean>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicBoolean atomicBoolean) {
if (methodMatcher.matches(method)) {
assertThat(method.getMethodType().toString())
.isEqualTo("kotlin.collections.CollectionsKt{name=listOf,return=kotlin.collections.List<kotlin.Pair<kotlin.String, Generic{kotlin.Comparable<Generic{*}> & java.io.Serializable}>>,parameters=[kotlin.Array<Generic{? extends Generic{T}}>]}");
Copy link
Contributor Author

@traceyyoshima traceyyoshima Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so you know -- In Java, a generic type intersection contains the bounded generic name like T, V, U. So the type becomes Generic{<name> extends <typeA> & <typeB>}.

In Kotlin, the ConeIntersectionType contains a generic name it and appears as it(<typeA>, <typeB>). So, I left the name out. It's better if we add the name of the Generic type variable. So, I'll check on that tomorrow.

found.set(true);
}
return super.visitMethodInvocation(method, atomicBoolean);
}
}.visit(cu, found);
assertThat(found.get()).isTrue();
})
)
);
}

@Test
void coneProjection() {
rewriteRun(
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/openrewrite/kotlin/KotlinTypeSignatureBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ static void afterAll() {
}

public KotlinTypeSignatureBuilder signatureBuilder() {
return new KotlinTypeSignatureBuilder(compiledSource.getFirSession());
return new KotlinTypeSignatureBuilder(compiledSource.getFirSession(), compiledSource.getSources().iterator().next().getFirFile().getSymbol());
}

private FirFile getCompiledSource() {
Expand Down Expand Up @@ -181,7 +181,7 @@ void fileFunction() {
.filter(it -> it instanceof FirSimpleFunction && "function".equals(((FirSimpleFunction) it).getName().asString()))
.map(it -> (FirSimpleFunction) it).findFirst().orElseThrow();
assertThat(signatureBuilder().methodDeclarationSignature(function.getSymbol(), getCompiledSource().getSymbol()))
.isEqualTo("KotlinTypeGoatKt{name=function,return=kotlin.Unit,parameters=[]}");
.isEqualTo("KotlinTypeGoatKt{name=function,return=kotlin.Unit,parameters=[org.openrewrite.kotlin.C]}");
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion src/test/resources/KotlinTypeGoat.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ package org.openrewrite.kotlin
import java.lang.Object

const val field = 10
fun function() {}
fun function(arg: C) {}

@AnnotationWithRuntimeRetention
@AnnotationWithSourceRetention
Expand Down