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 |
---|---|---|
|
@@ -29,6 +29,7 @@ | |
import com.sun.tools.javac.code.Type; | ||
import com.sun.tools.javac.code.Types; | ||
import com.sun.tools.javac.util.Context; | ||
import com.uber.nullaway.Config; | ||
import com.uber.nullaway.NullAway; | ||
import com.uber.nullaway.Nullness; | ||
import com.uber.nullaway.dataflow.AccessPath; | ||
|
@@ -54,7 +55,11 @@ public class ApacheThriftIsSetHandler extends BaseNoOpHandler { | |
|
||
@Override | ||
public void onMatchTopLevelClass( | ||
NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { | ||
NullAway analysis, | ||
ClassTree tree, | ||
VisitorState state, | ||
Symbol.ClassSymbol classSymbol, | ||
Config config) { | ||
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. Don't pass this here, pass it to the handler's constructor in 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 👍 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. Before I go ahead and do a full pass, it looks to me like this remains to be addressed :) 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 now 😅 |
||
if (tbaseType == null) { | ||
tbaseType = | ||
Optional.ofNullable(state.getTypeFromString(TBASE_NAME)).map(state.getTypes()::erasure); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ | |
import com.sun.tools.javac.code.Type; | ||
import com.sun.tools.javac.code.Types; | ||
import com.sun.tools.javac.util.Context; | ||
import com.uber.nullaway.Config; | ||
import com.uber.nullaway.ErrorMessage; | ||
import com.uber.nullaway.NullAway; | ||
import com.uber.nullaway.Nullness; | ||
|
@@ -49,8 +50,6 @@ | |
*/ | ||
public class OptionalEmptinessHandler extends BaseNoOpHandler { | ||
|
||
private static String OPTIONAL_PATH = "java.util.Optional"; | ||
|
||
@Nullable private Optional<Type> optionalType; | ||
|
||
@Override | ||
|
@@ -65,10 +64,14 @@ && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.get | |
|
||
@Override | ||
public void onMatchTopLevelClass( | ||
NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { | ||
NullAway analysis, | ||
ClassTree tree, | ||
VisitorState state, | ||
Symbol.ClassSymbol classSymbol, | ||
Config config) { | ||
if (optionalType == null) { | ||
optionalType = | ||
Optional.ofNullable(state.getTypeFromString(OPTIONAL_PATH)) | ||
Optional.ofNullable(state.getTypeFromString(config.getOptionalClassPath())) | ||
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. Must be generalized to deal with multiple 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 👍 |
||
.map(state.getTypes()::erasure); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1233,6 +1233,65 @@ public void OptionalEmptinessHandlerTest() { | |
.doTest(); | ||
} | ||
|
||
@Test | ||
public void OptionalEmptinessHandlerWithCustomPathTest() { | ||
compilationHelper | ||
.setArgs( | ||
Arrays.asList( | ||
"-d", | ||
temporaryFolder.getRoot().getAbsolutePath(), | ||
"-XepOpt:NullAway:AnnotatedPackages=com.uber", | ||
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated", | ||
"-XepOpt:NullAway:CheckOptionalEmptiness=true", | ||
"-XepOpt:NullAway:OptionalClassPath=com.google.common.base.Optional")) | ||
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. 👍 Would suggest also:
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. Should we not have 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. The way I would do it, if possible:
This allows people using this handler for their own custom Then again, not sure if it's more consistent with how we do other stuff to just have @msridhar , thoughts ? 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. I think we should always check 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. Okay then. So first two cases as Lazaro described and and in case 3. Explicit option passed, parsable non-empty set => the classes in that set + 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. I am not sure if we really need the unparsable check. Since we do not have it for other configs. Also I'm not sure how to define it exactly. So let me know if you think it should be necessary. 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. I think given what Manu is saying, then there is just two cases:
Still worth to check that it works with zero (no 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. Added test for zero, one and two custom paths case. |
||
.addSourceLines( | ||
"TestNegative.java", | ||
"package com.uber;", | ||
"import com.google.common.base.Optional;", | ||
"import javax.annotation.Nullable;", | ||
"import com.google.common.base.Function;", | ||
"public class TestNegative {", | ||
" void foo() {", | ||
" Optional<Object> a = Optional.absent();", | ||
" // no error since a.isPresent() is called", | ||
" if(a.isPresent()){", | ||
" a.get().toString();", | ||
" }", | ||
" }", | ||
" public void lambdaConsumer(Function a){", | ||
" return;", | ||
" }", | ||
" void bar() {", | ||
" Optional<Object> b = Optional.absent();", | ||
" if(b.isPresent()){", | ||
" lambdaConsumer(v -> b.get().toString());", | ||
" }", | ||
" }", | ||
"}") | ||
.addSourceLines( | ||
"TestPositive.java", | ||
"package com.uber;", | ||
"import com.google.common.base.Optional;", | ||
"import javax.annotation.Nullable;", | ||
"import com.google.common.base.Function;", | ||
"public class TestPositive {", | ||
" void foo() {", | ||
" Optional<Object> a = Optional.absent();", | ||
" // BUG: Diagnostic contains: Optional a can be empty", | ||
" a.get().toString();", | ||
" }", | ||
" public void lambdaConsumer(Function a){", | ||
" return;", | ||
" }", | ||
" void bar() {", | ||
" Optional<Object> b = Optional.absent();", | ||
" // BUG: Diagnostic contains: Optional b can be empty", | ||
" lambdaConsumer(v -> b.get().toString());", | ||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
|
||
@Test | ||
public void OptionalEmptinessUncheckedTest() { | ||
compilationHelper | ||
|
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.
Should probably be an
ImmutableSet<String>
, like withunannotatedClasses
. It should also be@Nullable
(for the case wherecheckOptionalEmptiness
isfalse
). We want to support the case of: a) No Optional classes passed, b) One Optional class passed (or taken from the default), c) N Optional classes passed. This field would only support (b)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.
Done 👍