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 OptionalIdentity Refaster template #245

Merged
merged 5 commits into from
Sep 19, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,22 @@ Optional<T> after(Optional<T> optional1, Optional<T> optional2) {
}
}

/**
* Don't use the redundant chain of {@link Optional#stream()} followed by either {@link
* Stream#findFirst()} or {@link Stream#findAny()}.
*/
abstract static class OptionalSkipStreamFindFirst<T> {
Copy link
Member

Choose a reason for hiding this comment

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

@rickie interesting case w.r.t. automated naming: we wouldn't be able to use Optional here. I'll go with OptionalIdentity for now.

svavahb marked this conversation as resolved.
Show resolved Hide resolved
@BeforeTemplate
Optional<T> before(Optional<T> optional) {
return Refaster.anyOf(optional.stream().findFirst(), optional.stream().findAny());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we could also add stream().max() and stream().min()?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good one! Sure; wanna add a proposal? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do!

Copy link
Contributor Author

@svavahb svavahb Sep 19, 2022

Choose a reason for hiding this comment

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

I'm having a small issue: The test keeps failing because the Input file has the import for java.util.Comparator.reverseOrder, while the Output file doesn't (the output doesn't use any comparator because it doesn't have the max/min function calls). It keeps suggesting to add this (unused) import:

[ERROR] Failures: 
[ERROR]   RefasterTemplatesTest.validateTemplateCollection:81 value of:
    maybeFormat(...)
diff (-expected +actual):
    @@ -1,6 +1,7 @@
     package tech.picnic.errorprone.refastertemplates;
     
     import static com.google.common.collect.Streams.stream;
    +import static java.util.Comparator.reverseOrder;
     
     import com.google.common.collect.ImmutableSet;
     import com.google.common.collect.Streams;

I can of course try to fix it by using something that doesn't require an import as the comparator, however it feels strange that it would fail on the imports? Shouldn't the import get removed from the input when the refaster template is applied in the error prone compilation?

Copy link
Member

@Stephan202 Stephan202 Sep 19, 2022

Choose a reason for hiding this comment

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

In general it's hard to know whether an import can be removed, because other code may still depend on it (and rules are applied independently). So in practice we instead rely on Google Java Format to drop unused imports after the fact. This does impose a challenge for the tests, as you have noticed. For this purpose we have that funny elidedTypesAndStaticImports method at the top of some of those test classes. But your String::compareTo usage is a nice workaround 👍.

(This does raise another point: we should have a rule that favours naturalOrder() over T::compareTo 😄 But that's for another day/PR.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks for the explanation! 😄

}

@AfterTemplate
Optional<T> after(Optional<T> optional) {
return optional;
}
}

// XXX: Add a rule for:
// `optional.flatMap(x -> pred(x) ? Optional.empty() : Optional.of(x))` and variants.
// (Maybe canonicalize the inner expression. Maybe we rewrite already.)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,9 @@ ImmutableSet<Optional<String>> testOptionalOrOtherOptional() {
Optional.of("baz").map(Optional::of).orElseGet(() -> Optional.of("qux")),
Stream.of(Optional.of("quux"), Optional.of("quuz")).flatMap(Optional::stream).findFirst());
}

ImmutableSet<Optional<String>> testOptionalSkipStreamFindFirst() {
return ImmutableSet.of(
Optional.of("foo").stream().findFirst(), Optional.of("bar").stream().findAny());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,8 @@ ImmutableSet<Optional<String>> testOptionalOrOtherOptional() {
Optional.of("baz").or(() -> Optional.of("qux")),
Optional.of("quux").or(() -> Optional.of("quuz")));
}

ImmutableSet<Optional<String>> testOptionalSkipStreamFindFirst() {
return ImmutableSet.of(Optional.of("foo"), Optional.of("bar"));
}
}