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
Custom path to Optional class for Optional emptiness handler #288
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,6 @@ | |
*/ | ||
package com.uber.nullaway.handlers; | ||
|
||
import com.google.common.base.Preconditions; | ||
import com.google.errorprone.VisitorState; | ||
import com.google.errorprone.util.ASTHelpers; | ||
import com.sun.source.tree.ClassTree; | ||
|
@@ -38,6 +37,8 @@ | |
import com.uber.nullaway.dataflow.AccessPath; | ||
import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; | ||
import java.util.Optional; | ||
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
import javax.annotation.Nullable; | ||
import javax.lang.model.element.Element; | ||
import javax.lang.model.element.ElementKind; | ||
|
@@ -50,7 +51,7 @@ | |
*/ | ||
public class OptionalEmptinessHandler extends BaseNoOpHandler { | ||
|
||
@Nullable private Optional<Type> optionalType; | ||
@Nullable private Set<Optional<Type>> optionalTypes; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a Also, it probably should be an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done 👍 |
||
|
||
@Override | ||
public boolean onOverrideMayBeNullExpr( | ||
|
@@ -69,11 +70,15 @@ public void onMatchTopLevelClass( | |
VisitorState state, | ||
Symbol.ClassSymbol classSymbol, | ||
Config config) { | ||
if (optionalType == null) { | ||
optionalType = | ||
Optional.ofNullable(state.getTypeFromString(config.getOptionalClassPath())) | ||
.map(state.getTypes()::erasure); | ||
} | ||
optionalTypes = | ||
config | ||
.getOptionalClassPaths() | ||
.stream() | ||
.map( | ||
type -> | ||
Optional.ofNullable(state.getTypeFromString(type)) | ||
.map(state.getTypes()::erasure)) | ||
.collect(Collectors.toSet()); | ||
} | ||
|
||
@Override | ||
|
@@ -144,20 +149,22 @@ private void updateNonNullAPsForElement( | |
} | ||
|
||
private boolean optionalIsPresentCall(Symbol.MethodSymbol symbol, Types types) { | ||
Preconditions.checkNotNull(optionalType); | ||
// noinspection ConstantConditions | ||
return optionalType.isPresent() | ||
&& symbol.getSimpleName().toString().equals("isPresent") | ||
&& symbol.getParameters().length() == 0 | ||
&& types.isSubtype(symbol.owner.type, optionalType.get()); | ||
for (Optional<Type> optionalType : optionalTypes) { | ||
if (optionalType.isPresent() | ||
&& symbol.getSimpleName().toString().equals("isPresent") | ||
&& symbol.getParameters().length() == 0 | ||
&& types.isSubtype(symbol.owner.type, optionalType.get())) return true; | ||
} | ||
return false; | ||
} | ||
|
||
private boolean optionalIsGetCall(Symbol.MethodSymbol symbol, Types types) { | ||
Preconditions.checkNotNull(optionalType); | ||
// noinspection ConstantConditions | ||
return optionalType.isPresent() | ||
&& symbol.getSimpleName().toString().equals("get") | ||
&& symbol.getParameters().length() == 0 | ||
&& types.isSubtype(symbol.owner.type, optionalType.get()); | ||
for (Optional<Type> optionalType : optionalTypes) { | ||
if (optionalType.isPresent() | ||
&& symbol.getSimpleName().toString().equals("get") | ||
&& symbol.getParameters().length() == 0 | ||
&& types.isSubtype(symbol.owner.type, optionalType.get())) return true; | ||
} | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this should be
CustomOptionalClasses
, otherwise I keep parsing it on my head as "Optional (class paths)" rather than "(Optional class) paths"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that my alternative is perfect, but... mmm... maybe
CheckOptionalEmptinessCustomClasses
? A bit verbose, but at least ties the two options together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to
CheckOptionalEmptinessCustomClasses
, sounds better