From 64ce6e2cd170d4e546f231298977a3724997d088 Mon Sep 17 00:00:00 2001 From: Vlad Chesnokov Date: Fri, 13 Dec 2024 18:18:29 +0100 Subject: [PATCH] Lint rule to fix the space assignment syntax --- gradle/wrapper/gradle-wrapper.properties | 2 +- gradlew | 3 +- .../nebula/lint/rule/GradleLintRule.groovy | 27 +-- .../nebula/lint/rule/GradleModelAware.groovy | 186 +++++++++++++++++- .../lint/rule/dsl/SpaceAssignmentRule.groovy | 50 +++++ .../lint-rules/space-assignment.properties | 1 + .../rule/dsl/SpaceAssignmentRuleSpec.groovy | 149 ++++++++++++++ 7 files changed, 402 insertions(+), 16 deletions(-) create mode 100644 src/main/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRule.groovy create mode 100644 src/main/resources/META-INF/lint-rules/space-assignment.properties create mode 100644 src/test/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRuleSpec.groovy diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index b6d150fa..cea7a793 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,6 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https://services.gradle.org/distributions/gradle-8.11.1-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.12-bin.zip networkTimeout=10000 validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME diff --git a/gradlew b/gradlew index f5feea6d..f3b75f3b 100755 --- a/gradlew +++ b/gradlew @@ -86,8 +86,7 @@ done # shellcheck disable=SC2034 APP_BASE_NAME=${0##*/} # Discard cd standard output in case $CDPATH is set (https://github.com/gradle/gradle/issues/25036) -APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s -' "$PWD" ) || exit +APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s\n' "$PWD" ) || exit # Use the maximum available, or set MAX_FD != -1 to use that value. MAX_FD=maximum diff --git a/src/main/groovy/com/netflix/nebula/lint/rule/GradleLintRule.groovy b/src/main/groovy/com/netflix/nebula/lint/rule/GradleLintRule.groovy index 03277144..07069a8f 100644 --- a/src/main/groovy/com/netflix/nebula/lint/rule/GradleLintRule.groovy +++ b/src/main/groovy/com/netflix/nebula/lint/rule/GradleLintRule.groovy @@ -20,16 +20,7 @@ import com.netflix.nebula.lint.GradleViolation import com.netflix.nebula.lint.plugin.LintRuleRegistry import org.codehaus.groovy.ast.ASTNode import org.codehaus.groovy.ast.ClassNode -import org.codehaus.groovy.ast.expr.ArgumentListExpression -import org.codehaus.groovy.ast.expr.BinaryExpression -import org.codehaus.groovy.ast.expr.ClosureExpression -import org.codehaus.groovy.ast.expr.ConstantExpression -import org.codehaus.groovy.ast.expr.Expression -import org.codehaus.groovy.ast.expr.GStringExpression -import org.codehaus.groovy.ast.expr.MapExpression -import org.codehaus.groovy.ast.expr.MethodCallExpression -import org.codehaus.groovy.ast.expr.PropertyExpression -import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.ast.expr.* import org.codehaus.groovy.ast.stmt.ExpressionStatement import org.codenarc.rule.AbstractAstVisitorRule import org.codenarc.rule.AstVisitor @@ -43,7 +34,6 @@ import org.slf4j.LoggerFactory import java.text.ParseException abstract class GradleLintRule extends GroovyAstVisitor implements Rule { - Project project // will be non-null if type is GradleModelAware, otherwise null BuildFiles buildFiles SourceCode sourceCode List gradleViolations = [] @@ -152,6 +142,21 @@ abstract class GradleLintRule extends GroovyAstVisitor implements Rule { calls.collect { call -> _dslStack(call) }.flatten() as List } + final List typedDslStack(List calls) { + def _dslStack + _dslStack = { Expression expr -> + if (expr instanceof PropertyExpression) + _dslStack(expr.objectExpression) + expr.property + else if (expr instanceof MethodCallExpression) + _dslStack(expr.objectExpression) + expr + else if (expr instanceof VariableExpression) + expr.text == 'this' ? [] : [expr] + else [] + } + + calls.collect { call -> _dslStack(call) }.flatten() as List + } + private final String containingConfiguration(MethodCallExpression call) { def stackStartingWithConfName = dslStack(callStack + call).dropWhile { it != 'configurations' }.drop(1) stackStartingWithConfName.isEmpty() ? null : stackStartingWithConfName[0] diff --git a/src/main/groovy/com/netflix/nebula/lint/rule/GradleModelAware.groovy b/src/main/groovy/com/netflix/nebula/lint/rule/GradleModelAware.groovy index 233aa51c..cc5f3dce 100644 --- a/src/main/groovy/com/netflix/nebula/lint/rule/GradleModelAware.groovy +++ b/src/main/groovy/com/netflix/nebula/lint/rule/GradleModelAware.groovy @@ -16,12 +16,194 @@ package com.netflix.nebula.lint.rule +import org.codehaus.groovy.ast.expr.* +import org.gradle.api.Action import org.gradle.api.Project +import org.gradle.api.Task +import org.gradle.api.internal.DefaultDomainObjectCollection +import org.gradle.api.plugins.ExtensionAware +import org.gradle.configuration.ImportsReader + +import javax.annotation.Nullable /** * Decorate lint rule visitors with this interface in order to use the * evaluated Gradle project model in the rule */ -interface GradleModelAware { - void setProject(Project project) +trait GradleModelAware { + Project project + Map> projectDefaultImports = null + + TypeInformation receiver(MethodCallExpression call) { + List fullCallStack = typedDslStack(callStack + call) + List typedStack = [] + for (Expression currentMethod in fullCallStack) { + if (typedStack.empty) { + typedStack.add(new TypeInformation(project)) + } + while (!typedStack.empty) { + def current = typedStack.last() + def candidate = findDirectCandidate(current, currentMethod) + if (candidate != null) { + typedStack.add(candidate) + break + } + typedStack.removeLast() + } + } + if (typedStack.size() >= 2) { //there should be the method return type and the receiver at least + return typedStack[-2] + } else { + return null + } + } + + private findDirectCandidate(TypeInformation current, Expression currentExpression) { + String methodName + switch (currentExpression) { + case MethodCallExpression: + methodName = currentExpression.methodAsString + break + case PropertyExpression: + methodName = currentExpression.propertyAsString + break + case VariableExpression: + methodName = currentExpression.text + break + case ConstantExpression: + methodName = currentExpression.text + break + default: + return null + } + def getter = current.clazz.getMethods().find { it.name == "get${methodName.capitalize()}" } + if (getter != null) { + if (current.object != null) { + try { + return new TypeInformation(getter.invoke(current.object)) + } catch (ignored) { + // ignore and fallback to the return type + } + } + return new TypeInformation(null, getter.returnType) + } + + // there is no public API for DomainObjectCollection.type + if (current.object != null && DefaultDomainObjectCollection.class.isAssignableFrom(current.clazz)) { + def collectionItemType = ((DefaultDomainObjectCollection) current.object).type + + if (methodName == "withType" && currentExpression instanceof MethodCallExpression && currentExpression.arguments.size() >= 1) { + def className = currentExpression.arguments[0] + def candidate = findSuitableClass(className.text, collectionItemType) + if (candidate != null) { + collectionItemType = candidate + } + } + + if ((methodName == "create" || methodName == "register") && currentExpression instanceof MethodCallExpression && currentExpression.arguments.size() >= 2 && currentExpression.arguments[1] !instanceof ClosureExpression) { + def className = currentExpression.arguments[1] + def candidate = findSuitableClass(className.text, collectionItemType) + if (candidate != null) { + collectionItemType = candidate + } + } + + def transformationOrFactoryMethod = current.clazz.getMethods().find { it.name == methodName && it.parameterTypes[-1] == Action.class } + if (transformationOrFactoryMethod != null) { + if (collectionItemType.isAssignableFrom(transformationOrFactoryMethod.returnType)) { + return new TypeInformation(null, transformationOrFactoryMethod.returnType) + } else { + // assume that all actions are done on the collection type + return new TypeInformation(null, collectionItemType) + } + } + } + + // note that we can't use tasks.findByName because it may lead to unwanted side effects because of potential task creation + if (Project.class.isAssignableFrom(current.clazz) && methodName == "task" && currentExpression instanceof MethodCallExpression) { + def taskType = extractTaskType(currentExpression) + if (taskType != null) { + return new TypeInformation(null, taskType) + } + return new TypeInformation(null, Task.class) + } + + def factoryMethod = current.clazz.getMethods().find { it.name == methodName && it.parameterTypes[-1] == Action.class } + if (factoryMethod != null) { + // assume that this is a factory method that returns the created type + return new TypeInformation(null, factoryMethod.returnType) + } + + if (current.object != null && current.object instanceof ExtensionAware) { + def extension = current.object.extensions.findByName(methodName) + if (extension != null) { + return new TypeInformation(extension) + } + } + return null; + } + + private List findClassInScope(String name) { + if (this.projectDefaultImports == null) { + this.projectDefaultImports = project.services.get(ImportsReader.class).getSimpleNameToFullClassNamesMapping() + } + return this.projectDefaultImports.get(name); + } + + @Nullable + private Class findSuitableClass(String className, Class parentClass) { + def candidates = (findClassInScope(className) ?: []) + [className] + for (String candidate in candidates) { + try { + def candidateClass = Class.forName(candidate) + if (parentClass.isAssignableFrom(candidateClass)) { + return candidateClass + } + } catch (ignored) { + // ignore and try the next candidate + } + } + return null + } + + @Nullable + private Class extractTaskType(MethodCallExpression currentExpression) { + for (Expression arg in currentExpression.arguments) { + if (arg instanceof VariableExpression || arg instanceof ConstantExpression) { + def candidate = findSuitableClass(arg.text, Task.class) + if (candidate != null) { + return candidate + } + } else if (arg instanceof MapExpression) { + def type = arg + .mapEntryExpressions + .find { it.keyExpression.text == "type" } + ?.valueExpression?.text + if (type != null) { + def candidate = findSuitableClass(type, Task.class) + if (candidate != null) { + return candidate + } + } + } + } + return null + } +} + +class TypeInformation { + @Nullable + Class clazz + @Nullable + Object object + + TypeInformation(Object object) { + this.object = object + this.clazz = object.class + } + + TypeInformation(Object object, Class clazz) { + this.object = object + this.clazz = clazz + } } \ No newline at end of file diff --git a/src/main/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRule.groovy b/src/main/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRule.groovy new file mode 100644 index 00000000..ccc6a343 --- /dev/null +++ b/src/main/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRule.groovy @@ -0,0 +1,50 @@ +package com.netflix.nebula.lint.rule.dsl + +import com.netflix.nebula.lint.rule.GradleLintRule +import com.netflix.nebula.lint.rule.GradleModelAware +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.MethodCallExpression + +class SpaceAssignmentRule extends GradleLintRule implements GradleModelAware { + + String description = "space-assignment syntax is deprecated" + + @Override + void visitMethodCallExpression(MethodCallExpression call) { + if (call.arguments.size() != 1 || call.arguments[-1] instanceof ClosureExpression) { + return + } + + def receiverClass = receiver(call)?.clazz + if (receiverClass == null) { + return // no enough data to analyze + } + + def invokedMethodName = call.method.value + + // check if the method has a matching property + def setter = receiverClass.getMethods().find { it.name == "set${invokedMethodName.capitalize()}" } + if (setter == null) { + return // no matching property + } + + // check if it's a generated method for space assignment + def exactMethod = receiverClass.getMethods().find { it.name == invokedMethodName } + if (exactMethod != null) { + def deprecatedAnnotation = exactMethod.getAnnotation(Deprecated) + if (deprecatedAnnotation != null) { + // may be false positive when the explicit method is deprecated + addBuildLintViolation(description, call) + .replaceWith(call, getReplacement(call)) + } + } else { + addBuildLintViolation(description, call) + .replaceWith(call, getReplacement(call)) + } + } + + def getReplacement(MethodCallExpression call){ + def originalLine = getSourceCode().line(call.lineNumber-1) + return originalLine.replaceFirst(call.methodAsString, call.methodAsString + " =") + } +} diff --git a/src/main/resources/META-INF/lint-rules/space-assignment.properties b/src/main/resources/META-INF/lint-rules/space-assignment.properties new file mode 100644 index 00000000..b5e04596 --- /dev/null +++ b/src/main/resources/META-INF/lint-rules/space-assignment.properties @@ -0,0 +1 @@ +implementation-class=com.netflix.nebula.lint.rule.dsl.SpaceAssignmentRule \ No newline at end of file diff --git a/src/test/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRuleSpec.groovy b/src/test/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRuleSpec.groovy new file mode 100644 index 00000000..0c0ee0e0 --- /dev/null +++ b/src/test/groovy/com/netflix/nebula/lint/rule/dsl/SpaceAssignmentRuleSpec.groovy @@ -0,0 +1,149 @@ +package com.netflix.nebula.lint.rule.dsl + +import com.netflix.nebula.lint.BaseIntegrationTestKitSpec +import spock.lang.Subject + +@Subject(SpaceAssignmentRule) +class SpaceAssignmentRuleSpec extends BaseIntegrationTestKitSpec { + + def 'reports and fixes a violation if space assignment syntax is used - simple cases'() { + buildFile << """ + import java.util.regex.Pattern; + import org.gradle.internal.SystemProperties; + + plugins { + id 'java' + id 'nebula.lint' + } + + println(file("text")) + getRepositories().gradlePluginPortal() + + buildDir file("out") + + repositories { + 'hello' + maven { + url "https://example.com" + } + maven { + url("https://example2.com") + } + maven { + url = "https://another.example.com" + } + } + + java { + sourceCompatibility JavaVersion.VERSION_1_8 + } + gradleLint.rules = ['space-assignment'] + println(Pattern.quote("test")) + SystemProperties i = SystemProperties.getInstance() + i.getJavaIoTmpDir() + """ + + when: + def result = runTasks('autoLintGradle', '--warning-mode', 'none') + + then: + result.output.contains("4 problems (0 errors, 4 warnings)") + + when: + runTasks('fixLintGradle', '--warning-mode', 'none') + + then: + buildFile.text.contains('buildDir = file("out")') + buildFile.text.contains('url = "https://example.com"') + buildFile.text.contains('url =("https://example2.com")') + buildFile.text.contains('sourceCompatibility = JavaVersion.VERSION_1_8') + } + + def 'reports and fixes a violation if space assignment syntax is used in some complex cases'() { + buildFile << """ + plugins { + id 'nebula.lint' + id 'java' + } + gradleLint.rules = ['space-assignment'] + + java { + toolchain { + languageVersion = JavaLanguageVersion.of(11) + sourceCompatibility JavaVersion.VERSION_1_8 // wrong level, should be in java block + } + } + + tasks.withType(JavaCompile) { + description "Compiles the Java source" + options.forkOptions { + tempDir "temp" + } + doLast { + repositories { + maven { + url "https://example3.com" + } + } + } + } + tasks.configureEach { // an action on DomainObjectCollection + group "pew pew" + } + tasks.withType(JavaCompile).configureEach { + options.forkOptions { + tempDir "wololo" // not supported since it's hard + } + } + task(t3) { + description "t3" + } + task t6 { + description "t6" + } + task (t7, type: Wrapper){ + distributionPath "t7" + } + task t9(type: Wrapper){ // unsupported since it's a nested call (with "t9" as method name) and it's harder to detect it reliably + distributionPath "t9" + } + task ("t300", type: Wrapper){ + distributionPath "t300" + } + tasks.create([name: 't15']) { + description "t15" + } + tasks.create('t18', Wrapper) { + distributionPath "t18" + } + tasks.register('t22', Wrapper) { + distributionPath "t22" + } + """ + + when: + def result = runTasks('autoLintGradle', '--warning-mode', 'none') + + then: + result.output.contains("12 problems (0 errors, 12 warnings)") + + when: + runTasks('fixLintGradle', '--warning-mode', 'none') + + then: + buildFile.text.contains('description = "Compiles the Java source"') + buildFile.text.contains('tempDir = "temp"') + buildFile.text.contains('url = "https://example3.com"') + buildFile.text.contains('sourceCompatibility = JavaVersion.VERSION_1_8') + buildFile.text.contains('group = "pew pew"') + //buildFile.text.contains('tempDir = "wololo"') //not supported + buildFile.text.contains('description = "t3"') + buildFile.text.contains('description = "t6"') + buildFile.text.contains('distributionPath = "t7"') + // buildFile.text.contains('distributionPath = "t9"') //not supported + buildFile.text.contains('distributionPath = "t300"') + buildFile.text.contains('description = "t15"') + buildFile.text.contains('distributionPath = "t18"') + buildFile.text.contains('distributionPath = "t22"') + } +}