diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index 13656339c..1780101a5 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -31,6 +31,7 @@ dependencies { testImplementation 'com.palantir.tritium:tritium-registry' testImplementation 'com.palantir.conjure.java:conjure-lib' testImplementation 'com.palantir.conjure.java.api:test-utils' + testImplementation 'com.palantir.ri:resource-identifier' testCompileOnly 'org.immutables:value::annotations' testImplementation 'org.junit.jupiter:junit-jupiter' testRuntimeOnly 'org.junit.jupiter:junit-jupiter-migrationsupport' diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ResourceIdentifierGetEqualsUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ResourceIdentifierGetEqualsUsage.java new file mode 100644 index 000000000..7b7b40780 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ResourceIdentifierGetEqualsUsage.java @@ -0,0 +1,128 @@ +/* + * (c) Copyright 2024 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.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ErrorProneToken; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.List; +import java.util.regex.Pattern; +import javax.lang.model.element.ElementKind; + +@AutoService(BugChecker.class) +@BugPattern( + summary = "Disallowed usage of ResourceIdentifier#get{Instance,Locator,Service,Type}#equals", + explanation = "ResourceIdentifier internally stores a single string for the entire RID. Each of the getX " + + "methods allocates a new string for that specific part of the RID. Use " + + "ResourceIdentifier#has{Instance,Locator,Service,Type} instead, which does not allocate any memory.", + severity = SeverityLevel.WARNING, + linkType = BugPattern.LinkType.CUSTOM, + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks") +public final class ResourceIdentifierGetEqualsUsage extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher EQUALS_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(String.class.getName()) + .named("equals"); + private static final Matcher GET_MATCHER = MethodMatchers.instanceMethod() + .onExactClass("com.palantir.ri.ResourceIdentifier") + .namedAnyOf("getInstance", "getLocator", "getService", "getType"); + private static final Pattern SOURCE_GET_EQUALS_PATTERN = + Pattern.compile("get(Instance|Locator|Service|Type)\\(\\)\\.equals\\("); + private static final Pattern SOURCE_EQUALS_GET_PATTERN = + Pattern.compile("(.*)\\.equals\\((.*)\\.get(Instance|Locator|Service|Type)\\(\\)"); + + private static final Matcher INVOCATION_TREE_MATCHER = Matchers.anyOf( + Matchers.allOf(EQUALS_MATCHER, Matchers.receiverOfInvocation(GET_MATCHER)), + EQUALS_MATCHER, + Matchers.allOf(Matchers.receiverOfInvocation(GET_MATCHER), EQUALS_MATCHER)); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!INVOCATION_TREE_MATCHER.matches(tree, state)) { + return Description.NO_MATCH; + } + + String source = state.getSourceForNode(tree); + if (source == null) { + return Description.NO_MATCH; + } + + ExpressionTree receiverTree = ASTHelpers.getReceiver(tree); + if (receiverTree == null) { + return Description.NO_MATCH; + } + + MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); + List errorProneTokens = state.getOffsetTokensForNode(tree); + int receiverEnd = state.getEndPosition(receiverTree); + int treeEnd = state.getEndPosition(tree); + + if (EQUALS_MATCHER.matches(tree, state)) { + if (GET_MATCHER.matches(receiverTree, state)) { + Type receiverType = ASTHelpers.getReceiverType(receiverTree); + Symbol receiverSymbol = ASTHelpers.getSymbol(receiverTree); + ElementKind receiverKind = receiverSymbol.getKind(); + System.out.printf( + "%n%nState" + "tree: %s%n" + + "method: %s%n" + "receiver: %s %s %s%n" + "tokens: %s%n" + + "positions: rec %d tree %d%n%n%n", + tree, + methodSymbol, + receiverKind, + receiverType, + receiverSymbol, + errorProneTokens, + receiverEnd, + treeEnd); + System.out.flush(); + + String replacement = SOURCE_GET_EQUALS_PATTERN.matcher(source).replaceAll("has$1("); + return buildDescription(tree) + .addFix(SuggestedFix.builder() + .replace(tree, replacement) + .build()) + .build(); + } + + System.out.printf( + "%n%nState" + "tree: %s%n" + "method: %s%n" + "tokens: %s%n" + "positions: rec %d tree %d%n%n%n", + tree, methodSymbol, errorProneTokens, receiverEnd, treeEnd); + System.out.flush(); + String replacement = SOURCE_EQUALS_GET_PATTERN.matcher(source).replaceAll("$2.has$3($1"); + SuggestedFix fix = SuggestedFix.builder().replace(tree, replacement).build(); + + return buildDescription(tree).addFix(fix).build(); + } + + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ResourceIdentifierGetEqualsUsageTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ResourceIdentifierGetEqualsUsageTest.java new file mode 100644 index 000000000..53fd1461a --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ResourceIdentifierGetEqualsUsageTest.java @@ -0,0 +1,174 @@ +/* + * (c) Copyright 2021 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 org.junit.jupiter.api.Test; + +final class ResourceIdentifierGetEqualsUsageTest { + @Test + void testHasInstance() { + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasInstance(\"test\");", + " }", + "}") + .expectUnchanged() + .doTest(); + + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.getInstance().equals(\"test\");", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasInstance(\"test\");", + " }", + "}") + .doTest(); + } + + @Test + void testHasInstanceFlipped() { + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return \"test\".equals(rid.getInstance());", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasInstance(\"test\");", + " }", + "}") + .doTest(); + } + + @Test + void testHasLocator() { + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasLocator(\"test\");", + " }", + "}") + .expectUnchanged() + .doTest(); + + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.getLocator().equals(\"test\");", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasLocator(\"test\");", + " }", + "}") + .doTest(); + } + + @Test + void testHasService() { + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasService(\"test\");", + " }", + "}") + .expectUnchanged() + .doTest(); + + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.getService().equals(\"test\");", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasService(\"test\");", + " }", + "}") + .doTest(); + } + + @Test + void testHasType() { + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasType(\"test\");", + " }", + "}") + .expectUnchanged() + .doTest(); + + fix().addInputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.getType().equals(\"test\");", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.ri.ResourceIdentifier;", + "public class Test {", + " boolean f(ResourceIdentifier rid) {", + " return rid.hasType(\"test\");", + " }", + "}") + .doTest(); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(ResourceIdentifierGetEqualsUsage.class, getClass()); + } +} diff --git a/versions.lock b/versions.lock index 74d2a5601..006294193 100644 --- a/versions.lock +++ b/versions.lock @@ -77,7 +77,7 @@ org.slf4j:slf4j-api:1.7.36 (10 constraints: 769c0f4c) [Test dependencies] cglib:cglib-nodep:3.2.2 (1 constraints: 490ded24) -com.fasterxml.jackson.core:jackson-annotations:2.16.1 (4 constraints: 603526bb) +com.fasterxml.jackson.core:jackson-annotations:2.16.1 (4 constraints: 603528bb) com.fasterxml.jackson.core:jackson-core:2.16.1 (2 constraints: 0c2ad913) com.fasterxml.jackson.core:jackson-databind:2.16.1 (5 constraints: b24a4c62) com.fasterxml.jackson.module:jackson-module-afterburner:2.16.1 (1 constraints: 3d05413b) @@ -93,13 +93,13 @@ com.palantir.conjure.java:conjure-lib:6.77.0 (1 constraints: 4605743b) com.palantir.conjure.java.api:errors:2.43.0 (2 constraints: 6d21c7b0) com.palantir.conjure.java.api:test-utils:2.43.0 (1 constraints: 3b05443b) com.palantir.conjure.java.runtime:conjure-java-annotations:8.2.0 (1 constraints: 0c051936) -com.palantir.ri:resource-identifier:2.5.0 (1 constraints: ef0f7999) +com.palantir.ri:resource-identifier:2.6.0 (2 constraints: f8142db7) com.palantir.safe-logging:logger:3.6.0 (2 constraints: e0122f37) com.palantir.safe-logging:logger-slf4j:3.6.0 (1 constraints: 040e6542) com.palantir.safe-logging:logger-spi:3.6.0 (2 constraints: 171e6d7b) com.palantir.safe-logging:preconditions:3.6.0 (6 constraints: 265782bd) com.palantir.safe-logging:preconditions-assertj:3.6.0 (1 constraints: 0b050c36) -com.palantir.safe-logging:safe-logging:3.6.0 (8 constraints: 7b772ae9) +com.palantir.safe-logging:safe-logging:3.6.0 (8 constraints: 4977db96) com.palantir.tokens:auth-tokens:3.18.0 (2 constraints: 601534db) com.palantir.tritium:tritium-registry:0.81.0 (1 constraints: 3b05423b) io.dropwizard.metrics:metrics-core:4.2.22 (1 constraints: c41050b6) diff --git a/versions.props b/versions.props index 93bc5e7c1..a68148057 100644 --- a/versions.props +++ b/versions.props @@ -24,6 +24,7 @@ com.netflix.nebula:nebula-test = 10.2.0 com.palantir.conjure.java:* = 6.77.0 com.palantir.conjure.java.api:* = 2.43.0 com.palantir.conjure.java.runtime:* = 8.2.0 +com.palantir.ri:* = 2.6.0 com.palantir.tokens:auth-tokens = 3.18.0 com.palantir.tritium:* = 0.81.0 commons-io:* = 2.15.1