From 436ac2ec2d90ed8a39ddd826b362a1a6adb9a72b Mon Sep 17 00:00:00 2001 From: Dagger Team Date: Wed, 14 Jul 2021 14:14:44 -0700 Subject: [PATCH] Check if user added methods have name that is a java reserved keyword. Fixes #2576 RELNOTES=Check if user added methods have name that is a java reserved keyword. PiperOrigin-RevId: 384781488 --- .../codegen/kotlin/KotlinMetadataUtil.java | 15 +++ .../validation/ComponentCreatorValidator.java | 23 +++- .../validation/ComponentValidator.java | 21 +++- java/dagger/testing/compile/BUILD | 2 + .../dagger/testing/compile/CompilerTests.java | 18 ++- javatests/dagger/internal/codegen/BUILD | 29 +++++ .../codegen/ComponentValidationKtTest.java | 106 ++++++++++++++++++ 7 files changed, 209 insertions(+), 5 deletions(-) create mode 100644 javatests/dagger/internal/codegen/ComponentValidationKtTest.java diff --git a/java/dagger/internal/codegen/kotlin/KotlinMetadataUtil.java b/java/dagger/internal/codegen/kotlin/KotlinMetadataUtil.java index 980ff343840..bd25ea182b4 100644 --- a/java/dagger/internal/codegen/kotlin/KotlinMetadataUtil.java +++ b/java/dagger/internal/codegen/kotlin/KotlinMetadataUtil.java @@ -18,6 +18,8 @@ import static com.google.auto.common.AnnotationMirrors.getAnnotatedAnnotations; import static com.google.auto.common.MoreElements.isAnnotationPresent; +import static com.google.common.base.Preconditions.checkState; +import static dagger.internal.codegen.extension.DaggerStreams.toImmutableMap; import static dagger.internal.codegen.langmodel.DaggerElements.closestEnclosingTypeElement; import static kotlinx.metadata.Flag.Class.IS_COMPANION_OBJECT; import static kotlinx.metadata.Flag.Class.IS_DATA; @@ -26,7 +28,9 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import dagger.internal.codegen.extension.DaggerCollectors; +import dagger.internal.codegen.kotlin.KotlinMetadata.FunctionMetadata; import java.lang.annotation.Annotation; import java.util.Optional; import javax.inject.Inject; @@ -162,6 +166,17 @@ public boolean containsConstructorWithDefaultParam(TypeElement typeElement) { && metadataFactory.create(typeElement).containsConstructorWithDefaultParam(); } + /** + * Returns a map mapping all method signatures within the given class element, including methods + * that it inherits from its ancestors, to their method names. + */ + public ImmutableMap getAllMethodNamesBySignature(TypeElement element) { + checkState( + hasMetadata(element), "Can not call getAllMethodNamesBySignature for non-Kotlin class"); + return metadataFactory.create(element).classMetadata().functionsBySignature().values().stream() + .collect(toImmutableMap(FunctionMetadata::signature, FunctionMetadata::name)); + } + /** * Returns {@code true} if the @JvmStatic annotation is present in the given element. */ diff --git a/java/dagger/internal/codegen/validation/ComponentCreatorValidator.java b/java/dagger/internal/codegen/validation/ComponentCreatorValidator.java index 7cddc7e5cfb..99cfc7a0b91 100644 --- a/java/dagger/internal/codegen/validation/ComponentCreatorValidator.java +++ b/java/dagger/internal/codegen/validation/ComponentCreatorValidator.java @@ -20,6 +20,7 @@ import static dagger.internal.codegen.base.Util.reentrantComputeIfAbsent; import static dagger.internal.codegen.binding.ComponentCreatorAnnotation.getCreatorAnnotations; import static dagger.internal.codegen.langmodel.DaggerElements.isAnnotationPresent; +import static javax.lang.model.SourceVersion.isKeyword; import static javax.lang.model.element.Modifier.ABSTRACT; import static javax.lang.model.element.Modifier.PRIVATE; import static javax.lang.model.element.Modifier.STATIC; @@ -34,6 +35,7 @@ import dagger.internal.codegen.binding.ErrorMessages; import dagger.internal.codegen.binding.ErrorMessages.ComponentCreatorMessages; import dagger.internal.codegen.javapoet.TypeNames; +import dagger.internal.codegen.kotlin.KotlinMetadataUtil; import dagger.internal.codegen.langmodel.DaggerElements; import dagger.internal.codegen.langmodel.DaggerTypes; import java.util.HashMap; @@ -58,11 +60,14 @@ public final class ComponentCreatorValidator implements ClearableCache { private final DaggerElements elements; private final DaggerTypes types; private final Map> reports = new HashMap<>(); + private final KotlinMetadataUtil metadataUtil; @Inject - ComponentCreatorValidator(DaggerElements elements, DaggerTypes types) { + ComponentCreatorValidator( + DaggerElements elements, DaggerTypes types, KotlinMetadataUtil metadataUtil) { this.elements = elements; this.types = types; + this.metadataUtil = metadataUtil; } @Override @@ -206,6 +211,7 @@ private void validateTypeRequirements() { } private void validateBuilder() { + validateClassMethodName(); ExecutableElement buildMethod = null; for (ExecutableElement method : elements.getUnimplementedMethods(type)) { switch (method.getParameters().size()) { @@ -244,6 +250,21 @@ private void validateBuilder() { } } + private void validateClassMethodName() { + // Only Kotlin class can have method name the same as a Java reserved keyword, so only check + // the method name if this class is a Kotlin class. + if (metadataUtil.hasMetadata(type)) { + metadataUtil + .getAllMethodNamesBySignature(type) + .forEach( + (signature, name) -> { + if (isKeyword(name)) { + report.addError("Can not use a Java keyword as method name: " + signature); + } + }); + } + } + private void validateSetterMethod(ExecutableElement method) { TypeMirror returnType = types.resolveExecutableType(method, type.asType()).getReturnType(); if (returnType.getKind() != TypeKind.VOID && !types.isSubtype(type.asType(), returnType)) { diff --git a/java/dagger/internal/codegen/validation/ComponentValidator.java b/java/dagger/internal/codegen/validation/ComponentValidator.java index b74c975b75f..9ac77b46348 100644 --- a/java/dagger/internal/codegen/validation/ComponentValidator.java +++ b/java/dagger/internal/codegen/validation/ComponentValidator.java @@ -65,6 +65,7 @@ import dagger.internal.codegen.binding.MethodSignatureFormatter; import dagger.internal.codegen.binding.ModuleKind; import dagger.internal.codegen.javapoet.TypeNames; +import dagger.internal.codegen.kotlin.KotlinMetadataUtil; import dagger.internal.codegen.langmodel.DaggerElements; import dagger.internal.codegen.langmodel.DaggerTypes; import dagger.producers.ProductionComponent; @@ -79,6 +80,7 @@ import java.util.Set; import javax.inject.Inject; import javax.inject.Singleton; +import javax.lang.model.SourceVersion; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.element.ExecutableElement; @@ -105,6 +107,7 @@ public final class ComponentValidator implements ClearableCache { private final MethodSignatureFormatter methodSignatureFormatter; private final DependencyRequestFactory dependencyRequestFactory; private final Map> reports = new HashMap<>(); + private final KotlinMetadataUtil metadataUtil; @Inject ComponentValidator( @@ -115,7 +118,8 @@ public final class ComponentValidator implements ClearableCache { DependencyRequestValidator dependencyRequestValidator, MembersInjectionValidator membersInjectionValidator, MethodSignatureFormatter methodSignatureFormatter, - DependencyRequestFactory dependencyRequestFactory) { + DependencyRequestFactory dependencyRequestFactory, + KotlinMetadataUtil metadataUtil) { this.elements = elements; this.types = types; this.moduleValidator = moduleValidator; @@ -124,6 +128,7 @@ public final class ComponentValidator implements ClearableCache { this.membersInjectionValidator = membersInjectionValidator; this.methodSignatureFormatter = methodSignatureFormatter; this.dependencyRequestFactory = dependencyRequestFactory; + this.metadataUtil = metadataUtil; } @Override @@ -241,11 +246,25 @@ private void validateNoReusableAnnotation() { } private void validateComponentMethods() { + validateClassMethodName(); elements.getUnimplementedMethods(component).stream() .map(ComponentMethodValidator::new) .forEachOrdered(ComponentMethodValidator::validateMethod); } + private void validateClassMethodName() { + if (metadataUtil.hasMetadata(component)) { + metadataUtil + .getAllMethodNamesBySignature(component) + .forEach( + (signature, name) -> { + if (SourceVersion.isKeyword(name)) { + report.addError("Can not use a Java keyword as method name: " + signature); + } + }); + } + } + private class ComponentMethodValidator { private final ExecutableElement method; private final ExecutableType resolvedMethod; diff --git a/java/dagger/testing/compile/BUILD b/java/dagger/testing/compile/BUILD index 20e6a3d203c..b1d08eea6db 100644 --- a/java/dagger/testing/compile/BUILD +++ b/java/dagger/testing/compile/BUILD @@ -28,5 +28,7 @@ java_library( "//java/dagger/internal/guava:collect", "//java/dagger/internal/guava:io", "@google_bazel_common//third_party/java/compile_testing", + "//java/dagger/internal/codegen:processor", + "@maven//:com_github_tschuchortdev_kotlin_compile_testing", ], ) diff --git a/java/dagger/testing/compile/CompilerTests.java b/java/dagger/testing/compile/CompilerTests.java index 6750d0c65a2..a7fbef71b95 100644 --- a/java/dagger/testing/compile/CompilerTests.java +++ b/java/dagger/testing/compile/CompilerTests.java @@ -29,10 +29,10 @@ import java.nio.file.Paths; import java.util.Map; import java.util.NoSuchElementException; +import com.tschuchort.compiletesting.KotlinCompilation; +import dagger.internal.codegen.ComponentProcessor; -/** - * A helper class for working with java compiler tests. - */ +/** A helper class for working with java compiler tests. */ public final class CompilerTests { private CompilerTests() {} @@ -53,6 +53,18 @@ public static Compiler compiler() { return javac().withClasspath(ImmutableList.of(compilerDepsJar())); } + public static KotlinCompilation kotlinCompiler() { + KotlinCompilation compilation = new KotlinCompilation(); + compilation.setAnnotationProcessors(ImmutableList.of(new ComponentProcessor())); + compilation.setClasspaths( + ImmutableList.builder() + .addAll(compilation.getClasspaths()) + .add(compilerDepsJar()) + .build() + ); + return compilation; + } + private static File getRunfilesDir() { return getRunfilesPath().toFile(); } diff --git a/javatests/dagger/internal/codegen/BUILD b/javatests/dagger/internal/codegen/BUILD index 9346db65b56..d1f2eef4187 100644 --- a/javatests/dagger/internal/codegen/BUILD +++ b/javatests/dagger/internal/codegen/BUILD @@ -18,6 +18,7 @@ load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_library") load("//:build_defs.bzl", "DOCLINT_HTML_AND_SYNTAX") load("//:test_defs.bzl", "GenJavaTests") +load("//java/dagger/testing/compile:macros.bzl", "compiler_test") package(default_visibility = ["//:src"]) @@ -58,6 +59,7 @@ GenJavaTests( "CompilerMode.java", "Compilers.java", "JavaFileBuilder.java", + "ComponentValidationKtTest.java", ], ), functional = False, @@ -96,3 +98,30 @@ GenJavaTests( "@google_bazel_common//third_party/java/truth", ], ) + +compiler_test( + name = "ComponentValidationKtTest", + srcs = ["ComponentValidationKtTest.java"], + compiler_deps = [ + "//java/dagger:core", + "//java/dagger/internal/codegen:package_info", + "//java/dagger/internal/codegen:processor", + "//java/dagger/internal/codegen/base", + "//java/dagger/internal/codegen/binding", + "//java/dagger/internal/codegen/bindinggraphvalidation", + "//java/dagger/internal/codegen/compileroption", + "//java/dagger/internal/codegen/javapoet", + "//java/dagger/internal/codegen/langmodel", + "//java/dagger/internal/codegen/validation", + "//java/dagger/internal/codegen/writing", + "//java/dagger/model/testing", + "//java/dagger/producers", + "//java/dagger/spi", + ], + deps = [ + "//java/dagger/internal/guava:collect", + "@google_bazel_common//third_party/java/junit", + "@google_bazel_common//third_party/java/truth", + "@maven//:com_github_tschuchortdev_kotlin_compile_testing", + ], +) diff --git a/javatests/dagger/internal/codegen/ComponentValidationKtTest.java b/javatests/dagger/internal/codegen/ComponentValidationKtTest.java new file mode 100644 index 00000000000..4600f99238f --- /dev/null +++ b/javatests/dagger/internal/codegen/ComponentValidationKtTest.java @@ -0,0 +1,106 @@ +/* + * Copyright (C) 2021 The Dagger Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package dagger.internal.codegen; + +import static com.google.common.truth.Truth.assertThat; +import static dagger.testing.compile.CompilerTests.kotlinCompiler; + +import com.google.common.collect.ImmutableList; +import com.tschuchort.compiletesting.KotlinCompilation; +import com.tschuchort.compiletesting.KotlinCompilation.ExitCode; +import com.tschuchort.compiletesting.SourceFile; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class ComponentValidationKtTest { + @Test + public void creatorMethodNameIsJavaKeyword_compilationError() { + SourceFile componentSrc = + SourceFile.Companion.kotlin( + "FooComponent.kt", + String.join( + "\n", + "package test", + "", + "import dagger.BindsInstance", + "import dagger.Component", + "", + "@Component", + "interface FooComponent {", + " @Component.Builder", + " interface Builder {", + " @BindsInstance public fun int(str: Int): Builder", + " public fun build(): FooComponent", + " }", + "}"), + false); + KotlinCompilation compilation = kotlinCompiler(); + compilation.setSources(ImmutableList.of(componentSrc)); + + KotlinCompilation.Result result = compilation.compile(); + + // TODO(b/192396673): Add error count when the feature request is fulfilled. + assertThat(result.getExitCode()).isEqualTo(ExitCode.COMPILATION_ERROR); + assertThat(result.getMessages()) + .contains("Can not use a Java keyword as method name: int(I)Ltest/FooComponent$Builder"); + } + + @Test + public void componentMethodNameIsJavaKeyword_compilationError() { + SourceFile componentSrc = + SourceFile.Companion.kotlin( + "FooComponent.kt", + String.join( + "\n", + "package test", + "", + "import dagger.BindsInstance", + "import dagger.Component", + "", + "@Component(modules = [TestModule::class])", + "interface FooComponent {", + " fun int(str: Int): String", + "}"), + false); + SourceFile moduleSrc = + SourceFile.Companion.kotlin( + "TestModule.kt", + String.join( + "\n", + "package test", + "", + "import dagger.Module", + "", + "@Module", + "interface TestModule {", + " fun providesString(): String {", + " return \"test\"", + " }", + "}"), + false); + KotlinCompilation compilation = kotlinCompiler(); + compilation.setSources(ImmutableList.of(componentSrc, moduleSrc)); + + KotlinCompilation.Result result = compilation.compile(); + + assertThat(result.getExitCode()).isEqualTo(ExitCode.COMPILATION_ERROR); + assertThat(result.getMessages()) + .contains("Can not use a Java keyword as method name: int(I)Ljava/lang/String"); + } +}