Comments (18)
Yes, though the codegen is only for minimizing fields and the logic is in the base class. I would expect inlining to remove the downcasts, so it would be free. But the project has exceeded its complexity budget and this would add obscurity when already challenging. But this is a good trick to know if I decide to introduce NullAway to a work project. Thanks!
from nullaway.
Our C++ colleagues at Google have done some work to handle variables like the definitelyNotNull
example above. My understanding is that they're currently using satisfiability checking but that they are considering alternative approaches that might perform better, including essentially "inlining" potentiallyNull != null
in place of usages of definitelyNotNull
(potentially inlining further recursively). (I'm sure there would be caveats around cases in which potentiallyNull
might change value, for example.) I'm told that one approach (similar but not identical to "inlining") is being demoed in llvm/llvm-project#82950.
from nullaway.
Yeah, NullAway does not track local variable aliases so it misses this case. Like #35 I think we could support this eventually by doing a more precise analysis in cases where we are going to report an error. Going to mark this as low priority for now but it would be a good project to take on eventually
from nullaway.
BTW thanks for all the reports @ben-manes! Please keep them coming 😄
from nullaway.
Unfortunately I think that I have to give up on the target project since it is non-idiomatic for performance reasons. It uses a lot of nullable fields that are enabled in certain configurations, as indicated by code generated subclasses. Since those usage paths can only occur when non-null, and NullAway cannot track deeply enough, many false positives are hard to easily resolve without turning the whole thing off. But it was a nice experiment to try this out!
from nullaway.
Ok no problem! One thing is if you control the code generator, you can tweak it to generate downcasts or other warning suppressions so you can still use NullAway on other more idiomatic parts of the code base. (The downcasts may not be tolerable if the code is super performance critical.) We'll keep working on improving NullAway in the meantime.
from nullaway.
Making progress using your trick, so we'll see how it comes out :)
Another case where this issue occurs in the pattern,
boolean create = (foo == null);
if (!create) {
foo.bar()
}
This scenario is about readability of the code by naming the condition when used multiple times in a method. Using the explicit check each time would be okay, but convey less when maintaining and need reparsing of what null means. But due to not tracking the invocation on foo
is a false positive.
from nullaway.
Making progress using your trick, so we'll see how it comes out :)
Awesome! :) Regarding your create
example, yeah, this would be good to handle too. If you have any rough categorization of how often this case happens vs. the first cast example that could be helpful for prioritization. The solutions are different: the cast example requires tracking equalities between variables / fields, while the create
example requires tracking conditional (non-)nullness, facts like "if create
is false then foo
is not null.
from nullaway.
In this case it was only once, but that is because I usually extract those out to query methods. Since method tracking isn't supported, that has been a largest case of false positives.
My present goal is to have it pass in order to review the benefits. There are a lot more suppressions than I'd like since inference is too locally scoped. But I also think it may have found one or two possible errors in a JSR adapter, so it will be worthwhile regardless. Since this requires more extensive annotations just like the Checker Framework does, I'm curious to run their analysis afterwards to see how it compares.
from nullaway.
Everything works nicely! I had forgotten why the CheckerFramework is painful, so this is a nice compromise. :)
from nullaway.
Great, so glad you got things working! And thanks again for all the reports. We'll dig through them more in the new year.
Regarding the query methods, I think we can actually support those more easily with a new annotation, rather than requiring a library model. I'll re-open another issue specifically around that one.
from nullaway.
Joy, integrated into my project. There are 50 targeted suppressions and neutral impact to code readability. Thanks for all of the hard work :)
from nullaway.
Joy, integrated into my project. There are 50 targeted suppressions and neutral impact to code readability. Thanks for all of the hard work :)
Awesome! Thanks for all your reports and feedback!
from nullaway.
@msridhar, what are the chances of this being picked up? We at Canva are rolling out NullAway in our code base, and there's been pushback from some engineers finding it cumbersome to work around NullAway's limitation. Here's an example that's similar to the create
example above:
String potentiallyNull = // ...
boolean definitelyNotNull = potentiallyNull != null;
if (definitelyNotNull) {
int length = potentiallyNull.length(); // <-- NullAway warning
}
if (potentiallyNull != null) {
int length = potentiallyNull.length(); // <-- No warning
}
We'd also be happy to contribute if that's the quickest way forward, just let me know how to help.
from nullaway.
@wbadam exciting to hear you are working on rolling out NullAway at Canva! Given the number of times this has come up, I would be open to trying to address cases like your example. Unfortunately it may not be a super-quick thing to implement. An over-simplified view is that right now, NullAway keeps track of a set of variables that are @Nullable
and a set of variables that are @NonNull
before and after each line of code in a method. We would need to change this representation so that we could track that a variable is @NonNull
if some other condition holds, like some boolean variable is true
. I was always hesitant to add such support since the dataflow analysis that computes this information is performance critical for NullAway, and we've always tried to keep the compile-time overhead of NullAway as low as possible, so like other Error Prone checks it can be run on every build. I think it would take some careful design and measurement to add this extra reasoning without compromising performance too much (it's ok to compromise a bit). Maybe we could use a flag to control whether the extra reasoning is enabled, but that might make the code excessively ugly, so hopefully we wouldn't need that.
Do you or one of your teammates feel willing / able to dig into this a bit? Having more help would definitely make the exploration go faster. I am slammed for at least the next couple of days and won't have time to look more closely. But I can try to give pointers to the relevant code.
from nullaway.
@msridhar certainly happy to look into it, a direction to the relevant code would be much appreciated!
from nullaway.
I have renamed this issue to focus on the false positives stemming from cases like those in #98 (comment).
@msridhar certainly happy to look into it, a direction to the relevant code would be much appreciated!
Thanks, @wbadam, I appreciate it. I still need to do some thinking and exploration as to the best way to support this. Right now, the state tracked by NullAway during local dataflow analysis is a NullnessStore
, which maps access paths to their nullness state. To support this use case we need to instead track something like "conditional nullness", i.e., facts of the form, "if this variable is true/false, then this access path is nullable / non-null." This would involve updating the store data structure to be able to hold such conditional nullness facts (and to properly compute least upper bounds on the stores), updating the transfer functions in AccessPathNullnessPropagation
to generate the facts, and also updating relevant APIs that query the dataflow analysis for nullness information at different program points. And, I would like to do this in a way that does not hurt NullAway performance too much in cases where conditional nullness facts do not need to be tracked.
I have some other deadlines and an upcoming holiday, so I won't be able to dig into this more until after mid-April. If anyone has cycles and wants to attempt some prototyping based on the above, feel free. Otherwise, I will take a look when I have some time available.
from nullaway.
Thanks a lot @cpovirk! That's very interesting. The inlining approach is appealing in terms of trying to make the results of dataflow not depend on whether you store certain types of conditions in an (effectively final?) local variable or check them directly. I'm still unsure of the best way to proceed for NullAway. I hope to be able to put in more time on this soon.
from nullaway.
Related Issues (20)
- The combination of JSpecify + Lombok does not seem to work well HOT 6
- Method references of nullable instances are not detected HOT 1
- Detected NPE causes EP's Var rule to go nuts. HOT 5
- Validation of null in a switch-case with pattern matching does not work as expected. HOT 1
- False positive warning on switch statements with case null
- Build failure : 'options.compilerArgumentProviders.errorprone$0.name' is missing an input or output annotation. HOT 1
- Support org.apache.commons.lang3.StringUtils.isNotEmpty HOT 3
- Support for `collect()` in Streams API HOT 4
- Control flow analysis fails within a stream collector HOT 5
- Validation of null in a method used in a boolean expression does not work as expected. HOT 1
- `if (Foo.this.bar != null)` in nested class not properly analyzed HOT 1
- Rename the Jar Infer Flags `JarInferEnabled`,`JarInferUseReturnAnnotations`
- Variable checked non-null outside a `forEach` or `Stream` lambda triggers an error HOT 13
- `if` statement on a boolean does not prevent a dereferenced nullable error HOT 1
- Null `if` check and failing function to taken in account HOT 2
- Extract methods in StreamNullabilityPropagator
- ignoring generic Nullable on Supplier HOT 14
- Published NonNull Present HOT 6
- Tracking issue: making `jspecify/jdk` library models available to NullAway users
- Remove OptionalEmptinessHandler method for propagating access paths to lambdas
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from nullaway.