Enforce LazyLog rule on Logger type instead of variable name (#3022)

Author: rli

core/src/software/aws/toolkits/core/utils/LogUtils.kt

@@ -1,6 +1,7 @@
 // Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+@file:Suppress("LazyLog")
 package software.aws.toolkits.core.utils
 
 import org.slf4j.Logger

core/tst/software/aws/toolkits/core/utils/LogUtilsTest.kt

@@ -1,6 +1,7 @@
 // Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+@file:Suppress("LazyLog")
 package software.aws.toolkits.core.utils
 
 import org.assertj.core.api.Assertions.assertThat

core/tst/software/aws/toolkits/core/utils/RemoteResourceResolverTest.kt

@@ -145,7 +145,7 @@ class RemoteResourceResolverTest {
             // It's possible for it to be done writing but path.exists to not work yet which
             // makes the canDownloadAFileOnce test fail (on CodeBuild).
             while (!path.exists()) {
-                LOG.debug("writeDataToFile path does not exist yet: $path")
+                LOG.debug { "writeDataToFile path does not exist yet: $path" }
                 Thread.sleep(10)
             }
         }

detekt-rules/build.gradle.kts

@@ -11,4 +11,5 @@ dependencies {
     testImplementation(libs.detekt.test)
     testImplementation(libs.junit4)
     testImplementation(libs.assertj)
+    testRuntimeOnly(libs.slf4j.api)
 }

detekt-rules/src/software/aws/toolkits/gradle/detekt/rules/LazyLogRule.kt

@@ -9,18 +9,18 @@ import io.gitlab.arturbosch.detekt.api.Entity
 import io.gitlab.arturbosch.detekt.api.Issue
 import io.gitlab.arturbosch.detekt.api.Rule
 import io.gitlab.arturbosch.detekt.api.Severity
-import org.jetbrains.kotlin.name.Name
+import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
+import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
 import org.jetbrains.kotlin.psi.KtCallExpression
 import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression
-import org.jetbrains.kotlin.psi.psiUtil.getReceiverExpression
-import org.jetbrains.kotlin.psi.psiUtil.referenceExpression
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.callUtil.getReceiverExpression
+import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
 
+@RequiresTypeResolution
 class LazyLogRule : Rule() {
     override val issue = Issue("LazyLog", Severity.Style, "Use lazy logging synatax (e.g. warning {\"abc\"} ) instead of warning(\"abc\")", Debt.FIVE_MINS)
 
-    private val logMethods = setOf("error", "warn", "info", "debug", "trace")
-    private val logNames = setOf("log", "logger")
-
     // UI tests have issues with this TODO see if we want multiple detekt.yml files or disable for certain modules in this rule
     private val optOut = setOf("software.aws.toolkits.jetbrains.uitests")
 
@@ -31,25 +31,34 @@ class LazyLogRule : Rule() {
                 return
             }
 
-            if (optOut.any { name -> element.containingKtFile.packageFqName.startsWith(Name.identifier(name)) }) {
+            if (optOut.any { name -> element.containingKtFile.packageFqName.asString().startsWith(name) }) {
                 return
             }
 
-            val referenceExpression = it.getReceiverExpression()?.referenceExpression() ?: return
+            if (bindingContext == BindingContext.EMPTY) return
+            val resolvedCall = it.getResolvedCall(bindingContext)
+            val type = resolvedCall?.extensionReceiver?.type?.fqNameOrNull()?.asString()
+                ?: resolvedCall?.dispatchReceiver?.type?.fqNameOrNull()?.asString()
 
-            if (!logNames.contains(referenceExpression.text.toLowerCase())) {
+            if (type !in loggers) {
                 return
             }
 
             if (element.lambdaArguments.size != 1) {
+                val receiverName = resolvedCall?.getReceiverExpression()?.text ?: type
                 report(
                     CodeSmell(
                         issue,
                         Entity.from(element),
-                        message = "Use the Lambda version of ${referenceExpression.text}.${it.text} instead"
+                        message = "Use the lambda version of $receiverName.${it.text} instead"
                     )
                 )
             }
         }
     }
+
+    companion object {
+        private val logMethods = setOf("error", "warn", "info", "debug", "trace")
+        val loggers = setOf("org.slf4j.Logger")
+    }
 }

detekt-rules/tst/software/aws/toolkits/gradle/detekt/rules/LazyLogRuleTest.kt

@@ -3,23 +3,31 @@
 
 package software.aws.toolkits.gradle.detekt.rules
 
-import io.gitlab.arturbosch.detekt.test.lint
+import io.github.detekt.test.utils.createEnvironment
+import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.Test
+import java.io.File
 
 class LazyLogRuleTest {
     private val rule = LazyLogRule()
+    private val environment = createEnvironment(
+        additionalRootPaths = LazyLogRule.loggers.map {
+            File(Class.forName(it).protectionDomain.codeSource.location.path)
+        }
+    ).env
 
     @Test
     fun lambdaIsUsedToLog() {
         assertThat(
-            rule.lint(
+            rule.compileAndLintWithContext(
+                environment,
                 """
 import org.slf4j.LoggerFactory
 
-val LOG = LoggerFactory.getLogger(T::class.java)
+val LOG = LoggerFactory.getLogger(LoggerFactory::class.java)
 fun foo() {
-    LOG.debug {"Hi" }
+    LOG.debug { "Hi" }
 }
                 """.trimIndent()
             )
@@ -29,30 +37,32 @@ fun foo() {
     @Test
     fun methodCallIsUsedToLog() {
         assertThat(
-            rule.lint(
+            rule.compileAndLintWithContext(
+                environment,
                 """
 import org.slf4j.LoggerFactory
 
-val LOG = LoggerFactory.getLogger(T::class.java)
+val LOG = LoggerFactory.getLogger(LoggerFactory::class.java)
 fun foo() {
     LOG.debug("Hi")
 }
                 """.trimIndent()
             )
         ).singleElement()
             .matches {
-                it.id == "LazyLog" && it.message == "Use the Lambda version of LOG.debug instead"
+                it.id == "LazyLog" && it.message == "Use the lambda version of LOG.debug instead"
             }
     }
 
     @Test
     fun lambdaIsUsedToLogButWithException() {
         assertThat(
-            rule.lint(
+            rule.compileAndLintWithContext(
+                environment,
                 """
 import org.slf4j.LoggerFactory
 
-val LOG = LoggerFactory.getLogger(T::class.java)
+val LOG = LoggerFactory.getLogger(LoggerFactory::class.java)
 fun foo() {
     val e = RuntimeException()
     LOG.debug(e) {"Hi" }
@@ -65,13 +75,14 @@ fun foo() {
     @Test
     fun methodCallIsUsedToLogInUiTests() {
         assertThat(
-            rule.lint(
+            rule.compileAndLintWithContext(
+                environment,
                 """
 package software.aws.toolkits.jetbrains.uitests.really.cool.test
 
 import org.slf4j.LoggerFactory
 
-val LOG = LoggerFactory.getLogger(T::class.java)
+val LOG = LoggerFactory.getLogger(LoggerFactory::class.java)
 fun foo() {
     LOG.debug("Hi")
 }

gradle/libs.versions.toml

@@ -17,6 +17,7 @@ mockito = "4.0.0"
 telemetryGenerator = "1.0.25"
 testLogger = "3.1.0"
 testRetry = "1.2.1"
+slf4j = "1.7.36"
 wiremock = "2.27.2"
 zjsonpatch = "0.4.11"
 
@@ -78,6 +79,7 @@ kotlin-test = { module = "org.jetbrains.kotlin:kotlin-test-junit", version.ref =
 mockito-core = { module = "org.mockito:mockito-core", version.ref = "mockito" }
 mockito-kotlin = { module = "org.mockito.kotlin:mockito-kotlin", version.ref = "mockito" }
 telemetryGenerator = { module = "software.aws.toolkits:telemetry-generator", version.ref = "telemetryGenerator" }
+slf4j-api = { module = "org.slf4j:slf4j-api", version.ref = "slf4j" }
 wiremock = { module = "com.github.tomakehurst:wiremock", version.ref = "wiremock" }
 zjsonpatch = { module = "com.flipkart.zjsonpatch:zjsonpatch", version.ref = "zjsonpatch" }
 

jetbrains-core/src/software/aws/toolkits/jetbrains/core/credentials/AwsConnectionManager.kt

@@ -159,7 +159,7 @@ abstract class AwsConnectionManager(private val project: Project) : SimpleModifi
             if (modificationStamp == this.modificationCount) {
                 connectionState = it
             } else {
-                LOGGER.warn("validateCredentials returned but the account manager state has been manipulated before results were back, ignoring")
+                LOGGER.warn { "validateCredentials returned but the account manager state has been manipulated before results were back, ignoring" }
             }
         }
     }

jetbrains-core/src/software/aws/toolkits/jetbrains/core/docker/ToolkitDockerAdapter.kt

@@ -81,7 +81,7 @@ abstract class AbstractToolkitDockerAdapter(protected val project: Project, prot
         // unfortunately no callbacks available to grab the Deployment instance
         val deploymentPromise = AsyncPromise<Deployment>()
         serverConnection.deploy(task) { deploymentName ->
-            EcrUtils.LOG.debug("Retrieving Deployment associated with '$deploymentName'")
+            LOG.debug { "Retrieving Deployment associated with '$deploymentName'" }
             RemoteServersView.getInstance(project).showDeployment(serverConnection, deploymentName)
             runInEdt {
                 deploymentPromise.setResult(serverConnection.deployments.first { it.name == deploymentName })
@@ -110,7 +110,7 @@ abstract class AbstractToolkitDockerAdapter(protected val project: Project, prot
         }
 
         if (!wasBuildOnly) {
-            EcrUtils.LOG.debug("Configuration specified additional 'run' parameters in Dockerfile that will be ignored")
+            LOG.debug { "Configuration specified additional 'run' parameters in Dockerfile that will be ignored" }
             logHandler.print("Skipping 'Run' portion of Dockerfile build configuration\n")
         }
 

jetbrains-core/src/software/aws/toolkits/jetbrains/services/cloudwatch/logs/CloudWatchActor.kt

@@ -24,6 +24,7 @@ import software.amazon.awssdk.services.cloudwatchlogs.model.QueryStatus
 import software.amazon.awssdk.services.cloudwatchlogs.model.ResourceNotFoundException
 import software.aws.toolkits.core.utils.error
 import software.aws.toolkits.core.utils.getLogger
+import software.aws.toolkits.core.utils.info
 import software.aws.toolkits.core.utils.warn
 import software.aws.toolkits.jetbrains.core.coroutines.disposableCoroutineScope
 import software.aws.toolkits.jetbrains.core.coroutines.getCoroutineBgContext
@@ -507,15 +508,15 @@ class InsightsQueryResultsActor(
             }.chunked(1000)
 
             dedupedResults.forEach { chunk ->
-                LOG.info("loading block of ${chunk.size}")
+                LOG.info { "loading block of ${chunk.size}" }
                 table.listTableModel.addRows(chunk)
             }
         } while (isQueryRunning(response.status()))
 
-        LOG.info("done, ${loadedQueryResults.size} entries in set")
+        LOG.info { "done, ${loadedQueryResults.size} entries in set" }
         tableDoneLoading()
         table.emptyText.text = emptyText
-        LOG.info("total items in table: ${table.listTableModel.items.size}")
+        LOG.info { "total items in table: ${table.listTableModel.items.size}" }
         channel.close()
     }.also {
         loadJob = it
@@ -540,7 +541,7 @@ class InsightsQueryResultsActor(
                 }
             } catch (e: Exception) {
                 // best effort; this will fail if the query raced to completion or user does not have permission
-                LOG.warn("Failed to stop query", e)
+                LOG.warn(e) { "Failed to stop query" }
             }
         }
     }