Skip to content

Commit 088ce74

Browse files
committed
RUM-16478: UnsafeThirdPartyFunctionCall improvements
Adds wildcard support and overlap detection to the rule, and splits it into focused modules. Wildcards: YAML entries in knownSafeCalls/knownThrowingCalls now accept per-parameter wildcards — * (any non-nullable type) and ? (any nullable type). Valid only at generic parameter positions; misuse throws IllegalStateException. Overlap detection: Rejects entries that are both safe and throwing, and catches duplicate/overlapping patterns within each list. Removed 17 pre-existing duplicates plus two genuine conflicts (Thread.interrupt(), Array.first(Function1)). Config cleanup: Collapsed repeated generic-parameter entries to wildcards across safe-calls and detekt configs. Refactor: RulesParser (YAML parsing), CodeParser (call-expression extraction), WildcardPattern (matching + validation); the rule now keeps only visitor wiring and the safe/throwing/unknown decision.
1 parent bc840ec commit 088ce74

11 files changed

Lines changed: 1175 additions & 276 deletions

File tree

detekt_custom_safe_calls.yml

Lines changed: 30 additions & 189 deletions
Large diffs are not rendered by default.

detekt_custom_unsafe_calls.yml

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,7 @@ datadog:
5454
- "android.widget.FrameLayout.addView(android.view.View, android.view.ViewGroup.LayoutParams):java.lang.IllegalArgumentException"
5555
- "android.widget.LinearLayout.addView(android.view.View):java.lang.IllegalArgumentException"
5656
- "androidx.collection.LruCache.constructor(kotlin.Int):java.lang.IllegalArgumentException"
57-
- "androidx.collection.LruCache.get(java.io.File):java.lang.NullPointerException"
58-
- "androidx.collection.LruCache.get(kotlin.String):java.lang.NullPointerException"
57+
- "androidx.collection.LruCache.get(*):java.lang.NullPointerException"
5958
- "androidx.collection.LruCache.put(java.io.File, kotlin.Unit):java.lang.NullPointerException"
6059
- "androidx.collection.LruCache.remove(java.io.File):java.lang.NullPointerException"
6160
- "androidx.metrics.performance.JankStats.createAndTrack(android.view.Window, androidx.metrics.performance.JankStats.OnFrameListener):java.lang.IllegalStateException"
@@ -174,8 +173,7 @@ datadog:
174173
# endregion
175174
# region Java Collections
176175
- "java.util.Collections.newSetFromMap(kotlin.collections.MutableMap?):java.lang.IllegalArgumentException"
177-
- "java.util.LinkedList.add(kotlin.Int, android.view.View):java.lang.IndexOutOfBoundsException"
178-
- "java.util.LinkedList.add(kotlin.Int, com.datadog.android.webview.internal.rum.domain.WebViewNativeRumViewsCache.ViewEntry):java.lang.IndexOutOfBoundsException"
176+
- "java.util.LinkedList.add(kotlin.Int, *):java.lang.IndexOutOfBoundsException"
179177
- "java.util.LinkedList.offer(com.datadog.android.core.internal.data.upload.UploadWorker.UploadNextBatchTask):java.lang.ClassCastException,java.lang.NullPointerException,java.lang.IllegalArgumentException"
180178
- "java.util.LinkedList.removeFirst():java.util.NoSuchElementException"
181179
- "java.util.LinkedList.removeLast():java.util.NoSuchElementException"
@@ -197,14 +195,7 @@ datadog:
197195
- "kotlin.String.substring(kotlin.Int):java.lang.IndexOutOfBoundsException"
198196
- "kotlin.String.substring(kotlin.Int, kotlin.Int):java.lang.IndexOutOfBoundsException"
199197
- "kotlin.String.takeLast(kotlin.Int):java.lang.IllegalArgumentException"
200-
- "kotlin.String.takeLast(kotlin.Int):kotlin.IllegalArgumentException"
201198
- "kotlin.String.toLong():java.lang.NumberFormatException"
202-
- "kotlin.collections.MutableSet.first():java.util.NoSuchElementException"
203-
- "kotlin.String.format(kotlin.Array):java.util.IllegalFormatException"
204-
- "kotlin.ByteArray.copyOf(kotlin.Int):java.lang.NegativeArraySizeException"
205-
- "kotlin.ByteArray.copyOfRange(kotlin.Int, kotlin.Int):java.lang.IndexOutOfBoundsException,java.lang.IllegalArgumentException"
206-
- "kotlin.ByteArray.get(kotlin.Int):java.lang.IndexOutOfBoundsException"
207-
- "kotlin.Double.roundToLong():java.lang.IllegalArgumentException"
208199
# endregion
209200
# region Kotlin Collections
210201
- "kotlin.Array.first(kotlin.Function1):java.util.NoSuchElementException"
@@ -236,12 +227,10 @@ datadog:
236227
- "okhttp3.Request.Builder.build():java.lang.IllegalStateException"
237228
- "okhttp3.Request.Builder.post(okhttp3.RequestBody):java.lang.NullPointerException,java.lang.IllegalArgumentException"
238229
- "okhttp3.Request.Builder.method(kotlin.String, okhttp3.RequestBody?):java.lang.NullPointerException,java.lang.IllegalArgumentException"
239-
- "okhttp3.Request.Builder.tag(java.lang.Class, com.datadog.android.okhttp.internal.graphql.GraphQLAttributes?):java.lang.ClassCastException"
240-
- "okhttp3.Request.Builder.tag(java.lang.Class, com.datadog.android.okhttp.TraceContext?):java.lang.ClassCastException"
230+
- "okhttp3.Request.Builder.tag(java.lang.Class, ?):java.lang.ClassCastException"
241231
- "okhttp3.Request.Builder.url(kotlin.String):java.lang.NullPointerException,java.lang.IllegalArgumentException"
242232
- "okhttp3.RequestBody.writeTo(okio.BufferedSink):java.io.IOException"
243233
- "okhttp3.Interceptor.Chain.proceed(okhttp3.Request):java.io.IOException"
244-
- "okhttp3.RequestBody.writeTo(okio.BufferedSink):java.io.IOException"
245234
- "okhttp3.Response.close():java.lang.IllegalStateException"
246235
- "okhttp3.Response.peekBody(kotlin.Long):java.io.IOException,java.lang.IllegalArgumentException,java.lang.IllegalStateException"
247236
- "okhttp3.ResponseBody.string():java.io.IOException"

