From 146f87a0662002aba49df0ab9cad9ec9e090572b Mon Sep 17 00:00:00 2001 From: Evgeny Mandrikov Date: Fri, 31 Jan 2025 11:03:00 +0100 Subject: [PATCH 1/2] (WIP) FunMatcher --- .../EqualsOverridenWithHashCodeCheckSample.kt | 7 +- .../kotlin/api/checks/FunMatcher.kt | 101 +++++++++++++----- 2 files changed, 82 insertions(+), 26 deletions(-) diff --git a/kotlin-checks-test-sources/src/main/kotlin/checks/EqualsOverridenWithHashCodeCheckSample.kt b/kotlin-checks-test-sources/src/main/kotlin/checks/EqualsOverridenWithHashCodeCheckSample.kt index bd18588c3..3460d62c9 100644 --- a/kotlin-checks-test-sources/src/main/kotlin/checks/EqualsOverridenWithHashCodeCheckSample.kt +++ b/kotlin-checks-test-sources/src/main/kotlin/checks/EqualsOverridenWithHashCodeCheckSample.kt @@ -2,7 +2,6 @@ package checks class EqualsOverridenWithHashCodeCheckSample { - override fun equals(other: Any?): Boolean { // Noncompliant {{This class overrides "equals()" and should therefore also override "hashCode()".}} return super.equals(other) } @@ -89,3 +88,9 @@ abstract class AmbiguousParent3 { return super.equals(other) } } + +private val testAnonymousClass = object : Any() { + override fun equals(other: Any?): Boolean { // Noncompliant + return super.equals(other) + } +} diff --git a/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/FunMatcher.kt b/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/FunMatcher.kt index 189dad594..58e229d87 100644 --- a/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/FunMatcher.kt +++ b/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/FunMatcher.kt @@ -20,12 +20,17 @@ import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall import org.jetbrains.kotlin.analysis.api.resolution.KaFunctionCall import org.jetbrains.kotlin.analysis.api.resolution.KaReceiverValue -import org.jetbrains.kotlin.analysis.api.resolution.KaVariableAccessCall +import org.jetbrains.kotlin.analysis.api.resolution.KaSimpleVariableAccess +import org.jetbrains.kotlin.analysis.api.resolution.KaSimpleVariableAccessCall import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull +import org.jetbrains.kotlin.analysis.api.resolution.symbol import org.jetbrains.kotlin.analysis.api.signatures.KaCallableSignature +import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol import org.jetbrains.kotlin.analysis.api.symbols.KaConstructorSymbol import org.jetbrains.kotlin.analysis.api.symbols.KaFunctionSymbol import org.jetbrains.kotlin.analysis.api.symbols.KaNamedFunctionSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaPropertySymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaSyntheticJavaPropertySymbol import org.jetbrains.kotlin.analysis.api.symbols.name import org.jetbrains.kotlin.analysis.api.types.KaClassType import org.jetbrains.kotlin.backend.common.descriptors.isSuspend @@ -39,9 +44,9 @@ import org.jetbrains.kotlin.psi.KtCallExpression import org.jetbrains.kotlin.psi.KtNamedFunction import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.BindingContext.RESOLVED_CALL -import org.jetbrains.kotlin.resolve.calls.util.getCall import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall import org.jetbrains.kotlin.resolve.calls.tasks.isDynamic +import org.jetbrains.kotlin.resolve.calls.util.getCall import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe import org.jetbrains.kotlin.resolve.descriptorUtil.isExtension import org.jetbrains.kotlin.resolve.descriptorUtil.overriddenTreeUniqueAsSequence @@ -91,7 +96,8 @@ class FunMatcherImpl( @OptIn(KaExperimentalApi::class) fun matches(node: KtNamedFunction): Boolean = withKaSession { - return matches(null, node.symbol.asSignature()) + val symbol = node.symbol + return matches(null, symbol, symbol.asSignature()) } @Deprecated("use kotlin-analysis-api instead") @@ -102,17 +108,57 @@ class FunMatcherImpl( fun matches(resolvedCall: ResolvedCall<*>?) = preCheckArgumentCount(resolvedCall?.call) && matches(resolvedCall?.resultingDescriptor) - fun matches(resolvedCall: KaCallableMemberCall<*, *>?) = + @OptIn(KaExperimentalApi::class) + fun matches(resolvedCall: KaCallableMemberCall<*, *>?): Boolean = withKaSession { when (resolvedCall) { - is KaFunctionCall -> matches( - resolvedCall.partiallyAppliedSymbol.dispatchReceiver, - resolvedCall.partiallyAppliedSymbol.signature - ) - is KaVariableAccessCall -> - // TODO handle calls of getters and setters - false - else -> false + null -> false + is KaFunctionCall -> { + val symbol = resolvedCall.partiallyAppliedSymbol + matches(symbol.dispatchReceiver, symbol.signature.symbol, symbol.signature) + } + is KaSimpleVariableAccessCall -> { + val propertySymbol = (resolvedCall.symbol as? KaPropertySymbol) ?: return false + when (resolvedCall.simpleAccess) { + is KaSimpleVariableAccess.Read -> { + val symbolForNameCheck = + /** + * Note that this allows to use matcher with name `"getExample"` + * for both function call `getExample(...)` and read by property name `example`. + */ + if (propertySymbol is KaSyntheticJavaPropertySymbol) + propertySymbol.javaGetterSymbol + else + propertySymbol + val getterSignature = propertySymbol.getter?.asSignature() ?: return false + checkName(symbolForNameCheck) && + checkCallParameters(getterSignature) && + checkReturnType(getterSignature) && + checkTypeOrSupertype(null, propertySymbol) + } + is KaSimpleVariableAccess.Write -> { + val symbolForNameCheck = + /** + * Note that this allows to use matcher with name `"setExample"` + * for both function call `setExample(...)` and write by property name `example`. + */ + if (propertySymbol is KaSyntheticJavaPropertySymbol) + propertySymbol.javaSetterSymbol ?: return false + else + propertySymbol + val setterSignature = propertySymbol.setter?.asSignature() ?: return false + checkName(symbolForNameCheck) && + checkCallParameters(setterSignature) && + checkReturnType(setterSignature) && + checkTypeOrSupertype( + null, + // TODO propertySymbol works only in K2 (see ScheduledThreadPoolExecutorZeroCheck): + symbolForNameCheck + ) + } + } + } } + } private fun preCheckArgumentCount(call: Call?) = call == null || call.valueArguments.size <= maxArgumentCount @@ -130,6 +176,7 @@ class FunMatcherImpl( private fun matches( dispatchReceiver: KaReceiverValue?, + symbol: KaCallableSymbol, callableSignature: KaCallableSignature<*> ): Boolean { if (isDynamic != null) TODO() @@ -138,8 +185,8 @@ class FunMatcherImpl( checkIsSuspending(callableSignature) && checkIsOperator(callableSignature) && checkReturnType(callableSignature) && - checkName(callableSignature) && - checkTypeOrSupertype(dispatchReceiver, callableSignature) && + checkName(symbol) && + checkTypeOrSupertype(dispatchReceiver, symbol) && checkCallParameters(callableSignature) } @@ -150,7 +197,7 @@ class FunMatcherImpl( private fun checkTypeOrSupertype( dispatchReceiver: KaReceiverValue?, - callableSignature: KaCallableSignature<*>, + symbol: KaCallableSymbol, ): Boolean { if (qualifiersOrDefiningSupertypes.isEmpty()) return true @@ -158,9 +205,9 @@ class FunMatcherImpl( // java.lang.Math.random has kotlin/Unit receiver type in K2 and null in K1? val receiverFQN = (dispatchReceiver?.type as? KaClassType)?.classId?.asFqNameString(); if (qualifiersOrDefiningSupertypes.contains(receiverFQN)) return true - if (qualifiersOrDefiningSupertypes.contains(getActualQualifier(callableSignature))) return true + if (qualifiersOrDefiningSupertypes.contains(getActualQualifier(symbol))) return true - if (!definingSupertypes.isNullOrEmpty() && checkSubType(callableSignature)) return true + if (!definingSupertypes.isNullOrEmpty() && checkSubType(symbol)) return true return false } @@ -174,8 +221,7 @@ class FunMatcherImpl( // TODO when null? /** @return dot-separated package name for top-level functions, class name otherwise */ - private fun getActualQualifier(callableSignature: KaCallableSignature<*>): String? { - val symbol = callableSignature.symbol + private fun getActualQualifier(symbol: KaCallableSymbol): String? { return if (symbol is KaConstructorSymbol) { symbol.containingClassId?.asFqNameString() } else { @@ -201,9 +247,9 @@ class FunMatcherImpl( } } - private fun checkSubType(callableSignature: KaCallableSignature<*>): Boolean = withKaSession { - if (callableSignature.symbol is KaConstructorSymbol) return false - return callableSignature.symbol.allOverriddenSymbols.any { + private fun checkSubType(symbol: KaCallableSymbol): Boolean = withKaSession { + if (symbol is KaConstructorSymbol) return false + return symbol.allOverriddenSymbols.any { val className: String? = it.callableId?.asSingleFqName()?.parent()?.asString() definingSupertypes.contains(className) } @@ -218,9 +264,9 @@ class FunMatcherImpl( (nameRegex == null && names.isEmpty()) || name in names || (nameRegex != null && nameRegex.matches(name)) } else false - private fun checkName(callableSignature: KaCallableSignature<*>): Boolean { - if (matchConstructor) return callableSignature.symbol is KaConstructorSymbol - val name = callableSignature.symbol.name?.asString() ?: return false + private fun checkName(symbol: KaCallableSymbol): Boolean { + if (matchConstructor) return symbol is KaConstructorSymbol + val name = symbol.name?.asString() ?: return false return (nameRegex == null && names.isEmpty()) || name in names || (nameRegex != null && nameRegex.matches(name)) } @@ -265,6 +311,11 @@ class FunMatcherImpl( else null } ?: true + /** + * Note that [KaCallableSignature] carries use-site type information, + * so for `fun example(): T` matcher with [returnType] `"kotlin.String"` + * matches calls of `example()`. + */ private fun checkReturnType(callableSignature: KaCallableSignature<*>) = returnType?.let { it == callableSignature.returnType.asFqNameString() From a7454d83cb1e5bae73cad4a95b24d451c629570e Mon Sep 17 00:00:00 2001 From: Evgeny Mandrikov Date: Sun, 2 Feb 2025 23:58:18 +0100 Subject: [PATCH 2/2] Migrate UnpredictableHashSaltCheck to kotlin-analysis-api --- .../kotlin/api/checks/ApiExtensions.kt | 25 +++++++++++++++ .../checks/UnpredictableHashSaltCheck.kt | 31 +++++++++---------- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt b/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt index f356a4b2f..97990c3c7 100644 --- a/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt +++ b/sonar-kotlin-api/src/main/java/org/sonarsource/kotlin/api/checks/ApiExtensions.kt @@ -23,9 +23,11 @@ import com.intellij.psi.impl.source.tree.LeafPsiElement import com.intellij.psi.util.PsiTreeUtil import org.jetbrains.kotlin.analysis.api.KaIdeApi import org.jetbrains.kotlin.analysis.api.resolution.KaCall +import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall import org.jetbrains.kotlin.analysis.api.resolution.KaExplicitReceiverValue import org.jetbrains.kotlin.analysis.api.resolution.KaFunctionCall import org.jetbrains.kotlin.analysis.api.resolution.KaImplicitReceiverValue +import org.jetbrains.kotlin.analysis.api.resolution.singleCallOrNull import org.jetbrains.kotlin.analysis.api.resolution.singleFunctionCallOrNull import org.jetbrains.kotlin.analysis.api.resolution.successfulCallOrNull import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull @@ -689,6 +691,7 @@ fun ResolvedValueArgument.isNull(bindingContext: BindingContext) = ( fun KtExpression.isPredictedNull() = predictRuntimeValueExpression().isNull() +@Deprecated("use kotlin-analysis-api instead") fun KtExpression.getCalleeOrUnwrappedGetMethod(bindingContext: BindingContext) = (this as? KtDotQualifiedExpression)?.let { dotQualifiedExpression -> (dotQualifiedExpression.selectorExpression as? KtNameReferenceExpression)?.let { nameSelector -> @@ -699,12 +702,24 @@ fun KtExpression.getCalleeOrUnwrappedGetMethod(bindingContext: BindingContext) = /** * Checks whether the expression is a call, matches the FunMatchers in [STRING_TO_BYTE_FUNS] and is called on a constant string value. */ +@Deprecated("use kotlin-analysis-api instead", ReplaceWith("this.isBytesInitializedFromString()")) fun KtExpression.isBytesInitializedFromString(bindingContext: BindingContext) = getCalleeOrUnwrappedGetMethod(bindingContext)?.let { callee -> STRING_TO_BYTE_FUNS.any { it.matches(callee) } && (getCall(bindingContext)?.explicitReceiver as? ExpressionReceiver)?.expression?.predictRuntimeStringValue(bindingContext) != null } ?: false +fun KtExpression.isBytesInitializedFromString(): Boolean = withKaSession { + val call = this@isBytesInitializedFromString.resolveToCall()?.successfulCallOrNull>() + ?: return@withKaSession false + STRING_TO_BYTE_FUNS.any { it.matches(call) } && + ((call.partiallyAppliedSymbol.extensionReceiver ?: call.partiallyAppliedSymbol.dispatchReceiver) + as? KaExplicitReceiverValue) + ?.expression + ?.predictRuntimeStringValue() != null +} + +@Deprecated("use kotlin-analysis-api instead") fun ResolvedCall<*>.simpleArgExpressionOrNull(index: Int) = this.valueArgumentsByIndex ?.getOrNull(index) @@ -762,12 +777,22 @@ fun KtProperty.findUsages( /** * Checks whether the variable has been initialized with the help of a secure random function */ +@Deprecated("use kotlin-analysis-api instead", ReplaceWith("this.isInitializedPredictably(searchStartNode)")) fun KtExpression.isInitializedPredictably(searchStartNode: KtExpression, bindingContext: BindingContext): Boolean { return this !is KtNameReferenceExpression || this.findUsages(searchStartNode) { it.getParentOfType(false).getResolvedCall(bindingContext) matches SECURE_RANDOM_FUNS }.isEmpty() } +fun KtExpression.isInitializedPredictably(searchStartNode: KtExpression): Boolean { + return this !is KtNameReferenceExpression || this.findUsages(searchStartNode) { + withKaSession { + it.getParentOfType(false)?.resolveToCall() + ?.successfulFunctionCallOrNull() matches SECURE_RANDOM_FUNS + } + }.isEmpty() +} + /** * Checks if an expression is a function local variable */ diff --git a/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/UnpredictableHashSaltCheck.kt b/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/UnpredictableHashSaltCheck.kt index 86be89cfd..3c0c881df 100644 --- a/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/UnpredictableHashSaltCheck.kt +++ b/sonar-kotlin-checks/src/main/java/org/sonarsource/kotlin/checks/UnpredictableHashSaltCheck.kt @@ -16,24 +16,24 @@ */ package org.sonarsource.kotlin.checks +import org.jetbrains.kotlin.analysis.api.resolution.KaFunctionCall +import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull import org.jetbrains.kotlin.psi.KtCallExpression -import org.jetbrains.kotlin.resolve.BindingContext -import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall -import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall import org.sonar.check.Rule import org.sonarsource.kotlin.api.checks.BYTE_ARRAY_CONSTRUCTOR import org.sonarsource.kotlin.api.checks.BYTE_ARRAY_CONSTRUCTOR_SIZE_ARG_ONLY import org.sonarsource.kotlin.api.checks.CallAbstractCheck import org.sonarsource.kotlin.api.checks.ConstructorMatcher import org.sonarsource.kotlin.api.checks.FunMatcherImpl +import org.sonarsource.kotlin.api.checks.getFirstArgumentExpression import org.sonarsource.kotlin.api.checks.isBytesInitializedFromString import org.sonarsource.kotlin.api.checks.isInitializedPredictably import org.sonarsource.kotlin.api.checks.matches import org.sonarsource.kotlin.api.checks.predictRuntimeIntValue import org.sonarsource.kotlin.api.checks.predictRuntimeValueExpression -import org.sonarsource.kotlin.api.checks.simpleArgExpressionOrNull import org.sonarsource.kotlin.api.frontend.KotlinFileContext import org.sonarsource.kotlin.api.frontend.secondaryOf +import org.sonarsource.kotlin.api.visiting.withKaSession private const val SPECS_PACKAGE = "javax.crypto.spec" private const val KEY_SPEC_FUN_NAME = "PBEKeySpec" @@ -49,17 +49,16 @@ private val matcherSaltIndexMap = mapOf( ConstructorMatcher("$SPECS_PACKAGE.$PARAMETER_SPEC_FUN_NAME") to 0, ) -@org.sonarsource.kotlin.api.frontend.K1only @Rule(key = "S2053") class UnpredictableHashSaltCheck : CallAbstractCheck() { override val functionsToVisit = matcherSaltIndexMap.keys override fun visitFunctionCall( callExpression: KtCallExpression, - resolvedCall: ResolvedCall<*>, + resolvedCall: KaFunctionCall<*>, matchedFun: FunMatcherImpl, kotlinFileContext: KotlinFileContext - ) { + ) = withKaSession { val saltArgIndex = matcherSaltIndexMap[matchedFun]!! if (callExpression.valueArguments.size < 2) { @@ -67,13 +66,11 @@ class UnpredictableHashSaltCheck : CallAbstractCheck() { return } - val bindingContext = kotlinFileContext.bindingContext - val saltArg = resolvedCall.valueArgumentsByIndex?.elementAtOrNull(saltArgIndex) - ?.arguments?.elementAtOrNull(0)?.getArgumentExpression() ?: return + val saltArg = resolvedCall.argumentMapping.keys.elementAtOrNull(saltArgIndex) ?: return - val predictedSaltValue = saltArg.predictRuntimeValueExpression(bindingContext) + val predictedSaltValue = saltArg.predictRuntimeValueExpression() - if (predictedSaltValue.isBytesInitializedFromString(bindingContext)) { + if (predictedSaltValue.isBytesInitializedFromString()) { kotlinFileContext.reportIssue( saltArg, MSG_MAKE_UNPREDICTABLE, @@ -82,15 +79,15 @@ class UnpredictableHashSaltCheck : CallAbstractCheck() { return } - val saltInitializer = predictedSaltValue.getResolvedCall(bindingContext) + val saltInitializer = predictedSaltValue.resolveToCall()?.successfulFunctionCallOrNull() ?.takeIf { it matches BYTE_ARRAY_CONSTRUCTOR } ?: return - if (saltInitializer.byteArrayInitSizeTooSmall(bindingContext)) { + if (saltInitializer.byteArrayInitSizeTooSmall()) { kotlinFileContext.reportIssue(saltArg, MSG_MIN_LEN, listOf(kotlinFileContext.secondaryOf(predictedSaltValue))) } if (saltInitializer matches BYTE_ARRAY_CONSTRUCTOR_SIZE_ARG_ONLY && - saltArg.isInitializedPredictably(predictedSaltValue, bindingContext) + saltArg.isInitializedPredictably(predictedSaltValue) ) { kotlinFileContext.reportIssue( saltArg, @@ -103,6 +100,6 @@ class UnpredictableHashSaltCheck : CallAbstractCheck() { /** * Checks whether the first argument of the call is an integer that is at least 16 */ - private fun ResolvedCall<*>.byteArrayInitSizeTooSmall(bindingContext: BindingContext) = - (this.simpleArgExpressionOrNull(0)?.predictRuntimeIntValue(bindingContext) ?: 16) < 16 + private fun KaFunctionCall<*>.byteArrayInitSizeTooSmall() = + (getFirstArgumentExpression()?.predictRuntimeIntValue() ?: 16) < 16 }