Skip to content

Commit b954a62

Browse files
marcosholgadomarcosholgado
authored
Fix MetricsPixelNumericValueDetector for compiled Kotlin classes (duckduckgo#7953)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1202552961248957/task/1213634576464400 ### Description The `MetricsPixelNumericValueDetector` lint rule was silently missing violations when `MetricsPixel` was used in modules where the class is loaded from a compiled dependency JAR — the constructor `PsiMethod` can't be resolved in that case, so `visitConstructor` never fired. This PR adds a fallback detection path via `getApplicableUastTypes`/`createUastHandler` that triggers on all `UCallExpression` nodes and uses source text matching + return type filtering to identify `MetricsPixel` calls. Named-argument lookup was also reworked to use `UNamedExpression` with a Kotlin PSI fallback, making it robust regardless of argument order. All tests now skip `TestMode.REORDER_ARGUMENTS` to suppress a known lint test infrastructure warning caused by overlapping edits when positional args are nested inside outer named args. ### Steps to test this PR _MetricsPixelNumericValueDetector_ - [x] Change `SearchMetricPixelsPlugin` so one of them has a value that's not a number, i.e. "test" - [x] Run `./gradlew :feature-toggles-impl:lint` - [x] Lint should throw an error ### UI changes | Before | After | | ------ | ----- | !(Upload before screenshot)|(Upload after screenshot)| <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Medium risk because it changes lint detection logic to rely on UAST/PSI fallbacks and source-text matching, which could introduce false positives/negatives across Kotlin call shapes. > > **Overview** > Fixes `MetricsPixelNumericValueDetector` missing violations when `MetricsPixel` comes from a compiled dependency by adding a fallback scan over all `UCallExpression`s and filtering to `MetricsPixel` calls when the constructor can’t be resolved. > > Reworks argument extraction to be robust to named/out-of-order arguments by using `UNamedExpression` with a Kotlin PSI fallback, and improves error location selection by reporting on the `value` expression when possible. > > Updates tests to reflect the real `MetricsPixel` signature (default `type`), adds coverage for out-of-order named arguments, and skips `TestMode.REORDER_ARGUMENTS` to avoid test infra issues. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 693bca6. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 557b40a commit b954a62

2 files changed

Lines changed: 121 additions & 15 deletions

File tree

‎lint-rules/src/main/java/com/duckduckgo/lint/MetricsPixelNumericValueDetector.kt‎

Lines changed: 77 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.duckduckgo.lint
1818

19+
import com.android.tools.lint.client.api.UElementHandler
1920
import com.android.tools.lint.detector.api.Category
2021
import com.android.tools.lint.detector.api.Detector
2122
import com.android.tools.lint.detector.api.Implementation
@@ -25,41 +26,104 @@ import com.android.tools.lint.detector.api.Scope
2526
import com.android.tools.lint.detector.api.Severity
2627
import com.android.tools.lint.detector.api.SourceCodeScanner
2728
import com.intellij.psi.PsiMethod
29+
import org.jetbrains.kotlin.psi.KtCallExpression
2830
import org.jetbrains.uast.UCallExpression
31+
import org.jetbrains.uast.UElement
2932
import org.jetbrains.uast.UExpression
33+
import org.jetbrains.uast.UNamedExpression
34+
import org.jetbrains.uast.UastFacade
3035
import java.util.EnumSet
3136

3237
@Suppress("UnstableApiUsage")
3338
class MetricsPixelNumericValueDetector : Detector(), SourceCodeScanner {
3439

35-
override fun getApplicableConstructorTypes() =
36-
listOf("com.duckduckgo.feature.toggles.api.MetricsPixel")
40+
// Path 1: fires when the constructor can be resolved to a PsiMethod (source stubs in tests).
41+
override fun getApplicableConstructorTypes() = listOf(METRICS_PIXEL_FQN)
3742

3843
override fun visitConstructor(context: JavaContext, node: UCallExpression, constructor: PsiMethod) {
39-
val typeArg = findArgument(node, constructor, "type") ?: return
40-
val typeText = typeArg.sourcePsi?.text ?: return
44+
checkNode(context, node)
45+
}
46+
47+
// Path 2: fires for all UCallExpressions. Used as a fallback when getApplicableConstructorTypes
48+
// doesn't trigger — which happens for compiled Kotlin data classes whose constructor PsiMethod
49+
// can't be resolved from a dependency JAR.
50+
override fun getApplicableUastTypes(): List<Class<out UElement>> = listOf(UCallExpression::class.java)
51+
52+
override fun createUastHandler(context: JavaContext) = object : UElementHandler() {
53+
override fun visitCallExpression(node: UCallExpression) {
54+
// Skip if constructor is resolvable — Path 1 (visitConstructor) handles it.
55+
if (node.resolve() != null) return
56+
// Use source text as a reliable name filter since UAST name resolution may be
57+
// unavailable when the constructor can't be resolved from a compiled JAR.
58+
val sourceText = node.sourcePsi?.text ?: return
59+
if (!sourceText.startsWith("MetricsPixel(")) return
60+
val fqn = node.returnType?.canonicalText
61+
if (fqn != null && fqn != METRICS_PIXEL_FQN) return
62+
checkNode(context, node)
63+
}
64+
}
65+
66+
private fun checkNode(context: JavaContext, node: UCallExpression) {
67+
val typeText = findArgText(node, "type") ?: return
4168
if (!typeText.contains("COUNT_WHEN_IN_WINDOW") && !typeText.contains("COUNT_ALWAYS")) return
4269

43-
val valueArg = findArgument(node, constructor, "value") ?: return
44-
val valueStr = valueArg.evaluate() as? String ?: return
70+
val valueText = findArgText(node, "value") ?: return
71+
// Strip surrounding quotes from a string literal so we can check if it's an integer.
72+
val valueStr = valueText.removeSurrounding("\"")
4573

4674
if (valueStr.toIntOrNull() == null) {
75+
// Report on the value argument expression; fall back to the call site if the
76+
// expression can't be resolved (e.g. UastFacade.convertElement fails in Path 2).
77+
val valueExpr: UExpression = findArgExpr(node, "value") ?: node
4778
context.report(
4879
NUMERIC_VALUE_REQUIRED,
49-
valueArg,
50-
context.getLocation(valueArg),
80+
valueExpr,
81+
context.getLocation(valueExpr),
5182
"`value` must be a valid integer string for COUNT_WHEN_IN_WINDOW/COUNT_ALWAYS pixel types (got \"$valueStr\")",
5283
)
5384
}
5485
}
5586

56-
private fun findArgument(node: UCallExpression, constructor: PsiMethod, paramName: String): UExpression? {
57-
val params = constructor.parameterList.parameters
58-
val idx = params.indexOfFirst { it.name == paramName }
59-
return if (idx >= 0 && idx < node.valueArguments.size) node.valueArguments[idx] else null
87+
/**
88+
* Returns the source text of a named Kotlin argument, unwrapping any [UNamedExpression]
89+
* wrapper. Falls back to Kotlin PSI for reliable named-argument lookup when UAST does not
90+
* emit [UNamedExpression] wrappers (observed for compiled-class constructor calls in real
91+
* lint runs where valueArguments are in source order without name info).
92+
*/
93+
private fun findArgText(node: UCallExpression, name: String): String? {
94+
// UAST named-argument lookup (works when valueArguments contain UNamedExpression).
95+
for (arg in node.valueArguments) {
96+
if (arg is UNamedExpression && arg.name == name) {
97+
return arg.expression.sourcePsi?.text
98+
}
99+
}
100+
// Kotlin PSI fallback: directly reads the named argument from the Kotlin parse tree,
101+
// which is reliable regardless of argument order and UAST wrapper availability.
102+
val ktCall = node.sourcePsi as? KtCallExpression ?: return null
103+
return ktCall.valueArguments
104+
.find { it.getArgumentName()?.asName?.identifier == name }
105+
?.getArgumentExpression()
106+
?.text
107+
}
108+
109+
/**
110+
* Returns the UAST [UExpression] for a named argument for use as a lint report location.
111+
* Mirrors [findArgText] but returns the expression node instead of text.
112+
*/
113+
private fun findArgExpr(node: UCallExpression, name: String): UExpression? {
114+
for (arg in node.valueArguments) {
115+
if (arg is UNamedExpression && arg.name == name) return arg.expression
116+
}
117+
val ktCall = node.sourcePsi as? KtCallExpression ?: return null
118+
val ktExpr = ktCall.valueArguments
119+
.find { it.getArgumentName()?.asName?.identifier == name }
120+
?.getArgumentExpression() ?: return null
121+
return UastFacade.convertElement(ktExpr, null) as? UExpression
60122
}
61123

62124
companion object {
125+
private const val METRICS_PIXEL_FQN = "com.duckduckgo.feature.toggles.api.MetricsPixel"
126+
63127
val NUMERIC_VALUE_REQUIRED = Issue.create(
64128
"MetricsPixelNumericValue",
65129
"MetricsPixel value must be a numeric string for counting types",
@@ -74,4 +138,4 @@ class MetricsPixelNumericValueDetector : Detector(), SourceCodeScanner {
74138
),
75139
)
76140
}
77-
}
141+
}

‎lint-rules/src/test/java/com/duckduckgo/lint/MetricsPixelNumericValueDetectorTest.kt‎

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package com.duckduckgo.lint
1818

1919
import com.android.tools.lint.checks.infrastructure.TestFiles.kt
2020
import com.android.tools.lint.checks.infrastructure.TestLintTask.lint
21+
import com.android.tools.lint.checks.infrastructure.TestMode
2122
import com.duckduckgo.lint.MetricsPixelNumericValueDetector.Companion.NUMERIC_VALUE_REQUIRED
2223
import org.junit.Test
2324

@@ -51,6 +52,7 @@ class MetricsPixelNumericValueDetectorTest {
5152
)
5253
.allowCompilationErrors()
5354
.issues(NUMERIC_VALUE_REQUIRED)
55+
.skipTestModes(TestMode.REORDER_ARGUMENTS)
5456
.run()
5557
.expectClean()
5658
}
@@ -82,6 +84,7 @@ class MetricsPixelNumericValueDetectorTest {
8284
)
8385
.allowCompilationErrors()
8486
.issues(NUMERIC_VALUE_REQUIRED)
87+
.skipTestModes(TestMode.REORDER_ARGUMENTS)
8588
.run()
8689
.expectClean()
8790
}
@@ -113,6 +116,7 @@ class MetricsPixelNumericValueDetectorTest {
113116
)
114117
.allowCompilationErrors()
115118
.issues(NUMERIC_VALUE_REQUIRED)
119+
.skipTestModes(TestMode.REORDER_ARGUMENTS)
116120
.run()
117121
.expectClean()
118122
}
@@ -144,6 +148,7 @@ class MetricsPixelNumericValueDetectorTest {
144148
)
145149
.allowCompilationErrors()
146150
.issues(NUMERIC_VALUE_REQUIRED)
151+
.skipTestModes(TestMode.REORDER_ARGUMENTS)
147152
.run()
148153
.expectContains("MetricsPixelNumericValue")
149154
}
@@ -175,10 +180,47 @@ class MetricsPixelNumericValueDetectorTest {
175180
)
176181
.allowCompilationErrors()
177182
.issues(NUMERIC_VALUE_REQUIRED)
183+
.skipTestModes(TestMode.REORDER_ARGUMENTS)
178184
.run()
179185
.expectContains("MetricsPixelNumericValue")
180186
}
181187