tools/detekt/src/main/kotlin/com/datadog/tools/detekt/rules/sdk/UnsafeThirdPartyFunctionCall.kt

Lines changed: 72 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,12 @@
66

77
package com.datadog.tools.detekt.rules.sdk
88

9-
import com.datadog.tools.detekt.ext.fqTypeName
109
import com.datadog.tools.detekt.rules.AbstractCallExpressionRule
10+
import com.datadog.tools.detekt.rules.sdk.rule.thirdparty.CodeParser
11+
import com.datadog.tools.detekt.rules.sdk.rule.thirdparty.CodeParser.KtMethodParameter
12+
import com.datadog.tools.detekt.rules.sdk.rule.thirdparty.DetektConfigParser
13+
import com.datadog.tools.detekt.rules.sdk.rule.thirdparty.DetektConfigValidator
14+
import com.datadog.tools.detekt.rules.sdk.rule.thirdparty.SignatureRule
1115
import io.gitlab.arturbosch.detekt.api.CodeSmell
1216
import io.gitlab.arturbosch.detekt.api.Config
1317
import io.gitlab.arturbosch.detekt.api.Debt
@@ -18,41 +22,31 @@ import io.gitlab.arturbosch.detekt.api.config
1822
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
1923
import org.jetbrains.kotlin.psi.KtCallExpression
2024
import org.jetbrains.kotlin.psi.KtTryExpression
21-
import org.jetbrains.kotlin.resolve.BindingContext
2225
import java.util.Stack
2326

