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 Optional{Filter,Map} Refaster rules #327

Merged
merged 2 commits into from
Nov 4, 2022
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -355,6 +355,42 @@ Optional<T> after(Optional<T> optional) {
}
}

/**
* Avoid unnecessary {@link Optional} to {@link Stream} conversion when filtering a value of the
* former type.
*/
static final class OptionalFilter<T> {
@BeforeTemplate
Optional<T> before(Optional<T> optional, Predicate<? super T> predicate) {
return Refaster.anyOf(
optional.stream().filter(predicate).findFirst(),
optional.stream().filter(predicate).findAny());
}

@AfterTemplate
Optional<T> after(Optional<T> optional, Predicate<? super T> predicate) {
return optional.filter(predicate);
}
}

/**
* Avoid unnecessary {@link Optional} to {@link Stream} conversion when mapping a value of the
* former type.
*/
// XXX: If `StreamMapFirst` also simplifies `.findAny()` expressions, then this rule can be
Copy link
Member

Choose a reason for hiding this comment

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

IIUC in that case the self-check would identify this case right?

In that case I'd argue that it would be better to drop this XXX 😬.
@Stephan202

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we could drop it for that reason, but without it on might ask "why not add .findFirst()?" IMHO now it's clearer why this rule exists in the first place 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Okay okay. Agreed, it doesn't really fit in a @Description or something like that, so XXX is most reasonable.

// dropped in favour of `StreamMapFirst` and `OptionalIdentity`.
static final class OptionalMap<S, T> {
@BeforeTemplate
Optional<? extends T> before(Optional<S> optional, Function<? super S, ? extends T> function) {
return optional.stream().map(function).findAny();
}

@AfterTemplate
Optional<? extends T> after(Optional<S> optional, Function<? super S, ? extends T> function) {
return optional.map(function);
}
}

// 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 @@ -168,8 +168,9 @@ Stream<R> after(Stream<T> stream, Function<? super S, ? extends Stream<? extends
* Where possible, clarify that a mapping operation will be applied only to a single stream
* element.
*/
// XXX: Consider whether to have a similar rule for `.findAny()`. For parallel streams it
// wouldn't be quite the same....
// XXX: Implement a similar rule for `.findAny()`. For parallel streams this wouldn't be quite the
// same, so such a rule requires a `Matcher` that heuristically identifies `Stream` expressions
// with deterministic order.
// XXX: This change is not equivalent for `null`-returning functions, as the original code throws
// an NPE if the first element is `null`, while the latter yields an empty `Optional`.
static final class StreamMapFirst<T, S> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,4 +109,14 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
Optional.of("baz").stream().min(String::compareTo),
Optional.of("qux").stream().max(String::compareTo));
}

ImmutableSet<Optional<String>> testOptionalFilter() {
return ImmutableSet.of(
Optional.of("foo").stream().filter(String::isEmpty).findFirst(),
Optional.of("bar").stream().filter(String::isEmpty).findAny());
}

Optional<String> testOptionalMap() {
return Optional.of(1).stream().map(String::valueOf).findAny();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,13 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
return ImmutableSet.of(
Optional.of("foo"), Optional.of("bar"), Optional.of("baz"), Optional.of("qux"));
}

ImmutableSet<Optional<String>> testOptionalFilter() {
return ImmutableSet.of(
Optional.of("foo").filter(String::isEmpty), Optional.of("bar").filter(String::isEmpty));
}

Optional<String> testOptionalMap() {
return Optional.of(1).map(String::valueOf);
}
}