188+
@Test
189+
fun `COUNT_ALWAYS with non-integer value and out-of-order named args - error reported`() {
190+
lint()
191+
.files(
192+
metricsPixelStub(),
193+
kt("""
194+
package com.duckduckgo.somefeature.impl
195+
196+
import com.duckduckgo.feature.toggles.api.MetricsPixel
197+
import com.duckduckgo.feature.toggles.api.MetricType
198+
import com.duckduckgo.feature.toggles.api.ConversionWindow
199+
200+
class SomeFeature {
201+
fun doSomething(toggle: Any) {
202+
MetricsPixel(
203+
type = MetricType.COUNT_ALWAYS,
204+
metric = "some_metric",
205+
conversionWindow = listOf(ConversionWindow(0, 1)),
206+
value = "not_a_number",
207+
toggle = toggle as com.duckduckgo.feature.toggles.api.Toggle,
208+
)
209+
}
210+
}
211+
""").indented(),
212+
)
213+
.allowCompilationErrors()
214+
.issues(NUMERIC_VALUE_REQUIRED)
215+
.skipTestModes(TestMode.REORDER_ARGUMENTS)
216+
.run()
217+
.expectContains("MetricsPixelNumericValue")
218+
}
219+
220+
// Stub matches the real MetricsPixel signature: declaration order is
221+
// metric, value, conversionWindow, toggle, type (type has a default value).
222+
// Tests pass toggle before conversionWindow (source order ≠ declaration order)
223+
// to exercise named-argument lookup rather than positional index mapping.
182224
private fun metricsPixelStub() = kt("""
183225
package com.duckduckgo.feature.toggles.api
184226
@@ -188,9 +230,9 @@ class MetricsPixelNumericValueDetectorTest {
188230
data class MetricsPixel(
189231
val metric: String,
190232
val value: String,
191-
val toggle: Toggle,
192233
val conversionWindow: List<ConversionWindow>,
193-
val type: MetricType,
234+
val toggle: Toggle,
235+
val type: MetricType = MetricType.NORMAL,
194236
)
195237
""").indented()
196238
}

0 commit comments

Comments
 (0)