`JUnitMethodDeclarationCheck`: emit warning instead of `SuggestedFix` if method name clashes #35

rickie · 2022-01-24T11:55:28Z

No description provided.

werli

Frankly, these suggestions are only on a syntactical level, and no more.

Actual functionality looks to be covered by tests quite well 🙂

werli · 2022-01-24T19:51:31Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

-      // XXX: In theory this rename could clash with an existing method or static import. In that
-      // case we should emit a warning without a suggested replacement.
-      tryCanonicalizeMethodName(tree, state).ifPresent(builder::merge);
+      String newMethodName = tryCanonicalizeMethodName(tree).orElse("");


Seeing how we use the newMethodName, we should not fall back to an empty string only to check whether the string is empty. Why can't we use an empty optional?

werli · 2022-01-24T19:54:39Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+    return state.findEnclosing(ClassTree.class).getMembers().stream()
+        .filter(member -> !ASTHelpers.isGeneratedConstructor((MethodTree) member))
+        .map(MethodTree.class::cast)
+        .map(MethodTree::getName)
+        .map(Name::toString)
+        .anyMatch(n -> n.equals(methodName));


(1) The casting without check looks off.
(2) anyMatch(methodName::equals)

return state.findEnclosing(ClassTree.class).getMembers().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) .filter(member -> !ASTHelpers.isGeneratedConstructor(member)) .map(MethodTree::getName) .map(Name::toString) .anyMatch(methodName::equals);

werli · 2022-01-24T19:55:15Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+
+    return compilationUnit.getImports().stream()
+        .filter(Objects::nonNull)
+        .map(ImportTree.class::cast)


Looks like we don't need this cast?

werli · 2022-01-24T19:56:46Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+  private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) {
+    String source = Util.treeToString(tree, state);
+    int lastDot = source.lastIndexOf('.');
+    return lastDot < 0 ? source : source.subSequence(lastDot + 1, source.length());
+  }


This looks 10/10 hacky, there must be a different way for this 🤔

I reused logic from RefasterCheck but I'll try to improve. 10/10 is a good score though 😉.

Just realised we can omit the hacky check. This should be enough right? WDYT? @werli

rickie · 2022-01-25T07:28:32Z

Frankly, these suggestions are only on a syntactical level, and no more.

This is exactly what this PR needed, so thanks for that. Not sure when I'll go over the suggestions yet.

rickie · 2022-01-26T12:47:11Z

Thanks for the changes btw, they look good 🚀 !

werli

Applied my solely syntactical suggestions.
Semantically I cannot contribute meaningful, sorry!

werli · 2022-02-15T12:31:55Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+    return state.findEnclosing(ClassTree.class).getMembers().stream()
+        .filter(MethodTree.class::isInstance)
+        .map(MethodTree.class::cast)
+        .filter(member -> !ASTHelpers.isGeneratedConstructor(member))


Should do the same thing.

Suggested change

.filter(member -> !ASTHelpers.isGeneratedConstructor(member))

.filter(not(ASTHelpers::isGeneratedConstructor))

werli · 2022-02-15T12:40:15Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+      Optional<String> newMethodName = tryCanonicalizeMethodName(tree);
+
+      if (newMethodName.isEmpty()) {
+        return describeMatchesIfPresent(tree, builder);
+      }
+
+      boolean reportedNameClash =
+          reportDescriptionForPossibleNameClash(tree, newMethodName.orElseThrow(), state);
+      if (!reportedNameClash) {
+        builder.merge(SuggestedFixes.renameMethod(tree, newMethodName.orElseThrow(), state));
+      }
    }
+    return describeMatchesIfPresent(tree, builder);
+  }

+  private Description describeMatchesIfPresent(MethodTree tree, SuggestedFix.Builder builder) {
    return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build());


I feel like we can utilize the Optional API a little more 👀

if (isTestMethod) { tryCanonicalizeMethodName(tree) .filter(methodName -> !reportDescriptionForPossibleNameClash(tree, methodName, state)) .ifPresent( methodName -> builder.merge(SuggestedFixes.renameMethod(tree, methodName, state))); } return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build());

Really nice, thanks, this makes it a lot better. Will definitely look into utilizing that API better next time 😄!

Stephan202

Rebased and added a commit. So far only reviewed the JavaKeywords class; remainder TBD.

Stephan202 · 2022-04-09T13:07:25Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java

