Introduce NestedOptionals check
#202
Conversation
Venorcis
force-pushed
the
vkoeman/optional-to-optional-mapping
branch
2 times, most recently
from
b683883 to
9dab86c
Compare
Stephan202
force-pushed
the
vkoeman/optional-to-optional-mapping
branch
from
9dab86c to
18f2dbf
Compare
| linkType = NONE, | ||
| severity = WARNING, | ||
| tags = SIMPLIFICATION) | ||
| public final class NestingOptionals extends BugChecker implements MethodInvocationTreeMatcher { |
There was a problem hiding this comment.
W.r.t. the name: WDYT of NestedOptionals?
There was a problem hiding this comment.
Was doubting between NestedOptionals and NestedOptional. Looking at the existing checks, in most cases we currently use the singular form of writing.
I think the name NestedOptional would be an even better fit. WDYT?
There was a problem hiding this comment.
Actually, wait. I did consider that, but was worried that a Stream<Optional<T>> also covers that name. Hence the plural (i.e. "at least two Optionals).
There was a problem hiding this comment.
Ah good one, didn't think of that case. Let's roll with this.
| private static class NestedOptionalDetector extends TreeScanner<Boolean, VisitorState> { | ||
| @Nullable | ||
| @Override | ||
| public Boolean visitMethodInvocation(MethodInvocationTree node, VisitorState state) { | ||
| if (ASTHelpers.getType(node).getTypeArguments().stream() | ||
| .anyMatch(NestedOptionalDetector::typeIsJavaOptional)) { | ||
| return true; | ||
| } | ||
| return super.visitMethodInvocation(node, state); | ||
| } | ||
|
|
||
| private static boolean typeIsJavaOptional(Type type) { | ||
| return type.asElement().getQualifiedName().contentEquals(JAVA_OPTIONAL); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we can simplify this code and avoid the string comparison in the process (which should make things more efficient). Will push a proposal :)
| @@ -156,6 +156,7 @@ private static Optional<SuggestedFix.Builder> constructMethodRef( | |||
| return constructFix(lambdaExpr, lhsType.tsym, subTree.getIdentifier()); | |||
| } | |||
|
|
|||
| @SuppressWarnings("NestingOptionals") | |||
There was a problem hiding this comment.
Yeah, we're doing some dodgy stuff here 🤔
| @@ -311,6 +311,7 @@ Optional<R> after( | |||
| /** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */ | |||
| abstract static class OptionalOrOtherOptional<T> { | |||
| @BeforeTemplate | |||
| @SuppressWarnings("NestingOptionals") | |||
There was a problem hiding this comment.
| @SuppressWarnings("NestingOptionals") | |
| @SuppressWarnings("NestingOptionals" /* Auto-fix for the `NestingOptionals` check. */) |
|
Thanks @Stephan202, great improvements 🚀 |
There was a problem hiding this comment.
Nice check and congrats on your first contribution @Venorcis 🚀 !
@Stephan202 TBH I don't think it is worth the time now to fix the suppressions in MethodReferenceUsage. The check isn't enabled at the moment and we'll have to put quite some effort into it anyway before it will be.
Suggested commit message:
Introduce `NestedOptionals` check (#202)
| " // BUG: Diagnostic contains:", | ||
| " Optional.of(Optional.empty());", | ||
| " // BUG: Diagnostic contains:", | ||
| " Optional.of(Optional.of(1));", |
There was a problem hiding this comment.
For readability we could add an empty line here, below the two ofNullable examples and before the Stream.of examples. Not a big win though.
We do this in some other tests as well.
| "Avoid nesting `Optional`s inside `Optional`s; the resultant code is hard to reason about", | ||
| linkType = NONE, | ||
| severity = WARNING, | ||
| tags = SIMPLIFICATION) |
There was a problem hiding this comment.
We are not actually doing the simplification, we are flagging fragile code 😄.
| tags = SIMPLIFICATION) | |
| tags = FRAGILE_CODE) |
Right?
| linkType = NONE, | ||
| severity = WARNING, | ||
| tags = SIMPLIFICATION) | ||
| public final class NestingOptionals extends BugChecker implements MethodInvocationTreeMatcher { |
There was a problem hiding this comment.
Ah good one, didn't think of that case. Let's roll with this.
There was a problem hiding this comment.
I had hoped that the MethodReferenceUsage code on this branch didn't suffer from the same issue, but alas.
Added a comment to clarify that the current code is undesirable.
rickie
changed the title
Optional<Optional<...>> typesNestedOptionals check
Based on my own suggestion and input in this Slack thread.
Disallows any (embedded) use of nested optionals.