Introduce a check to simplify time expressions in annotations #17

nathankooij · 2021-09-30T15:33:21Z

I wish to expand the test suite further, but it should already give a good indication of what is possible with this check.

A few things:

I am not entirely certain if what I'm doing in getTimeUnit and getDefaultTimeUnit is always safe.
We can extract some of the things to utilities which will a) make this code cleaner b) allow us to re-use it in other checks, some things:
- Returning a map of attributes -> expression (or assignment trees if that's always the tree) by the matcher;
- Extracting enum value from an expression;
- Extracting default attributes on annotations;
- An "annotation builder" to avoid special casing the implict value attribute.

I know @Stephan202 mentioned he has some of these on a branch already, so curious to see what we can already do out of the box. :)

nathankooij · 2021-09-30T15:45:40Z

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

+    MethodSymbol argumentSymbol =
+        (MethodSymbol)
+            Iterables.getOnlyElement(
+                scope.getSymbols(symbol -> symbol.getQualifiedName().contentEquals(argument)));
+    return (VarSymbol) argumentSymbol.getDefaultValue().getValue();


This is perhaps dodgy (and NullAway complains about #getDefaultValue here, although the Iterables#getOnlyElement could be just as problematic), but I assumed that the AnnotationDescriptor is correct (and thus the attribute must exist). Maybe we should just return an Optional<VarSymbol> instead, and short circuit in trySimplification if we couldn't find a time unit for whatever reason. WDYT?

I think returning the Optional with the short circuit is a clean solution and makes NullAway happy :). So sounds good to me.

nathankooij · 2021-09-30T15:46:31Z

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

