Skip to content

KE2: Extract binary plus on numeric types #17729

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

Merged
merged 2 commits into from
Oct 11, 2024
Merged

KE2: Extract binary plus on numeric types #17729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e8b20d
KE2: Extract binary plus on numeric types
tamasvajk Oct 10, 2024
ea68837
Apply review findings
tamasvajk Oct 11, 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
17 changes: 0 additions & 17 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 @@ -3515,18 +3515,6 @@ OLD: KE1
isNullable: Boolean? = false
) = isFunction(target, pkgName, className, { it == className }, fName, isNullable)

private fun isNumericFunction(target: IrFunction, 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: IrFunction, vararg fNames: String) =
fNames.any { isNumericFunction(target, it) }

private fun isArrayType(typeName: String) =
when (typeName) {
"Array" -> true
Expand Down Expand Up @@ -3747,11 +3735,6 @@ OLD: KE1
val type = useType(c.type)
val id: Label<out DbExpr> =
when (val targetName = target.name.asString()) {
"plus" -> {
val id = tw.getFreshIdLabel<DbAddexpr>()
tw.writeExprs_addexpr(id, type.javaResult.id, parent, idx)
id
}
"minus" -> {
val id = tw.getFreshIdLabel<DbSubexpr>()
tw.writeExprs_subexpr(id, type.javaResult.id, parent, idx)
Expand Down
80 changes: 80 additions & 0 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 @@ -3,6 +3,10 @@ package com.github.codeql
import com.github.codeql.KotlinFileExtractor.StmtExprParent
import org.jetbrains.kotlin.KtNodeTypes
import org.jetbrains.kotlin.analysis.api.KaSession
import org.jetbrains.kotlin.analysis.api.resolution.KaSimpleFunctionCall
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.lexer.KtTokens
import org.jetbrains.kotlin.parsing.parseNumericLiteral
Expand Down Expand Up @@ -211,6 +215,78 @@ OLD: KE1
}
*/

private fun isFunction(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if something like isFunctionNamed would be clearer. And maybe add a comment

/**
 * True iff `symbol` is the function `packageName.className::functionName`
 */

(I'm not sure if I have the best punctuation in packageName.className::functionName OTTOMH)

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 KE1 equivalent of this function has some extra parameters, such as nullability, so adding Named would not necessarily make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK; I don't like the name isFunction, because it sounds like it tells you whether something is a function or something else (e.g. a class), but perhaps it's too early to pick the best name.

symbol: KaFunctionSymbol,
packageName: String,
className: String,
functionName: String
): Boolean {

return symbol.callableId?.packageName?.asString() == packageName &&
symbol.callableId?.className?.asString() == className &&
symbol.callableId?.callableName?.asString() == 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)
}

/**
* Extracts a binary expression as either a binary expression or a function call.
*
* Overloaded operators are extracted as function calls.
*
* ```
* data class Counter(val dayIndex: Int) {
* operator fun plus(increment: Int): Counter {
* return Counter(dayIndex + increment)
* }
* }
* ```
*
* `Counter(1) + 3` is extracted as `Counter(1).plus(3)`.
*
*/
context(KaSession)
private fun KotlinFileExtractor.extractBinaryExpression(
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 a doc comment here would be useful, saying that sometimes these map to primitives like DbAddExpr, whereas at other times they are overloads (and give an example of how an overload of plus is defined in Kotlin).

expression: KtBinaryExpression,
callable: Label<out DbCallable>,
parent: StmtExprParent
) {
val op = expression.operationToken
val target = ((expression.resolveToCall() as? KaSuccessCallInfo)?.call as? KaSimpleFunctionCall)?.symbol

when (op) {
KtTokens.PLUS -> {
if (target == null) {
TODO()
}

if (isNumericFunction(target, "plus")) {
val id = tw.getFreshIdLabel<DbAddexpr>()
val type = useType(expression.expressionType)
val exprParent = parent.expr(expression, callable)
tw.writeExprs_addexpr(id, type.javaResult.id, exprParent.parent, exprParent.idx)
tw.writeExprsKotlinType(id, type.kotlinResult.id)

extractExprContext(id, tw.getLocation(expression), callable, exprParent.enclosingStmt)
extractExpressionExpr(expression.left!!, callable, id, 0, exprParent.enclosingStmt)
extractExpressionExpr(expression.right!!, callable, id, 1, exprParent.enclosingStmt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we going to want to do these 3 lines in every case, after the when (op) has finished?

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 don't know, maybe. In many cases X + Y will be extracted as X.opPlus(Y), so as a method call. Would we want to handle this case together with the above? Maybe we could, because the child indices are the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point. Maybe best to leave it as-is for now and possibly refactor later.

} else {
TODO()
}
}

else -> TODO()
}

}

context(KaSession)
private fun KotlinFileExtractor.extractExpression(
e: KtExpression,
Expand All @@ -225,6 +301,10 @@ private fun KotlinFileExtractor.extractExpression(
extractExpression(e.baseExpression!!, callable, parent)
}

is KtBinaryExpression -> {
extractBinaryExpression(e, callable, parent)
}

is KtIsExpression -> {

val locId = tw.getLocation(e)
Expand Down
Loading