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

KE2: Extract String.plus and String?.plus calls #17752

Merged
merged 3 commits into from
Oct 16, 2024
Merged

KE2: Extract String.plus and String?.plus calls #17752

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3a93d8
KE2: Extract `String.plus` and `String?.plus` calls
tamasvajk Oct 14, 2024
125797c
Improve code quality
tamasvajk Oct 14, 2024
7b198da
Improve code quality
tamasvajk Oct 15, 2024
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
23 changes: 0 additions & 23 deletions java/kotlin-extractor2/src/main/kotlin/KotlinFileExtractor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3695,29 +3695,6 @@ OLD: KE1

val dr = c.dispatchReceiver
when {
isFunction(target, "kotlin", "String", "plus", false) -> {
val id = tw.getFreshIdLabel<DbAddexpr>()
val type = useType(c.type)
tw.writeExprs_addexpr(id, type.javaResult.id, parent, idx)
tw.writeExprsKotlinType(id, type.kotlinResult.id)
binopDisp(id)
}
isFunction(target, "kotlin", "String", "plus", true) -> {
findJdkIntrinsicOrWarn("stringPlus", c)?.let { stringPlusFn ->
extractRawMethodAccess(
stringPlusFn,
c,
c.type,
callable,
parent,
idx,
enclosingStmt,
listOf(c.extensionReceiver, c.getValueArgument(0)),
null,
null
)
}
}
isNumericFunction(
target,
"plus",
Expand Down
75 changes: 61 additions & 14 deletions java/kotlin-extractor2/src/main/kotlin/entities/Expression.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@ import org.jetbrains.kotlin.analysis.api.resolution.KaSuccessCallInfo
import org.jetbrains.kotlin.analysis.api.resolution.symbol
import org.jetbrains.kotlin.analysis.api.symbols.KaFunctionSymbol
import org.jetbrains.kotlin.analysis.api.types.KaType
import org.jetbrains.kotlin.analysis.api.types.KaTypeNullability
import org.jetbrains.kotlin.analysis.api.types.symbol
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.name.CallableId
import org.jetbrains.kotlin.name.ClassId
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.name.Name
import org.jetbrains.kotlin.parsing.parseNumericLiteral
import org.jetbrains.kotlin.psi.*

Expand Down Expand Up @@ -215,25 +221,57 @@ OLD: KE1
}
*/

private fun isFunction(
private fun isFunctionMatchingNames(
symbol: KaFunctionSymbol,
name: CallableId,
isExtension: Boolean = false,
nullability: KaTypeNullability = KaTypeNullability.UNKNOWN,
extensionReceiverClassName: ClassId? = null
): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if this would read more clearly as an extension, so you'd have symbol.isFunctionMatchingNames(name, isExtension, nullability, extensionReceiverClassName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed an extra commit that changed the functions to extensions.


if (symbol.callableId != name)
return false

if (!isExtension)
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused what's going on here. Should this be if (!isExtension && receiverType == null)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reworked this a bit in an extra commit. My goal was to only use the last two parameters if isExtension==true.


val receiverType = symbol.receiverParameter?.type
if (receiverType == null)
return false

if (receiverType.nullability != nullability)
return false

if (receiverType.symbol?.classId != extensionReceiverClassName)
return false

return true
}

private fun isFunctionMatchingName(
symbol: KaFunctionSymbol,
packageName: String,
className: String,
className: String?,
functionName: String
): Boolean {

return symbol.callableId?.packageName?.asString() == packageName &&
symbol.callableId?.className?.asString() == className &&
symbol.callableId?.callableName?.asString() == functionName
return isFunctionMatchingNames(
symbol,
CallableId(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it would be better to avoid constructing anything from kotlinc if possible, especially if we are aiming to use concurrency, as then we don't have to worry about what side-effects etc there might be. Here I think we can just as easily destruct the CallableId at the other end, as construct it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CallableId and ClassId are tuples of 3 and 2 names. Should we introduce custom data classes instead of reusing the kotlin compiler types? I don't think we'd want to pass 3 + 2 String args instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just been looking at how we might handle this problem in general; I'm more interested in cases where we want to extract e.g. a generated function, for which the analysis API doesn't give us the AST and we have to fabricate it. I was hoping that we could have our own CodeQlFunction interface, and have it be implemented by KtFunction using something like delegates. However, it looks like that isn't possible; there have long been discussions and proposals (e.g. https://youtrack.jetbrains.com/issue/KT-505, Kotlin/KEEP#87) but no implementation yet.

Perhaps CallableId and ClassId are simple enough that we should just reuse them? Or at least, do that for now until we come to something like the generated-function problem?

FqName(packageName),
if (className == null) null else FqName(className),
Name.identifier(functionName)
)
)
}

private fun isNumericFunction(target: KaFunctionSymbol, fName: String): Boolean {
return isFunction(target, "kotlin", "Int", fName) ||
isFunction(target, "kotlin", "Byte", fName) ||
isFunction(target, "kotlin", "Short", fName) ||
isFunction(target, "kotlin", "Long", fName) ||
isFunction(target, "kotlin", "Float", fName) ||
isFunction(target, "kotlin", "Double", fName)
private fun isNumericFunction(target: KaFunctionSymbol, functionName: String): Boolean {
return isFunctionMatchingName(target, "kotlin", "Int", functionName) ||
isFunctionMatchingName(target, "kotlin", "Byte", functionName) ||
isFunctionMatchingName(target, "kotlin", "Short", functionName) ||
isFunctionMatchingName(target, "kotlin", "Long", functionName) ||
isFunctionMatchingName(target, "kotlin", "Float", functionName) ||
isFunctionMatchingName(target, "kotlin", "Double", functionName)
}

/**
Expand Down Expand Up @@ -267,7 +305,16 @@ private fun KotlinFileExtractor.extractBinaryExpression(
TODO()
}

if (isNumericFunction(target, "plus")) {
if (isNumericFunction(target, "plus") ||
isFunctionMatchingName(target, "kotlin", "String", "plus") ||
isFunctionMatchingNames(
target,
CallableId(FqName("kotlin"), null, Name.identifier("plus")),
isExtension = true,
nullability = KaTypeNullability.NULLABLE,
ClassId(FqName("kotlin"), Name.identifier("String"))
)
) {
val id = tw.getFreshIdLabel<DbAddexpr>()
val type = useType(expression.expressionType)
val exprParent = parent.expr(expression, callable)
Expand All @@ -278,7 +325,7 @@ private fun KotlinFileExtractor.extractBinaryExpression(
extractExpressionExpr(expression.left!!, callable, id, 0, exprParent.enclosingStmt)
extractExpressionExpr(expression.right!!, callable, id, 1, exprParent.enclosingStmt)
} else {
TODO()
TODO("Extract as method call")
}
}

Expand Down
Loading