From fcd39390073d0d9c9cb96746f9179e9acb3898f5 Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Wed, 11 Sep 2024 12:59:17 -0700 Subject: [PATCH] Don't allow `@InlineMe` on methods with params names matching `arg[0-9]+`. #inlineme PiperOrigin-RevId: 673502523 --- .../inlineme/InlinabilityResult.java | 23 +++++++--- .../bugpatterns/inlineme/Inliner.java | 2 +- .../bugpatterns/inlineme/Suggester.java | 2 +- .../bugpatterns/inlineme/SuggesterTest.java | 45 +++++++++++++++++++ .../bugpatterns/inlineme/ValidatorTest.java | 19 ++++++++ 5 files changed, 83 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/InlinabilityResult.java b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/InlinabilityResult.java index 89350fa6672..842430be464 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/InlinabilityResult.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/InlinabilityResult.java @@ -92,7 +92,7 @@ static InlinabilityResult inlinable(ExpressionTree body) { boolean isValidForSuggester() { return isValidForValidator() - || error() == InlineValidationErrorReason.METHOD_CAN_BE_OVERIDDEN_BUT_CAN_BE_FIXED; + || error() == InlineValidationErrorReason.METHOD_CAN_BE_OVERRIDDEN_BUT_CAN_BE_FIXED; } boolean isValidForValidator() { @@ -111,20 +111,21 @@ enum InlineValidationErrorReason { BODY_WOULD_EVALUATE_DIFFERENTLY( "Inlining this method will result in a change in evaluation timing for one or more" + " arguments to this method."), - METHOD_CAN_BE_OVERIDDEN_AND_CANT_BE_FIXED( + METHOD_CAN_BE_OVERRIDDEN_AND_CANT_BE_FIXED( "Methods that are inlined should not be overridable, as the implementation of an overriding" + " method may be different than the inlining"), // Technically an error in the case where an existing @InlineMe annotation is applied, but could // be fixed while suggesting - METHOD_CAN_BE_OVERIDDEN_BUT_CAN_BE_FIXED( + METHOD_CAN_BE_OVERRIDDEN_BUT_CAN_BE_FIXED( "Methods that are inlined should not be overridable, as the implementation of an overriding" + " method may be different than the inlining"), VARARGS_USED_UNSAFELY( "When using a varargs parameter, it must only be passed in the last position of a method" + " call to another varargs method"), EMPTY_VOID("InlineMe cannot yet be applied to no-op void methods"), - REUSE_OF_ARGUMENTS("Implementations cannot use an argument more than once:"); + REUSE_OF_ARGUMENTS("Implementations cannot use an argument more than once:"), + PARAM_NAMES_ARE_NAMED_ARGN("Method parameter names cannot match `arg[0-9]+`."); private final @Nullable String errorMessage; @@ -137,6 +138,10 @@ String getErrorMessage() { } } + static boolean matchesArgN(String paramName) { + return paramName.matches("arg[0-9]+"); + } + static InlinabilityResult forMethod(MethodTree tree, VisitorState state) { if (tree.getBody() == null) { return fromError(InlineValidationErrorReason.NO_BODY); @@ -179,6 +184,12 @@ static InlinabilityResult forMethod(MethodTree tree, VisitorState state) { return fromError(InlineValidationErrorReason.VARARGS_USED_UNSAFELY, body); } + for (VarSymbol param : methSymbol.params()) { + if (matchesArgN(param.name.toString())) { + return fromError(InlineValidationErrorReason.PARAM_NAMES_ARE_NAMED_ARGN); + } + } + // TODO(kak): declare a list of all the types we don't want to allow (e.g., ClassTree) and use // contains if (body.toString().contains("{") || body.getKind() == Kind.CONDITIONAL_EXPRESSION) { @@ -209,8 +220,8 @@ static InlinabilityResult forMethod(MethodTree tree, VisitorState state) { // overridden due to having no publicly-accessible constructors. return fromError( methSymbol.isDefault() - ? InlineValidationErrorReason.METHOD_CAN_BE_OVERIDDEN_AND_CANT_BE_FIXED - : InlineValidationErrorReason.METHOD_CAN_BE_OVERIDDEN_BUT_CAN_BE_FIXED, + ? InlineValidationErrorReason.METHOD_CAN_BE_OVERRIDDEN_AND_CANT_BE_FIXED + : InlineValidationErrorReason.METHOD_CAN_BE_OVERRIDDEN_BUT_CAN_BE_FIXED, body); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java index 197b041e34c..6d428bf5fe9 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java @@ -239,7 +239,7 @@ && stringContainsComments(state.getSourceForNode(tree), state.context)) { String varName = varNames.get(i); // If the parameter names are missing (b/365094947), don't perform the inlining. - if (varName.matches("arg[0-9]+")) { + if (InlinabilityResult.matchesArgN(varName)) { return Description.NO_MATCH; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Suggester.java b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Suggester.java index a30c56f2dfd..4b4b703d74a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Suggester.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Suggester.java @@ -86,7 +86,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { InlineMeData.buildExpectedInlineMeAnnotation(state, inlinabilityResult.body()) .buildAnnotation()); if (inlinabilityResult.error() - == InlineValidationErrorReason.METHOD_CAN_BE_OVERIDDEN_BUT_CAN_BE_FIXED) { + == InlineValidationErrorReason.METHOD_CAN_BE_OVERRIDDEN_BUT_CAN_BE_FIXED) { SuggestedFixes.addModifiers(tree, state, Modifier.FINAL).ifPresent(fixBuilder::merge); } return describeMatch(tree, fixBuilder.build()); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/SuggesterTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/SuggesterTest.java index 0d527e65f74..d51b7807614 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/SuggesterTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/SuggesterTest.java @@ -1046,4 +1046,49 @@ public void importStatic_getsIncorrectlySuggestedAsImportsInsteadOfStaticImports "}") .doTest(); } + + @Test + public void noInlineMeSuggestionWhenParameterNamesAreArgN() { + refactoringTestHelper + .addInputLines( + "Client.java", + "public final class Client {", + " @Deprecated", + " public void before(int arg0, int arg1) {", + " after(arg0, arg1);", + " }", + " public void after(int arg0, int arg1) {", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void inlineMeSuggestionWhenParameterNamesAreNotArgN() { + refactoringTestHelper + .addInputLines( + "Client.java", + "public final class Client {", + " @Deprecated", + " public void before(int int0, int int1) {", + " after(int0, int1);", + " }", + " public void after(int int0, int int1) {", + " }", + "}") + .addOutputLines( + "Client.java", + "import com.google.errorprone.annotations.InlineMe;", + "public final class Client {", + " @InlineMe(replacement = \"this.after(int0, int1)\")", + " @Deprecated", + " public void before(int int0, int int1) {", + " after(int0, int1);", + " }", + " public void after(int int0, int int1) {", + " }", + "}") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/ValidatorTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/ValidatorTest.java index 7260f384151..2ff2086fbc7 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/ValidatorTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/ValidatorTest.java @@ -795,6 +795,25 @@ public void customInlineMe() { .doTest(); } + @Test + public void validationFailsWhenParameterNamesAreArgN() { + helper + .addSourceLines( + "Client.java", + "import com.google.errorprone.annotations.InlineMe;", + "public final class Client {", + " @Deprecated", + " @InlineMe(replacement = \"this.after(arg0, arg1)\")", + " // BUG: Diagnostic contains: `arg[0-9]+`", + " public void before(int arg0, int arg1) {", + " after(arg0, arg1);", + " }", + " public void after(int arg0, int arg1) {", + " }", + "}") + .doTest(); + } + private BugCheckerRefactoringTestHelper getHelperInCleanupMode() { return BugCheckerRefactoringTestHelper.newInstance(Validator.class, getClass()) .setArgs("-XepOpt:" + Validator.CLEANUP_INLINE_ME_FLAG + "=true");