When variables are assigned then returned, instead directly return them

[SimonBaars]

Was feeling adventurous ;-)

[Stephan202]

The build fails because tests are missing. See RefasterCheckTest#TEMPLATE_GROUPS and the input/output files under src/test/resources/tech/picnic/errorprone/bugpatterns.

Really cool :)

[SimonBaars]

I created tests, not sure why build still fails:

[ERROR] Failed to execute goal on project refaster-resource-compiler: Could not resolve dependencies for project tech.picnic.error-prone-support:refaster-resource-compiler:jar:0.0.1-SNAPSHOT: The following artifacts could not be resolved: com.github.PicnicSupermarket.error-prone:error_prone_check_api:jar:v2.3.4-picnic-2, com.github.PicnicSupermarket.error-prone:error_prone_core:jar:v2.3.4-picnic-2: Failure to find com.github.PicnicSupermarket.error-prone:error_prone_check_api:jar:v2.3.4-picnic-2 in https://maven-central.storage-download.googleapis.com/maven2/ was cached in the local repository, resolution will not be reattempted until the update interval of google-maven-central has elapsed or updates are forced -> [Help 1]

[Stephan202]

Curious! Will have a look.

[Stephan202]

Ah, I see. It tries to download the fork from open-source repos, but it doesn't exist there. (Well, it exists on Jitpack, but that repo isn't configured.) I'll try to fix that, though I suspect this is going to bite me in the butt once we want to release to Maven Central. A concern for later. Stay tuned :)

[Stephan202]

Testing on branch sschroevers/jitpack.

[Stephan202]

The build now passes on master (yeah, not yet doing proper PRs in this repo. I'm a rebel). I've rebased this PR. Should now pass... 👀

[Stephan202]

Main build passed, PR build didn't. Restarting the later with cleared caches. Can't explain this yet.

[Stephan202]

Success 🎉. Next up: review the PR 😄. Might do this tomorrow; will want to make some OCD changes for consistency with the existing code. But it looks good :)

[SimonBaars]

I was wondering: I included a few cases in which I expect the script not to change anything. Do these tests even work? Doesn't it only check for any change, not all cases?

[Stephan202]

I was wondering: I included a few cases in which I expect the script not to change anything. Do these tests even work? Doesn't it only check for any change, not all cases?

Generally I don't include such test cases, but yes they do work, because in the output file they're unchanged, as required.

I rebased and added a commit with some renaming and shuffling to better match the "style" used by the other tests. I have one concern about merging this PR, though: there are a small number of cases where we would not want to apply this rule. An example from picnic-platform:

diff --git a/atom/targetedcontent-service/targetedcontent-service-logic/src/main/java/com/picnic/targetedcontent/logic/repository/impl/TargetedContentRepositoryImpl.java b/atom/targetedcontent-service/targetedcontent-service-logic/src/main/java/com/picnic/targetedcontent/logic/repository/impl/TargetedContentRepositoryImpl.java
index 1d47d84d48..4375c89b9a 100644
--- a/atom/targetedcontent-service/targetedcontent-service-logic/src/main/java/com/picnic/targetedcontent/logic/repository/impl/TargetedContentRepositoryImpl.java
+++ b/atom/targetedcontent-service/targetedcontent-service-logic/src/main/java/com/picnic/targetedcontent/logic/repository/impl/TargetedContentRepositoryImpl.java
@@ -91,10 +91,7 @@ public final class TargetedContentRepositoryImpl implements TargetedContentRepos
                                 .as(TargetedContent.class))
                 .map(
                         tc -> {
-                            @SuppressWarnings("unchecked")
-                            TargetedContent<TargetedContentPayload> content =
-                                    (TargetedContent<TargetedContentPayload>) tc;
-                            return content;
+                            return (TargetedContent<TargetedContentPayload>) tc;
                         });
     }
 }

Here the @SuppressWarnings should not be dropped. Unfortunately Refaster doesn't have a way to say "match this if it's not annotated". (Though perhaps we should build something like that.) Alternatively we could implement this logic as an Error Prone plugin, but that's more complicated.

I'll sit on this a bit longer.

[SimonBaars]

I'm quite surprised that it just drops such annotations. The stuff Error Prone can and cannot do is still a bit magic to me. It would indeed be interesting to look into implementing such logic ourselves.

[Stephan202]

Rebased and resolved conflicts. @SimonBaars are you interested in proposing such a BugPattern?

[Stephan202]

Rebased and resolved conflicts. @SimonBaars are you interested in proposing such a BugPattern?

Decided to pick this one up myself ;). See #513.