2427
/**
25-
* This rule will report any call to a "third party" function that is considered unsafe, that is,
26-
* which could throw an exception.
28+
* Reports any call to a "third party" function that is considered unsafe (i.e. could throw an
29+
* exception). Third party functions are detected based on an internal package prefix: any method
30+
* with that prefix is treated as first party.
2731
*
28-
* Third party functions are detected based on an internal package prefix: any method which has a
29-
* package name with this prefix is considered first party, anything else is third party.
32+
* The decision logic lives in this rule. Config-string parsing is delegated to [DetektConfigParser]
33+
* and PSI-level extraction to [CodeParser];
34+
* both feed into [com.datadog.tools.detekt.rules.sdk.rule.thirdparty.SignatureRule] which is the
35+
* single abstraction matching/validating either direct or wildcarded YAML records.
3036
*/
3137
@RequiresTypeResolution
3238
class UnsafeThirdPartyFunctionCall(
3339
config: Config
3440
) : AbstractCallExpressionRule(config, includeTypeArguments = false) {
3541

42+
private val codeParser = CodeParser()
43+
private val ruleParser = DetektConfigParser()
44+
private val configValidator = DetektConfigValidator(ruleParser)
3645
private val internalPackagePrefix: String by config(defaultValue = "")
3746
private val treatUnknownFunctionAsThrowing: Boolean by config(defaultValue = true)
38-
private val knownThrowingCalls: List<String> by config(defaultValue = emptyList())
39-
private val knownSafeCalls: List<String> by config(defaultValue = emptyList())
40-
41-
private val knownThrowingCallsMap: Map<String, List<String>> by lazy {
42-
knownThrowingCalls.map {
43-
val splitColon = it.split(':')
44-
val key = splitColon.first()
45-
if (splitColon.size == 1) {
46-
println("✘ ERROR WITH KNOWN THROWING CALL: $it")
47-
}
48-
val exceptions = splitColon[1].split(',').toList()
49-
key to exceptions
50-
}.toMap()
51-
}
52-
53-
private val caughtExceptions = Stack<List<String>>()
54-
55-
// region Rule
47+
private val knownSafeCalls: List<SignatureRule> by config(emptyList(), ruleParser::parseSafeCalls)
48+
private val knownThrowingCalls: List<SignatureRule> by config(emptyList(), ruleParser::parseThrowingCalls)
49+
private val caughtExceptions = Stack<Set<String>>()
5650

5751
override val issue: Issue = Issue(
5852
javaClass.simpleName,
@@ -62,85 +56,81 @@ class UnsafeThirdPartyFunctionCall(
6256
Debt.TWENTY_MINS
6357
)
6458

59+
init {
60+
configValidator.validate(knownSafeCalls, knownThrowingCalls)
61+
}
62+
6563
override fun visitTryExpression(expression: KtTryExpression) {
66-
val caughtTypes = expression.catchClauses
67-
.mapNotNull {
68-
val typeReference = it.catchParameter?.typeReference
69-
bindingContext.get(BindingContext.TYPE, typeReference)?.fqTypeName()
70-
}
71-
caughtExceptions.push(caughtTypes)
64+
caughtExceptions.push(codeParser.parseCaughtTypes(expression, bindingContext))
7265
super.visitTryExpression(expression)
7366
caughtExceptions.pop()
7467
}
7568

76-
// endregion
77-
78-
// region AbstractCallExpressionRule
79-
8069
@Suppress("ReturnCount")
8170
override fun visitResolvedFunctionCall(
8271
expression: KtCallExpression,
8372
resolvedCall: ResolvedFunCall
8473
) {
85-
if (internalPackagePrefix.isNotEmpty()) {
86-
val belongsToInternalContainer = resolvedCall.containerFqName.startsWith(internalPackagePrefix) ||
87-
resolvedCall.containingPackage.startsWith(internalPackagePrefix)
88-
if (belongsToInternalContainer) return
89-
}
90-
if (resolvedCall.functionName in kotlinHelperMethods) {
74+
if (resolvedCall.functionName in kotlinHelperMethods ||
75+
resolvedCall.isBelongsToInternalPrefix(internalPackagePrefix)
76+
) {
9177
return
9278
}
9379

94-
if (knownThrowingCallsMap.containsKey(resolvedCall.call)) {
95-
val knownThrowables = knownThrowingCallsMap[resolvedCall.call] ?: emptyList()
96-
checkCallThrowingExceptions(expression, resolvedCall.call, knownThrowables)
97-
} else if (treatUnknownFunctionAsThrowing && !knownSafeCalls.contains(resolvedCall.call)) {
98-
val message = "Calling ${resolvedCall.call} could throw exceptions, but this method is unknown"
99-
reportUnsafeCall(expression, message)
100-
}
80+
classifyAndReport(
81+
expression,
82+
signature = resolvedCall.call,
83+
params = codeParser.parseFormalParams(expression, bindingContext)
84+
)
10185
}
10286

103-
// endregion
87+
private fun classifyAndReport(expression: KtCallExpression, signature: String, params: List<KtMethodParameter>) {
88+
knownThrowingCalls.firstOrNull { it.matches(signature) }
89+
?.let { throwingMatch ->
90+
throwingMatch.validate(params)
91+
checkCallThrowingExceptions(expression, signature, throwingMatch.exceptions)
92+
return
93+
}
10494

105-
// region Internal
95+
knownSafeCalls.firstOrNull { it.matches(signature) }
96+
?.let { safeMatch ->
97+
safeMatch.validate(params)
98+
return
99+
}
100+
101+
if (treatUnknownFunctionAsThrowing) {
102+
reportUnsafeCall(
103+
expression,
104+
"Calling $signature could throw exceptions, but this method is unknown"
105+
)
106+
}
107+
}
106108

107109
private fun checkCallThrowingExceptions(
108110
expression: KtCallExpression,
109111
call: String,
110112
exceptions: List<String>
111113
) {
112-
val catchesAnyException = caughtExceptions.any { list ->
113-
list.any { e -> e in topLevelExceptions }
114-
}
115-
val catchesAnyError = caughtExceptions.any { list ->
116-
list.any { e -> e in topLevelErrors }
117-
}
118-
val uncaught = exceptions.filter { exception ->
119-
caughtExceptions.none { it.contains(exception) }
114+
val catchesAnyException = caughtExceptions.any { list -> list.any { e -> e in topLevelExceptions } }
115+
val catchesAnyError = caughtExceptions.any { list -> list.any { e -> e in topLevelErrors } }
116+
val uncaught = exceptions.filter { exception -> caughtExceptions.none { it.contains(exception) } }.filter {
117+
val isUncaughtException = !catchesAnyException && it.endsWith("Exception")
118+
val isUncaughtError = !catchesAnyError && it.endsWith("Error")
119+
isUncaughtException || isUncaughtError
120120
}
121-
.filter {
122-
val isUncaughtException = it.endsWith("Exception") && !catchesAnyException
123-
val isUncaughtError = it.endsWith("Error") && !catchesAnyError
124-
isUncaughtException || isUncaughtError
125-
}
126121

127-
if (uncaught.isEmpty()) {
128-
return
129-
}
122+
if (uncaught.isEmpty()) return
130123

131-
val msg = "Calling $call can throw the following exceptions: ${exceptions.joinToString()}."
132-
reportUnsafeCall(expression, msg)
124+
reportUnsafeCall(
125+
expression = expression,
126+
message = "Calling $call can throw the following exceptions: ${exceptions.joinToString()}."
127+
)
133128
}
134129

135-
private fun reportUnsafeCall(
136-
expression: KtCallExpression,
137-
message: String
138-
) {
130+
private fun reportUnsafeCall(expression: KtCallExpression, message: String) {
139131
report(CodeSmell(issue, Entity.from(expression), message = message))
140132
}
141133

142-
// endregion
143-
144134
companion object {
145135
private const val JAVA_EXCEPTION_CLASS = "java.lang.Exception"
146136
private const val JAVA_ERROR_CLASS = "java.lang.Error"
@@ -167,5 +157,14 @@ class UnsafeThirdPartyFunctionCall(
167157
"let", "run", "with", "apply", "also",
168158
"print", "println", "toString", "invoke"
169159
)
160+
161+
private fun ResolvedFunCall.isBelongsToInternalPrefix(
162+
internalPackagePrefix: String
163+
): Boolean {
164+
val isInternal =
165+
containerFqName.startsWith(internalPackagePrefix) || containingPackage.startsWith(internalPackagePrefix)
166+
167+
return internalPackagePrefix.isNotEmpty() && isInternal
168+
}
170169
}
171170
}
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
* Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
3+
* This product includes software developed at Datadog (https://www.datadoghq.com/).
4+
* Copyright 2016-Present Datadog, Inc.
5+
*/
6+
7+
package com.datadog.tools.detekt.rules.sdk.rule.thirdparty
8+
9+
import com.datadog.tools.detekt.ext.fqTypeName
10+
import org.jetbrains.kotlin.descriptors.TypeParameterDescriptor
11+
import org.jetbrains.kotlin.descriptors.ValueParameterDescriptor
12+
import org.jetbrains.kotlin.psi.KtCallExpression
13+
import org.jetbrains.kotlin.psi.KtTryExpression
14+
import org.jetbrains.kotlin.resolve.BindingContext
15+
import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
16+
import org.jetbrains.kotlin.types.KotlinType
17+
18+
/**
19+
* Extracts structured Kotlin PSI data needed by the rule: formal parameters of a call expression
20+
* and the caught-exception types declared by a `try`/`catch` block. The rule owns all decision
21+
* logic — this object only translates PSI nodes into plain Kotlin values.
22+
*/
23+
internal class CodeParser {
24+
25+
/**
26+
* Returns the formal parameters of the resolved function call as plain [KtMethodParameter] values,
27+
* sorted by their declaration order. Returns an empty list if the call cannot be resolved.
28+
*
29+
* e.g. `fun <T> MutableList.add(index: Int, element: T)` →
30+
* ```
31+
* [
32+
* KtMethodParameter(name="index", type="kotlin.Int", isGeneric=false),
33+
* KtMethodParameter(name="element", type="T", isGeneric=true)
34+
* ]
35+
* ```
36+
*
37+
* Used by [SignatureRule.validate] to enforce that wildcard slots only target generic
38+
* type parameters, not concrete types.
39+
*/
40+
internal fun parseFormalParams(
41+
expression: KtCallExpression,
42+
bindingContext: BindingContext
43+
): List<KtMethodParameter> = expression.getResolvedCall(bindingContext)
44+
?.valueArguments?.keys
45+
?.sortedBy { it.index }
46+
?.map { it.toKtMethodParameter() }
47+
.orEmpty()
48+
49+
/**
50+
* Returns the fully-qualified exception type names declared in all `catch` clauses of [expression].
51+
* e.g. `try { } catch (e: IOException) { }` → `{"java.io.IOException"}`.
52+
*
53+
* Used by the rule to determine whether a known-throwing call's exceptions are already handled.
54+
*/
55+
internal fun parseCaughtTypes(
56+
expression: KtTryExpression,
57+
bindingContext: BindingContext
58+
): Set<String> = expression.catchClauses
59+
.mapNotNull { bindingContext.get(BindingContext.TYPE, it.catchParameter?.typeReference)?.fqTypeName() }
60+
.toSet()
61+
62+
private fun ValueParameterDescriptor.toKtMethodParameter() = KtMethodParameter(
63+
name = name.toString(),
64+
type = type.toString(),
65+
isGeneric = original.type.containsTypeParameter()
66+
)
67+
68+
private fun KotlinType.containsTypeParameter(): Boolean {
69+
if (constructor.declarationDescriptor is TypeParameterDescriptor) return true
70+
return arguments.any { !it.isStarProjection && it.type.containsTypeParameter() }
71+
}
72+
73+
internal data class KtMethodParameter(
74+
val name: String,
75+
val type: String,
76+
val isGeneric: Boolean
77+
)
78+
}

0 commit comments

Comments
 (0)