From 3c520d1d9812d95a315135e4e5db6303dac3536f Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Tue, 6 Oct 2020 16:34:25 +0100 Subject: [PATCH 01/19] Check that all required builder fields have been initialized --- ...mmutablesBuilderMissingInitialization.java | 175 ++++++++++++++++++ ...ablesBuilderMissingInitializationTest.java | 125 +++++++++++++ 2 files changed, 300 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java new file mode 100644 index 000000000..5222c2e1d --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -0,0 +1,175 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * 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 com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.common.base.CaseFormat; +import com.google.common.base.Converter; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompoundAssignmentTree; +import com.sun.source.tree.ExpressionStatementTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.UnaryTree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.immutables.value.Generated; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ImmutablesBuilderMissingInitialization", + linkType = LinkType.CUSTOM, + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + severity = BugPattern.SeverityLevel.ERROR, + summary = "__TODO__") +public final class ImmutablesBuilderMissingInitialization extends BugChecker implements MethodInvocationTreeMatcher { + private static final String FIELD_INIT_BITS_PREFIX = "INIT_BIT_"; + + private static final Joiner COMMA_JOINER = Joiner.on(", "); + private static final Converter UPPER_TO_CAMEL_CASE_CONVERTER = + CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL); + private static final Matcher BUILDER_METHOD_MATCHER = + Matchers.instanceMethod().anyClass().named("build").withParameters(); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + // Check that the method is a builder's build() method + if (!BUILDER_METHOD_MATCHER.matches(tree, state)) { + return Description.NO_MATCH; + } + ClassSymbol classSymbol = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); + if (Optional.ofNullable(classSymbol.getAnnotation(Generated.class)) + .map(annotation -> !annotation.generator().equals("Immutables")) + .orElse(true)) { + return Description.NO_MATCH; + } + + // Find all of the initBits in the generated builder, to determine the mandatory fields + ClassTree classTree = ASTHelpers.findClass(classSymbol, state); + if (classTree == null) { + return Description.NO_MATCH; + } + Set initBits = classTree.getMembers().stream() + .flatMap(memberTree -> { + if (memberTree instanceof VariableTree) { + return Stream.of((VariableTree) memberTree); + } + return Stream.empty(); + }) + .filter(variableTree -> variableTree.getName().toString().startsWith(FIELD_INIT_BITS_PREFIX)) + .map(variableTree -> variableTree.getName().toString()) + .collect(Collectors.toSet()); + + Set missingInitBits = checkInitialization(ASTHelpers.getReceiver(tree), initBits, state, classSymbol); + if (missingInitBits.isEmpty()) { + return Description.NO_MATCH; + } + + return buildDescription(tree) + .setMessage(String.format( + "Some builder fields have not been initialized: %s", + COMMA_JOINER.join(missingInitBits.stream() + .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) + .map(UPPER_TO_CAMEL_CASE_CONVERTER::convert) + .iterator()))) + .build(); + } + + private Set checkInitialization( + ExpressionTree tree, Set uninitializedInitBits, VisitorState state, ClassSymbol builderClass) { + if (tree != null && tree.getKind().equals(Kind.METHOD_INVOCATION)) { + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree; + MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree); + ClassSymbol classSymbol = methodSymbol.enclClass(); + + // If this is the builder's constructor or a builder method on the parent class, stop here; if it's another + // method on a different class, give up because it will quickly become impossible to track whether it's + // been fully initialized + if (classSymbol == null || !classSymbol.equals(builderClass)) { + if (methodSymbol.getSimpleName().contentEquals("builder") + && methodSymbol.getParameters().isEmpty()) { + // No args builder method that returns this type can be assumed to just construct the builder + return uninitializedInitBits; + } + // If this is any other method, there may be initialization that occurs in the method, and trying to + // track that down will quickly become impossible, so give up here + return ImmutableSet.of(); + } + + MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); + Set initializedInitBits = methodTree.getBody().getStatements().stream() + .flatMap(filterByType(ExpressionStatementTree.class)) + .map(ExpressionStatementTree::getExpression) + .flatMap(filterByType(CompoundAssignmentTree.class)) + .filter(assignmentTree -> { + if (assignmentTree.getVariable() instanceof IdentifierTree) { + return ((IdentifierTree) assignmentTree.getVariable()) + .getName() + .contentEquals("initBits"); + } + return false; + }) + .map(CompoundAssignmentTree::getExpression) + .flatMap(filterByType(UnaryTree.class)) + .flatMap(unaryTree -> { + if (unaryTree.getKind().equals(Kind.BITWISE_COMPLEMENT)) { + return Stream.of(unaryTree.getExpression()); + } + return Stream.empty(); + }) + .flatMap(filterByType(IdentifierTree.class)) + .map(identifierTree -> identifierTree.getName().toString()) + .collect(Collectors.toSet()); + + Set remainingInitBits = Sets.difference(uninitializedInitBits, initializedInitBits); + return checkInitialization(ASTHelpers.getReceiver(tree), remainingInitBits, state, builderClass); + } + + // If the chain started with something other than a method call to create the builder, give up + return ImmutableSet.of(); + } + + private Function> filterByType(Class clazz) { + return value -> { + if (clazz.isInstance(value)) { + return Stream.of(clazz.cast(value)); + } + return Stream.empty(); + }; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java new file mode 100644 index 000000000..6a7866649 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -0,0 +1,125 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * 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 com.palantir.baseline.errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; + +public class ImmutablesBuilderMissingInitializationTest { + + @Test + public void testPass() { + helper().addSourceLines( + "Person.java", + "import org.immutables.value.Value;", + "@Value.Immutable", + "public interface Person {", + " String name();", + " static ImmutablePerson.Builder builder() {", + " return new ImmutablePerson.Builder();", + " }", + "}") + .addSourceLines( + "ImmutablePerson.java", + "import java.util.Objects;", + "import javax.annotation.Nullable;", + "import org.immutables.value.Generated;", + "@Generated(from = \"Person\", generator = \"Immutables\")", + "final class ImmutablePerson implements Person {", + " private final String name;", + " private ImmutablePerson(String name) { this.name = name; }", + " @Override", + " public String name() { return name; }", + "", + " @Generated(from = \"Person\", generator = \"Immutables\")", + " public static final class Builder {", + " private static final long INIT_BIT_NAME = 0x1L;", + " private long initBits = 0x1L;", + " private @Nullable String name;", + " public final Builder name(String name) {", + " this.name = Objects.requireNonNull(name, \"name\");", + " initBits &= ~INIT_BIT_NAME;", + " return this;", + " }", + " public Person build() {", + " return new ImmutablePerson(name);", + " }", + " }", + "}") + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " Person.builder().name(\"name\").build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFail() { + helper().addSourceLines( + "Person.java", + "import org.immutables.value.Value;", + "@Value.Immutable", + "public interface Person {", + " String name();", + " static ImmutablePerson.Builder builder() {", + " return new ImmutablePerson.Builder();", + " }", + "}") + .addSourceLines( + "ImmutablePerson.java", + "import java.util.Objects;", + "import javax.annotation.Nullable;", + "import org.immutables.value.Generated;", + "@Generated(from = \"Person\", generator = \"Immutables\")", + "final class ImmutablePerson implements Person {", + " private final String name;", + " private ImmutablePerson(String name) { this.name = name; }", + " @Override", + " public String name() { return name; }", + "", + " @Generated(from = \"Person\", generator = \"Immutables\")", + " public static final class Builder {", + " private static final long INIT_BIT_NAME = 0x1L;", + " private long initBits = 0x1L;", + " private @Nullable String name;", + " public final Builder name(String name) {", + " this.name = Objects.requireNonNull(name, \"name\");", + " initBits &= ~INIT_BIT_NAME;", + " return this;", + " }", + " public Person build() {", + " return new ImmutablePerson(name);", + " }", + " }", + "}") + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", + " Person.builder().build();", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ImmutablesBuilderMissingInitialization.class, getClass()); + } +} From 4c070fc8166a3ae3b848bc4b15cad4097a28ec7c Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Tue, 6 Oct 2020 16:48:48 +0100 Subject: [PATCH 02/19] Handle constructors and from methods --- ...mmutablesBuilderMissingInitialization.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 5222c2e1d..e9e7caf1e 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -21,7 +21,6 @@ import com.google.common.base.Converter; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; import com.google.errorprone.VisitorState; @@ -117,19 +116,25 @@ private Set checkInitialization( MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree); ClassSymbol classSymbol = methodSymbol.enclClass(); - // If this is the builder's constructor or a builder method on the parent class, stop here; if it's another - // method on a different class, give up because it will quickly become impossible to track whether it's - // been fully initialized if (classSymbol == null || !classSymbol.equals(builderClass)) { if (methodSymbol.getSimpleName().contentEquals("builder") && methodSymbol.getParameters().isEmpty()) { - // No args builder method that returns this type can be assumed to just construct the builder + // A nullary method named "builder" which returns the Builder type can be assumed to just construct + // the builder return uninitializedInitBits; } // If this is any other method, there may be initialization that occurs in the method, and trying to // track that down will quickly become impossible, so give up here return ImmutableSet.of(); } + if (methodSymbol.getSimpleName().contentEquals("")) { + // It's rooted at the builder constructor, so all the fields should be initialized + return uninitializedInitBits; + } + if (methodSymbol.getSimpleName().contentEquals("from")) { + // from initializes all fields + return ImmutableSet.of(); + } MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); Set initializedInitBits = methodTree.getBody().getStatements().stream() @@ -156,7 +161,9 @@ private Set checkInitialization( .map(identifierTree -> identifierTree.getName().toString()) .collect(Collectors.toSet()); - Set remainingInitBits = Sets.difference(uninitializedInitBits, initializedInitBits); + Set remainingInitBits = uninitializedInitBits.stream() + .filter(initBit -> !initializedInitBits.contains(initBit)) + .collect(Collectors.toSet()); return checkInitialization(ASTHelpers.getReceiver(tree), remainingInitBits, state, builderClass); } From 9031d01cbc2e895e8449dc7c79eb6b83623bee29 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Tue, 6 Oct 2020 18:43:57 +0100 Subject: [PATCH 03/19] Add tests --- ...mmutablesBuilderMissingInitialization.java | 23 +- ...ablesBuilderMissingInitializationTest.java | 250 ++++++++++++++---- 2 files changed, 213 insertions(+), 60 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index e9e7caf1e..e7c576c1a 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -37,6 +37,7 @@ import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.UnaryTree; import com.sun.source.tree.VariableTree; @@ -55,7 +56,7 @@ linkType = LinkType.CUSTOM, link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", severity = BugPattern.SeverityLevel.ERROR, - summary = "__TODO__") + summary = "All required fields of an Immutables builder must be initialized") public final class ImmutablesBuilderMissingInitialization extends BugChecker implements MethodInvocationTreeMatcher { private static final String FIELD_INIT_BITS_PREFIX = "INIT_BIT_"; @@ -111,7 +112,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState private Set checkInitialization( ExpressionTree tree, Set uninitializedInitBits, VisitorState state, ClassSymbol builderClass) { - if (tree != null && tree.getKind().equals(Kind.METHOD_INVOCATION)) { + if (tree instanceof MethodInvocationTree) { MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree; MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree); ClassSymbol classSymbol = methodSymbol.enclClass(); @@ -127,12 +128,8 @@ private Set checkInitialization( // track that down will quickly become impossible, so give up here return ImmutableSet.of(); } - if (methodSymbol.getSimpleName().contentEquals("")) { - // It's rooted at the builder constructor, so all the fields should be initialized - return uninitializedInitBits; - } if (methodSymbol.getSimpleName().contentEquals("from")) { - // from initializes all fields + // `from` initializes all fields return ImmutableSet.of(); } @@ -165,9 +162,19 @@ private Set checkInitialization( .filter(initBit -> !initializedInitBits.contains(initBit)) .collect(Collectors.toSet()); return checkInitialization(ASTHelpers.getReceiver(tree), remainingInitBits, state, builderClass); + } else if (tree instanceof NewClassTree) { + NewClassTree newClassTree = (NewClassTree) tree; + + if (newClassTree.getArguments().isEmpty()) { + // The constructor returned the builder (otherwise we would have bailed out in a previous iteration), so + // we should have seen all the field initializations + return uninitializedInitBits; + } + // If the constructor takes arguments, it's doing something funky + return ImmutableSet.of(); } - // If the chain started with something other than a method call to create the builder, give up + // If the chain started with something other than a simple method call to create the builder, give up return ImmutableSet.of(); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 6a7866649..91a53a05c 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -21,105 +21,251 @@ public class ImmutablesBuilderMissingInitializationTest { @Test - public void testPass() { + public void testPassesWithAllFieldsPopulated() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " Person.builder().name(\"name\").currentAge(70).company(\"palantir\").build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWithOptionalFieldOmitted() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " Person.builder().name(\"name\").currentAge(60).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWithFrom() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " Person p1 = Person.builder().name(\"name\").currentAge(50).build();", + " Person.builder().from(p1).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWithAllFieldsPopulated_usingExtendedConstructor() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " new Person.Builder().name(\"name\").currentAge(40).company(\"palantir\").build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWithAllFieldsPopulated_usingImmutableConstructor() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " new ImmutablePerson.Builder().name(\"name\").currentAge(30).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithOneMandatoryFieldOmitted() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " Person.builder()", + " .currentAge(20)", + " .company(\"palantir\")", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: ", + " // name", + " .build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithAllFieldsOmitted() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, ", + " // currentAge", + " Person.builder().build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithOneFieldOmitted_usingExtendedConstructor() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", + " new Person.Builder().currentAge(10).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithOneFieldOmitted_usingImmutableConstructor() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", + " new ImmutablePerson.Builder().currentAge(0).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testSucceedsWithNoRequiredFields() { helper().addSourceLines( - "Person.java", + "Bug.java", + "import java.util.Optional;", "import org.immutables.value.Value;", "@Value.Immutable", - "public interface Person {", - " String name();", - " static ImmutablePerson.Builder builder() {", - " return new ImmutablePerson.Builder();", + "public interface Bug {", + " Optional author();", + " static Builder builder() {", + " return new Builder();", " }", "}") .addSourceLines( "ImmutablePerson.java", "import java.util.Objects;", + "import java.util.Optional;", "import javax.annotation.Nullable;", "import org.immutables.value.Generated;", - "@Generated(from = \"Person\", generator = \"Immutables\")", - "final class ImmutablePerson implements Person {", - " private final String name;", - " private ImmutablePerson(String name) { this.name = name; }", + "@Generated(from = \"Bug\", generator = \"Immutables\")", + "final class ImmutableBug implements Bug {", + " private final @Nullable String author;", + " private ImmutablePerson(@Nullable String author) {", + " this.author = author;", + " }", " @Override", - " public String name() { return name; }", + " public Optional author() { return Optional.ofNullable(author); }", "", - " @Generated(from = \"Person\", generator = \"Immutables\")", - " public static final class Builder {", - " private static final long INIT_BIT_NAME = 0x1L;", - " private long initBits = 0x1L;", - " private @Nullable String name;", - " public final Builder name(String name) {", - " this.name = Objects.requireNonNull(name, \"name\");", - " initBits &= ~INIT_BIT_NAME;", + " @Generated(from = \"Bug\", generator = \"Immutables\")", + " public static class Builder {", + " private @Nullable String author;", + " public final Builder author(String author) {", + " this.author = Objects.requireNonNull(author, \"author\");", " return this;", " }", " public Person build() {", - " return new ImmutablePerson(name);", + " return new ImmutableBug(author);", " }", " }", - "}") - .addSourceLines( - "MyTest.java", - "public class MyTest {", - " public static void main(String[] args) {", - " Person.builder().name(\"name\").build();", - " }", - "}") - .doTest(); + "}"); } - @Test - public void testFail() { - helper().addSourceLines( + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ImmutablesBuilderMissingInitialization.class, getClass()); + } + + private CompilationTestHelper helperWithImmutables() { + return helper().addSourceLines( "Person.java", + "import java.util.Optional;", "import org.immutables.value.Value;", "@Value.Immutable", "public interface Person {", " String name();", - " static ImmutablePerson.Builder builder() {", - " return new ImmutablePerson.Builder();", + " int currentAge();", + " Optional company();", + " static Builder builder() {", + " return new Builder();", " }", + " class Builder extends ImmutablePerson.Builder {}", "}") .addSourceLines( "ImmutablePerson.java", "import java.util.Objects;", + "import java.util.Optional;", "import javax.annotation.Nullable;", "import org.immutables.value.Generated;", "@Generated(from = \"Person\", generator = \"Immutables\")", "final class ImmutablePerson implements Person {", " private final String name;", - " private ImmutablePerson(String name) { this.name = name; }", + " private final int currentAge;", + " private final @Nullable String company;", + " private ImmutablePerson(String name, int currentAge, @Nullable String company) {", + " this.name = name; this.currentAge = currentAge; this.company = company;", + " }", " @Override", " public String name() { return name; }", + " @Override", + " public int currentAge() { return currentAge; }", + " @Override", + " public Optional company() { return Optional.ofNullable(company); }", "", " @Generated(from = \"Person\", generator = \"Immutables\")", - " public static final class Builder {", + " public static class Builder {", " private static final long INIT_BIT_NAME = 0x1L;", - " private long initBits = 0x1L;", + " private static final long INIT_BIT_CURRENT_AGE = 0x2L;", + " private long initBits = 0x3L;", " private @Nullable String name;", + " private @Nullable Integer currentAge;", + " private @Nullable String company;", " public final Builder name(String name) {", " this.name = Objects.requireNonNull(name, \"name\");", " initBits &= ~INIT_BIT_NAME;", " return this;", " }", + " public final Builder currentAge(int currentAge) {", + " this.currentAge = currentAge;", + " initBits &= ~INIT_BIT_CURRENT_AGE;", + " return this;", + " }", + " public final Builder company(String company) {", + " this.company = Objects.requireNonNull(company, \"company\");", + " return this;", + " }", + " public final Builder company(Optional company) {", + " this.company = company.orElse(null);", + " return this;", + " }", + " public final Builder from(Person instance) {", + " name(instance.name());", + " currentAge(instance.currentAge());", + " company(instance.company());", + " return this;", + " }", " public Person build() {", - " return new ImmutablePerson(name);", + " if (initBits != 0) { throw new IllegalStateException(); }", + " return new ImmutablePerson(name, currentAge, company);", " }", " }", - "}") - .addSourceLines( - "MyTest.java", - "public class MyTest {", - " public static void main(String[] args) {", - " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", - " Person.builder().build();", - " }", - "}") - .doTest(); - } - - private CompilationTestHelper helper() { - return CompilationTestHelper.newInstance(ImmutablesBuilderMissingInitialization.class, getClass()); + "}"); } } From 9792eb6714a8c25b4aecf678960e326c115063c6 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Tue, 6 Oct 2020 19:15:09 +0100 Subject: [PATCH 04/19] Reduce method complexity --- ...mmutablesBuilderMissingInitialization.java | 74 +++++++++++-------- ...ablesBuilderMissingInitializationTest.java | 30 ++++++++ 2 files changed, 72 insertions(+), 32 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index e7c576c1a..7f1d3f0fd 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -43,6 +43,7 @@ import com.sun.source.tree.VariableTree; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; @@ -112,59 +113,42 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState private Set checkInitialization( ExpressionTree tree, Set uninitializedInitBits, VisitorState state, ClassSymbol builderClass) { + if (uninitializedInitBits.isEmpty()) { + return ImmutableSet.of(); + } if (tree instanceof MethodInvocationTree) { - MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree; - MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree); - ClassSymbol classSymbol = methodSymbol.enclClass(); - - if (classSymbol == null || !classSymbol.equals(builderClass)) { + MethodSymbol methodSymbol = ASTHelpers.getSymbol((MethodInvocationTree) tree); + if (!Objects.equals(methodSymbol.enclClass(), builderClass)) { + // This method belongs to a class other than the builder, so we are at the end of the chain if (methodSymbol.getSimpleName().contentEquals("builder") && methodSymbol.getParameters().isEmpty()) { // A nullary method named "builder" which returns the Builder type can be assumed to just construct // the builder + // TODO(jwickham): This doesn't hold: + // https://sourcegraph.palantir.build/search?q=%5C+builder%28%29%5C+%7B%5Cn.*%5Cn%5B%5E%7D%5D*%5Cn+repo:/foundry/&patternType=regexp#6 return uninitializedInitBits; } - // If this is any other method, there may be initialization that occurs in the method, and trying to - // track that down will quickly become impossible, so give up here + // Otherwise, we can't rule out that the builder may be partially initialized in the method, so give + // up here return ImmutableSet.of(); } if (methodSymbol.getSimpleName().contentEquals("from")) { - // `from` initializes all fields + // `from` initializes all fields, so we can bail immediately return ImmutableSet.of(); } MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); - Set initializedInitBits = methodTree.getBody().getStatements().stream() - .flatMap(filterByType(ExpressionStatementTree.class)) - .map(ExpressionStatementTree::getExpression) - .flatMap(filterByType(CompoundAssignmentTree.class)) - .filter(assignmentTree -> { - if (assignmentTree.getVariable() instanceof IdentifierTree) { - return ((IdentifierTree) assignmentTree.getVariable()) - .getName() - .contentEquals("initBits"); - } - return false; - }) - .map(CompoundAssignmentTree::getExpression) - .flatMap(filterByType(UnaryTree.class)) - .flatMap(unaryTree -> { - if (unaryTree.getKind().equals(Kind.BITWISE_COMPLEMENT)) { - return Stream.of(unaryTree.getExpression()); - } - return Stream.empty(); - }) - .flatMap(filterByType(IdentifierTree.class)) - .map(identifierTree -> identifierTree.getName().toString()) - .collect(Collectors.toSet()); + if (methodTree == null) { + return ImmutableSet.of(); + } + Set initializedInitBits = initBitsInitializedBy(methodTree); Set remainingInitBits = uninitializedInitBits.stream() .filter(initBit -> !initializedInitBits.contains(initBit)) .collect(Collectors.toSet()); return checkInitialization(ASTHelpers.getReceiver(tree), remainingInitBits, state, builderClass); } else if (tree instanceof NewClassTree) { NewClassTree newClassTree = (NewClassTree) tree; - if (newClassTree.getArguments().isEmpty()) { // The constructor returned the builder (otherwise we would have bailed out in a previous iteration), so // we should have seen all the field initializations @@ -178,6 +162,32 @@ private Set checkInitialization( return ImmutableSet.of(); } + private Set initBitsInitializedBy(MethodTree methodTree) { + return methodTree.getBody().getStatements().stream() + .flatMap(filterByType(ExpressionStatementTree.class)) + .map(ExpressionStatementTree::getExpression) + .flatMap(filterByType(CompoundAssignmentTree.class)) + .filter(assignmentTree -> { + if (assignmentTree.getVariable() instanceof IdentifierTree) { + return ((IdentifierTree) assignmentTree.getVariable()) + .getName() + .contentEquals("initBits"); + } + return false; + }) + .map(CompoundAssignmentTree::getExpression) + .flatMap(filterByType(UnaryTree.class)) + .flatMap(unaryTree -> { + if (unaryTree.getKind().equals(Kind.BITWISE_COMPLEMENT)) { + return Stream.of(unaryTree.getExpression()); + } + return Stream.empty(); + }) + .flatMap(filterByType(IdentifierTree.class)) + .map(identifierTree -> identifierTree.getName().toString()) + .collect(Collectors.toSet()); + } + private Function> filterByType(Class clazz) { return value -> { if (clazz.isInstance(value)) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 91a53a05c..08799d721 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -60,6 +60,36 @@ public void testPassesWithFrom() { .doTest(); } + @Test + public void testPassesWhenBuilderAssignedToVariable() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " Person.Builder Builder = Person.builder();", + " builder.build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWhenBuilderFromLocalMethod() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " getBuilder().build();", + " }", + " private static Person.Builder getBuilder() {", + " return new Person.Builder();", + " }", + "}") + .doTest(); + } + @Test public void testPassesWithAllFieldsPopulated_usingExtendedConstructor() { helperWithImmutables() From a26f562325748b50c3c122d01ec0e8def6344386 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Tue, 6 Oct 2020 19:45:39 +0100 Subject: [PATCH 05/19] Check that method does nothing but construct builder --- ...mmutablesBuilderMissingInitialization.java | 35 +++++--- ...ablesBuilderMissingInitializationTest.java | 82 +++++++++++-------- 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 7f1d3f0fd..53fb5357c 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -38,6 +38,7 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ReturnTree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.UnaryTree; import com.sun.source.tree.VariableTree; @@ -118,18 +119,17 @@ private Set checkInitialization( } if (tree instanceof MethodInvocationTree) { MethodSymbol methodSymbol = ASTHelpers.getSymbol((MethodInvocationTree) tree); + MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); + if (methodTree == null) { + return ImmutableSet.of(); + } if (!Objects.equals(methodSymbol.enclClass(), builderClass)) { // This method belongs to a class other than the builder, so we are at the end of the chain - if (methodSymbol.getSimpleName().contentEquals("builder") - && methodSymbol.getParameters().isEmpty()) { - // A nullary method named "builder" which returns the Builder type can be assumed to just construct - // the builder - // TODO(jwickham): This doesn't hold: - // https://sourcegraph.palantir.build/search?q=%5C+builder%28%29%5C+%7B%5Cn.*%5Cn%5B%5E%7D%5D*%5Cn+repo:/foundry/&patternType=regexp#6 + // If the only thing this method does is construct and return the builder, we continue and expect + // everything to have been initialized at this point, but if it does anything more complex then give up + if (methodJustCallsConstructor(methodTree)) { return uninitializedInitBits; } - // Otherwise, we can't rule out that the builder may be partially initialized in the method, so give - // up here return ImmutableSet.of(); } if (methodSymbol.getSimpleName().contentEquals("from")) { @@ -137,11 +137,6 @@ private Set checkInitialization( return ImmutableSet.of(); } - MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); - if (methodTree == null) { - return ImmutableSet.of(); - } - Set initializedInitBits = initBitsInitializedBy(methodTree); Set remainingInitBits = uninitializedInitBits.stream() .filter(initBit -> !initializedInitBits.contains(initBit)) @@ -188,6 +183,20 @@ private Set initBitsInitializedBy(MethodTree methodTree) { .collect(Collectors.toSet()); } + /** + * Make sure a method only does one thing, which is to call a constructor with no arguments and return the result. + */ + private boolean methodJustCallsConstructor(MethodTree methodTree) { + if (methodTree.getBody().getStatements().size() != 1) { + return false; + } + return methodTree.getBody().getStatements().stream() + .flatMap(filterByType(ReturnTree.class)) + .map(ReturnTree::getExpression) + .flatMap(filterByType(NewClassTree.class)) + .anyMatch(newClassTree -> newClassTree.getArguments().isEmpty()); + } + private Function> filterByType(Class clazz) { return value -> { if (clazz.isInstance(value)) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 08799d721..2c82be396 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -21,39 +21,39 @@ public class ImmutablesBuilderMissingInitializationTest { @Test - public void testPassesWithAllFieldsPopulated() { + public void testPassesWithAllFieldsPopulated_usingBuilderMethod() { helperWithImmutables() .addSourceLines( "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " Person.builder().name(\"name\").currentAge(70).company(\"palantir\").build();", + " Person.builder().name(\"name\").age(0).company(\"palantir\").build();", " }", "}") .doTest(); } @Test - public void testPassesWithOptionalFieldOmitted() { + public void testPassesWithOptionalFieldOmitted_usingBuilderMethod() { helperWithImmutables() .addSourceLines( "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " Person.builder().name(\"name\").currentAge(60).build();", + " Person.builder().name(\"name\").age(10).build();", " }", "}") .doTest(); } @Test - public void testPassesWithFrom() { + public void testPassesWithFrom_usingBuilderMethod() { helperWithImmutables() .addSourceLines( "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " Person p1 = Person.builder().name(\"name\").currentAge(50).build();", + " Person p1 = Person.builder().name(\"name\").age(20).build();", " Person.builder().from(p1).build();", " }", "}") @@ -61,13 +61,13 @@ public void testPassesWithFrom() { } @Test - public void testPassesWhenBuilderAssignedToVariable() { + public void testPassesWhenBuilderAssignedToVariable_usingBuilderMethod() { helperWithImmutables() .addSourceLines( "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " Person.Builder Builder = Person.builder();", + " Person.Builder builder = Person.builder();", " builder.build();", " }", "}") @@ -75,16 +75,16 @@ public void testPassesWhenBuilderAssignedToVariable() { } @Test - public void testPassesWhenBuilderFromLocalMethod() { + public void testPassesWithUninitializedFields_whenBuilderFromMethodThatSetsSomeFields() { helperWithImmutables() .addSourceLines( "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " getBuilder().build();", + " builder().build();", " }", - " private static Person.Builder getBuilder() {", - " return new Person.Builder();", + " private static ImmutablePerson.Builder builder() {", + " return new Person.Builder().name(\"name\").age(30);", " }", "}") .doTest(); @@ -97,7 +97,7 @@ public void testPassesWithAllFieldsPopulated_usingExtendedConstructor() { "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " new Person.Builder().name(\"name\").currentAge(40).company(\"palantir\").build();", + " new Person.Builder().name(\"name\").age(40).company(\"palantir\").build();", " }", "}") .doTest(); @@ -110,7 +110,7 @@ public void testPassesWithAllFieldsPopulated_usingImmutableConstructor() { "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " new ImmutablePerson.Builder().name(\"name\").currentAge(30).build();", + " new ImmutablePerson.Builder().name(\"name\").age(50).build();", " }", "}") .doTest(); @@ -124,10 +124,10 @@ public void testFailsWithOneMandatoryFieldOmitted() { "public class MyTest {", " public static void main(String[] args) {", " Person.builder()", - " .currentAge(20)", + " .age(60)", " .company(\"palantir\")", - " // BUG: Diagnostic contains: Some builder fields have not been initialized: ", - " // name", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: " + + "name", " .build();", " }", "}") @@ -141,8 +141,7 @@ public void testFailsWithAllFieldsOmitted() { "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, ", - " // currentAge", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", " Person.builder().build();", " }", "}") @@ -157,7 +156,7 @@ public void testFailsWithOneFieldOmitted_usingExtendedConstructor() { "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", - " new Person.Builder().currentAge(10).build();", + " new Person.Builder().age(70).build();", " }", "}") .doTest(); @@ -171,7 +170,24 @@ public void testFailsWithOneFieldOmitted_usingImmutableConstructor() { "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", - " new ImmutablePerson.Builder().currentAge(0).build();", + " new ImmutablePerson.Builder().age(80).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithOneFieldOmitted_fromLocalMethod() { + helperWithImmutables() + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", + " getBuilder().build();", + " }", + " private static Person.Builder getBuilder() {", + " return new Person.Builder();", " }", "}") .doTest(); @@ -231,7 +247,7 @@ private CompilationTestHelper helperWithImmutables() { "@Value.Immutable", "public interface Person {", " String name();", - " int currentAge();", + " int age();", " Optional company();", " static Builder builder() {", " return new Builder();", @@ -247,34 +263,34 @@ private CompilationTestHelper helperWithImmutables() { "@Generated(from = \"Person\", generator = \"Immutables\")", "final class ImmutablePerson implements Person {", " private final String name;", - " private final int currentAge;", + " private final int age;", " private final @Nullable String company;", - " private ImmutablePerson(String name, int currentAge, @Nullable String company) {", - " this.name = name; this.currentAge = currentAge; this.company = company;", + " private ImmutablePerson(String name, int age, @Nullable String company) {", + " this.name = name; this.age = age; this.company = company;", " }", " @Override", " public String name() { return name; }", " @Override", - " public int currentAge() { return currentAge; }", + " public int age() { return age; }", " @Override", " public Optional company() { return Optional.ofNullable(company); }", "", " @Generated(from = \"Person\", generator = \"Immutables\")", " public static class Builder {", " private static final long INIT_BIT_NAME = 0x1L;", - " private static final long INIT_BIT_CURRENT_AGE = 0x2L;", + " private static final long INIT_BIT_AGE = 0x2L;", " private long initBits = 0x3L;", " private @Nullable String name;", - " private @Nullable Integer currentAge;", + " private @Nullable Integer age;", " private @Nullable String company;", " public final Builder name(String name) {", " this.name = Objects.requireNonNull(name, \"name\");", " initBits &= ~INIT_BIT_NAME;", " return this;", " }", - " public final Builder currentAge(int currentAge) {", - " this.currentAge = currentAge;", - " initBits &= ~INIT_BIT_CURRENT_AGE;", + " public final Builder age(int age) {", + " this.age = age;", + " initBits &= ~INIT_BIT_AGE;", " return this;", " }", " public final Builder company(String company) {", @@ -287,13 +303,13 @@ private CompilationTestHelper helperWithImmutables() { " }", " public final Builder from(Person instance) {", " name(instance.name());", - " currentAge(instance.currentAge());", + " age(instance.age());", " company(instance.company());", " return this;", " }", " public Person build() {", " if (initBits != 0) { throw new IllegalStateException(); }", - " return new ImmutablePerson(name, currentAge, company);", + " return new ImmutablePerson(name, age, company);", " }", " }", "}"); From 68d68c5e1b882b55182ab1462361299709a58401 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Wed, 7 Oct 2020 18:04:59 +0100 Subject: [PATCH 06/19] Clean up code --- ...mmutablesBuilderMissingInitialization.java | 61 +++++++++++----- ...ablesBuilderMissingInitializationTest.java | 71 +++++++++++++++++-- 2 files changed, 107 insertions(+), 25 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 53fb5357c..1df10b46f 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -18,7 +18,6 @@ import com.google.auto.service.AutoService; import com.google.common.base.CaseFormat; -import com.google.common.base.Converter; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; @@ -44,6 +43,7 @@ import com.sun.source.tree.VariableTree; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.HashSet; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -52,6 +52,18 @@ import java.util.stream.Stream; import org.immutables.value.Generated; +/** + * Checks that all required fields in an immutables.org generated builder have been populated. + * + * To make it decidable, it is limited to builders that are constructed by calling {@code new ImmutableType.Builder()}, + * {@code new Type.Builder()} (where Type.Builder extends ImmutableType.Builder), or a method that only calls one of + * those constructors and returns the result, and are never stored into a variable. Builders that do not meet these + * conditions are assumed to populate all fields, and are ignored. + * + * Mandatory fields are determined by inspecting the generated builder source to find the initBits that are updated by + * each method, to find any that do not get set. If Immutables changes the way that they check for required fields, this + * check will stop working (but the check will probably pass). + */ @AutoService(BugChecker.class) @BugPattern( name = "ImmutablesBuilderMissingInitialization", @@ -62,16 +74,13 @@ public final class ImmutablesBuilderMissingInitialization extends BugChecker implements MethodInvocationTreeMatcher { private static final String FIELD_INIT_BITS_PREFIX = "INIT_BIT_"; - private static final Joiner COMMA_JOINER = Joiner.on(", "); - private static final Converter UPPER_TO_CAMEL_CASE_CONVERTER = - CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL); - private static final Matcher BUILDER_METHOD_MATCHER = + private static final Matcher builderMethodMatcher = Matchers.instanceMethod().anyClass().named("build").withParameters(); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { // Check that the method is a builder's build() method - if (!BUILDER_METHOD_MATCHER.matches(tree, state)) { + if (!builderMethodMatcher.matches(tree, state)) { return Description.NO_MATCH; } ClassSymbol classSymbol = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); @@ -97,21 +106,27 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .map(variableTree -> variableTree.getName().toString()) .collect(Collectors.toSet()); + // Make sure all the init bits get initialized by the preceding method chain Set missingInitBits = checkInitialization(ASTHelpers.getReceiver(tree), initBits, state, classSymbol); + if (missingInitBits.isEmpty()) { return Description.NO_MATCH; } - return buildDescription(tree) .setMessage(String.format( "Some builder fields have not been initialized: %s", - COMMA_JOINER.join(missingInitBits.stream() - .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) - .map(UPPER_TO_CAMEL_CASE_CONVERTER::convert) - .iterator()))) + Joiner.on(", ") + .join(missingInitBits.stream() + .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) + .map(CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL)::convert) + .iterator()))) .build(); } + /** + * Recursively check that all of the uninitializedInitBits are initialized in the expression, returning any that + * are not. + */ private Set checkInitialization( ExpressionTree tree, Set uninitializedInitBits, VisitorState state, ClassSymbol builderClass) { if (uninitializedInitBits.isEmpty()) { @@ -125,7 +140,7 @@ private Set checkInitialization( } if (!Objects.equals(methodSymbol.enclClass(), builderClass)) { // This method belongs to a class other than the builder, so we are at the end of the chain - // If the only thing this method does is construct and return the builder, we continue and expect + // If the only thing this method does is construct and return the builder, we continue and require // everything to have been initialized at this point, but if it does anything more complex then give up if (methodJustCallsConstructor(methodTree)) { return uninitializedInitBits; @@ -133,14 +148,13 @@ private Set checkInitialization( return ImmutableSet.of(); } if (methodSymbol.getSimpleName().contentEquals("from")) { - // `from` initializes all fields, so we can bail immediately + // `from` initializes all fields, so we don't need to continue return ImmutableSet.of(); } - Set initializedInitBits = initBitsInitializedBy(methodTree); - Set remainingInitBits = uninitializedInitBits.stream() - .filter(initBit -> !initializedInitBits.contains(initBit)) - .collect(Collectors.toSet()); + Set remainingInitBits = new HashSet<>(uninitializedInitBits); + remainingInitBits.removeAll(initBitsInitializedBy(methodTree)); + return checkInitialization(ASTHelpers.getReceiver(tree), remainingInitBits, state, builderClass); } else if (tree instanceof NewClassTree) { NewClassTree newClassTree = (NewClassTree) tree; @@ -153,10 +167,15 @@ private Set checkInitialization( return ImmutableSet.of(); } - // If the chain started with something other than a simple method call to create the builder, give up + // If the chain started with something other than a method call or constructor to create the builder, give up return ImmutableSet.of(); } + /** + * Extracts the init bits that are marked as initialized in this method. + * + * Assumes that they are used as {@code initBits &= ~INIT_BIT_VAR}. + */ private Set initBitsInitializedBy(MethodTree methodTree) { return methodTree.getBody().getStatements().stream() .flatMap(filterByType(ExpressionStatementTree.class)) @@ -185,6 +204,9 @@ private Set initBitsInitializedBy(MethodTree methodTree) { /** * Make sure a method only does one thing, which is to call a constructor with no arguments and return the result. + * + * We don't check which class's constructor because it must return something compatible with Builder for us to have + * got this far, and that's all we care about. */ private boolean methodJustCallsConstructor(MethodTree methodTree) { if (methodTree.getBody().getStatements().size() != 1) { @@ -197,6 +219,9 @@ private boolean methodJustCallsConstructor(MethodTree methodTree) { .anyMatch(newClassTree -> newClassTree.getArguments().isEmpty()); } + /** + * Returns a function for use in Stream.flatMap that filters by type, and casts to that type. + */ private Function> filterByType(Class clazz) { return value -> { if (clazz.isInstance(value)) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 2c82be396..5afb016f2 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -202,12 +202,9 @@ public void testSucceedsWithNoRequiredFields() { "@Value.Immutable", "public interface Bug {", " Optional author();", - " static Builder builder() {", - " return new Builder();", - " }", "}") .addSourceLines( - "ImmutablePerson.java", + "ImmutableBug.java", "import java.util.Objects;", "import java.util.Optional;", "import javax.annotation.Nullable;", @@ -215,7 +212,7 @@ public void testSucceedsWithNoRequiredFields() { "@Generated(from = \"Bug\", generator = \"Immutables\")", "final class ImmutableBug implements Bug {", " private final @Nullable String author;", - " private ImmutablePerson(@Nullable String author) {", + " private ImmutableBug(@Nullable String author) {", " this.author = author;", " }", " @Override", @@ -228,11 +225,71 @@ public void testSucceedsWithNoRequiredFields() { " this.author = Objects.requireNonNull(author, \"author\");", " return this;", " }", - " public Person build() {", + " public Bug build() {", " return new ImmutableBug(author);", " }", " }", - "}"); + "}") + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " new ImmutableBug.Builder().build();", + " }", + "}") + .doTest(); + } + + @Test + public void testComputesMissingFieldNamesCorrectly() { + helper().addSourceLines( + "Company.java", + "import java.util.Optional;", + "import org.immutables.value.Value;", + "@Value.Immutable", + "public interface Company {", + " int employeeCount();", + "}") + .addSourceLines( + "ImmutableCompany.java", + "import java.util.Objects;", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "import org.immutables.value.Generated;", + "@Generated(from = \"Company\", generator = \"Immutables\")", + "final class ImmutableCompany implements Company {", + " private final int employeeCount;", + " private ImmutableCompany(int employeeCount) {", + " this.employeeCount = employeeCount;", + " }", + " @Override", + " public int employeeCount() { return employeeCount; }", + "", + " @Generated(from = \"Company\", generator = \"Immutables\")", + " public static class Builder {", + " private static final long INIT_BIT_EMPLOYEE_COUNT = 0x1L;", + " private long initBits = 0x1L;", + " private @Nullable Integer employeeCount;", + " public final Builder employeeCount(int employeeCount) {", + " this.employeeCount = employeeCount;", + " initBits &= ~INIT_BIT_EMPLOYEE_COUNT;", + " return this;", + " }", + " public Company build() {", + " return new ImmutableCompany(employeeCount);", + " }", + " }", + "}") + .addSourceLines( + "MyTest.java", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: " + + "employeeCount", + " new ImmutableCompany.Builder().build();", + " }", + "}") + .doTest(); } private CompilationTestHelper helper() { From 3da67e651d714069658ed7a68690d137900e376d Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 15:42:22 +0100 Subject: [PATCH 07/19] Switch to using method names to detect initialized fields --- .gitignore | 1 + ...mmutablesBuilderMissingInitialization.java | 205 ++++++----- ...ablesBuilderMissingInitializationTest.java | 335 +++++++----------- 3 files changed, 251 insertions(+), 290 deletions(-) diff --git a/.gitignore b/.gitignore index 3dc6b5ccd..ff10e03fd 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ build out/ docs/node_modules/ generated_src +generated_testSrc diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 1df10b46f..48a888d0d 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -20,6 +20,7 @@ import com.google.common.base.CaseFormat; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; import com.google.errorprone.VisitorState; @@ -29,27 +30,20 @@ import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; import com.google.errorprone.util.ASTHelpers; -import com.sun.source.tree.ClassTree; -import com.sun.source.tree.CompoundAssignmentTree; -import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ReturnTree; -import com.sun.source.tree.Tree.Kind; -import com.sun.source.tree.UnaryTree; -import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; -import java.util.HashSet; +import com.sun.tools.javac.code.Type; import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.immutables.value.Generated; /** @@ -74,8 +68,10 @@ public final class ImmutablesBuilderMissingInitialization extends BugChecker implements MethodInvocationTreeMatcher { private static final String FIELD_INIT_BITS_PREFIX = "INIT_BIT_"; - private static final Matcher builderMethodMatcher = - Matchers.instanceMethod().anyClass().named("build").withParameters(); + private static final Matcher builderMethodMatcher = Matchers.instanceMethod() + .onClass(ImmutablesBuilderMissingInitialization::extendsImmutablesGeneratedClass) + .named("build") + .withParameters(); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -83,85 +79,94 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (!builderMethodMatcher.matches(tree, state)) { return Description.NO_MATCH; } - ClassSymbol classSymbol = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); - if (Optional.ofNullable(classSymbol.getAnnotation(Generated.class)) - .map(annotation -> !annotation.generator().equals("Immutables")) - .orElse(true)) { + ClassSymbol builderClass = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); + ClassSymbol immutableClass = ASTHelpers.enclosingClass(builderClass); + if (immutableClass == null) { return Description.NO_MATCH; } - - // Find all of the initBits in the generated builder, to determine the mandatory fields - ClassTree classTree = ASTHelpers.findClass(classSymbol, state); - if (classTree == null) { + Optional interfaceClass = immutableClass.getInterfaces().stream() + .map(type -> type.tsym) + .findAny() + .flatMap(filterByType(ClassSymbol.class)); + if (!interfaceClass.isPresent()) { return Description.NO_MATCH; } - Set initBits = classTree.getMembers().stream() - .flatMap(memberTree -> { - if (memberTree instanceof VariableTree) { - return Stream.of((VariableTree) memberTree); - } - return Stream.empty(); - }) - .filter(variableTree -> variableTree.getName().toString().startsWith(FIELD_INIT_BITS_PREFIX)) - .map(variableTree -> variableTree.getName().toString()) + + // Mandatory fields have a private static final constant in the generated builder named INIT_BIT_varname, where + // varname is the UPPER_UNDERSCORE version of the variable name. Find these fields to get the mandatory fields. + Set requiredFields = Streams.stream(builderClass.members().getSymbols()) + .filter(Symbol::isStatic) + .filter(symbol -> symbol.getKind().isField()) + .filter(symbol -> symbol.getSimpleName().toString().startsWith(FIELD_INIT_BITS_PREFIX)) + .map(Symbol::toString) + .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) + .map(CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL)::convert) .collect(Collectors.toSet()); - // Make sure all the init bits get initialized by the preceding method chain - Set missingInitBits = checkInitialization(ASTHelpers.getReceiver(tree), initBits, state, classSymbol); + // Run the check + Set uninitializedFields = checkInitialization( + ASTHelpers.getReceiver(tree), + requiredFields, + state, + builderClass, + immutableClass, + interfaceClass.get()); - if (missingInitBits.isEmpty()) { + if (uninitializedFields.isEmpty()) { return Description.NO_MATCH; } return buildDescription(tree) .setMessage(String.format( "Some builder fields have not been initialized: %s", - Joiner.on(", ") - .join(missingInitBits.stream() - .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) - .map(CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL)::convert) - .iterator()))) + Joiner.on(", ").join(uninitializedFields))) .build(); } /** - * Recursively check that all of the uninitializedInitBits are initialized in the expression, returning any that + * Recursively check that all of the uninitializedFields are initialized in the expression, returning any that * are not. */ private Set checkInitialization( - ExpressionTree tree, Set uninitializedInitBits, VisitorState state, ClassSymbol builderClass) { - if (uninitializedInitBits.isEmpty()) { + ExpressionTree tree, + Set uninitializedFields, + VisitorState state, + ClassSymbol builderClass, + ClassSymbol immutableClass, + ClassSymbol interfaceClass) { + if (uninitializedFields.isEmpty()) { return ImmutableSet.of(); } if (tree instanceof MethodInvocationTree) { MethodSymbol methodSymbol = ASTHelpers.getSymbol((MethodInvocationTree) tree); - MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); - if (methodTree == null) { - return ImmutableSet.of(); - } if (!Objects.equals(methodSymbol.enclClass(), builderClass)) { // This method belongs to a class other than the builder, so we are at the end of the chain // If the only thing this method does is construct and return the builder, we continue and require // everything to have been initialized at this point, but if it does anything more complex then give up - if (methodJustCallsConstructor(methodTree)) { - return uninitializedInitBits; + if (methodJustConstructsBuilder(methodSymbol, state, immutableClass, interfaceClass)) { + return uninitializedFields; + } else { + return ImmutableSet.of(); } - return ImmutableSet.of(); } if (methodSymbol.getSimpleName().contentEquals("from")) { // `from` initializes all fields, so we don't need to continue return ImmutableSet.of(); } - Set remainingInitBits = new HashSet<>(uninitializedInitBits); - remainingInitBits.removeAll(initBitsInitializedBy(methodTree)); - - return checkInitialization(ASTHelpers.getReceiver(tree), remainingInitBits, state, builderClass); + return checkInitialization( + ASTHelpers.getReceiver(tree), + removeFieldsPotentiallyInitializedBy( + uninitializedFields, methodSymbol.getSimpleName().toString()), + state, + builderClass, + immutableClass, + interfaceClass); } else if (tree instanceof NewClassTree) { NewClassTree newClassTree = (NewClassTree) tree; if (newClassTree.getArguments().isEmpty()) { // The constructor returned the builder (otherwise we would have bailed out in a previous iteration), so // we should have seen all the field initializations - return uninitializedInitBits; + return uninitializedFields; } // If the constructor takes arguments, it's doing something funky return ImmutableSet.of(); @@ -172,33 +177,13 @@ private Set checkInitialization( } /** - * Extracts the init bits that are marked as initialized in this method. - * - * Assumes that they are used as {@code initBits &= ~INIT_BIT_VAR}. + * Takes a set of uninitialized fields, and returns a set containing the fields that cannot have been initialized by + * the method methodName. */ - private Set initBitsInitializedBy(MethodTree methodTree) { - return methodTree.getBody().getStatements().stream() - .flatMap(filterByType(ExpressionStatementTree.class)) - .map(ExpressionStatementTree::getExpression) - .flatMap(filterByType(CompoundAssignmentTree.class)) - .filter(assignmentTree -> { - if (assignmentTree.getVariable() instanceof IdentifierTree) { - return ((IdentifierTree) assignmentTree.getVariable()) - .getName() - .contentEquals("initBits"); - } - return false; - }) - .map(CompoundAssignmentTree::getExpression) - .flatMap(filterByType(UnaryTree.class)) - .flatMap(unaryTree -> { - if (unaryTree.getKind().equals(Kind.BITWISE_COMPLEMENT)) { - return Stream.of(unaryTree.getExpression()); - } - return Stream.empty(); - }) - .flatMap(filterByType(IdentifierTree.class)) - .map(identifierTree -> identifierTree.getName().toString()) + private Set removeFieldsPotentiallyInitializedBy(Set uninitializedFields, String methodName) { + String methodNameLowerCase = methodName.toLowerCase(); + return uninitializedFields.stream() + .filter(field -> !field.toLowerCase().endsWith(methodNameLowerCase)) .collect(Collectors.toSet()); } @@ -208,26 +193,68 @@ private Set initBitsInitializedBy(MethodTree methodTree) { * We don't check which class's constructor because it must return something compatible with Builder for us to have * got this far, and that's all we care about. */ - private boolean methodJustCallsConstructor(MethodTree methodTree) { - if (methodTree.getBody().getStatements().size() != 1) { - return false; + private boolean methodJustConstructsBuilder( + MethodSymbol methodSymbol, VisitorState state, ClassSymbol immutableClass, ClassSymbol interfaceClass) { + MethodTree methodTree = ASTHelpers.findMethod(methodSymbol, state); + if (methodTree != null) { + // Check that the method just contains one statement, which is of the form `return new Something();` or + // `return ImmutableType.builder();` + if (methodTree.getBody().getStatements().size() != 1) { + return false; + } + return methodTree.getBody().getStatements().stream() + .findAny() + .flatMap(filterByType(ReturnTree.class)) + .map(ReturnTree::getExpression) + .map(expressionTree -> { + if (expressionTree instanceof NewClassTree) { + // To have got here, the return type must be compatible with ImmutableType, so we don't need + // to check the class being constructed + return ((NewClassTree) expressionTree) + .getArguments() + .isEmpty(); + } else if (expressionTree instanceof MethodInvocationTree) { + return Optional.ofNullable(ASTHelpers.getSymbol(expressionTree)) + .flatMap(filterByType(MethodSymbol.class)) + .filter(symbol -> symbol.getParameters().isEmpty()) + .filter(symbol -> symbol.getSimpleName().contentEquals("builder")) + .map(Symbol::enclClass) + .flatMap(filterByType(ClassSymbol.class)) + .filter(symbol -> symbol.equals(immutableClass)) + .isPresent(); + } else { + return false; + } + }) + .orElse(false); + } + // The method that was called is in a different compilation unit, so we can't access the source. + // If the method is .builder() and it's on a trusted class (the immutable implementation or the interface that + // it was generated from), assume that it just constructs the builder and nothing else. + return ((Objects.equals(methodSymbol.enclClass(), immutableClass) + || Objects.equals(methodSymbol.enclClass(), interfaceClass)) + && methodSymbol.getSimpleName().contentEquals("builder") + && methodSymbol.getParameters().isEmpty()); + } + + private static boolean extendsImmutablesGeneratedClass(Type type, VisitorState state) { + if (type.tsym instanceof ClassSymbol) { + return Optional.ofNullable(type.tsym.getAnnotation(Generated.class)) + .map(generated -> generated.generator().equals("Immutables")) + .orElseGet(() -> extendsImmutablesGeneratedClass(((ClassSymbol) type.tsym).getSuperclass(), state)); } - return methodTree.getBody().getStatements().stream() - .flatMap(filterByType(ReturnTree.class)) - .map(ReturnTree::getExpression) - .flatMap(filterByType(NewClassTree.class)) - .anyMatch(newClassTree -> newClassTree.getArguments().isEmpty()); + return false; } /** - * Returns a function for use in Stream.flatMap that filters by type, and casts to that type. + * Returns a function for use in Optional.flatMap that filters by type, and casts to that type. */ - private Function> filterByType(Class clazz) { + private Function> filterByType(Class clazz) { return value -> { if (clazz.isInstance(value)) { - return Stream.of(clazz.cast(value)); + return Optional.of(clazz.cast(value)); } - return Stream.empty(); + return Optional.empty(); }; } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 5afb016f2..dd673763d 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -16,58 +16,62 @@ package com.palantir.baseline.errorprone; import com.google.errorprone.CompilationTestHelper; +import java.util.List; +import java.util.Optional; +import javax.annotation.Nullable; +import org.immutables.value.Value; +import org.immutables.value.Value.Style.ImplementationVisibility; import org.junit.Test; public class ImmutablesBuilderMissingInitializationTest { - @Test - public void testPassesWithAllFieldsPopulated_usingBuilderMethod() { - helperWithImmutables() - .addSourceLines( + public void testPassesWithAllFieldsPopulated() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", - " Person.builder().name(\"name\").age(0).company(\"palantir\").build();", + " new Person.Builder().name(\"name\").age(100).partner(\"partner\").build();", " }", "}") .doTest(); } @Test - public void testPassesWithOptionalFieldOmitted_usingBuilderMethod() { - helperWithImmutables() - .addSourceLines( + public void testPassesWithOptionalFieldOmitted() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", - " Person.builder().name(\"name\").age(10).build();", + " new Person.Builder().name(\"name\").age(100).build();", " }", "}") .doTest(); } @Test - public void testPassesWithFrom_usingBuilderMethod() { - helperWithImmutables() - .addSourceLines( + public void testPassesWithFrom() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", - " Person p1 = Person.builder().name(\"name\").age(20).build();", - " Person.builder().from(p1).build();", + " Person p1 = new Person.Builder().name(\"name\").age(100).build();", + " new Person.Builder().from(p1).build();", " }", "}") .doTest(); } @Test - public void testPassesWhenBuilderAssignedToVariable_usingBuilderMethod() { - helperWithImmutables() - .addSourceLines( + public void testPassesWhenBuilderAssignedToVariable() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", - " Person.Builder builder = Person.builder();", + " Person.Builder builder = new Person.Builder();", " builder.build();", " }", "}") @@ -76,28 +80,42 @@ public void testPassesWhenBuilderAssignedToVariable_usingBuilderMethod() { @Test public void testPassesWithUninitializedFields_whenBuilderFromMethodThatSetsSomeFields() { - helperWithImmutables() - .addSourceLines( + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablePerson;", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", " builder().build();", " }", " private static ImmutablePerson.Builder builder() {", - " return new Person.Builder().name(\"name\").age(30);", + " return new Person.Builder().name(\"name\").age(100);", " }", "}") .doTest(); } @Test - public void testPassesWithAllFieldsPopulated_usingExtendedConstructor() { - helperWithImmutables() - .addSourceLines( + public void testPassesWithAllFieldsPopulated_usingInterfaceBuilderMethod() { + helper().addSourceLines( + "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + "public class MyTest {", + " public static void main(String[] args) {", + " Person.builder().name(\"name\").age(100).partner(\"partner\").build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWithAllFieldsPopulated_usingImmutableBuilderMethod() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutableNotOvershadowedType;", "public class MyTest {", " public static void main(String[] args) {", - " new Person.Builder().name(\"name\").age(40).company(\"palantir\").build();", + " ImmutableNotOvershadowedType.builder().name(\"name\").build();", " }", "}") .doTest(); @@ -105,12 +123,25 @@ public void testPassesWithAllFieldsPopulated_usingExtendedConstructor() { @Test public void testPassesWithAllFieldsPopulated_usingImmutableConstructor() { - helperWithImmutables() - .addSourceLines( + helper().addSourceLines( + "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablePerson;", + "public class MyTest {", + " public static void main(String[] args) {", + " new ImmutablePerson.Builder().name(\"name\").age(100).build();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWithNoRequiredFields() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutableTypeWithNoMandatoryFields;", "public class MyTest {", " public static void main(String[] args) {", - " new ImmutablePerson.Builder().name(\"name\").age(50).build();", + " ImmutableTypeWithNoMandatoryFields.builder().build();", " }", "}") .doTest(); @@ -118,14 +149,14 @@ public void testPassesWithAllFieldsPopulated_usingImmutableConstructor() { @Test public void testFailsWithOneMandatoryFieldOmitted() { - helperWithImmutables() - .addSourceLines( + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", " Person.builder()", - " .age(60)", - " .company(\"palantir\")", + " .age(100)", + " .partner(\"partner\")", " // BUG: Diagnostic contains: Some builder fields have not been initialized: " + "name", " .build();", @@ -136,105 +167,81 @@ public void testFailsWithOneMandatoryFieldOmitted() { @Test public void testFailsWithAllFieldsOmitted() { - helperWithImmutables() - .addSourceLines( + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", - " Person.builder().build();", + " new Person.Builder().build();", " }", "}") .doTest(); } @Test - public void testFailsWithOneFieldOmitted_usingExtendedConstructor() { - helperWithImmutables() - .addSourceLines( + public void testFailsWithOneFieldOmitted_usingImmutableConstructor() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablePerson;", "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", - " new Person.Builder().age(70).build();", + " new ImmutablePerson.Builder().age(100).build();", " }", "}") .doTest(); } @Test - public void testFailsWithOneFieldOmitted_usingImmutableConstructor() { - helperWithImmutables() - .addSourceLines( + public void testFailsWithOneFieldOmitted_usingInterfaceBuilderMethod() { + helper().addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", - " new ImmutablePerson.Builder().age(80).build();", + " Person.builder().age(100).build();", " }", "}") .doTest(); } @Test - public void testFailsWithOneFieldOmitted_fromLocalMethod() { - helperWithImmutables() + public void testFailsWithOneFieldOmitted_usingSimulatedBuilderMethod() { + // This simulates using Person.builder() which delegates to ImmutablePerson.builder(), when Person is in the + // same compilation unit + helper().addSourceLines( + "Helper.java", + "import com.palantir.baseline.errorprone.ImmutableNotOvershadowedType;", + "public class Helper {", + " public static ImmutableNotOvershadowedType.Builder builder() {", + " return ImmutableNotOvershadowedType.builder();", + " }", + "}") .addSourceLines( "MyTest.java", "public class MyTest {", " public static void main(String[] args) {", - " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", - " getBuilder().build();", - " }", - " private static Person.Builder getBuilder() {", - " return new Person.Builder();", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", + " Helper.builder().build();", " }", "}") .doTest(); } @Test - public void testSucceedsWithNoRequiredFields() { + public void testFailsWithOneFieldOmitted_fromLocalMethod() { helper().addSourceLines( - "Bug.java", - "import java.util.Optional;", - "import org.immutables.value.Value;", - "@Value.Immutable", - "public interface Bug {", - " Optional author();", - "}") - .addSourceLines( - "ImmutableBug.java", - "import java.util.Objects;", - "import java.util.Optional;", - "import javax.annotation.Nullable;", - "import org.immutables.value.Generated;", - "@Generated(from = \"Bug\", generator = \"Immutables\")", - "final class ImmutableBug implements Bug {", - " private final @Nullable String author;", - " private ImmutableBug(@Nullable String author) {", - " this.author = author;", - " }", - " @Override", - " public Optional author() { return Optional.ofNullable(author); }", - "", - " @Generated(from = \"Bug\", generator = \"Immutables\")", - " public static class Builder {", - " private @Nullable String author;", - " public final Builder author(String author) {", - " this.author = Objects.requireNonNull(author, \"author\");", - " return this;", - " }", - " public Bug build() {", - " return new ImmutableBug(author);", - " }", - " }", - "}") - .addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", "public class MyTest {", " public static void main(String[] args) {", - " new ImmutableBug.Builder().build();", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", + " getBuilder().build();", + " }", + " private static Person.Builder getBuilder() {", + " return new Person.Builder();", " }", "}") .doTest(); @@ -243,50 +250,13 @@ public void testSucceedsWithNoRequiredFields() { @Test public void testComputesMissingFieldNamesCorrectly() { helper().addSourceLines( - "Company.java", - "import java.util.Optional;", - "import org.immutables.value.Value;", - "@Value.Immutable", - "public interface Company {", - " int employeeCount();", - "}") - .addSourceLines( - "ImmutableCompany.java", - "import java.util.Objects;", - "import java.util.Optional;", - "import javax.annotation.Nullable;", - "import org.immutables.value.Generated;", - "@Generated(from = \"Company\", generator = \"Immutables\")", - "final class ImmutableCompany implements Company {", - " private final int employeeCount;", - " private ImmutableCompany(int employeeCount) {", - " this.employeeCount = employeeCount;", - " }", - " @Override", - " public int employeeCount() { return employeeCount; }", - "", - " @Generated(from = \"Company\", generator = \"Immutables\")", - " public static class Builder {", - " private static final long INIT_BIT_EMPLOYEE_COUNT = 0x1L;", - " private long initBits = 0x1L;", - " private @Nullable Integer employeeCount;", - " public final Builder employeeCount(int employeeCount) {", - " this.employeeCount = employeeCount;", - " initBits &= ~INIT_BIT_EMPLOYEE_COUNT;", - " return this;", - " }", - " public Company build() {", - " return new ImmutableCompany(employeeCount);", - " }", - " }", - "}") - .addSourceLines( "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutableTypeWithLongNames;", "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: " - + "employeeCount", - " new ImmutableCompany.Builder().build();", + + "mandatoryFieldWithLongName", + " ImmutableTypeWithLongNames.builder().build();", " }", "}") .doTest(); @@ -296,79 +266,42 @@ private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(ImmutablesBuilderMissingInitialization.class, getClass()); } - private CompilationTestHelper helperWithImmutables() { - return helper().addSourceLines( - "Person.java", - "import java.util.Optional;", - "import org.immutables.value.Value;", - "@Value.Immutable", - "public interface Person {", - " String name();", - " int age();", - " Optional company();", - " static Builder builder() {", - " return new Builder();", - " }", - " class Builder extends ImmutablePerson.Builder {}", - "}") - .addSourceLines( - "ImmutablePerson.java", - "import java.util.Objects;", - "import java.util.Optional;", - "import javax.annotation.Nullable;", - "import org.immutables.value.Generated;", - "@Generated(from = \"Person\", generator = \"Immutables\")", - "final class ImmutablePerson implements Person {", - " private final String name;", - " private final int age;", - " private final @Nullable String company;", - " private ImmutablePerson(String name, int age, @Nullable String company) {", - " this.name = name; this.age = age; this.company = company;", - " }", - " @Override", - " public String name() { return name; }", - " @Override", - " public int age() { return age; }", - " @Override", - " public Optional company() { return Optional.ofNullable(company); }", - "", - " @Generated(from = \"Person\", generator = \"Immutables\")", - " public static class Builder {", - " private static final long INIT_BIT_NAME = 0x1L;", - " private static final long INIT_BIT_AGE = 0x2L;", - " private long initBits = 0x3L;", - " private @Nullable String name;", - " private @Nullable Integer age;", - " private @Nullable String company;", - " public final Builder name(String name) {", - " this.name = Objects.requireNonNull(name, \"name\");", - " initBits &= ~INIT_BIT_NAME;", - " return this;", - " }", - " public final Builder age(int age) {", - " this.age = age;", - " initBits &= ~INIT_BIT_AGE;", - " return this;", - " }", - " public final Builder company(String company) {", - " this.company = Objects.requireNonNull(company, \"company\");", - " return this;", - " }", - " public final Builder company(Optional company) {", - " this.company = company.orElse(null);", - " return this;", - " }", - " public final Builder from(Person instance) {", - " name(instance.name());", - " age(instance.age());", - " company(instance.company());", - " return this;", - " }", - " public Person build() {", - " if (initBits != 0) { throw new IllegalStateException(); }", - " return new ImmutablePerson(name, age, company);", - " }", - " }", - "}"); + @Value.Immutable + @Value.Style(visibility = ImplementationVisibility.PUBLIC, overshadowImplementation = true) + public interface Person { + String name(); + + int age(); + + Optional partner(); + + class Builder extends ImmutablePerson.Builder {} + + static Builder builder() { + return new Builder(); + } + } + + @Value.Immutable + @Value.Style(visibility = ImplementationVisibility.PUBLIC) + public interface NotOvershadowedType { + String name(); + } + + @Value.Immutable + @Value.Style(visibility = ImplementationVisibility.PUBLIC) + public interface TypeWithNoMandatoryFields { + Optional notRequired(); + + List list(); + + @Nullable + String maybeString(); + } + + @Value.Immutable + @Value.Style(visibility = ImplementationVisibility.PUBLIC) + public interface TypeWithLongNames { + String mandatoryFieldWithLongName(); } } From ba6da262690b17ae9dc7ffd39a29b66a34b6b7ca Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 15:58:18 +0100 Subject: [PATCH 08/19] Support custom init formats --- ...mmutablesBuilderMissingInitialization.java | 2 +- ...ablesBuilderMissingInitializationTest.java | 57 +++++++++++++++---- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 48a888d0d..504edf7a3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -183,7 +183,7 @@ private Set checkInitialization( private Set removeFieldsPotentiallyInitializedBy(Set uninitializedFields, String methodName) { String methodNameLowerCase = methodName.toLowerCase(); return uninitializedFields.stream() - .filter(field -> !field.toLowerCase().endsWith(methodNameLowerCase)) + .filter(fieldName -> !methodNameLowerCase.endsWith(fieldName.toLowerCase())) .collect(Collectors.toSet()); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index dd673763d..e71fc3359 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -25,7 +25,7 @@ public class ImmutablesBuilderMissingInitializationTest { @Test - public void testPassesWithAllFieldsPopulated() { + public void testPassesWhenAllFieldsPopulated() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -38,7 +38,7 @@ public void testPassesWithAllFieldsPopulated() { } @Test - public void testPassesWithOptionalFieldOmitted() { + public void testPassesWhenOptionalFieldOmitted() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -96,7 +96,7 @@ public void testPassesWithUninitializedFields_whenBuilderFromMethodThatSetsSomeF } @Test - public void testPassesWithAllFieldsPopulated_usingInterfaceBuilderMethod() { + public void testPassesWhenAllFieldsPopulated_usingInterfaceBuilderMethod() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -109,7 +109,7 @@ public void testPassesWithAllFieldsPopulated_usingInterfaceBuilderMethod() { } @Test - public void testPassesWithAllFieldsPopulated_usingImmutableBuilderMethod() { + public void testPassesWhenAllFieldsPopulated_usingImmutableBuilderMethod() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutableNotOvershadowedType;", @@ -122,7 +122,7 @@ public void testPassesWithAllFieldsPopulated_usingImmutableBuilderMethod() { } @Test - public void testPassesWithAllFieldsPopulated_usingImmutableConstructor() { + public void testPassesWhenAllFieldsPopulated_usingImmutableConstructor() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablePerson;", @@ -135,7 +135,7 @@ public void testPassesWithAllFieldsPopulated_usingImmutableConstructor() { } @Test - public void testPassesWithNoRequiredFields() { + public void testPassesWhenNoRequiredFields() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutableTypeWithNoMandatoryFields;", @@ -148,7 +148,20 @@ public void testPassesWithNoRequiredFields() { } @Test - public void testFailsWithOneMandatoryFieldOmitted() { + public void testPassesWhenAllFieldsPopulated_withCustomInitMethodStyle() { + helper().addSourceLines( + "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomInitMethodStyle;", + "public class MyTest {", + " public static void main(String[] args) {", + " ImmutableTypeWithCustomInitMethodStyle.builder().setValue(\"value\").build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWhenOneMandatoryFieldOmitted() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -166,7 +179,7 @@ public void testFailsWithOneMandatoryFieldOmitted() { } @Test - public void testFailsWithAllFieldsOmitted() { + public void testFailsWhenAllFieldsOmitted() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -180,7 +193,7 @@ public void testFailsWithAllFieldsOmitted() { } @Test - public void testFailsWithOneFieldOmitted_usingImmutableConstructor() { + public void testFailsWhenOneFieldOmitted_usingImmutableConstructor() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablePerson;", @@ -194,7 +207,7 @@ public void testFailsWithOneFieldOmitted_usingImmutableConstructor() { } @Test - public void testFailsWithOneFieldOmitted_usingInterfaceBuilderMethod() { + public void testFailsWhenOneFieldOmitted_usingInterfaceBuilderMethod() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -208,7 +221,7 @@ public void testFailsWithOneFieldOmitted_usingInterfaceBuilderMethod() { } @Test - public void testFailsWithOneFieldOmitted_usingSimulatedBuilderMethod() { + public void testFailsWhenOneFieldOmitted_usingSimulatedBuilderMethod() { // This simulates using Person.builder() which delegates to ImmutablePerson.builder(), when Person is in the // same compilation unit helper().addSourceLines( @@ -231,7 +244,7 @@ public void testFailsWithOneFieldOmitted_usingSimulatedBuilderMethod() { } @Test - public void testFailsWithOneFieldOmitted_fromLocalMethod() { + public void testFailsWhenOneFieldOmitted_fromLocalMethod() { helper().addSourceLines( "MyTest.java", "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", @@ -247,6 +260,20 @@ public void testFailsWithOneFieldOmitted_fromLocalMethod() { .doTest(); } + @Test + public void testFailsWhenAllOneFieldOmitted_withCustomInitMethodStyle() { + helper().addSourceLines( + "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomInitMethodStyle;", + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: value", + " ImmutableTypeWithCustomInitMethodStyle.builder().build();", + " }", + "}") + .doTest(); + } + @Test public void testComputesMissingFieldNamesCorrectly() { helper().addSourceLines( @@ -304,4 +331,10 @@ public interface TypeWithNoMandatoryFields { public interface TypeWithLongNames { String mandatoryFieldWithLongName(); } + + @Value.Immutable + @Value.Style(visibility = ImplementationVisibility.PUBLIC, init = "set*") + public interface TypeWithCustomInitMethodStyle { + String value(); + } } From 6db947c5c589429b3aaa0c74ba65ac43bfc789eb Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 16:53:19 +0100 Subject: [PATCH 09/19] Support custom get format --- ...mmutablesBuilderMissingInitialization.java | 66 ++++++++++++++++--- ...ablesBuilderMissingInitializationTest.java | 18 ++--- 2 files changed, 66 insertions(+), 18 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 504edf7a3..652d9a872 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -39,12 +39,15 @@ import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Type; +import java.util.Arrays; import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.immutables.value.Generated; +import org.immutables.value.Value; /** * Checks that all required fields in an immutables.org generated builder have been populated. @@ -92,6 +95,26 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } + Optional maybeStyle = findImmutablesStyle(interfaceClass.get()); + Set getterPrefixes; + if (maybeStyle.isPresent()) { + if (!maybeStyle.get().init().endsWith("*")) { + // The builder methods don't end with the field name, so this logic won't work + return Description.NO_MATCH; + } + if (Arrays.stream(maybeStyle.get().get()).anyMatch(getter -> !getter.endsWith("*"))) { + // The field names might be cut off and the end as well as the beginning, which we can't handle + return Description.NO_MATCH; + } + getterPrefixes = Arrays.stream(maybeStyle.get().get()) + .map(getter -> getter.replace("*", "")) + .map(CaseFormat.LOWER_CAMEL.converterTo(CaseFormat.UPPER_UNDERSCORE)) + .map(prefix -> prefix + "_") + .collect(Collectors.toSet()); + } else { + getterPrefixes = ImmutableSet.of("GET_"); + } + // Mandatory fields have a private static final constant in the generated builder named INIT_BIT_varname, where // varname is the UPPER_UNDERSCORE version of the variable name. Find these fields to get the mandatory fields. Set requiredFields = Streams.stream(builderClass.members().getSymbols()) @@ -100,7 +123,12 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .filter(symbol -> symbol.getSimpleName().toString().startsWith(FIELD_INIT_BITS_PREFIX)) .map(Symbol::toString) .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) - .map(CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL)::convert) + .map(fieldName -> getterPrefixes.stream() + .filter(fieldName::startsWith) + .findAny() + .map(prefix -> fieldName.replace(prefix, "")) + .orElse(fieldName)) + .map(CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL)) .collect(Collectors.toSet()); // Run the check @@ -237,24 +265,44 @@ private boolean methodJustConstructsBuilder( && methodSymbol.getParameters().isEmpty()); } - private static boolean extendsImmutablesGeneratedClass(Type type, VisitorState state) { - if (type.tsym instanceof ClassSymbol) { - return Optional.ofNullable(type.tsym.getAnnotation(Generated.class)) - .map(generated -> generated.generator().equals("Immutables")) - .orElseGet(() -> extendsImmutablesGeneratedClass(((ClassSymbol) type.tsym).getSuperclass(), state)); + private Optional findImmutablesStyle(ClassSymbol symbol) { + Optional directAnnotation = Optional.ofNullable(symbol.getAnnotation(Value.Style.class)); + if (directAnnotation.isPresent()) { + return directAnnotation; } - return false; + return symbol.getAnnotationMirrors().stream() + .map(compound -> compound.type.tsym) + .flatMap(filterStreamByType(ClassSymbol.class)) + .map(this::findImmutablesStyle) + .flatMap(Streams::stream) + .findAny(); } /** * Returns a function for use in Optional.flatMap that filters by type, and casts to that type. */ private Function> filterByType(Class clazz) { + return this.filterStreamByType(clazz).andThen(Stream::findAny); + } + + /** + * Returns a function for use in Stream.flatMap that filters by type, and casts to that type. + */ + private Function> filterStreamByType(Class clazz) { return value -> { if (clazz.isInstance(value)) { - return Optional.of(clazz.cast(value)); + return Stream.of(clazz.cast(value)); } - return Optional.empty(); + return Stream.empty(); }; } + + private static boolean extendsImmutablesGeneratedClass(Type type, VisitorState state) { + if (type.tsym instanceof ClassSymbol) { + return Optional.ofNullable(type.tsym.getAnnotation(Generated.class)) + .map(generated -> generated.generator().equals("Immutables")) + .orElseGet(() -> extendsImmutablesGeneratedClass(((ClassSymbol) type.tsym).getSuperclass(), state)); + } + return false; + } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index e71fc3359..154ce39aa 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -148,13 +148,13 @@ public void testPassesWhenNoRequiredFields() { } @Test - public void testPassesWhenAllFieldsPopulated_withCustomInitMethodStyle() { + public void testPassesWhenAllFieldsPopulated_withCustomMethodStyle() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomInitMethodStyle;", + "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomMethodStyle;", "public class MyTest {", " public static void main(String[] args) {", - " ImmutableTypeWithCustomInitMethodStyle.builder().setValue(\"value\").build();", + " ImmutableTypeWithCustomMethodStyle.builder().setValue(\"value\").build();", " }", "}") .doTest(); @@ -261,14 +261,14 @@ public void testFailsWhenOneFieldOmitted_fromLocalMethod() { } @Test - public void testFailsWhenAllOneFieldOmitted_withCustomInitMethodStyle() { + public void testFailsWhenAllOneFieldOmitted_withCustomMethodStyle() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomInitMethodStyle;", + "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomMethodStyle;", "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: value", - " ImmutableTypeWithCustomInitMethodStyle.builder().build();", + " ImmutableTypeWithCustomMethodStyle.builder().build();", " }", "}") .doTest(); @@ -333,8 +333,8 @@ public interface TypeWithLongNames { } @Value.Immutable - @Value.Style(visibility = ImplementationVisibility.PUBLIC, init = "set*") - public interface TypeWithCustomInitMethodStyle { - String value(); + @Value.Style(visibility = ImplementationVisibility.PUBLIC, init = "set*", get = "use*") + public interface TypeWithCustomMethodStyle { + String useValue(); } } From 8dc6b5662d0584caffa28661d290601e177c27f0 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 17:12:56 +0100 Subject: [PATCH 10/19] Fix stack overflow exception when annotations are included recursively --- .../ImmutablesBuilderMissingInitialization.java | 11 ++++++++--- .../ImmutablesBuilderMissingInitializationTest.java | 5 ++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 652d9a872..d9804049d 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -95,7 +95,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - Optional maybeStyle = findImmutablesStyle(interfaceClass.get()); + Optional maybeStyle = findImmutablesStyle(interfaceClass.get(), ImmutableSet.of()); Set getterPrefixes; if (maybeStyle.isPresent()) { if (!maybeStyle.get().init().endsWith("*")) { @@ -265,15 +265,20 @@ private boolean methodJustConstructsBuilder( && methodSymbol.getParameters().isEmpty()); } - private Optional findImmutablesStyle(ClassSymbol symbol) { + private Optional findImmutablesStyle(ClassSymbol symbol, Set encounteredClasses) { Optional directAnnotation = Optional.ofNullable(symbol.getAnnotation(Value.Style.class)); if (directAnnotation.isPresent()) { return directAnnotation; } + Set updatedEncounteredClasses = ImmutableSet.builder() + .addAll(encounteredClasses) + .add(symbol) + .build(); return symbol.getAnnotationMirrors().stream() .map(compound -> compound.type.tsym) .flatMap(filterStreamByType(ClassSymbol.class)) - .map(this::findImmutablesStyle) + .filter(annotationSymbol -> !updatedEncounteredClasses.contains(annotationSymbol)) + .map(annotationSymbol -> findImmutablesStyle(annotationSymbol, updatedEncounteredClasses)) .flatMap(Streams::stream) .findAny(); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 154ce39aa..c096e1c0e 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -333,8 +333,11 @@ public interface TypeWithLongNames { } @Value.Immutable - @Value.Style(visibility = ImplementationVisibility.PUBLIC, init = "set*", get = "use*") + @CustomStyleAnnotation public interface TypeWithCustomMethodStyle { String useValue(); } + + @Value.Style(visibility = ImplementationVisibility.PUBLIC, init = "set*", get = "use*") + private @interface CustomStyleAnnotation {} } From 25520271f6b0c238ac5ee30aa1d916c1c77af387 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 18:09:22 +0100 Subject: [PATCH 11/19] Support style annotations on packages --- ...mmutablesBuilderMissingInitialization.java | 47 ++++++++++--------- ...ablesBuilderMissingInitializationTest.java | 22 +++++++++ 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index d9804049d..b2aa19938 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -38,14 +38,15 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.PackageSymbol; import com.sun.tools.javac.code.Type; import java.util.Arrays; +import java.util.HashSet; import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.immutables.value.Generated; import org.immutables.value.Value; @@ -95,7 +96,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - Optional maybeStyle = findImmutablesStyle(interfaceClass.get(), ImmutableSet.of()); + Optional maybeStyle = + findImmutablesStyle(interfaceClass.get(), ImmutableSet.of(interfaceClass.get())); Set getterPrefixes; if (maybeStyle.isPresent()) { if (!maybeStyle.get().init().endsWith("*")) { @@ -106,6 +108,9 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // The field names might be cut off and the end as well as the beginning, which we can't handle return Description.NO_MATCH; } + // Get all the defined get method prefixes, and convert them into constant format for use with init bits + // If the getter is get*, for a method named getMyValue the builder methods will be named myValue and the + // init bit will be named INIT_BIT_GET_MY_VALUE, so we need to remember to strip GET_. getterPrefixes = Arrays.stream(maybeStyle.get().get()) .map(getter -> getter.replace("*", "")) .map(CaseFormat.LOWER_CAMEL.converterTo(CaseFormat.UPPER_UNDERSCORE)) @@ -265,20 +270,25 @@ private boolean methodJustConstructsBuilder( && methodSymbol.getParameters().isEmpty()); } - private Optional findImmutablesStyle(ClassSymbol symbol, Set encounteredClasses) { - Optional directAnnotation = Optional.ofNullable(symbol.getAnnotation(Value.Style.class)); + /** + * Find the closest applicable @Value.Style annotation, if it exists. + * + * Annotations can be present on the class, an enclosing class, or their parent package, as well as being annotated + * on another annotation that is applied in any of those places. + */ + private Optional findImmutablesStyle(Symbol currentSymbol, Set initialEncounteredClasses) { + Optional directAnnotation = Optional.ofNullable(currentSymbol.getAnnotation(Value.Style.class)); if (directAnnotation.isPresent()) { return directAnnotation; } - Set updatedEncounteredClasses = ImmutableSet.builder() - .addAll(encounteredClasses) - .add(symbol) - .build(); - return symbol.getAnnotationMirrors().stream() - .map(compound -> compound.type.tsym) - .flatMap(filterStreamByType(ClassSymbol.class)) - .filter(annotationSymbol -> !updatedEncounteredClasses.contains(annotationSymbol)) - .map(annotationSymbol -> findImmutablesStyle(annotationSymbol, updatedEncounteredClasses)) + Set encounteredClasses = new HashSet<>(initialEncounteredClasses); + return Streams.concat( + currentSymbol.getAnnotationMirrors().stream().map(compound -> compound.type.tsym), + Streams.stream(Optional.ofNullable(currentSymbol.owner))) + .filter(symbol -> symbol instanceof ClassSymbol || symbol instanceof PackageSymbol) + .filter(symbol -> !encounteredClasses.contains(symbol)) + .peek(encounteredClasses::add) + .map(annotationSymbol -> findImmutablesStyle(annotationSymbol, encounteredClasses)) .flatMap(Streams::stream) .findAny(); } @@ -287,18 +297,11 @@ private Optional findImmutablesStyle(ClassSymbol symbol, Set Function> filterByType(Class clazz) { - return this.filterStreamByType(clazz).andThen(Stream::findAny); - } - - /** - * Returns a function for use in Stream.flatMap that filters by type, and casts to that type. - */ - private Function> filterStreamByType(Class clazz) { return value -> { if (clazz.isInstance(value)) { - return Stream.of(clazz.cast(value)); + return Optional.of(clazz.cast(value)); } - return Stream.empty(); + return Optional.empty(); }; } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index c096e1c0e..6ce4394ed 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -160,6 +160,23 @@ public void testPassesWhenAllFieldsPopulated_withCustomMethodStyle() { .doTest(); } + @Test + public void testPassesWhenUnsupportedGetterFormat_withCustomStyleOnPackage() { + helper().addSourceLines( + "package-info.java", + "@org.immutables.value.Value.Style(get = \"*unsupportedGetFormat\")", + "package " + getClass().getPackage().getName() + ";") + .addSourceLines( + "MyTest.java", + "import com.palantir.baseline.errorprone.ImmutableTypeWithNoStyleAnnotations;", + "public class MyTest {", + " public static void main(String[] args) {", + " ImmutableTypeWithNoStyleAnnotations.builder().build();", + " }", + "}") + .doTest(); + } + @Test public void testFailsWhenOneMandatoryFieldOmitted() { helper().addSourceLines( @@ -332,6 +349,11 @@ public interface TypeWithLongNames { String mandatoryFieldWithLongName(); } + @Value.Immutable + public interface TypeWithNoStyleAnnotations { + String value(); + } + @Value.Immutable @CustomStyleAnnotation public interface TypeWithCustomMethodStyle { From 20a2c85d8ddaba31d8516d6413a840db26a84c75 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 18:35:57 +0100 Subject: [PATCH 12/19] Refactor tests --- ...mmutablesBuilderMissingInitialization.java | 3 + ...ablesBuilderMissingInitializationTest.java | 72 ++++++++++++------- 2 files changed, 50 insertions(+), 25 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index b2aa19938..785af4fed 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -305,6 +305,9 @@ private Function> filterByType(Class clazz) { }; } + /** + * Returns whether the provided type is a class that was generated by Immutables, or extends a class that was. + */ private static boolean extendsImmutablesGeneratedClass(Type type, VisitorState state) { if (type.tsym instanceof ClassSymbol) { return Optional.ofNullable(type.tsym.getAnnotation(Generated.class)) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 6ce4394ed..602ff14ca 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -16,6 +16,7 @@ package com.palantir.baseline.errorprone; import com.google.errorprone.CompilationTestHelper; +import com.palantir.baseline.errorprone.immutablebuildersmissinginitialization.ImmutablePerson; import java.util.List; import java.util.Optional; import javax.annotation.Nullable; @@ -28,7 +29,7 @@ public class ImmutablesBuilderMissingInitializationTest { public void testPassesWhenAllFieldsPopulated() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " new Person.Builder().name(\"name\").age(100).partner(\"partner\").build();", @@ -41,7 +42,7 @@ public void testPassesWhenAllFieldsPopulated() { public void testPassesWhenOptionalFieldOmitted() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " new Person.Builder().name(\"name\").age(100).build();", @@ -54,7 +55,7 @@ public void testPassesWhenOptionalFieldOmitted() { public void testPassesWithFrom() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " Person p1 = new Person.Builder().name(\"name\").age(100).build();", @@ -68,7 +69,7 @@ public void testPassesWithFrom() { public void testPassesWhenBuilderAssignedToVariable() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " Person.Builder builder = new Person.Builder();", @@ -82,14 +83,13 @@ public void testPassesWhenBuilderAssignedToVariable() { public void testPassesWithUninitializedFields_whenBuilderFromMethodThatSetsSomeFields() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablePerson;", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importImmutable("Person"), "public class MyTest {", " public static void main(String[] args) {", " builder().build();", " }", " private static ImmutablePerson.Builder builder() {", - " return new Person.Builder().name(\"name\").age(100);", + " return new ImmutablePerson.Builder().name(\"name\").age(100);", " }", "}") .doTest(); @@ -99,7 +99,7 @@ public void testPassesWithUninitializedFields_whenBuilderFromMethodThatSetsSomeF public void testPassesWhenAllFieldsPopulated_usingInterfaceBuilderMethod() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " Person.builder().name(\"name\").age(100).partner(\"partner\").build();", @@ -112,7 +112,7 @@ public void testPassesWhenAllFieldsPopulated_usingInterfaceBuilderMethod() { public void testPassesWhenAllFieldsPopulated_usingImmutableBuilderMethod() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableNotOvershadowedType;", + importImmutable("NotOvershadowedType"), "public class MyTest {", " public static void main(String[] args) {", " ImmutableNotOvershadowedType.builder().name(\"name\").build();", @@ -125,7 +125,7 @@ public void testPassesWhenAllFieldsPopulated_usingImmutableBuilderMethod() { public void testPassesWhenAllFieldsPopulated_usingImmutableConstructor() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablePerson;", + importImmutable("Person"), "public class MyTest {", " public static void main(String[] args) {", " new ImmutablePerson.Builder().name(\"name\").age(100).build();", @@ -138,7 +138,7 @@ public void testPassesWhenAllFieldsPopulated_usingImmutableConstructor() { public void testPassesWhenNoRequiredFields() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithNoMandatoryFields;", + importImmutable("TypeWithNoMandatoryFields"), "public class MyTest {", " public static void main(String[] args) {", " ImmutableTypeWithNoMandatoryFields.builder().build();", @@ -151,7 +151,7 @@ public void testPassesWhenNoRequiredFields() { public void testPassesWhenAllFieldsPopulated_withCustomMethodStyle() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomMethodStyle;", + importImmutable("TypeWithCustomMethodStyle"), "public class MyTest {", " public static void main(String[] args) {", " ImmutableTypeWithCustomMethodStyle.builder().setValue(\"value\").build();", @@ -181,7 +181,7 @@ public void testPassesWhenUnsupportedGetterFormat_withCustomStyleOnPackage() { public void testFailsWhenOneMandatoryFieldOmitted() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " Person.builder()", @@ -199,7 +199,7 @@ public void testFailsWhenOneMandatoryFieldOmitted() { public void testFailsWhenAllFieldsOmitted() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", @@ -213,7 +213,7 @@ public void testFailsWhenAllFieldsOmitted() { public void testFailsWhenOneFieldOmitted_usingImmutableConstructor() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablePerson;", + importImmutable("Person"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", @@ -227,7 +227,7 @@ public void testFailsWhenOneFieldOmitted_usingImmutableConstructor() { public void testFailsWhenOneFieldOmitted_usingInterfaceBuilderMethod() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name", @@ -243,7 +243,7 @@ public void testFailsWhenOneFieldOmitted_usingSimulatedBuilderMethod() { // same compilation unit helper().addSourceLines( "Helper.java", - "import com.palantir.baseline.errorprone.ImmutableNotOvershadowedType;", + importImmutable("NotOvershadowedType"), "public class Helper {", " public static ImmutableNotOvershadowedType.Builder builder() {", " return ImmutableNotOvershadowedType.builder();", @@ -264,7 +264,7 @@ public void testFailsWhenOneFieldOmitted_usingSimulatedBuilderMethod() { public void testFailsWhenOneFieldOmitted_fromLocalMethod() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutablesBuilderMissingInitializationTest.Person;", + importInterface("Person"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: name, age", @@ -281,7 +281,7 @@ public void testFailsWhenOneFieldOmitted_fromLocalMethod() { public void testFailsWhenAllOneFieldOmitted_withCustomMethodStyle() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithCustomMethodStyle;", + importImmutable("TypeWithCustomMethodStyle"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: value", @@ -295,7 +295,7 @@ public void testFailsWhenAllOneFieldOmitted_withCustomMethodStyle() { public void testComputesMissingFieldNamesCorrectly() { helper().addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithLongNames;", + importImmutable("TypeWithLongNames;"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: " @@ -310,8 +310,21 @@ private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(ImmutablesBuilderMissingInitialization.class, getClass()); } + private String importInterface(String interfaceName) { + return String.format("import %s.%s;", getClass().getCanonicalName(), interfaceName); + } + + private String importImmutable(String interfaceName) { + return String.format( + "import %s.immutablebuildersmissinginitialization.Immutable%s;", + getClass().getPackage().getName(), interfaceName); + } + @Value.Immutable - @Value.Style(visibility = ImplementationVisibility.PUBLIC, overshadowImplementation = true) + @Value.Style( + visibility = ImplementationVisibility.PUBLIC, + overshadowImplementation = true, + packageGenerated = "*.immutablebuildersmissinginitialization") public interface Person { String name(); @@ -327,13 +340,13 @@ static Builder builder() { } @Value.Immutable - @Value.Style(visibility = ImplementationVisibility.PUBLIC) + @DefaultImmutableStyle public interface NotOvershadowedType { String name(); } @Value.Immutable - @Value.Style(visibility = ImplementationVisibility.PUBLIC) + @DefaultImmutableStyle public interface TypeWithNoMandatoryFields { Optional notRequired(); @@ -344,7 +357,7 @@ public interface TypeWithNoMandatoryFields { } @Value.Immutable - @Value.Style(visibility = ImplementationVisibility.PUBLIC) + @DefaultImmutableStyle public interface TypeWithLongNames { String mandatoryFieldWithLongName(); } @@ -360,6 +373,15 @@ public interface TypeWithCustomMethodStyle { String useValue(); } - @Value.Style(visibility = ImplementationVisibility.PUBLIC, init = "set*", get = "use*") + @Value.Style( + visibility = ImplementationVisibility.PUBLIC, + packageGenerated = "*.immutablebuildersmissinginitialization") + private @interface DefaultImmutableStyle {} + + @Value.Style( + visibility = ImplementationVisibility.PUBLIC, + init = "set*", + get = "use*", + packageGenerated = "*.immutablebuildersmissinginitialization") private @interface CustomStyleAnnotation {} } From eac63baeccc57e5a5175f6dad064d22e06c089fc Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Thu, 8 Oct 2020 18:53:51 +0100 Subject: [PATCH 13/19] Support immutables based on abstract classes too --- ...mmutablesBuilderMissingInitialization.java | 10 ++++-- ...ablesBuilderMissingInitializationTest.java | 35 ++++++++++++++++++- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 785af4fed..25129ae09 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -47,6 +47,7 @@ import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.immutables.value.Generated; import org.immutables.value.Value; @@ -88,10 +89,13 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (immutableClass == null) { return Description.NO_MATCH; } - Optional interfaceClass = immutableClass.getInterfaces().stream() + Optional interfaceClass = Streams.concat( + immutableClass.getInterfaces().stream(), Stream.of(immutableClass.getSuperclass())) .map(type -> type.tsym) - .findAny() - .flatMap(filterByType(ClassSymbol.class)); + .map(filterByType(ClassSymbol.class)) + .flatMap(Streams::stream) + .filter(classSymbol -> classSymbol.getAnnotation(Value.Immutable.class) != null) + .findAny(); if (!interfaceClass.isPresent()) { return Description.NO_MATCH; } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index 602ff14ca..f18d40f91 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -177,6 +177,19 @@ public void testPassesWhenUnsupportedGetterFormat_withCustomStyleOnPackage() { .doTest(); } + @Test + public void testPassesWhenAllFieldsPopulated_withImmutableOnAbstractClass() { + helper().addSourceLines( + "MyTest.java", + importImmutable("ClassType"), + "public class MyTest {", + " public static void main(String[] args) {", + " ImmutableClassType.builder().value(\"value\").build();", + " }", + "}") + .doTest(); + } + @Test public void testFailsWhenOneMandatoryFieldOmitted() { helper().addSourceLines( @@ -278,7 +291,7 @@ public void testFailsWhenOneFieldOmitted_fromLocalMethod() { } @Test - public void testFailsWhenAllOneFieldOmitted_withCustomMethodStyle() { + public void testFailsWhenOneFieldOmitted_withCustomMethodStyle() { helper().addSourceLines( "MyTest.java", importImmutable("TypeWithCustomMethodStyle"), @@ -291,6 +304,20 @@ public void testFailsWhenAllOneFieldOmitted_withCustomMethodStyle() { .doTest(); } + @Test + public void testPassesWhenOneFieldOmitted_withImmutableOnAbstractClass() { + helper().addSourceLines( + "MyTest.java", + importImmutable("ClassType"), + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: value", + " ImmutableClassType.builder().build();", + " }", + "}") + .doTest(); + } + @Test public void testComputesMissingFieldNamesCorrectly() { helper().addSourceLines( @@ -362,6 +389,12 @@ public interface TypeWithLongNames { String mandatoryFieldWithLongName(); } + @Value.Immutable + @DefaultImmutableStyle + public abstract static class ClassType { + public abstract String value(); + } + @Value.Immutable public interface TypeWithNoStyleAnnotations { String value(); From 2ba55cb4f17c951806734c3c7c9b38e00607a6f5 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Fri, 9 Oct 2020 11:32:54 +0100 Subject: [PATCH 14/19] Avoid class not found exception at runtime by not instantiating the immutable annotation --- ...mmutablesBuilderMissingInitialization.java | 65 ++++++++++++------- ...ablesBuilderMissingInitializationTest.java | 2 +- 2 files changed, 41 insertions(+), 26 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 25129ae09..6907d4681 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -19,6 +19,7 @@ import com.google.auto.service.AutoService; import com.google.common.base.CaseFormat; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; @@ -35,13 +36,15 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ReturnTree; +import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.Attribute.Compound; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.PackageSymbol; import com.sun.tools.javac.code.Type; -import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -94,35 +97,44 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .map(type -> type.tsym) .map(filterByType(ClassSymbol.class)) .flatMap(Streams::stream) - .filter(classSymbol -> classSymbol.getAnnotation(Value.Immutable.class) != null) + .filter(classSymbol -> ASTHelpers.hasAnnotation(classSymbol, Value.Immutable.class, state)) .findAny(); if (!interfaceClass.isPresent()) { return Description.NO_MATCH; } - Optional maybeStyle = + // If there's an immutables style, check that it's compatible + Optional maybeStyle = findImmutablesStyle(interfaceClass.get(), ImmutableSet.of(interfaceClass.get())); - Set getterPrefixes; - if (maybeStyle.isPresent()) { - if (!maybeStyle.get().init().endsWith("*")) { - // The builder methods don't end with the field name, so this logic won't work - return Description.NO_MATCH; - } - if (Arrays.stream(maybeStyle.get().get()).anyMatch(getter -> !getter.endsWith("*"))) { - // The field names might be cut off and the end as well as the beginning, which we can't handle - return Description.NO_MATCH; - } - // Get all the defined get method prefixes, and convert them into constant format for use with init bits - // If the getter is get*, for a method named getMyValue the builder methods will be named myValue and the - // init bit will be named INIT_BIT_GET_MY_VALUE, so we need to remember to strip GET_. - getterPrefixes = Arrays.stream(maybeStyle.get().get()) - .map(getter -> getter.replace("*", "")) - .map(CaseFormat.LOWER_CAMEL.converterTo(CaseFormat.UPPER_UNDERSCORE)) - .map(prefix -> prefix + "_") - .collect(Collectors.toSet()); - } else { - getterPrefixes = ImmutableSet.of("GET_"); + Optional maybeInit = maybeStyle + .map(style -> style.member(state.getName("init"))) + .map(attribute -> (String) attribute.getValue()); + if (maybeInit.isPresent() && !maybeInit.get().endsWith("*")) { + // The builder methods don't end with the field name, so this logic won't work + return Description.NO_MATCH; } + List getters = maybeStyle + .map(style -> style.member(state.getName("get"))) + .map(Attribute::getValue) + .map(rawValue -> ((List) rawValue) + .stream() + .map(attribute -> (Attribute) attribute) + .map(Attribute::getValue) + .map(value -> (String) value) + .collect(Collectors.toList())) + .orElseGet(() -> ImmutableList.of("get*")); + if (getters.stream().anyMatch(getter -> !getter.endsWith("*"))) { + // The field names might be cut off and the end as well as the beginning, which we can't handle + return Description.NO_MATCH; + } + // Get all the defined get method prefixes, and convert them into constant format for use with init bits + // If the getter is get*, for a method named getMyValue the builder methods will be named myValue and the + // init bit will be named INIT_BIT_GET_MY_VALUE, so we need to remember to strip GET_. + Set getterPrefixes = getters.stream() + .map(getter -> getter.replace("*", "")) + .map(CaseFormat.LOWER_CAMEL.converterTo(CaseFormat.UPPER_UNDERSCORE)) + .map(prefix -> prefix + "_") + .collect(Collectors.toSet()); // Mandatory fields have a private static final constant in the generated builder named INIT_BIT_varname, where // varname is the UPPER_UNDERSCORE version of the variable name. Find these fields to get the mandatory fields. @@ -280,8 +292,11 @@ private boolean methodJustConstructsBuilder( * Annotations can be present on the class, an enclosing class, or their parent package, as well as being annotated * on another annotation that is applied in any of those places. */ - private Optional findImmutablesStyle(Symbol currentSymbol, Set initialEncounteredClasses) { - Optional directAnnotation = Optional.ofNullable(currentSymbol.getAnnotation(Value.Style.class)); + private Optional findImmutablesStyle(Symbol currentSymbol, Set initialEncounteredClasses) { + Optional directAnnotation = currentSymbol.getRawAttributes().stream() + .filter(compound -> + compound.type.tsym.getQualifiedName().contentEquals("org.immutables.value.Value.Style")) + .findAny(); if (directAnnotation.isPresent()) { return directAnnotation; } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index f18d40f91..afc94878f 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -164,7 +164,7 @@ public void testPassesWhenAllFieldsPopulated_withCustomMethodStyle() { public void testPassesWhenUnsupportedGetterFormat_withCustomStyleOnPackage() { helper().addSourceLines( "package-info.java", - "@org.immutables.value.Value.Style(get = \"*unsupportedGetFormat\")", + "@org.immutables.value.Value.Style(get = {\"*unsupportedGetFormat\", \"get*\"})", "package " + getClass().getPackage().getName() + ";") .addSourceLines( "MyTest.java", From ea74fece470b556786baf3a8278048e4233b1dde Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Fri, 9 Oct 2020 14:40:23 +0100 Subject: [PATCH 15/19] Don't search for @Value.Style annotations because it will only lead to class not found exceptions --- ...mmutablesBuilderMissingInitialization.java | 104 ++++++------------ 1 file changed, 36 insertions(+), 68 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 6907d4681..320c745f3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -19,8 +19,8 @@ import com.google.auto.service.AutoService; import com.google.common.base.CaseFormat; import com.google.common.base.Joiner; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; @@ -36,15 +36,10 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ReturnTree; -import com.sun.tools.javac.code.Attribute; -import com.sun.tools.javac.code.Attribute.Compound; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; -import com.sun.tools.javac.code.Symbol.PackageSymbol; import com.sun.tools.javac.code.Type; -import java.util.HashSet; -import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -52,7 +47,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import org.immutables.value.Generated; -import org.immutables.value.Value; /** * Checks that all required fields in an immutables.org generated builder have been populated. @@ -75,6 +69,10 @@ summary = "All required fields of an Immutables builder must be initialized") public final class ImmutablesBuilderMissingInitialization extends BugChecker implements MethodInvocationTreeMatcher { private static final String FIELD_INIT_BITS_PREFIX = "INIT_BIT_"; + // Prefixes on the interface getter methods that may be stripped by immutables - by default only GET_ is stripped, + // but some places set the style to remove IS_ too and we can't load the style to check what to remove. It doesn't + // matter if we remove too much, because we only do a suffix match on the methods. + private static final ImmutableSet GET_PREFIXES = ImmutableSet.of("GET_", "IS_"); private static final Matcher builderMethodMatcher = Matchers.instanceMethod() .onClass(ImmutablesBuilderMissingInitialization::extendsImmutablesGeneratedClass) @@ -97,45 +95,13 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .map(type -> type.tsym) .map(filterByType(ClassSymbol.class)) .flatMap(Streams::stream) - .filter(classSymbol -> ASTHelpers.hasAnnotation(classSymbol, Value.Immutable.class, state)) + .filter(classSymbol -> + ASTHelpers.hasAnnotation(classSymbol, "org.immutables.value.Value.Immutable", state)) .findAny(); if (!interfaceClass.isPresent()) { return Description.NO_MATCH; } - // If there's an immutables style, check that it's compatible - Optional maybeStyle = - findImmutablesStyle(interfaceClass.get(), ImmutableSet.of(interfaceClass.get())); - Optional maybeInit = maybeStyle - .map(style -> style.member(state.getName("init"))) - .map(attribute -> (String) attribute.getValue()); - if (maybeInit.isPresent() && !maybeInit.get().endsWith("*")) { - // The builder methods don't end with the field name, so this logic won't work - return Description.NO_MATCH; - } - List getters = maybeStyle - .map(style -> style.member(state.getName("get"))) - .map(Attribute::getValue) - .map(rawValue -> ((List) rawValue) - .stream() - .map(attribute -> (Attribute) attribute) - .map(Attribute::getValue) - .map(value -> (String) value) - .collect(Collectors.toList())) - .orElseGet(() -> ImmutableList.of("get*")); - if (getters.stream().anyMatch(getter -> !getter.endsWith("*"))) { - // The field names might be cut off and the end as well as the beginning, which we can't handle - return Description.NO_MATCH; - } - // Get all the defined get method prefixes, and convert them into constant format for use with init bits - // If the getter is get*, for a method named getMyValue the builder methods will be named myValue and the - // init bit will be named INIT_BIT_GET_MY_VALUE, so we need to remember to strip GET_. - Set getterPrefixes = getters.stream() - .map(getter -> getter.replace("*", "")) - .map(CaseFormat.LOWER_CAMEL.converterTo(CaseFormat.UPPER_UNDERSCORE)) - .map(prefix -> prefix + "_") - .collect(Collectors.toSet()); - // Mandatory fields have a private static final constant in the generated builder named INIT_BIT_varname, where // varname is the UPPER_UNDERSCORE version of the variable name. Find these fields to get the mandatory fields. Set requiredFields = Streams.stream(builderClass.members().getSymbols()) @@ -143,15 +109,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .filter(symbol -> symbol.getKind().isField()) .filter(symbol -> symbol.getSimpleName().toString().startsWith(FIELD_INIT_BITS_PREFIX)) .map(Symbol::toString) - .map(initBitsName -> initBitsName.replace(FIELD_INIT_BITS_PREFIX, "")) - .map(fieldName -> getterPrefixes.stream() - .filter(fieldName::startsWith) - .findAny() - .map(prefix -> fieldName.replace(prefix, "")) - .orElse(fieldName)) + .map(initBitsName -> removeFromStart(initBitsName, FIELD_INIT_BITS_PREFIX)) + .map(fieldName -> GET_PREFIXES.stream().reduce(fieldName, this::removeFromStart)) .map(CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL)) .collect(Collectors.toSet()); + if (!checkAllFieldsCanBeInitialized(requiredFields, builderClass)) { + // There is likely a custom style applied that means the rules don't match + return Description.NO_MATCH; + } + // Run the check Set uninitializedFields = checkInitialization( ASTHelpers.getReceiver(tree), @@ -225,6 +192,23 @@ private Set checkInitialization( return ImmutableSet.of(); } + /** + * Check that every required field has a setter method, to ensure that there are no style rules that have not been + * accounted for here. + */ + private boolean checkAllFieldsCanBeInitialized(Set fields, Symbol builderClass) { + return Streams.stream(builderClass.members().getSymbols()) + .map(filterByType(MethodSymbol.class)) + .flatMap(Streams::stream) + .filter(symbol -> !symbol.isStaticOrInstanceInit() + && !symbol.isConstructor() + && !symbol.isAnonymous() + && symbol.getParameters().size() == 1) + .map(symbol -> symbol.getSimpleName().toString()) + .reduce(fields, this::removeFieldsPotentiallyInitializedBy, Sets::intersection) + .isEmpty(); + } + /** * Takes a set of uninitialized fields, and returns a set containing the fields that cannot have been initialized by * the method methodName. @@ -287,29 +271,13 @@ private boolean methodJustConstructsBuilder( } /** - * Find the closest applicable @Value.Style annotation, if it exists. - * - * Annotations can be present on the class, an enclosing class, or their parent package, as well as being annotated - * on another annotation that is applied in any of those places. + * If input starts with toRemove, returns the rest of input with toRemove removed, otherwise just returns input. */ - private Optional findImmutablesStyle(Symbol currentSymbol, Set initialEncounteredClasses) { - Optional directAnnotation = currentSymbol.getRawAttributes().stream() - .filter(compound -> - compound.type.tsym.getQualifiedName().contentEquals("org.immutables.value.Value.Style")) - .findAny(); - if (directAnnotation.isPresent()) { - return directAnnotation; + private String removeFromStart(String input, String toRemove) { + if (input.startsWith(toRemove)) { + return input.substring(toRemove.length()); } - Set encounteredClasses = new HashSet<>(initialEncounteredClasses); - return Streams.concat( - currentSymbol.getAnnotationMirrors().stream().map(compound -> compound.type.tsym), - Streams.stream(Optional.ofNullable(currentSymbol.owner))) - .filter(symbol -> symbol instanceof ClassSymbol || symbol instanceof PackageSymbol) - .filter(symbol -> !encounteredClasses.contains(symbol)) - .peek(encounteredClasses::add) - .map(annotationSymbol -> findImmutablesStyle(annotationSymbol, encounteredClasses)) - .flatMap(Streams::stream) - .findAny(); + return input; } /** From bac610f15b47af8f32252c47735ade4c0db90e0b Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Fri, 9 Oct 2020 16:24:54 +0100 Subject: [PATCH 16/19] Update tests and add to readme --- README.md | 1 + baseline-error-prone/build.gradle | 1 + ...mmutablesBuilderMissingInitialization.java | 17 ++- ...ablesBuilderMissingInitializationTest.java | 120 ++++++++++++++---- 4 files changed, 108 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 335734c8c..3f77a3119 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `PreferStaticLoggers`: Prefer static loggers over instance loggers. - `LogsafeArgName`: Prevent certain named arguments as being logged as safe. Specify unsafe argument names using `LogsafeArgName:UnsafeArgNames` errorProne flag. - `ImplicitPublicBuilderConstructor`: Prevent builders from unintentionally leaking public constructors. +- `ImmutablesBuildeMissingInitialization`: Prevent building Immutables.org builders when not all fields have been populated. ### Programmatic Application diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index 510823853..dbaf3f88a 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -28,6 +28,7 @@ dependencies { annotationProcessor 'com.google.auto.service:auto-service' annotationProcessor 'org.immutables:value' + testAnnotationProcessor 'org.immutables:value' compileOnly 'com.google.auto.service:auto-service' } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index 320c745f3..b96077c8e 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -68,10 +68,18 @@ severity = BugPattern.SeverityLevel.ERROR, summary = "All required fields of an Immutables builder must be initialized") public final class ImmutablesBuilderMissingInitialization extends BugChecker implements MethodInvocationTreeMatcher { + private static final String FIELD_INIT_BITS_PREFIX = "INIT_BIT_"; - // Prefixes on the interface getter methods that may be stripped by immutables - by default only GET_ is stripped, - // but some places set the style to remove IS_ too and we can't load the style to check what to remove. It doesn't - // matter if we remove too much, because we only do a suffix match on the methods. + + /** + * Prefixes on the interface getter methods that may be stripped by immutables. + * + * The default immutables style just uses get*, but some places use is* too and we can't load the style to check + * what to remove. It doesn't matter if we remove too much, because we only do a suffix match on the methods. + * + * This should only help when the unprefixed field name is an identifier, because in other cases immutables already + * uses the unprefixed name. + */ private static final ImmutableSet GET_PREFIXES = ImmutableSet.of("GET_", "IS_"); private static final Matcher builderMethodMatcher = Matchers.instanceMethod() @@ -104,6 +112,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // Mandatory fields have a private static final constant in the generated builder named INIT_BIT_varname, where // varname is the UPPER_UNDERSCORE version of the variable name. Find these fields to get the mandatory fields. + // nb. this isn't part of any immutables API, so could break. Set requiredFields = Streams.stream(builderClass.members().getSymbols()) .filter(Symbol::isStatic) .filter(symbol -> symbol.getKind().isField()) @@ -115,7 +124,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .collect(Collectors.toSet()); if (!checkAllFieldsCanBeInitialized(requiredFields, builderClass)) { - // There is likely a custom style applied that means the rules don't match + // Our expectation for the method names isn't right, so it's probably using a custom style return Description.NO_MATCH; } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java index afc94878f..b31fb7d21 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitializationTest.java @@ -22,7 +22,7 @@ import javax.annotation.Nullable; import org.immutables.value.Value; import org.immutables.value.Value.Style.ImplementationVisibility; -import org.junit.Test; +import org.junit.jupiter.api.Test; public class ImmutablesBuilderMissingInitializationTest { @Test @@ -52,7 +52,7 @@ public void testPassesWhenOptionalFieldOmitted() { } @Test - public void testPassesWithFrom() { + public void testPassesWhenPopulatedWithFrom() { helper().addSourceLines( "MyTest.java", importInterface("Person"), @@ -134,6 +134,19 @@ public void testPassesWhenAllFieldsPopulated_usingImmutableConstructor() { .doTest(); } + @Test + public void testPassesWhenDefaultAndDerivedFieldsOmitted() { + helper().addSourceLines( + "MyTest.java", + importImmutable("TypeWithDerivedAndDefaultFields"), + "public class MyTest {", + " public static void main(String[] args) {", + " ImmutableTypeWithDerivedAndDefaultFields.builder().value(\"value\").build();", + " }", + "}") + .doTest(); + } + @Test public void testPassesWhenNoRequiredFields() { helper().addSourceLines( @@ -148,30 +161,29 @@ public void testPassesWhenNoRequiredFields() { } @Test - public void testPassesWhenAllFieldsPopulated_withCustomMethodStyle() { + public void testPassesWithAllFieldsPopulated_whenGettersAndSettersPrefixed() { helper().addSourceLines( "MyTest.java", - importImmutable("TypeWithCustomMethodStyle"), + importImmutable("TypeWithGetStyle"), "public class MyTest {", " public static void main(String[] args) {", - " ImmutableTypeWithCustomMethodStyle.builder().setValue(\"value\").build();", + " ImmutableTypeWithGetStyle.builder().setValue(\"value\").build();", " }", "}") .doTest(); } @Test - public void testPassesWhenUnsupportedGetterFormat_withCustomStyleOnPackage() { + public void testPassesWithAllFieldsPopulated_whenGettersAndSettersPrefixedAndFieldIsIdentifier() { + // If there is a getter and setter prefix, and removing the prefix would leave a reserved word, immutables + // uses the prefixed version for the init bits. Without any special treatment, that would cause us to search for + // methods ending with isPublic (eg setIsPublic), so test that this edge case is handled correctly helper().addSourceLines( - "package-info.java", - "@org.immutables.value.Value.Style(get = {\"*unsupportedGetFormat\", \"get*\"})", - "package " + getClass().getPackage().getName() + ";") - .addSourceLines( "MyTest.java", - "import com.palantir.baseline.errorprone.ImmutableTypeWithNoStyleAnnotations;", + importImmutable("TypeWithStyleAndIdentifierFields"), "public class MyTest {", " public static void main(String[] args) {", - " ImmutableTypeWithNoStyleAnnotations.builder().build();", + " ImmutableTypeWithStyleAndIdentifierFields.builder().setPublic(true).build();", " }", "}") .doTest(); @@ -291,21 +303,52 @@ public void testFailsWhenOneFieldOmitted_fromLocalMethod() { } @Test - public void testFailsWhenOneFieldOmitted_withCustomMethodStyle() { + public void testFailsWhenMandatoryFieldsOmitted_withDefaultAndDerivedFieldsOmitted() { helper().addSourceLines( "MyTest.java", - importImmutable("TypeWithCustomMethodStyle"), + importImmutable("TypeWithDerivedAndDefaultFields"), "public class MyTest {", " public static void main(String[] args) {", " // BUG: Diagnostic contains: Some builder fields have not been initialized: value", - " ImmutableTypeWithCustomMethodStyle.builder().build();", + " ImmutableTypeWithDerivedAndDefaultFields.builder().build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithMandatoryFieldOmitted_whenGettersAndSettersPrefixed() { + helper().addSourceLines( + "MyTest.java", + importImmutable("TypeWithGetStyle"), + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: value", + " ImmutableTypeWithGetStyle.builder().build();", + " }", + "}") + .doTest(); + } + + @Test + public void testFailsWithMandatoryFieldOmitted_whenGettersAndSettersPrefixedAndFieldIsIdentifier() { + // If there is a getter and setter prefix, and removing the prefix would leave a reserved word, immutables + // uses the prefixed version for the init bits. Without any special treatment, that would cause us to search for + // methods ending with isPublic (eg setIsPublic), so test that this edge case is handled correctly + helper().addSourceLines( + "MyTest.java", + importImmutable("TypeWithStyleAndIdentifierFields"), + "public class MyTest {", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: Some builder fields have not been initialized: public", + " ImmutableTypeWithStyleAndIdentifierFields.builder().build();", " }", "}") .doTest(); } @Test - public void testPassesWhenOneFieldOmitted_withImmutableOnAbstractClass() { + public void testFailsWhenOneFieldOmitted_withImmutableOnAbstractClass() { helper().addSourceLines( "MyTest.java", importImmutable("ClassType"), @@ -391,30 +434,53 @@ public interface TypeWithLongNames { @Value.Immutable @DefaultImmutableStyle - public abstract static class ClassType { - public abstract String value(); + public interface TypeWithDerivedAndDefaultFields { + String value(); + + @Value.Derived + default String derivedString() { + return value(); + } + + @Value.Default + default String defaultString() { + return "default"; + } + + @Value.Lazy + default String lazyString() { + return "lazy"; + } } @Value.Immutable - public interface TypeWithNoStyleAnnotations { - String value(); + @DefaultImmutableStyle + public abstract static class ClassType { + public abstract String value(); } @Value.Immutable - @CustomStyleAnnotation - public interface TypeWithCustomMethodStyle { + @Value.Style( + visibility = ImplementationVisibility.PUBLIC, + packageGenerated = "*.immutablebuildersmissinginitialization", + get = "use*", + init = "set*") + public interface TypeWithGetStyle { String useValue(); } + @Value.Immutable @Value.Style( visibility = ImplementationVisibility.PUBLIC, - packageGenerated = "*.immutablebuildersmissinginitialization") - private @interface DefaultImmutableStyle {} + packageGenerated = "*.immutablebuildersmissinginitialization", + get = "is*", + init = "set*") + public interface TypeWithStyleAndIdentifierFields { + boolean isPublic(); + } @Value.Style( visibility = ImplementationVisibility.PUBLIC, - init = "set*", - get = "use*", packageGenerated = "*.immutablebuildersmissinginitialization") - private @interface CustomStyleAnnotation {} + private @interface DefaultImmutableStyle {} } From 354254907e8a1e6c44083d59a5e71708e777e579 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Fri, 9 Oct 2020 15:24:54 +0000 Subject: [PATCH 17/19] Add generated changelog entries --- changelog/@unreleased/pr-1504.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-1504.v2.yml diff --git a/changelog/@unreleased/pr-1504.v2.yml b/changelog/@unreleased/pr-1504.v2.yml new file mode 100644 index 000000000..74673f6cb --- /dev/null +++ b/changelog/@unreleased/pr-1504.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Add an Errorprone rule to check that all fields in Immutables builders + have been initialized + links: + - https://github.com/palantir/gradle-baseline/pull/1504 From cbdf5e31a651c8a14ee13f1cce6c23d29ccb15f3 Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Fri, 9 Oct 2020 16:49:46 +0100 Subject: [PATCH 18/19] Update README.md Co-authored-by: Carter Kozak --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3f77a3119..e4a9dce74 100644 --- a/README.md +++ b/README.md @@ -193,7 +193,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `PreferStaticLoggers`: Prefer static loggers over instance loggers. - `LogsafeArgName`: Prevent certain named arguments as being logged as safe. Specify unsafe argument names using `LogsafeArgName:UnsafeArgNames` errorProne flag. - `ImplicitPublicBuilderConstructor`: Prevent builders from unintentionally leaking public constructors. -- `ImmutablesBuildeMissingInitialization`: Prevent building Immutables.org builders when not all fields have been populated. +- `ImmutablesBuilderMissingInitialization`: Prevent building Immutables.org builders when not all fields have been populated. ### Programmatic Application From e0412d3a05b8d5bb9af55382cad7ab5704d5b69f Mon Sep 17 00:00:00 2001 From: Jack Wickham Date: Fri, 9 Oct 2020 17:48:40 +0100 Subject: [PATCH 19/19] Update javadoc --- .../ImmutablesBuilderMissingInitialization.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java index b96077c8e..3a5ec9722 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesBuilderMissingInitialization.java @@ -51,14 +51,15 @@ /** * Checks that all required fields in an immutables.org generated builder have been populated. * - * To make it decidable, it is limited to builders that are constructed by calling {@code new ImmutableType.Builder()}, - * {@code new Type.Builder()} (where Type.Builder extends ImmutableType.Builder), or a method that only calls one of - * those constructors and returns the result, and are never stored into a variable. Builders that do not meet these + * To make it decidable, the implementation is limited to the common case where: the builder is constructed in place + * using new ImmutableType.Builder() or new Type.Builder() (where Type.Builder extends ImmutableType.Builder), or using + * a method that does nothing other than construct and return the builder; and the builder is never stored to a + * variable, so the builder only lives within a single statement. Builders that do not meet these * conditions are assumed to populate all fields, and are ignored. * - * Mandatory fields are determined by inspecting the generated builder source to find the initBits that are updated by - * each method, to find any that do not get set. If Immutables changes the way that they check for required fields, this - * check will stop working (but the check will probably pass). + * Mandatory fields are determined by inspecting the generated builder class to find fields that have INIT_BIT_ + * constants, which are used by Immutables to track whether required fields have been initialized. Methods that end with + * the name of a field are assumed to initialize that field. */ @AutoService(BugChecker.class) @BugPattern(