Summary:
The problem is that in `AnnotatedField.special_case_nullability` we
first check the _generic_ nullability and if it is `nonnullish` we
apply refinements for enums, synthetic fields, etc.
The problem is that the definition of `is_nonnullish` changed in
D25186043 (7dcbacf693) to a stricter one `UncheckedNonnull`, but generic
nullability stayed the same `ThirdPartyNonnull`.
Therefore enum elements were not considered `nonnullish` under
`--no-nullsafe-optimistic-third-party-in-default-mode` and the enum
refinements were not applied, which led to bogus errors.
**Example:**
There's a third-party enum
```
enum EnumClass {
ENUM_ELEMENT
}
```
`ENUM_ELEMENT` is represented as a private static field of
`EnumClass`.
Then we have first party code that does
```
EnumClass.ENUM_ELEMENT
```
If this first party class is not `Nullsafe` and the checker is ran
with `--no-nullsafe-optimistic-third-party-in-default-mode`, the user
gets an incorrect warning about `ENUM_ELEMENT` being unvetted third
party.
Reviewed By: ngorogiannis
Differential Revision: D25560119
fbshipit-source-id: 4ad0760c5
Summary:
This diff makes the issue to be rendered more clearly. Before, we used to report
weirdly looking unconventional mode names like NullsafeLocal, even when
exact mode name was irrelevant.
Reviewed By: artempyanykh
Differential Revision: D25186041
fbshipit-source-id: 2619bcbd2
Summary:
In all other places, we index params from 0, but accidentally recorded
the wrong number in json. It was because of the confusion between index
and user-visible param position that we show for the user message.
This diff fixes it: now we use 0-based indices internally (but of course still
report 1-based ones in the error message).
Reviewed By: artempyanykh
Differential Revision: D24916878
fbshipit-source-id: 45532c5ff
Summary:
If the issue one of:
- Field Not Nullable
- Field Not Initialized
- Field Overannotated,
we record field_name to .json result.
NoTE: Design choice for representation. For Field Not Initialized and Field Overannotated
this is always internal (relative to the class) field, but for Field Not
Nullable it can be either internal or external. We could have a
structured output, or always output full name. I preferred to output
short name for convenience of the main usacase I am anticipating.
NOTE: not to be confused with the case where the field is nullable but
we e.g. try to dereference it. This is indirectly related to the issue
(can be several such fields for starters) and if we one day output it,
it will be provided in a separate way (similarly to how we output
nullable_methods).
Reviewed By: artempyanykh
Differential Revision: D24730320
fbshipit-source-id: c995ec221
Summary:
Since we report issues types without a prefix (e.g. ERADICATE_) and
with spaces we should also allow for prefix-less issue types in
SuppressLint, so both should work
- SuppressLint("eradicate-field-not-initialized")
- SuppressLint("Field Not Initialized")
Reviewed By: jvillard
Differential Revision: D24760341
fbshipit-source-id: 1590cf6d0
Summary:
The problem in Reporting.ml:log_issue_from_summary is that it merely
checks the presence of `SuppressLint` annotation on method's body to
decide whether to log or not the issue. This means that regardless of
issue types specified in `SuppressLint`, all issues on such method will
get blocked.
Here we fix that.
Reviewed By: ngorogiannis, mityal
Differential Revision: D24726604
fbshipit-source-id: c9cae3833
Summary:
This can be used by additional tooling for further analysis (e.g.
codemods, autofixes, etc).
Reviewed By: ngorogiannis
Differential Revision: D23987694
fbshipit-source-id: b9fa343ac
Summary:
The previous diffs recorded it for the case when the unvetted value is
dereferenced or otherwise used wrongly. This case finishes the work,
recording the needed signature for the remaining case (when the
offending third party has a non-nullable param with nullable passed
inside)
Reviewed By: ngorogiannis
Differential Revision: D23706679
fbshipit-source-id: e6f641223
Summary:
If the issue is related to the use of an unvetted third party method
that can not be trusted, we record this fact.
Reviewed By: ngorogiannis
Differential Revision: D23705626
fbshipit-source-id: 851328fe5
Summary:
It was inconsistent before (we recorded it only for meta issues).
Now we always record it, which will simplify further diffs.
In the next diffs, we are going to add more fields inside `nullsafe_extra`.
Reviewed By: ngorogiannis
Differential Revision: D23705592
fbshipit-source-id: 8bbb0e7c8
Summary:
We already take into account inheritance if the method inherits a known
modelled initializer method (e.g. Activity.onCreate()).
But if the method is explicitly marked as Initializer, we require its
overrides to be also marked.
This diffs fixes the behavior and makes it consistent, now this is
enough to annotate only parent class.
Reviewed By: jvillard
Differential Revision: D23135177
fbshipit-source-id: a21ff4a0e
Summary:
Current handling of lambdas is quite rudimentary. Looking at test
results we can see that errors are all over the place: False Positives,
False Negatives and just plain wrong results.
These tests can be grouped in 2 sets:
1. Basic support which implies:
- understanding method signatures,
- providing comprehensible error messages.
2. Extended support with implies:
- understanding scoping of values captures in lambdas (needs proper aliasing analysis).
- understanding parametric nullability in generics (needs "some"
support for Generics in our Java frontend).
With follow-up patches I'll attempt to implement "Basic" support for
Lambdas. "Extended" support will be out of scope unless there's
significant demand.
Reviewed By: mityal
Differential Revision: D23058673
fbshipit-source-id: 621551cca
Summary:
Synthetic/autogenerated methods/fields usually contain `$` in their names.
Reporting nullability violations on such code doesn't make much sense
since the violations are not actionable for users and likely need to be
resolved on another level.
This diff contributes:
1. Several test cases that involve synthetic code of different
complexity.
2. Code that handles some particular types of errors (but not all!).
Reviewed By: mityal
Differential Revision: D22984578
fbshipit-source-id: d25806209
Summary:
There are two ways to suppress it.
1. Field level suppression annotation (was already tested). This will
apply to all constructors.
2. Constructor level annotation (this is what this test does). Sometimes
there are "fake" constructors that are not intended to be called in
prod, they might leave some fields not initialized.
Note that there are two ways to add a suppression; one has a known
problem that is documented here.
Reviewed By: artempyanykh
Differential Revision: D22864869
fbshipit-source-id: f95aaa26a
Summary:
When expressions use generics or typecasts, CFG contains intermediate `_fun_cast` nodes which break some nullsafe heuristics and lead to false positives.
This affects different types of checks (null-check on assignment expressions, `map.containsKey` checks in assignment expressions, regular typecasts, etc.
See added tests for examples.
Reviewed By: mityal
Differential Revision: D22815631
fbshipit-source-id: 80d444b1c
Summary:
This diff:
1. Adds general capability to model any field as nullable /
non-nullable.
2. Uses it for Boolean.TRUE and Boolean.FALSE
Reviewed By: artempyanykh
Differential Revision: D22794226
fbshipit-source-id: 95f586592
Summary:
We already had a heuristic to deal with assignment expressions, but it
relied on the very previous CFG node to have a non-empty list of instrs.
In some cases, however, this previous node is a Join_node with no instrs,
so we need to take one more step back to find what we're looking for.
I've also added a bit more logging around this functionality, so it's
easier to debug/tune in future.
Reviewed By: ngorogiannis
Differential Revision: D22282930
fbshipit-source-id: 024eec145
Summary: Let's make package name match the directory name to follow Java's file lookup conventions
Reviewed By: skcho
Differential Revision: D22183964
fbshipit-source-id: b9958b975
Summary:
Nullability of the assignment result is not refined in code snippets
like:
```
while ((a = foo.getA()) != null) {
nonNullableVal = a;
}
```
Let's add a test for this.
Reviewed By: jvillard
Differential Revision: D22136218
fbshipit-source-id: 206c368d6
Summary: Later on, this can be changed again or made customizable.
Reviewed By: artempyanykh
Differential Revision: D21618730
fbshipit-source-id: fe517c766
Summary:
These 2 methods are automatically supplied for all enums, with
predefined behavior and nullability: https://www.geeksforgeeks.org/enum-in-java/
(Note that they are not part of java.lang.Enum class).
This will allow using them in unvetted third part and under strict mode.
Reviewed By: artempyanykh
Differential Revision: D21501716
fbshipit-source-id: 104082d15
Summary:
This diff changes way we treat classes w.r.t. to Nullsafe modes when
issuing meta-issues.
Previously, we considered nested class independently of the outer one.
This was leading to a tricky case: when the class is clean but nested
class needs fixing, meta-info told that class can be Nullsafe.
This is counter-intutive and lead to problems when users tried to follow
wrong nullsafe suggestions for this case.
After this diff we:
1. Start calculating meta-issues only on the outermost level. This will simplify
reasoning about nullsafe stats.
2. Aggregate all nested issues counters to corresponding outer-level class.
Among others, CLASS_CAN_BE_NULLSAFE Advice will finally become
actionable in all known cases.
Reviewed By: artempyanykh
Differential Revision: D21353607
fbshipit-source-id: a17c6958a
Summary:
We have a common entry point where we skip analysis in nullsafe.
This logic is copied from `Reporting.log_issue_from_summary`.
I believe this should not exist in Reporting: it is not the right place
to decide whether to suppress issues: we should not try to report it in
first place.
Because of that we falsely report "needs improvement" meta-issue while
we don't issue any (they were suppressed but participated in needs
improvement count calculations).
Now this change will make meta-issue to be synced with what the user
actually sees.
Down the line we should have a more reliable fix for that.
So far I reviewed suppressing code and looks like we should not suppress
anything else (unless explicitly SuppressLint-ed, which is fine).
Reviewed By: artempyanykh
Differential Revision: D21328634
fbshipit-source-id: 120ce06d1
Summary:
The directory was created to have several sets of nullsafe tests but
there is only one in the end. Remove the redundant "-default".
Reviewed By: mityal
Differential Revision: D21300205
fbshipit-source-id: 46ed8b032
Artem Pianykh
Mitya Lyubarskiy
David Pichardie
Jules Villard