Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ImmutablesSortedSetComparator check #102

Merged
merged 15 commits into from
Aug 18, 2022

Conversation

ferdinand-swoboda
Copy link
Contributor

@ferdinand-swoboda ferdinand-swoboda commented May 18, 2022

Ticket

When using Immutables, collections of type ImmutableSortedSet do not default to an empty collection which is different from other collection types such as ImmutableSet.
This unintuitive behaviour has already caused deserialization errors in prod twice.

The new check + patch suggestion addresses this concern.

@ferdinand-swoboda ferdinand-swoboda added help wanted Extra attention is needed WIP Work in progress labels May 18, 2022
@ferdinand-swoboda ferdinand-swoboda self-assigned this May 18, 2022
@ferdinand-swoboda ferdinand-swoboda requested a review from rickie May 18, 2022 13:16
"interface A {",
" // BUG: Diagnostic matches: X",
" ImmutableSortedSet<String> sortedSet();",
" default ImmutableSortedSet<String> defaultSortedSet() {",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in the tests we make sure to use google-java-format with 2 spaces :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to fix this in all test cases :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a formatter for this or and IDE command?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope... --> :cat_typing:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, would be good to add a linter if we care about the indentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this topic: I filed #147.

@ferdinand-swoboda ferdinand-swoboda force-pushed the ferdinand/CORE-97-add-check-immutable-sorted-set branch from 7cd1477 to 59e5ba7 Compare May 31, 2022 17:44
// provide a default implementation.
return buildDescription(tree)
.addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build())
.addFix(SuggestedFix.postfixWith(tree, "return ImmutableSortedSet.of();"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually not quite what I want as it suggests

abstract ImmutableSortedSet<String> sortedSet();return ImmutableSortedSet.of();

instead of

abstract ImmutableSortedSet<String> sortedSet() {
     return ImmutableSortedSet.of();
}

However, the intuitive solution

Suggested change
.addFix(SuggestedFix.postfixWith(tree, "return ImmutableSortedSet.of();"))
.addFix(SuggestedFix.builder()
.replace(tree.getBody(), "return ImmutableSortedSet.of();")
.build())

fails because tree.getBody() is null. How can I suggest a method body?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first solution that comes to mind (probably there is a more elegant one) is to use another overload of this method:
public static SuggestedFix replace(int startPos, int endPos, String replaceWith) { we can get start and endposition, like we do in #43. However I'm not yet sure if that is the desired solution.

Copy link
Member

@rickie rickie Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASTHelpers#getStartPosition(Tree) and VisitorState#getEndPosition(Tree).

I'll try to do some extra digging, but for now this seems the best approach... Couldn't yet find any better ways of doing this in google/error-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed something but it doesn't work very nicely for abstract/default methods. If there's something better please let me know.

IMO making this a perfect replacement is not worth it because it's the second suggestion and any developer would know how to adapt the existing second suggestion so that it passes the compiler stage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see the second replacement currently is a bit tricky.

What we could also consider is something like we do for IdentityConversionCheck.
There we set a custom message for the description (this means it'll not take the text from the summary field anymore), that is shown when a violation is detected. We could use that text + the first suggested fix to explain to the user what is wrong and let it write the other option out by him/her self.

Providing a non-perfect replacement is not an option. We either make it so that it is a perfect replacement, or we omit a normal message explaining what is wrong and what needs to be done.

@ferdinand-swoboda ferdinand-swoboda requested a review from rickie June 1, 2022 10:04
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not fully review yet, but already flusing some comments.

"interface A {",
" // BUG: Diagnostic matches: X",
" ImmutableSortedSet<String> sortedSet();",
" default ImmutableSortedSet<String> defaultSortedSet() {",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to fix this in all test cases :)

@ferdinand-swoboda ferdinand-swoboda requested a review from rickie June 14, 2022 09:27
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made quite some changes, PTAL.
Note that I'm far from happy with the names I used for the Matchers, but I extracted them anyway to show what could be a nice setup of the BugPattern, let me know what you think of it 😄. Open to feedback and improvements.

I did not look at the replacement logic yet.

Will respond to the last remark there to explain a different way in which we can solve this.

"interface A {",
" // BUG: Diagnostic matches: X",
" ImmutableSortedSet<String> sortedSet();",
" default ImmutableSortedSet<String> defaultSortedSet() {",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;

public final class MissingImmutableSortedSetDefaultCheckTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know others are not all like that, but it can be package private.

// is not within immutable or modifiable class -> no match
if (enclosingClass(
allOf(
not(hasAnnotation(org.immutables.value.Value.Immutable.class)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI showed we can import this one. We don't need them fully qualified 😉.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Flushing other comment)

// provide a default implementation.
return buildDescription(tree)
.addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build())
.addFix(SuggestedFix.postfixWith(tree, "return ImmutableSortedSet.of();"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see the second replacement currently is a bit tricky.

What we could also consider is something like we do for IdentityConversionCheck.
There we set a custom message for the description (this means it'll not take the text from the summary field anymore), that is shown when a violation is detected. We could use that text + the first suggested fix to explain to the user what is wrong and let it write the other option out by him/her self.

Providing a non-perfect replacement is not an option. We either make it so that it is a perfect replacement, or we omit a normal message explaining what is wrong and what needs to be done.

@rickie rickie changed the title CORE-97 Add MissingImmutableSortedSetDefaultCheck Introduce MissingImmutableSortedSetDefaultCheck Jun 14, 2022
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still had some time left to dive into the tests :).

}

@Test
void replacementInImmutableInterface() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the identification test we have a nice and extensive suite that shows when the check flags violations.

I don't think we need the non-matching examples in all the replacement tests, because we already verified in the identification test that nothing is happening with these methods.

If we clean up the tests to only verify the thing they need to do, we can end up with something like this:

  @Test
  void replacementInImmutableInterface() {
    refactoringTestHelper
        .addInputLines(
            "A.java",
            "import org.immutables.value.Value;",
            "import com.google.common.collect.ImmutableSortedSet;",
            "",
            "@Value.Immutable",
            "interface A {",
            "  ImmutableSortedSet<String> sortedSet();",
            "}")
        .addOutputLines(
            "A.java",
            "import org.immutables.value.Value;",
            "import com.google.common.collect.ImmutableSortedSet;",
            "",
            "@Value.Immutable",
            "interface A {",
            "  @Value.NaturalOrder",
            "  ImmutableSortedSet<String> sortedSet();",
            "}")
        .doTest(TestMode.TEXT_MATCH);
  }

If we would do that for all tests, it would be easier to grasp the tests and see what we are actually testing per test.

Perhaps we can even take it further and combine the three examples in one code example and call the test method replacement.
It would have an abstract class with the two variants of the @Value.Immutable and @Value.Modifiable.
Or we have two test cases, an (modifiable) abstract class with the modifiable interface and the other test has the other two cases.
Didn't write that out to see what is the nicest solution there.

I tried writing down my thought process, is it somewhat clear what I'm trying to show/say 😄. If not, please let me know and we can have a call about it :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to condense the replacement tests into one.
There are a few conditions for a "hit" whereas the suggested fix is very simple so to me it makes sense to have separate tests for identification & replacement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now there are too few tests 😅 . I didn't mean to remove all tests, just to combine all the different cases in one replacement test. The fact that we had a failing test was "good" as it showed a situation that we didn't fix yet and now that is not covered.

I liked that the tests where exhaustive, now it tests only one situation.

Let's discuss today 😄.

@ferdinand-swoboda ferdinand-swoboda removed WIP Work in progress help wanted Extra attention is needed labels Jun 14, 2022
@ferdinand-swoboda ferdinand-swoboda force-pushed the ferdinand/CORE-97-add-check-immutable-sorted-set branch 2 times, most recently from 8316c1a to 98ce3f7 Compare June 15, 2022 13:19
@ferdinand-swoboda ferdinand-swoboda requested a review from rickie June 15, 2022 13:34
@ferdinand-swoboda
Copy link
Contributor Author

@rickie With the latest changes, can we merge this?

@rickie
Copy link
Member

rickie commented Jun 22, 2022

@ferdinand-swoboda I'm reviewing as we speak. We cannot simply merge it as I'm the first to review this now 😄. TBD when we can merge this.

@Stephan202 Stephan202 force-pushed the ferdinand/CORE-97-add-check-immutable-sorted-set branch from 98ce3f7 to 5f9015c Compare June 26, 2022 10:41
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and added a commit. So far I only reviewed the pom.xml files; the rest is TBD.

Comment on lines 139 to 150
<dependency>
<groupId>org.immutables</groupId>
<artifactId>value</artifactId>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only use value-annotations, so we can reference the smaller artifact. Additionally, this project has rules for many third-party libraries, and we try to avoid a runtime classpath explosion. For this reason we declare them test-scoped (or provided-scoped in case a Refaster rule must reference them) and make sure that the code also works in the relevant dependency isn't on the classpath.

Suggested change
<dependency>
<groupId>org.immutables</groupId>
<artifactId>value</artifactId>
</dependency>
<dependency>
<groupId>org.immutables</groupId>
<artifactId>value-annotations</artifactId>
<scope>test</scope>
</dependency>

implements MethodTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Matcher<Tree> ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES =
enclosingClass(anyOf(hasAnnotation(Immutable.class), hasAnnotation(Modifiable.class)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above: in cases like these we prefer to reference the FQCN as a string, so that the checker is simply a no-op in case Immutables isn't on the classpath.

pom.xml Outdated
<dependency>
<groupId>org.immutables</groupId>
<artifactId>value</artifactId>
<version>2.8.2</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<version>2.8.2</version>
<version>2.9.0</version>

(Internally we still use 2.8.8 due to immutables/immutables#1355, but that issue doesn't impact this project, so by going for the latest release we avoid a Renovate PR.)

@rickie rickie force-pushed the ferdinand/CORE-97-add-check-immutable-sorted-set branch from 5f9015c to 1d81508 Compare June 29, 2022 15:46
@Stephan202 Stephan202 force-pushed the ferdinand/CORE-97-add-check-immutable-sorted-set branch from 1d81508 to 7ba62d5 Compare June 30, 2022 12:10
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some changes, PTAL, curious to hear opinions :).

Admittedly, I'm still not happy with the messages. I feel they can be more concise (especially when comparing them with other BugPatterns). On the other hand, it is quite a specific check so I'm not able to do better. So, I'll leave that to @Stephan202 now 😬.

}

@Test
void noReplacementInNormalClass() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can omit this test, as this is already covered in the identification test.

return Description.NO_MATCH;
}

// The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's improve the code such that we don't need this comment :)

return buildDescription(tree)
.setMessage(
"Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class "
+ "should include additional deserialization information for absent sets. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ "should include additional deserialization information for absent sets. "
+ "should include deserialization information for absent sets. "

? Perhaps?

"Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class "
+ "should include additional deserialization information for absent sets. "
+ "Alternatively, specify a default implementation.")
.addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people would use the annotations like this, the BugPattern would result in non-compilable code as the import org.immutables.value.Value would not be present. Therefore we have to add the import.

*/
@AutoService(BugChecker.class)
@BugPattern(
name = "MissingImmutableSortedSetDefault",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time determining what would be the perfect name for the check. As it is now, it is not 100% in line with the other checks.
I'd say that ideally it should be named ImmutablesImmutableSortedSetUsageUsageCheck, but that's not so nice as it uses Immutable twice. So I'm leaning towards Immutable(s)SortedSetUsageCheck. With the s being optional. I know both options do not really convey what we are trying to improve in this check.

What do other reviewers think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either way one has to read the description to understand the check's intention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it's quite a specific check. 🤔

I'd say based on the AssertJIsNull check, we could also say ImmutablesImmutableSortedSet check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: ImmutablesSortedSetComparator.

"",
"abstract class E {",
" abstract ImmutableSortedSet<String> sortedSet();",
" ImmutableSortedSet<String> defaultSortedSet() {",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the defaultSortedSet{,2,3} are not matching for the former cases, it's safe to omit them here. We want to validate that the things that do match, in a "normal" context do not match.

" ImmutableSortedSet<String> defaultSortedSet() {",
" return ImmutableSortedSet.of();",
" }",
" @Value.Default",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I realize I'm not 100% consistent on this one (I think)) but because we are not actually doing anything with the @Value.Default in this BugPattern I'd argue it's a lot cleaner to omit this from the tests.

OTOH I can see why it is good to know it doesn't flag these cases. If someone else thinks we should show this, I'd like to ask the question; should we add it to all cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps only in the first, interface A?

import com.sun.source.tree.Tree;

/**
* A {@link BugChecker} which flags methods with return type {@link
Copy link
Member

@rickie rickie Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferdinand-swoboda I already added a suggestion here. Could you go over it and improve / tweak it further :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a pass ;)

return buildDescription(tree)
.setMessage(
"Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class "
+ "should include additional deserialization information for absent sets. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the deserialization information for absent sets is something that can be omitted from this message an perhaps be moved to the Javadoc. Making a proposal. PTAL and make it more specific / correct if possible @ferdinand-swoboda.

/**
* A {@link BugChecker} which flags methods with return type {@link
* com.google.common.collect.ImmutableSortedSet} within an {@code @Value.Immutable} or
* {@code @Value.Modifiable} class that lack either a default implementation or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only classes, it does more than that 😉.

@ferdinand-swoboda
Copy link
Contributor Author

2. Look into support for the "org.springframework.beans.factory.annotation.Value case".

Is this a common enough case?

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Look into support for the "org.springframework.beans.factory.annotation.Value case".

Is this a common enough case?

In some repositories: yep. I found 48 occurrences of @org.immutables.value.Value. (As we migrate those cases to records the number will decrease, but still.)

anyOf(
hasAnnotation("org.immutables.value.Value.Immutable"),
hasAnnotation("org.immutables.value.Value.Modifiable")))),
hasAnnotation("org.immutables.value.Value.Default")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't check, but suspect not. Still my reasoning was that "if it's present, then so should the other annotation".

(If indeed @Value.Foo annotations are a no-op absent @Value.{Immutable,Modifiable}, then we could also add a checker for that 😄)

@Stephan202
Copy link
Member

(To be sure: I plan to cycle back to this PR at least for point (1); might take a few days, though.)

@Stephan202 Stephan202 added this to the 0.1.1 milestone Aug 18, 2022
@Stephan202 Stephan202 force-pushed the ferdinand/CORE-97-add-check-immutable-sorted-set branch from 751b70d to 42c5bb7 Compare August 18, 2022 07:46
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and added a commit (handling an import clash, increasing test coverage). That's it from my side!

Suggested commit message:

Introduce `ImmutablesSortedSetComparator` check (#102)

@rickie "everything" changed since you approved. Might want to re-review 🙃

@rickie rickie self-requested a review August 18, 2022 07:50
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! Very nice check and extensive tests.

Made one tiny tweak to the message of the BugPattern.

@BugPattern(
summary =
"`SortedSet` properties of a `@Value.Immutable` or `@Value.Modifiable` type must be "
+ "annotated with `@Value.NaturalOrder` or `@Value.ReverseOrder`",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually left out the "with" on purpose. But can live with it 🥁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on purpose.

This is not a surprise 😆.

But can live with it.

🥁 🥁 🥁

@ferdinand-swoboda ferdinand-swoboda changed the title Introduce MissingImmutableSortedSetDefaultCheck Introduce ImmutablesSortedSetComparator check Aug 18, 2022
@rickie rickie merged commit 7883b31 into master Aug 18, 2022
@rickie rickie deleted the ferdinand/CORE-97-add-check-immutable-sorted-set branch August 18, 2022 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants