Summary:
This diff handles live variables in catch blocks. To do that, this diff adds another metadata,
`CatchEntry`.
Domain change: The domain is changed to
```
(normal:variables) x (exn:try_id->variables)
```
`exn` is a map from try-catch-id to a set of live variables that are live at the corresponding entry
of catch blocks.
Semantics change: It is a backward analysis.
* on `CatchEntry`: It updates `exn` with `try_id` and current `normal`.
* on `Call`: As of now, we assume all function calls can raise an exception. Therefore, it copies
all live variables in `exn` to `normal`.
* on `TryEntry`: It removes corresponding `try_id` from `exn`.
Reviewed By: jvillard
Differential Revision: D26952755
fbshipit-source-id: 1da854a89
Summary: This diff adds TryEntry and TryExit statements to the entry and exit of C++ `try` block, in order to handle exceptional control flow better in analyses.
Reviewed By: da319, jvillard
Differential Revision: D26946188
fbshipit-source-id: 33f4ae9e7
Summary:
Previously, only names containing '$' were considered synthetic. We need
to extend the logic and look for "_UL_" in the name as well.
Also I deduped 4 different impls of "is_synthetic/generated/autogen".
Reviewed By: ngorogiannis
Differential Revision: D25899232
fbshipit-source-id: 9463eca6b
Summary:
This sometimes happens and brings down all of infer with it. Just log
the error instead.
Fixes https://github.com/facebook/infer/issues/1338
Reviewed By: ezgicicek
Differential Revision: D25637821
fbshipit-source-id: 681207813
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:
First argument is a boolean and thus is always non-null, rather than
nullable.
Reviewed By: ngorogiannis
Differential Revision: D25532156
fbshipit-source-id: e334e0886
Summary:
Currently, we don't issue warnings for third party return value in
non-@Nullsafe modes.
For some integrations, this feature is useful.
This diff repurposes the existing param to suit this goal.
Reviewed By: artempyanykh
Differential Revision: D25186043
fbshipit-source-id: 308101841
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:
Sometimes there are annotations that don't correspond to the user facing
code.
Previously we would fail, now process them gracefully.
Reviewed By: artempyanykh
Differential Revision: D24890895
fbshipit-source-id: e64a866ec
Summary:
They are indirectly encoded in "procedure" field already, but:
1/ To extract name and params one needs to parse the procedure on the
client side
2/ We already have class, package, and field: method and its params is a
consistent change
Reviewed By: artempyanykh
Differential Revision: D24731064
fbshipit-source-id: fae478e7e
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:
There is a feature in Nullsafe that is interfering with "annotation
graph" feature. Because of this we would not detect provisional
violations for misuses of params of equals() (They will be recorded
as user facing rather than provisional issues).
This diff turns this feature off for annotation graph mode.
Reviewed By: artempyanykh
Differential Revision: D24726655
fbshipit-source-id: 4b7577667
Summary:
Virtual "this" invisible param exists in the annotated signature, but
does not exist in some other places, which causes a lot of annoyance in
different places.
This diff does not intend to solve all this, but makes one step forward.
1/ AnnotatedNullability now explicitly distincts normal params and
VirtualThis.
2/ AnnotatedSignature accounts for this via: a) not having redundant
fake annotation points b) having corrent param indices (those were off
by 1 in non-virtul methods).
Reviewed By: artempyanykh
Differential Revision: D24726480
fbshipit-source-id: fdb8bb0fb
Summary: This is a complex enough feature so iterating on it in a safe manner will be useful.
Reviewed By: artempyanykh
Differential Revision: D24725406
fbshipit-source-id: 81b247143
Summary:
This diff glues the previous work together.
The ClassLevelAnalysis finds list of provisional violation, builds the
graph based on them, and outputs this graph as a separate issue.
Reviewed By: artempyanykh
Differential Revision: D24682802
fbshipit-source-id: 8174da91a
Summary:
This change is what makes the annotation graph a _graph_.
Now we can detect places when adding one annotation causes an issue that can be
fixed by adding another annotation.
Reviewed By: artempyanykh
Differential Revision: D24650979
fbshipit-source-id: b8452b822
Summary:
In the previous diff we introduced
AnnotatedNullability.is_annotatable_as_nullable method.
Lets now use it in all places where we issue ProvisionallyNullable
annotation.
This will lead to more precise nullability graph (without attempts to
annotate primitives).
Reviewed By: artempyanykh
Differential Revision: D24650951
fbshipit-source-id: 8ea0bb97d
Summary:
This change is on par with the logic that we already have for methods
and method params.
In this logic, we explicitly distinct class under analysis & the
external class when fetching AnnotatedNullability (we needed this to support
various Nullsafe modes as well).
Here we use the same approach for fields.
NOTE: this is not intended to work with nested classes just yet.
Reviewed By: artempyanykh
Differential Revision: D24650934
fbshipit-source-id: f555bcc3f
Summary: Cleanup `Typ` by moving all constant types to `StdTyp`. Also remove `Typ.typ` as it's just `Typ.t` now.
Reviewed By: jberdine
Differential Revision: D24620397
fbshipit-source-id: 4764f87ef
Summary:
Now we have all pieces of information needed to build the annotation
graph (to be finished as a follow up).
This diff utilizes the same approach that is done when we calculate node
promotions:
1/ AssignmentRule and DereferenceRule know that some issues are related
to provisional annotations and expose this information.
2/ ClassLevelAnalysis collects all the issues, extract violations from
them, and outputs them.
Next step will be changing the output to the meta-issue printing the
annotation graph in json.
Reviewed By: artempyanykh
Differential Revision: D24621410
fbshipit-source-id: c240da3aa
Summary:
In the previous diffs, we introduced
AnnotatedNullability.ProvisionallyNullable. This is a symmetric change
introducing the same for Nullability.t
Note how ProvisionallyNullable is made `is_nonnullish`. This means that
if we run nullsafe in --nullsafe-annotation-graph mode, the following
should start happening:
1/ A lot of new issues will be recorded - every time a local method or
non-null param is dereferenced etc.
2/ But all of those new issues will be filtered by corresponding Rules
(e.g. AssignmentRule). So those issues will not manifest to user-visible
errors. However, now we are capable to see and analyse those hidden
issues in ClassLevelAnalysis. This is exactly what we will be doing in
follow up diffs.
Note that the place of ProvisionallyNullable in the hierarchy is
somewhat arbitraily. It is important to make it less pri then Nullable -
but when it comes to comparing with other "nonnullish" modes - we can
tweak the behavior later on.
Reviewed By: artempyanykh
Differential Revision: D24621411
fbshipit-source-id: 3b99e5e55
Summary:
The fact that we did not preserve all joins (and used only the first
one) was a shorcut since the origin is used only for reporting, and
reporting the info about left joinee is enough for practical purposes.
Hovewer, to support annotation graph, we need all joinees.
This diff is a no-op since we use only the first one currently.
Reviewed By: artempyanykh
Differential Revision: D24621412
fbshipit-source-id: 8fe1174e7
Summary:
This was perhaps needed at some point, but now there is no need in this.
See the next diff that refactors InferredNullability so we store all
joinee results there.
Reviewed By: artempyanykh
Differential Revision: D24621413
fbshipit-source-id: d6f406b87
Summary:
This is the basic building blocks for the future annotation graph. This
diff introduces the first abstraction to be iterated on.
Reviewed By: artempyanykh
Differential Revision: D24621414
fbshipit-source-id: 19acdf216
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:
We in fact build NullsafeIssue.t by parts, filling needed details until
all the info is provided.
This is nicely expressed via partial functions.
See the next diff that uses this to add additional information to the created
`NullsafeIssue.t`
Reviewed By: skcho
Differential Revision: D23706674
fbshipit-source-id: ca5fac704
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:
This diff introduces `NullsafeIssue.t`: information about the issue
ready to be reported (and put to `result.json`).
Its notion was already implicitly used in a lot of code.
With this change, the achitechure becomes the following:
Firstly, `TypeErr.err_instance` represents issues at the moment of registration
during the typecheck. At this moment we don't always want to report
them, but it is important to store even non-reportable ones (we use it to calculate mode promotions).
Secondly, given the current `NullsafeMode.t`, we can either report the
issue or not. This logic lives in
`TypeErr.make_nullsafe_issue_if_reportable_lazy` that normally redirects
this decision to Rules (e.g. AssignmentRule or DereferenceRule).
Thirdly, if we want to report the issue, it is time to actually figure
out what to report (e.g. the precise error message, or additional
nullsafe specific `.json` params - see next diffs adding them).
Note that the exact logic of deciding if we want to report / how to
report / what should the message or .json param be is in practice
coupled (otherwise we'd have weird bugs when we decided the issue is
reportable, but don't have a good user facing reason).
In practice such logic for complix issues leaves in Rules.
C.f. `DereferenceRule` and `AssignmentRule` code.
The tricky part is that those rules actually share some common code
responsible for reporting, e.g. when it comes to processing third
parties (so the decision making & reporting is unified). See
`ErrorRenderingUtils.mk_nullsafe_issue_for_untrusted_values` which is
called both from `AssignmentRule` and `DereferenceRule`.
`NullsafeIssue.t` glues together those shared parts of code, and make
dependencies explicit.
Check out the next diff when we add more capabilities to
`NullsafeIssue.t` (e.g. ability to store dependent third party methods).
Without this refactoring, implementing this feature would be rather
tricky.
Reviewed By: skcho
Differential Revision: D23705587
fbshipit-source-id: b5246062a
Summary:
Since we don't yet support `.sig` files for fields, hardcoding this in Ocaml
for now.
Reviewed By: jvillard
Differential Revision: D23627390
fbshipit-source-id: 8df78068d
Summary: Most of the time, when the procdesc of a callee is requested, all that is really required is the procedure attributes. However, requesting the procdesc may return `None` when the procedure is undefined (in Java, and soon for Clang too). So, change all callsites to using attributes instead, where possible.
Reviewed By: jvillard
Differential Revision: D23539422
fbshipit-source-id: 3b1a52d48
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:
This is part of work aimed to reduce usage of language-agnostics modules
in Java-specific parts of nullsafe.
Reviewed By: ngorogiannis
Differential Revision: D23075333
fbshipit-source-id: 4029732b4
Summary:
This is one of two files in nullsafe that remains interface-less.
Let's fix it!
Reviewed By: jvillard
Differential Revision: D23056324
fbshipit-source-id: a6592f7ae
Sungkeun Cho
Andrey Makeev
Mitya Lyubarskiy
Joshua Selbo
Artem Pianykh
Jules Villard
Nikos Gorogiannis