+
+import com.google.common.collect.ImmutableSet;
+
+@SuppressWarnings("DeclarationOrder" /* The private constructor should come first. */)


Nope, fields go before the constructor.

Suggested change

@SuppressWarnings("DeclarationOrder" /* The private constructor should come first. */)

Stephan202 · 2022-04-09T13:11:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java

+   * <p>See: the <a
+   * href="https://docs.oracle.com/javase/tutorial/java/nutsandbolts/_keywords.html">Oracle
+   * Documentation</a> on Java Keywords.
+   *
+   * <p>In addition, `sealed` is added for Java 17. See: <a href="openjdk.net/jeps/409">JEP-409</a>.


For this kind of thing we should always link to the source of truth, which is the JLS.

This is what the Javadoc @see param is for :)

Stephan202 · 2022-04-09T13:18:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java

+   */
+  private static final ImmutableSet<String> JAVA_KEYWORDS =
+      ImmutableSet.of(
+          "assert",


We're missing abstract and _ 👀

Stephan202 · 2022-04-09T13:24:27Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java

+          "protected",
+          "public",
+          "return",
+          "sealed",


sealed is a contextual keyword and can be used as a method name. We can debate whether one should use it as such, but it is allowed.

Stephan202

Added one more commit. There are ~1-2 open points.

Suggested commit message:

`JUnitMethodDeclarationCheck`: better handle possibly-problematic method renames (#35)

By flagging affected test methods, but not suggesting a specific rename.

Stephan202 · 2022-04-09T16:22:32Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+  private void reportIncorrectMethodName(
+      String methodName, MethodTree tree, String message, VisitorState state) {
+    state.reportMatch(
+        buildDescription(tree).setMessage(String.format(message, methodName)).build());
+  }


This logic isn't tested

Stephan202 · 2022-04-09T16:27:35Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+    if (isMethodNameInClass(methodName, state)) {
+      reportIncorrectMethodName(
+          methodName, tree, "A method with name %s already exists in the class.", state);
+      return false;
+    }
+
+    if (isMethodNameStaticallyImported(methodName, state)) {
+      reportIncorrectMethodName(
+          methodName, tree, "A method with name %s is already statically imported.", state);
+      return false;
+    }


These heuristic aren't perfect:

The existing method may have a different signature, such that renaming the test method will merely produce an overload.

The code currently doesn't test superclass method declarations, while renaming could cause a method to be overridden.

The test method could be scoped such that upon renaming it does not conflict with any usages of the static import.

Not saying we should spend effort on improving this logic, but at the very least we should call it out.

Stephan202 · 2022-04-09T16:29:05Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+      reportIncorrectMethodName(
+          methodName,
+          tree,
+          "Method name `%s` is not possible because it is a Java keyword.",


We generally omit dots from these messages.

Stephan202 · 2022-04-09T16:29:10Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+      reportIncorrectMethodName(
+          methodName,
+          tree,
+          "Method name `%s` is not possible because it is a Java keyword.",


We generally omit dots from these messages.

Stephan202 · 2022-04-09T16:29:40Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+  private void reportIncorrectMethodName(
+      String methodName, MethodTree tree, String message, VisitorState state) {
+    state.reportMatch(
+        buildDescription(tree).setMessage(String.format(message, methodName)).build());
+  }


As-is Error Prone cannot prove that message has exactly one placeholder. With a bit of extra verbosity we can convert this into a proper @FormatMethod.

Edit: but with even more refactoring the problem goes away again ;)

Oh didn't know this existed 😅, nice one.

Stephan202 · 2022-04-10T07:28:04Z

...ontrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java

+            "  @Test void testBar() {}",
+            "  private void bar() {}",


It seems that in none of the new cases we verify that, while we don't auto-rename, we do auto-fix the method's modifiers.

Stephan202 · 2022-04-10T08:06:25Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

    return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build());
  }

-  private static Optional<SuggestedFix> tryCanonicalizeMethodName(
-      MethodTree tree, VisitorState state) {
+  private boolean isValidMethodName(MethodTree tree, String methodName, VisitorState state) {


This method signature hints at a pure method, while actually it may report an error as a side-effect. We should refactor the code to avoid this pattern.

Stephan202 · 2022-04-10T08:23:28Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+        .anyMatch(methodName::contentEquals);
+  }
+
+  private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) {


The whole thing is also an identifier.

Suggested change

private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) {

private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) {

Stephan202 · 2022-04-10T08:24:20Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+  }
+
+  private static boolean isMethodNameStaticallyImported(String methodName, VisitorState state) {
+    CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();


We can inline this one.

Stephan202 · 2022-04-10T08:34:26Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java

+        buildDescription(tree).setMessage(String.format(message, methodName)).build());
+  }
+
+  private static boolean isMethodNameInClass(String methodName, VisitorState state) {


Suggested change

private static boolean isMethodNameInClass(String methodName, VisitorState state) {

private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {

…shes

rickie · 2022-04-12T15:06:54Z

Looked into retrieving the inherited members but decided to defer it for now. It requires some more work to get it in a nice shape such that it would fit in this PR.

So with that, we addressed all open points for this PR, right 😄?

Stephan202

It does!

rickie requested review from Stephan202 and werli January 24, 2022 11:55

werli reviewed Jan 24, 2022

View reviewed changes

rickie requested a review from werli January 26, 2022 12:47

werli approved these changes Feb 15, 2022

View reviewed changes

rickie force-pushed the rossendrijver/junit_method_already_exists branch 2 times, most recently from 6209354 to 44d4179 Compare February 16, 2022 10:46

rickie added this to the 0.1.0 milestone Apr 4, 2022

Stephan202 force-pushed the rossendrijver/junit_method_already_exists branch from dfcf1ce to 9176a57 Compare April 9, 2022 13:29

Stephan202 reviewed Apr 9, 2022

View reviewed changes

Stephan202 reviewed Apr 10, 2022

View reviewed changes

rickie and others added 8 commits April 12, 2022 15:02

JUnitMethodDecl: emit warning instead of SuggestedFix if name cla…

56ac6ad

…shes

Suggestions

302fb01

Tweaks

610cce7

Suggestions

8eed4f3

JUnitMethodDeclaration: Prevent renaming methods to Java keywords

41d9692

JavaKeywords: Add sealed

744b41a

Review JavaKeywords

f2e1c73

Suggestions

c27c0ff

rickie force-pushed the rossendrijver/junit_method_already_exists branch from 5a4a644 to c27c0ff Compare April 12, 2022 13:03

Remove unnecessary filter on generated constructor

d9f283c

Stephan202 approved these changes Apr 12, 2022

View reviewed changes

Stephan202 merged commit e859c27 into master Apr 12, 2022

Stephan202 deleted the rossendrijver/junit_method_already_exists branch April 12, 2022 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`JUnitMethodDeclarationCheck`: emit warning instead of `SuggestedFix` if method name clashes #35

`JUnitMethodDeclarationCheck`: emit warning instead of `SuggestedFix` if method name clashes #35

rickie commented Jan 24, 2022

werli left a comment

werli Jan 24, 2022

werli Jan 24, 2022

werli Jan 24, 2022

werli Jan 24, 2022

rickie Jan 25, 2022

rickie Jan 26, 2022 •

edited

Loading

rickie commented Jan 25, 2022

rickie commented Jan 26, 2022

werli left a comment

werli Feb 15, 2022

werli Feb 15, 2022

rickie Feb 15, 2022

Stephan202 left a comment

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 left a comment

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

rickie Apr 12, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

rickie commented Apr 12, 2022

Stephan202 left a comment

	.filter(member -> !ASTHelpers.isGeneratedConstructor(member))
	.filter(not(ASTHelpers::isGeneratedConstructor))


		import com.google.common.collect.ImmutableSet;

		@SuppressWarnings("DeclarationOrder" /* The private constructor should come first. */)

	private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) {
	private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) {

	private static boolean isMethodNameInClass(String methodName, VisitorState state) {
	private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {

JUnitMethodDeclarationCheck: emit warning instead of SuggestedFix if method name clashes #35

JUnitMethodDeclarationCheck: emit warning instead of SuggestedFix if method name clashes #35

Conversation

rickie commented Jan 24, 2022

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

rickie commented Jan 25, 2022

rickie commented Jan 26, 2022

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Apr 12, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

`JUnitMethodDeclarationCheck`: emit warning instead of `SuggestedFix` if method name clashes #35

`JUnitMethodDeclarationCheck`: emit warning instead of `SuggestedFix` if method name clashes #35

rickie Jan 26, 2022 •

edited

Loading