+
+  private static Fix getImplicitValueAttributeFix(
+      AnnotationTree annotation, Number newValue, String timeUnitField, TimeUnit newTimeUnit) {
+    @SuppressWarnings("TreeToString")


Because of annotation#toString below, but SuggestedFixes#updateAnnotationArgumentValues does this too. 🤔

I pushed something that we can do specifically in Error Prone Support because Stephan added a Util.

nathankooij · 2021-09-30T15:47:43Z

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

+  }
+
+  private static Optional<TimeSimplifier.Simplification> trySimplify(Number value, TimeUnit unit) {
+    checkArgument(


Not sure how defensive we want to be here, or if we should prefer to return empty and avoid exceptions.

I'm also not sure, no preference here.

rickie

This is a really nice BugPattern 🚀

rickie · 2021-10-01T13:18:33Z

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

+    tags = BugPattern.StandardTags.SIMPLIFICATION)
+public final class SimplifyTimeAnnotationCheck extends BugChecker implements AnnotationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final AnnotationAttributeMatcher ARGUMENT_SELECTOR = getMatcher();


We can use a more descriptive name here instead of getMatcher. It is not directly clear what the matcher does.

rickie · 2021-10-01T13:19:07Z

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

+  private static final AnnotationAttributeMatcher ARGUMENT_SELECTOR = getMatcher();
+
+  @Override
+  public Description matchAnnotation(AnnotationTree annotationTree, VisitorState visitorState) {


Suggested change

public Description matchAnnotation(AnnotationTree annotationTree, VisitorState visitorState) {

public Description matchAnnotation(AnnotationTree annotationTree, VisitorState state) {

We typically use state instead of visitorState.

rickie · 2021-10-01T14:00:12Z

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

+       * in the given {@code unit}.
+       */
+      public long toUnit(TimeUnit unit) {
+        return unit.convert(value, this.unit);


Because of the this.unit I got a HidingField violation. Therefore renamed the variable.

rickie · 2021-10-01T14:02:44Z

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

+  private static Optional<TimeSimplifier.Simplification> trySimplify(Number value, TimeUnit unit) {
+    checkArgument(
+        value instanceof Integer || value instanceof Long,
+        "Only time expressed as a long or integer can be simplified");


Turned these two around to be in line with the order above this line.

rickie · 2021-10-01T14:04:26Z

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

+
+  private static Fix getImplicitValueAttributeFix(
+      AnnotationTree annotation, Number newValue, String timeUnitField, TimeUnit newTimeUnit) {
+    @SuppressWarnings("TreeToString")


I pushed something that we can do specifically in Error Prone Support because Stephan added a Util.

rickie · 2021-10-01T14:08:33Z

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

+  }
+
+  private static Optional<TimeSimplifier.Simplification> trySimplify(Number value, TimeUnit unit) {
+    checkArgument(


I'm also not sure, no preference here.

rickie · 2021-10-01T14:10:13Z

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

+    MethodSymbol argumentSymbol =
+        (MethodSymbol)
+            Iterables.getOnlyElement(
+                scope.getSymbols(symbol -> symbol.getQualifiedName().contentEquals(argument)));
+    return (VarSymbol) argumentSymbol.getDefaultValue().getValue();


I think returning the Optional with the short circuit is a clean solution and makes NullAway happy :). So sounds good to me.

rickie · 2021-10-01T14:17:27Z

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

+        .orElse(Description.NO_MATCH);
+  }
+
+  private static Optional<Fix> trySimplification(


trySimplification in itself is not too descriptive. It makes sense because it is in this particular class, but I would prefer something like the name simplifyAttributeValue which is used in the CanonicalAnnotationSyntaxCheck.

Especially because this file has now a method with the name trySimplificiation and 2 methods with trySimplify. I know one is strictly speaking in another class (the TimeSimplifier) but that's why I think changing the naming could make things a little more clear.

I want to remark the same for getValue since it has two arguments which makes it harder to directly see what is going on there. Changing the name could help there.

Edit:
Hmm, I see that SpringMvcAnnotationCheck does have the same setup as here.. So I get where you are coming from. Although I do think it would be an improvement 😄 .

rickie · 2021-10-01T14:41:11Z

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

+import java.util.stream.Stream;
+
+/**
+ * A {@link BugChecker} which flags methods annotated with annotations described by {@link


Suggested change

* A {@link BugChecker} which flags methods annotated with annotations described by {@link

* A {@link BugChecker} which flags methods annotated with annotations described by the {@link

👀

I don't think adding "the" is better. That said, will propose a more extensive rephrasing 👇

Stephan202

Added a small commit and did a partial review. More later/another time :)

Stephan202 · 2021-10-10T13:20:28Z

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

+     * order.
+     */
+    private static ImmutableSortedSet<TimeUnit> descendingLargerUnits(TimeUnit unit) {
+      return ImmutableSortedSet.copyOf(TimeUnit.values())


We can extract this one to a constant.

Stephan202 · 2021-10-10T13:21:28Z

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

+  }
+
+  /** Utility class to help simplify time expressions. */
+  private static final class TimeSimplifier {


This class name hints more at it being a type with instance methods. I think we can move the static methods to the Simplification type.

Stephan202 · 2021-10-10T13:23:01Z

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

+/**
+ * A {@link BugChecker} which flags methods annotated with annotations described by {@link
+ * AnnotationDescriptor} that can be written more concisely.
+ */


Suggested change

/**

* A {@link BugChecker} which flags methods annotated with annotations described by {@link

* AnnotationDescriptor} that can be written more concisely.

*/

/**

* A {@link BugChecker} which flags annotations with time attributes that can be written more

* concisely.

*/

Stephan202 · 2021-10-10T13:23:29Z

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

+import java.util.stream.Stream;
+
+/**
+ * A {@link BugChecker} which flags methods annotated with annotations described by {@link


I don't think adding "the" is better. That said, will propose a more extensive rephrasing 👇

nathankooij · 2021-10-11T09:21:53Z

Thx for the feedback guys! I'll try to circle back to this PR somewhere this week. :)

Stephan202

Rebased, resolved conflicts and added a commit. (Won't do a full review now, but saw that I had the branch checked out, and was wondering about its state.)

Stephan202 · 2022-01-03T06:07:39Z

...e-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SpringMvcAnnotationCheckTest.java

@@ -43,7 +42,7 @@ void identification() {
            "  // BUG: Diagnostic contains:",
            "  @RequestMapping(method = {DELETE}) A delete();",
            "  // BUG: Diagnostic contains:",
-            "  @RequestMapping(method = {PATCH}) A patch();",
+            "  @RequestMapping(method = {org.springframework.web.bind.annotation.RequestMethod.PATCH}) A patch();",


👀 Unrelated change?

(Not a bad one, just surprising.)

rickie · 2023-08-24T11:53:47Z

I encountered the CanonicalDuration BugPattern today. I made me think of this check, maybe there is some logic there that we can (re-)use?

Stephan202 · 2023-08-26T07:22:58Z

Yep, likely! Too bad it's all private, so that would entail some copying. Alternatively we could try to contribute improvements upstream, possibly by extracting shared logic to a separate class. (For this annotation case I'm not sure that a banlist applies.) Would then first open an issue to discuss that possibility, though.

Thx for the feedback guys! I'll try to circle back to this PR somewhere this week. :)

I suppose that @nathankooij currently resides in the vicinity of a black hole ;)

nathankooij requested review from Stephan202 and rickie September 30, 2021 15:33

nathankooij commented Sep 30, 2021

View reviewed changes

rickie reviewed Oct 1, 2021

View reviewed changes

Stephan202 reviewed Oct 10, 2021

View reviewed changes

rickie added the WIP Work in progress label Feb 19, 2022

nathankooij and others added 15 commits November 5, 2022 16:52

Extract TimeUnit, Number value, and determine a simplification

4830b5b

Suggest a fix

dd8d094

Notes and random things

79768a2

Some cleanup of simplifying code

63273a2

Support multiple attributes per annotation

89a3c60

Handle implicit value attribute case

226bfd0

Support banned fields

1348950

Assorted cleanup

cede5e4

Use a sorted set to derive the ceiling

0af1276

Fix most warnings

ad8c0a4

Simplify by only working with longs

85f402b

Suggestions

ba91c6b

Remove the TEXT_MATCH param from doTest

d88f226

Tweaks

389928c

Post rebase fixes

8ced020

Stephan202 force-pushed the nkooij/time-unit-annotations branch from 8baa0c9 to 8ced020 Compare November 5, 2022 16:25

Stephan202 reviewed Nov 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a check to simplify time expressions in annotations #17

Introduce a check to simplify time expressions in annotations #17

nathankooij commented Sep 30, 2021

nathankooij Sep 30, 2021

rickie Oct 1, 2021

nathankooij Sep 30, 2021

rickie Oct 1, 2021

nathankooij Sep 30, 2021

rickie Oct 1, 2021

rickie left a comment

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

rickie Oct 1, 2021

Stephan202 Oct 10, 2021

Stephan202 left a comment

Stephan202 Oct 10, 2021

Stephan202 Oct 10, 2021

Stephan202 Oct 10, 2021

Stephan202 Oct 10, 2021

nathankooij commented Oct 11, 2021

Stephan202 left a comment

Stephan202 Jan 3, 2022

rickie commented Aug 24, 2023

Stephan202 commented Aug 26, 2023

	public Description matchAnnotation(AnnotationTree annotationTree, VisitorState visitorState) {
	public Description matchAnnotation(AnnotationTree annotationTree, VisitorState state) {

	* A {@link BugChecker} which flags methods annotated with annotations described by {@link
	* A {@link BugChecker} which flags methods annotated with annotations described by the {@link

Introduce a check to simplify time expressions in annotations #17

Are you sure you want to change the base?

Introduce a check to simplify time expressions in annotations #17

Conversation

nathankooij commented Sep 30, 2021

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 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

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

nathankooij commented Oct 11, 2021

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Aug 24, 2023

Stephan202 commented Aug 26, 2023