Summary:
Invariant map of Inferbo, a map from node to abstract memory, is used in other checkers. In order to
avoid duplicated Inferbo analyses, it caches calculated invariant maps independently to summary
DB. The reason is that Inferbo's summary contains one abstract memory of the exit node, rather than
a map from all nodes to abstract memories.
This diff changes it to keep only one invariant map at a time. Since registered checkers are
applied to the same function one by one, usually the singleton cache should be enough to avoid the
duplicated Inferbo running. (I can think of a corner case when multiple checkers depend on
Inferbo's results and Inferbo's modeled functions are different to that of them, but it should work
in most of the cases.)
Reviewed By: jvillard
Differential Revision: D20248172
fbshipit-source-id: db86655d0
Summary:
Both TypeOrigin.Undef and TypeOrigin.OptimisticFallback are bad and
should be killed.
Let's start with Undef as the biggest offender, then try to gradually
reduce usage of the second one.
It is super unclear what does Undef even mean, and actually the code that
tries to use it (in a way that I can not fully comprehend) occurred to be "almost" no-op.
"Almost" means that the only place that is affected is
CONDITION_REDUNDANT checks: we have few extra (FP) warnings of his type.
Mostly those correspond to comparing with null result of array member
access: `myArray[i] == null` is marked as redundant. And I even don't
know how come it was not showing up before: we do assume all calls to
array are optimistically non-null (see TypeOrigin.ArrayAccess).
Anyways, we give up on this one: condition redundant is "broken" anyway
(has too many FP to be surfaced to the user); and when/if we want to fix
it, we can easily support array access idiomatically.
Reviewed By: jvillard
Differential Revision: D20248988
fbshipit-source-id: b20f61fd0
Summary: The classpath module doesn't need to know about model handling. Also, strengthen some invariants, use option types instead of strings, and hash sets instead of tree sets.
Reviewed By: skcho
Differential Revision: D20228873
fbshipit-source-id: 18b6bb276
Summary:
`TypeCheck.typecheck_node` is one of central parts in nullsafe
typechecking.
Lets make the code clearer:
1. Clear contract of the method - return a named record instead of a
tuple of lists.
2. Comments.
3. Making code functional, use List.fold and aggregate all needed data
there instead of storing information in references
4. Helper functions.
Reviewed By: skcho
Differential Revision: D20246273
fbshipit-source-id: 75f56497e
Summary:
This is the kind of property for which the previous syntax forced one to
use spurious registers.
Reviewed By: ngorogiannis
Differential Revision: D20118863
fbshipit-source-id: b49740d33
Summary:
See comment in the code.
Similarly to what we are already doing with RecentlyNullable, we should
treat this annotation exactly like NonNull.
Reviewed By: artempyanykh
Differential Revision: D20219284
fbshipit-source-id: 01f1127a6
Summary: This function uses uncompiled regexps plus it forgets to close the file when it finds a package declaration. Fix by using Core operations on strings.
Reviewed By: artempyanykh
Differential Revision: D20192957
fbshipit-source-id: 317caacea
Summary: When reporting CapturedStrongSelf we shouldn't report it when the block is "no escaping", as this won't lead to a retain cycle, so capturing strongSelf is ok.
Reviewed By: jvillard
Differential Revision: D20224359
fbshipit-source-id: c60dae333
Summary:
Update handling of `OffsetOfExpr` based on the new type definition
from updated version of clang-plugin.
Together with the change to clang-plugin, this essentially fixes hard
crash while analysing C/C++ files with non-literal `offsetof`
expression.
Fixes GH issues [#1178](https://github.com/facebook/infer/issues/1178), [#1212](https://github.com/facebook/infer/issues/1212)
Reviewed By: jvillard
Differential Revision: D20159173
fbshipit-source-id: 65fc228a4
Summary: Make the `ProcessPool` use one pipe per worker for worker-to-master communication.
Reviewed By: ngorogiannis
Differential Revision: D20158845
fbshipit-source-id: dc15607f8
Summary: Restore the global state also when `RestartScheduler.ProcnameAlreadyLocked` exceptions are catched.
Reviewed By: ngorogiannis
Differential Revision: D20189524
fbshipit-source-id: 8f8de5309
Summary:
This is so that all pre-analyses are together instead of spread across
several modules.
PS: this function is the worst.
Reviewed By: ngorogiannis
Differential Revision: D19973285
fbshipit-source-id: b326e99cd
Summary:
The check for whether a buck target needs quoting is incorrect, since it uses `-` inside a character class to denote a dash, not a character range. The correct way to do this is to put the dash as the first character in the class (or last).
Facebook
iknowthis_regex
Reviewed By: jberdine
Differential Revision: D20159112
fbshipit-source-id: be6750ed8
Summary:
This diff renames `ZERO_XXX` issues to more appropriately named and descriptive
`XXX_UNREACHABLE_AT_EXIT` and replaces bottom with
unreachable in cost kinds and issues.
Reviewed By: skcho
Differential Revision: D20140301
fbshipit-source-id: eb6076b30
Summary:
1. It is convenient to stick with the policy "ERROR if and only if it is
enforced". Among other, it makes CI integration much easier to implement
(enforcemend, UI and messaging is decided based on severity).
2. Since Nullsafe annotation is an idiomatic way to indicate classes
with enforced nullability checking, we want it to be the only way to
enforce issues.
3. This means we decrease the priority of GraphQL violation issues.
(In practice they were not enforced so we have plenty of violations in
codebase to reflect reality). The proper way dealing with GraphQL will
be detecting such issues as a special issue type and prioritizing fixing
and Nullsafe-ifying corresponding classes.
4. Among other, we downgrade severity of field overannotated to advice
to keep it consistent with condition redundant.
Reviewed By: artempyanykh
Differential Revision: D20141420
fbshipit-source-id: e2f12835a
Summary:
The issue type `ZERO_EXECUTION_TIME` actually corresponds to bottom state but has been mistakenly used to mean
- unreachable nodes (program never reaching exit state)
- having zero cost (e.g. for allocations).
Note that, for execution costs, the latter doesn't make sense since we always incur a unit cost for the start node. Hence, a function with empty body will have unit cost. For allocations or IO however, we only incur costs for specific primitives, so a function with no allocations/IO could have a zero cost. However, there is no point reporting functions with zero cost as a specific issue type. Instead, what we want to track is the former, i.e. functions whose cost becomes 0 due to program never reaching exit state.
This diff aims to split these cases into two by only reporting on the latter and adds traces to bottom/unreachable cost by creating a special category in polynomials.
Next diff will rename `ZERO_XXX` to `XXX_UNREACHABLE_AT_EXIT`.
Reviewed By: skcho
Differential Revision: D20005774
fbshipit-source-id: 46b9abd5a
Summary: When the progress bar was off (`--no-progress-bar`) the `TaskBar` didn't log updates but the workers still sent them to the master process. Now, the workers no longer send updates to the master process when the progress bar is off.
Reviewed By: ngorogiannis
Differential Revision: D20140577
fbshipit-source-id: 560d56991
Summary:
For Mode.Local this is kind of obvious decision.
But this diff does the same for strict mode as well.
See comment in [ExplicitNonnullThirdParty] for the detailed explanation.
Reviewed By: artempyanykh
Differential Revision: D20140056
fbshipit-source-id: 13c66df81
Summary:
In the previos diff we restructured error rendering utils for
TypeOrigin.MethodCall.
In this diff we do the same with TypeOrigin field: lets make the code
consistent.
We also clearly distinct third party from all other possible cases in
this branch.
This changes messaging and reported errors for strict modes (see test cases), and I believe this is a net improvement.
Reviewed By: artempyanykh
Differential Revision: D20139741
fbshipit-source-id: 84f502553
Summary:
Since artempyanykh introduced proper type for third party methods, we don't need
to write a sketchy heuristic in this place.
This will simplify shipping a feature in the follow up diff (otherwise
it would break here).
Reviewed By: artempyanykh
Differential Revision: D20139460
fbshipit-source-id: 00144dc48
Summary:
> We don't report when the cost is Top as it corresponds to subsequent 'don't know's. Instead, we
> report Top cost only at the top level per function
The previous code just ignored top costed nodes, so it was able to report a non-top cost that was
from another node. For example,
```
void foo() {
linear-cost();
top-cost();
}
```
It reported inconsistent reports: `EXPENSIVE_EXECUTION_TIME` with a linear cost and
`INFINITE_EXECUTION_TIME` at the same time.
This diff fixes it not to report `EXPENSIVE_EXECUTION_TIME` when there is a node with the top cost.
Reviewed By: ezgicicek
Differential Revision: D20139408
fbshipit-source-id: 9fedd4aec
Summary:
In the previous report, it reported the first cost of node that exceeds a threshold. However, this
may hide a bigger cost of node that appears later. This diff changes this to report the biggest
cost of node among the costs exceeding the threshold.
Reviewed By: ezgicicek
Differential Revision: D20116162
fbshipit-source-id: 06199fb46
Summary: No need to print the whole trace when there are other ways to view it.
Reviewed By: jberdine
Differential Revision: D20138515
fbshipit-source-id: 9765db2f0
Summary: The analysis time wall time was logged but not it wasn't a part of `BackendStats`. The analysis time has metric has been moved to `BackendStats` and also includes the user and sys times of the scheduler process.
Reviewed By: ngorogiannis
Differential Revision: D20115345
fbshipit-source-id: bd3f3d276
Summary:
Current domain of Inferbo cannot handle float values. This diff evaluates float constants to the top
interval.
Reviewed By: ezgicicek
Differential Revision: D20116361
fbshipit-source-id: e6e398bbd
Summary:
This syntax
- is less confusing (according to several people who are not me);
objectively, there's less magic under the hood
- gives fine control over register number (because condition/action are separated)
- lets one compare values of different arguments of the same call
(e.g., one could have a transition that is taken only if two
arguments of a method call are equal)
Reviewed By: ngorogiannis
Differential Revision: D20005403
fbshipit-source-id: fad8f3b3d
Summary:
The test shows what that TOPL can express, in addition to bugs,
efficiency properties. However, there seems to be an underlying problem
in biabdaction that prevents this particular problem from being caught.
Reviewed By: ngorogiannis
Differential Revision: D20005404
fbshipit-source-id: 466f79050
Summary:
# Current design
Infer analysis is currently two staged:
1) proc-level callbacks calculate summary, including writing down the
issues if applicable.
2) file-level callbacks (formerly cluster callbacks, see the prev diff) are executed next; they are supposed to emit
additional issues that are impossible to emit based on mere
proc-context.
Currently RacerD and Starvation use file-level callback; in near future
we plan to onboard Nullsafe checker as well.
# Problem
Contract of callback (1) is clear: given a proc and existing
summary, the checker updates it and returns a modified summary. This
summary later on gets serialized (in-memory + external) and can be consumed by
other chechers. Issues written in summary will get reported when
analysis is over.
In constrast, contract of (2) is wild west: the function returns unit.
In practice, what the checkers do is create IssueLog and serialize it to
checker-specific directory.
Then another part of program (InferPrint.ml) knows about this side
effect, reads the error log for checkers and ultimately get it reported
together with errors written at stage (1).
This is problematic because it is hard to reason about the system and it
makes onboarding new checkers to (2) error-prone.
# This diff
This diff brings (2) on par with (1): now file-level callback has a
clear contract: it should be side effect free, and the only
responsibility is to fill out and return IssueLog.
Additionally, we make the notion of "checker-specific issue directory"
an official thing, so the checker only needs to specify the name,
everything else will be made automatically by orchestation layer,
including cleanup.
# Starvation
Implementing the new contract is starvation is possible and desirable, but involved: see comment
in the code, so we leave it up to the future work to fix that.
Reviewed By: ngorogiannis
Differential Revision: D20115024
fbshipit-source-id: fb2f9b7e6
Summary: Add the wall time to the ExecutionDuration. If this is not included we are not considering off-cpu time.
Reviewed By: ngorogiannis
Differential Revision: D20099667
fbshipit-source-id: 49dbfd739
Summary:
Currently the call graph of all captured procedures is loaded and then traversed to flag reachable procedures from modified files, followed by deleting the unflagged part, and unflagging the rest. This is a bit wasteful, and doesn't lend itself nicely to constructing directly the reverse call graph, which further diffs will do.
This diff loads all captured procedures and callees in a hashconsed table, and performs a BFS from procedures in modified files, to build the call graph in one pass.
Reviewed By: fgasperij
Differential Revision: D19888965
fbshipit-source-id: eeb59356e
Summary:
To ease scheduling, it would be best to only load the procnames of procedures that are (a) defined and (b) reachable from the modified files. The frontends play various games with the DB properties:
- In Clang all methods have a CFG even if they are undefined. Also, looking for non-NULL CFG rows in the DB brings up methods unreachable from modified files (?).
- In Java, some procedures have NULL CFGs. In addition, some of those have `attr_kind!=0`.
We only load those procedures that have both non-NULL CFGs and `attr_kind!=0`. That seems to give meaningful numbers, esp. wrt reachable procedures from files.
Reviewed By: jberdine
Differential Revision: D20068376
fbshipit-source-id: 992b65b4a
Summary: The semantics of the `values` function of Java enum class was missing, when it is called outside the class initializer. This diff gets the size of the enum elements from the summary of class initializer function, `<clinit>`.
Reviewed By: ezgicicek
Differential Revision: D20094880
fbshipit-source-id: 7362bba1c
Summary: We had no tests that resulted in `ZERO_EXECUTION_COST`. Let's fix that.
Reviewed By: skcho
Differential Revision: D20097504
fbshipit-source-id: 56c23fea0
Summary:
1. Some invariants are tricky enough to be documented. This is especially
important for cases related with error reporting. Lets document it.
2. Cluster callback -> File callback rename.
Reviewed By: ngorogiannis
Differential Revision: D20093932
fbshipit-source-id: e716f1f5b
Summary:
When trying to add annotations to code examples, Javadoc gets confused about `@` sign and there's no good way to fix it so it's both OK to read as a comment in the editor and as a Javadoc HTML, no matter what combination of <code>/<pre>/{code} or escaping you use.
Here we prioritise ability to read the code comment from the editor and therefore the comments are detached from annotations.
Facebook
Reviewed By: mityal
Differential Revision: D20093801
fbshipit-source-id: 25867c27a
Summary:
Now when typechecking a class `A` marked with `Nullsafe(LOCAL)`,
classes from trusted list are properly recognized and nullability of
method params and return value are refined to `LocallyCheckedNonnull`
in a context of class `A`.
NOTE: refininng nullability when **accessing fields** on trusted classes
is **not implemented yet**, because the whole business of handling fields
in nullsafe is somewhat convoluted. This should not be a huge issue
though, since in Java fields are commonly accessed via getters any
way.
Reviewed By: mityal
Differential Revision: D20056158
fbshipit-source-id: 496433d90
Summary:
This ignores the error memory status (e.g. when condition expression is evaluated to bottom), in
order to keep analyze following code.
```
if ( e ) // e is evaluated to bottom due to a problem of Inferbo {
... // code here was not analyzed before
}
```
Reviewed By: ezgicicek
Differential Revision: D20067434
fbshipit-source-id: a1713722c
Summary:
This will help making error reporting more actionable.
Often methods that are nullable in general (like View.findViewById) are used as not-nullable due to app-invariants. In such cases suggesting a non-nullable alternative that does an assertion under the hood makes the error report more actionable and provides necessary guidance with respect to coding best practices
Follow up will include adding more methods to models.
If this goes well, we might support it in user-defined area (nullability
repository)
Reviewed By: artempyanykh
Differential Revision: D20001416
fbshipit-source-id: 46f03467c
Summary: Count the time used by the `RestartScheduler` for analysis (useful) and the time wasted because a a worker was not able to take a lock to have a metric to compare different versions of the scheduler. The wasted time is not actually count but it can be calculated by substracting useful time from the total time. This was implemented like these to avoid substractions that may make the floating point calculations more complicated.
Reviewed By: ngorogiannis
Differential Revision: D19969960
fbshipit-source-id: 68a1132ca
Summary: When a worker fails because it can't a get the lock of a `Procname` it will include it in the exception that it throws so the `RestartScheduler` can record it as a dependency. Then when scheduling a new work item from `RestartScheduler.next` it will check if this dependency is already met, if it isn't it will not schedule the `Procname` yet.
Reviewed By: ngorogiannis
Differential Revision: D19820331
fbshipit-source-id: b48cacc9a
Summary: Add files and procedures to the `RestartScheduler`'s work queue. This makes the chaining with the FileScheduler unnecessary so it's removed.
Reviewed By: ngorogiannis
Differential Revision: D19942354
fbshipit-source-id: 59e25c1c2
Summary: This diff finds dead modules, i.e, .ml files that is not used in the binaries.
Reviewed By: ngorogiannis
Differential Revision: D20035984
fbshipit-source-id: 56ac2e817
Summary:
Lets not also describe what it does, but emphasize that it is
nullsafe-specific and also what are benefits of using it.
Reviewed By: artempyanykh
Differential Revision: D20030497
fbshipit-source-id: f28c803fd
Summary:
The `--continue-analysis` option enables continuing analysis after more targets are captured by
`--continue`. For example,
```
$ infer capture -- buck build tgt1
$ infer analyze --merge
$ infer capture --continue -- bucck build tgt2
$ infer analyze --merge --continue-analyze
```
In the last analysis, it reuses the analysis results of `tgt1` from the previous analysis. If
`tgt1` and `tgt2` have a same dependency to a library, the analysis results of the library is also
reused.
Reviewed By: dulmarod
Differential Revision: D19996598
fbshipit-source-id: bb6874a6f
Summary:
Introduction of `ThirdPartyNonnull` nullability broke nullability
refinement heuristic for enums. This diff fixes it and also adds tests
so that we hopefully avoid such issues in future.
Reviewed By: mityal
Differential Revision: D19975810
fbshipit-source-id: f9245f305
Summary:
We need to be able to differentiate `UncheckedNonnull`s in internal vs
third-party code. Previously, those were under one `UncheckedNonnull`
nullability which led to hacks for optmistic third-party parameter
checks in `eradicateChecks.ml` and lack of third-party enforcement in
`Nullsafe(LOCAL, trust=all)` mode (i.e. we want to trust internal
unchecked code, but don't want to trust unvetted third-party).
Now such values are properly modelled and can be accounted for
regularly within rules.
Also, various whitelists are refactored using
`Nullability.is_considered_nonnull ~nullsafe_mode nullability`.
`ErrorRenderingUtils` became a tad more convoluted, but oh well, one
step at a time.
Reviewed By: mityal
Differential Revision: D19977086
fbshipit-source-id: 8337a47b9
Summary:
Add support for nullsafe mode with `trust=all` and `trust=none` a case
with a specific trust list is not supported yet and needs to be
implemented separately.
Tests introduce one unexpected
`ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION` issue which
complains about `this` having incorrect nullability; it is a bug and
needs to be fixed separately.
Reviewed By: mityal
Differential Revision: D19662708
fbshipit-source-id: 3bc1e3952
Summary: Was broken by previous diff: I forgot to update issues.exp
Reviewed By: artempyanykh
Differential Revision: D20001233
fbshipit-source-id: 67f534349
Summary: In all other cases we have period at the end, which is inconsistent.
Reviewed By: artempyanykh
Differential Revision: D20001065
fbshipit-source-id: 85ec6d751
Summary:
This helps debug nullsafe. Before, we would only print the initial and
last state of a given node but now we can see all the intermediate steps
too.
Example before:
```
before:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
after:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
```
After:
```
before:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
instr: n$0=*&this:Toto* [line 10]
new state:
n$0 -> [this] [StrictNonnull] Toto*
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
...
instr: EXIT_SCOPE(n$0,n$1,this); [line 10]
new state:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
```
Reviewed By: mityal
Differential Revision: D19973278
fbshipit-source-id: bcea33f96
Summary:
Use a record of package, class name to store (qualified) Java class names. This saves the round trip of concatenating then splitting again, etc, as well as saves some memory in the type environment as now the package paths can be shared across classes of the same package (about 10% in tests).
Also remove some unfortunate APIs.
Reviewed By: jvillard
Differential Revision: D19969325
fbshipit-source-id: f7b7f5a55
Summary: Change the ProcLocker implementation to use symlinks instead of files. Tests have indicated that they may use less resources.
Reviewed By: ngorogiannis
Differential Revision: D19822048
fbshipit-source-id: 991ababf2
Summary:
The previos one was too broad and did not indicate the main intended
usage, which is currently the nullsafe typechecker.
Also it was misleading: Initializer methods should NOT be called inside
constructors.
Finally, it recommended using Initializer in Builder pattern, which is
a questionable idea, so it better to avoid mentioning builders.
Reviewed By: artempyanykh
Differential Revision: D19942675
fbshipit-source-id: 0eb1ce796
Summary: The way `Mangled.t` is used in `JavaClassName` means that it's always a plain string (we never have a "mangled" part). Remove the indirection and extra allocation. Also, simplify the API by throwing away one function that was used just once and wastefully.
Reviewed By: artempyanykh
Differential Revision: D19950672
fbshipit-source-id: b61fcba6e
Summary: Instead of converting the class type name of a java procedure to a string and then back to a type name, just get it directly.
Reviewed By: jvillard
Differential Revision: D19950528
fbshipit-source-id: dadf6d130
Summary: Rather than recomputing the `proc_name`, let's pass it around.
Reviewed By: skcho
Differential Revision: D19951461
fbshipit-source-id: 90b57dcc7
Summary: This diff suppresses integer overflow issues in functions that includes "hash" in its name.
Reviewed By: jvillard
Differential Revision: D19942654
fbshipit-source-id: d86fa4f00
Summary: In line with changes to loom query in D19903057, let's adjust Infer's processing of the results.
Reviewed By: martintrojer
Differential Revision: D19902933
fbshipit-source-id: 200b3a03e
Summary: Add the number of cores used and the scheduler type to the environment info printed before running.
Reviewed By: ngorogiannis
Differential Revision: D19941243
fbshipit-source-id: 576e9f610
Summary: Not needed any more as infer's concurrency isn't controlled via make.
Reviewed By: jvillard
Differential Revision: D19905712
fbshipit-source-id: f97ef4421
Summary:
Some annotation processors / transormers might generate artificial fields. These
are not expicitly written by code writer, hence non actionable.
We distinct this fields heuristically: if they start with "$", they are
surely not user-written.
Reviewed By: ezgicicek
Differential Revision: D19947988
fbshipit-source-id: a0a15fc23
Summary:
Add let*/+ syntax to `result` types to simplify all the applications of
`>>=`, `>>|` that are followed by a binding (eg `>>= fun x -> ...`) in
pulse.
Reviewed By: skcho
Differential Revision: D19940728
fbshipit-source-id: 4df159029
Summary:
We can already tell that a summary cannot be applied by raising
`Contradiction`, so use this mechanism to stop applying a summary if the
number of formals doesn't match the number of actuals provided.
Previously we would return an option type and `None` in case of
mismatch, on top of the `raise Contradiction` mechanism (used for
aliasing and arithmetic contradictions).
This changes the behaviour of pulse in this case: before we would skip
over the function call, but now we stop the analysis.
Reviewed By: dulmarod
Differential Revision: D19940729
fbshipit-source-id: 6def40cd6
Summary: Once we identify a weakSelf variable that is being used in a Noescape block, we want to report only the first occurrence.
Reviewed By: skcho
Differential Revision: D19941502
fbshipit-source-id: 2b6d4648b
Summary: For each variable that we identify as a captured strong self, we want to report only the first occurrence.
Reviewed By: skcho
Differential Revision: D19940031
fbshipit-source-id: f38f642c9
Summary: When we discovered that a strongSelf var was not checked for null, we then report in each occurrence which is spammy. Now we report only the first occurrence. To achieve that, we store a `reported` flag in the domain that gets set to true after we report once, and we only report if it's false.
Reviewed By: jvillard
Differential Revision: D19877218
fbshipit-source-id: c44109ae9
Summary:
This adds `let*/+`, `and*/+` operators for Option. See [the manual](https://caml.inria.fr/pub/docs/manual-ocaml/manual046.html) for more information.
Example usage:
```
let foo =
let open IOption.Let_syntax in
let* a = get_optional () in
let* b = get_another_optional () in
return (a + b)
```
Reviewed By: jvillard
Differential Revision: D19880033
fbshipit-source-id: c7998b0c6
Summary:
Previous implementation supported only stringy params (strings and
stringified bools). Current one exposes a proper variant `Annot.t`,
with support for all possible param values in Java except
numbers (more on that below).
This change is required for implementing `Nullsafe(LOCAL)` as the
annotation used to specify nullsafe behaviour has a more complex
structure than what we've dealt with before.
**Why support for number values was not added**: supporting numbers
requires using `int64`. Unfortunately, adding another variant `Vnum
int64` to `Annot.t` causes a runtime failure on assert in
`MaximumSharing.ml:133`. It seems that it might be enough to flip
`fail_on_nonstring` from `true` to `false`, but since this would
require additional testing and is not required for my case, I'll leave
checking this to whoever needs to use numeric annot params in future.
Reviewed By: ezgicicek
Differential Revision: D19855923
fbshipit-source-id: 878e33856
Summary: No reason to use a set when an integer will suffice. This further reduces GC churn.
Reviewed By: fgasperij
Differential Revision: D19888300
fbshipit-source-id: 9fc8c73f5
Summary: Queues are implemented using a circular array, so should be less GC-heavy than continually allocating/freeing list nodes.
Reviewed By: jberdine, fgasperij
Differential Revision: D18504104
fbshipit-source-id: 93d29c253
Summary:
Building the call graph should be done only in the scheduler process after having forked all workers. This was achieved by a lazy init pattern, whereby the first time `next` was called, it would build the call graph, on the assumption that `next` is only ever called in the scheduler after forking.
D19769741 made this compulsory regardless the scheduler by passing a thunk to `ProcessPool` which is called to obtain the actual scheduler, on the right process and after the fork.
This means we don't need the custom lazy init logic any more. In addition, that set up used a DB query to overapproximate the number of procedures to analyse, because this was supposed to be provided *before* forking. Now this is also not needed, and on top of that we can provide the exact number after building the call graph.
Reviewed By: ezgicicek
Differential Revision: D19833974
fbshipit-source-id: 7f6d51d93
Summary:
In Inferbo, the bottom memory is introduced when a node is unreachable by pruning, i.e.
`[[e]] <= [0,0]` on `prune(e)`. This diff distinguishes whether `[[e]]` is `[0,0]` (unreachable)
or bottom (it could not evaluate `e` by some unknown reasons).
Reviewed By: ezgicicek
Differential Revision: D19902046
fbshipit-source-id: 7706017d6
Summary:
More newer = more better.
This flips the Not_found -> Not_found_s switch, and forbids a bunch more
polymorphic comparisons (mostly turned into `int` comparisons for
convenience). Earlier diffs prepare for this so this diff is only about
breaking changes in the API, of which there are only a few.
Reviewed By: jberdine
Differential Revision: D19861583
fbshipit-source-id: fe54ce8f0
Summary:
Core v13 APIs stopped raising `Not_found` and instead raise
`Not_found_s`, which wreaks havoc in our codebase. Carefully inspect
each `Not_found` and add `Not_found_s` where needed (that way it's
compatible with both Core v12 and v13 for now).
Reviewed By: jberdine
Differential Revision: D19861585
fbshipit-source-id: 9a5361ae9
Summary:
The big one:
- stop using polymorphic `<>`, `<`, `>`, ..
- add `<>` to `PolyVariantEqual` escape hatch now that `<>` is as taboo as `=`
- Interestingly, there were a lot of uses of `Z.(x < y)`, which although
they seem to use `Z.lt` actually used polymorphic comparison. The actual
comparison infix operators of `Z` are cleverly hidden in `Z.Compare`
instead, which makes them impractical to use...
Reviewed By: jberdine
Differential Revision: D19861584
fbshipit-source-id: 5dce08ad9
Summary:
Polymorphic compare is bad, 'mkay? I also tried changing this to do what
the comment says the function does (keep the first term instead of the
"largest" term) but that resulted in test changes so I kept the original
behaviour instead.
Reviewed By: jberdine
Differential Revision: D19861587
fbshipit-source-id: d57fd4a02
Summary:
That trick briefly worked when it was introduced but I think a change in
merlin broke it again as I see "sexp_list", "sexp_option" in the types
shown by merlin again. Disgusting!
Remove useless hack. Also referring to types such as `sexp_list` is
deprecated in core v13 for some reason.
Reviewed By: jberdine
Differential Revision: D19861586
fbshipit-source-id: 1c4c3af13
Summary:
This adds a violation of baos.topl found in github/seata/seata. However,
it is not a bug (see comment in commit).
Reviewed By: ngorogiannis
Differential Revision: D19518641
fbshipit-source-id: e219245ee
Summary:
Since Javalib 3.2, a new feature allows to rewrite
methods that contain (some specific form of) closures. Infer
now uses it. When loading each class we rewrite them and
new classes generated by Javalib to implements closures
(i.e. Java interfaces)<
Reviewed By: ngorogiannis
Differential Revision: D19389227
fbshipit-source-id: 245dd4404
Summary:
When finding a proper constructor for `std::make_shared`, the given parameter types are sometimes
slightly different, e.g., const int vs int. This diff loosens the condition of the types on finding
constructors.
Reviewed By: ngorogiannis
Differential Revision: D19743198
fbshipit-source-id: f90213109
Summary: Instrument the ProcLocker to get the aggregated systime that is used by `lock` and `unlocks`.
Reviewed By: ngorogiannis
Differential Revision: D19814554
fbshipit-source-id: 5fd928b9c
Summary:
This was introduced in D19770219, and skcho caught it, but for some
reason I seemed to have a blind spot and confused the semantics of `instanceof`.
Reviewed By: skcho
Differential Revision: D19813031
fbshipit-source-id: d939b981b
Summary:
This diff fixes the clang translation for switch statement. It assumed that `default:` comes always
at last, which introduced some unreachable nodes inadvertently, e.g. when `default:` comes at first.
Reviewed By: dulmarod
Differential Revision: D19793138
fbshipit-source-id: 1e8b52c0d
Summary: After looking at some reports with blocks inside blocks, it seemed more obvious that adding which method we are talking about makes more clear which block we are talking about.
Reviewed By: mityal
Differential Revision: D19789285
fbshipit-source-id: 20e0e6804
Summary:
The domain has a notion of lock state (taken/not taken) and any access occurring will remember the current lock state.
Methods in Java can be `synchronized` meaning the lock is taken automatically at method start and release on return.
Currently, instead of starting analysis of a `synchronized` method with an initial lock state of "lock taken", the summary would be computed as if there is no lock, and then a caller would peek at whether the callee is `synchronized` and change the callee summary accordingly before taking it into account. Also, when an access happens (not a call) the analysis always consults the pdesc of the current method to check whether the method is `synchronized`.
Do things the right way: if method is `synchronized`, start with the lock taken. When the method exits, the lock-state postcondition is computed by releasing once the lock.
This is a behaviour-preserving change.
Reviewed By: mityal
Differential Revision: D19742559
fbshipit-source-id: 1d0fce3f6
Summary: This diff makes the taint analysis in Inferbo inter-procedural: adding symbolic taint values and substitute them at function call statements.
Reviewed By: ezgicicek
Differential Revision: D19411022
fbshipit-source-id: 4ff9a590a
Summary:
We already warn about lack of nullable annotations in `equals()`, and even have a specialized error message for that.
But lack of an annotation is not as severe as direct dereference: the
latter is a plain bug which is also a time bomb: it will lead to an NPE not immediately.
This is widespread enough to be reported separately.
Reviewed By: dulmarod
Differential Revision: D19719598
fbshipit-source-id: a535d43ea
Summary:
Since we fixed a bug in implementation of FalseOnNull (see stack below),
we can finally ship this change.
Side note: this change is essential for the follow up diff (which adds extra check
for user-defined implementations of equal()), without it the follow
up change would introduce a lot of false positives.
Reviewed By: ngorogiannis
Differential Revision: D19771057
fbshipit-source-id: 7d7cf1ef7
Summary:
If we managed to whitelist a function as TrueOnNull, we should teach
nullsafe the nullability of its arguments, otherwise it will ask not to
pass null here.
This fixes a silly FP warning, see the test.
Reviewed By: dulmarod
Differential Revision: D19770341
fbshipit-source-id: 0f861fae1
Summary:
Yay, the previous refactoring finally makes it possible to do some actual
changes to the code in `TypeCheck.ml`!
Changes in this diff:
1. Fixes the bug: TrueOnNull and FalseOnNull were working only for
static methods. Surpsingly nobody noticed that. It is because the first
argument for non-static method was `this`.
2. Behavior change: TrueOnNull/FalseOnNull were not working correctly
where there are several argumens. See the task attached for the example
of the legit usecase. Now the behavior is the following: if there are
several Nullable arguments infer nullability for all of them.
Reviewed By: skcho
Differential Revision: D19770219
fbshipit-source-id: 7dffe42cd
Summary:
This diff proceeds clean up the mess in TypeState.ml
This refactoring unblocks the change in the logic for true on null and
allows to fix a bug, see follow up diffs.
The current code is trying to do two things at once: processing boolean
results (which normally need manipulations with the argument of a
function + additional logic for containsKey), and comparisons witn null
(which requires manipulation with return value in typestate plus check
for redundancy).
All this was done in sort of generic fashion, which, among other, lead
to weird situations for edge cases, e.g. in processing containsKey we used to override nullability
already inferred as a non-null twice with losing information about
original nullability; which made logs weird).
Reviewed By: ngorogiannis
Differential Revision: D19768795
fbshipit-source-id: c928e2cff
Summary: The main job the schedulers do is building their work queues. That's being performed before the workers are forked which means they get copied into all of them. These changes push the initialization of the schedulers just after the forking takes place.
Reviewed By: ngorogiannis
Differential Revision: D19769741
fbshipit-source-id: 0b20ddd5c
Summary:
This diff is part of cleaning up of Typestate.ml mess to make it
somewhat maintainable.
This method is always called with `default` equivalent to (exp,
typestate); also there is no semantical need to return typestate because it never
gets changed.
Reviewed By: ngorogiannis
Differential Revision: D19767366
fbshipit-source-id: 173dcbbca
Summary:
This refactoring is made possible by previous stack, which ensured we
don't do two completely different things in one code anymore (processing
results of functions returning booleans and objects in generic fashion).
Follow up diffs will clean up the code for Prune(a != zero) case.
Reviewed By: dulmarod
Differential Revision: D19745050
fbshipit-source-id: 61d3d02ad
Summary:
This refactoring unblocks the changes in follow up diffs (plus fixes a
bug).
So what was happening?
Each comparison with null leads to CFG being splitted into two branches, one branch
is PRUNE(a == null) and another is PRUNE(a != null).
PRUNE(a != null) is where most of logic happens, it is the place where
we infer non-null nullability for a, and this is a natural place to
leave a check for redundancy.
Before this diff we effectively checked the same thing twice, and used
`true_branch` (only one of 2 instruction will have it set to true) as a symmetry breaker.
This diff removes the `true_branch` checks, but leaves only one call out
of two, hence breaking symmetry in a different way.
## Bug fix
The code around the removed check was (crazily) doing two things at
once: it processed results of (returning booleans!)
TrueOnNull-annotated functions AND
results of (returning Objects!) other functions, using the fact that all
of them are encoded as zero literals (sic!).
Not surprisingly that lead to a bug where we accidentally call the check
for non intended places (arguments of trueOnNull functions), which lead
to really weird FP.
This diff fixes it.
Reviewed By: dulmarod
Differential Revision: D19744604
fbshipit-source-id: fe4e65a8f
Summary:
These two methods are called in processing prune instructions, when
instruction is Prune(expr == null) and Prune(expr != null), to correctly
infer nullability in corresponding branches.
Typechecking underlying expr makes little sense for two reasons:
1. In practice, expr it is as simple as a temporary SIL variable
2. If the idea is defensively typecheck everything for case when SIL
produces crazy expressions, well, that is not going to work: the code
around ignores many other forms of expressions, e.g. everything where
expr = <something not equal to null literal>. So this is inconsistent.
This will simplify further cleanup, see follow up diffs
Reviewed By: ngorogiannis
Differential Revision: D19743826
fbshipit-source-id: 319a80ee7
Summary:
The whole TypeCheck.ml is exceptionally hard to read and maintain, lets
clean up it a bit.
Reviewed By: ngorogiannis
Differential Revision: D19743632
fbshipit-source-id: c24c21a85
Summary:
The goals are:
- Increase precision in C-languages by ditching access paths.
- Help with eventually sharing the abstract address module with RacerD.
- Reports are now language-mode specific (eg `->` in clang vs `.` in Java).
It's not exactly access expressions used here. Instead the pattern `(base, access list)` is used where `access` is `HilExp.Access.t`. This is done to ease the way `deriving` is used for creating two comparison functions, one that cares about the root variable and one that doesn't; and also because the main function that recurses over accesses (`normalise_access_list`) visits the accesses from innermost to outermost.
Also, kill some dead code.
Reviewed By: skcho
Differential Revision: D19741545
fbshipit-source-id: 013bf1a89
Summary: We don't use allocation costs in prod at the moment. There is no plan to do so in the near future. Let's not report them anymore and also save some space in `costs-report.json`.
Reviewed By: skcho
Differential Revision: D19766828
fbshipit-source-id: 06dffa61d
Summary:
This diff introduces two issue types: `BUFFER_OVERRUN_T1` and `INFERBO_ALLOC_IS_TAINTED`, which
denotes tainted values are used in array accesses and memory allocations, respectively.
Note that the taint analysis is intra-procedural for now.
Reviewed By: ezgicicek
Differential Revision: D19410536
fbshipit-source-id: af85148ec
Summary:
This diff adds a taint domain in Inferbo. The taint value will be used to find vulnerable array
accesses in the following diffs.
Reviewed By: ezgicicek
Differential Revision: D19391028
fbshipit-source-id: 566b4c0fe
Summary: Fixing bugs in the report_unchecked_strongself_issues_on_args function: we were not recursing if the first arg wasn't a var, and we were not removing the annotations from self in instance methods. Here we fix those issues.
Reviewed By: ngorogiannis
Differential Revision: D19662886
fbshipit-source-id: 03820961e
Summary:
The RestartSchedulerTests were failing because they are run in parallel by OUnit and all share the same output
directory. This makes the different tests collide since they are using the same file locks directory.
Reviewed By: jvillard
Differential Revision: D19765647
fbshipit-source-id: 5390ad14e
Summary:
This test tests PropagatesNullable and TrueOnNull/FalseOnNull
annotations.
Both tests suites grew big so it is hard to observe them at glance and
make changes.
I could not figure out better name for TrueFalseOnNull.java, it is sort
of silly but I optimized for searchability, "FalseOnNull" will be
directly searched and "TrueOnNull" will be searched in IDEs that are
smart enough.
Reviewed By: skcho
Differential Revision: D19724512
fbshipit-source-id: 703961342
Summary:
The problem with is_override that it can be misleading: it does not
check that that class of the first method is a subtype of the second; in
fact it totally ignores the classes.
This method is publicly exposed so lets just call what it does.
Reviewed By: jvillard
Differential Revision: D19724295
fbshipit-source-id: dc0919193
Summary: This diff returns non-symbolic value (top) for unknown external function calls because the symbolic values sometimes make it hard to understand costs.
Reviewed By: ezgicicek
Differential Revision: D18685715
fbshipit-source-id: 1b39c718b
Summary:
Pulse has an extra invalidation mechanism (introduced in D18726203) to prevent something invalid (e.g. `null`) to be passed by reference to an initialisation function. Therefore, it havocs formals passed by reference to skipped functions. However, I don't think this makes sense in Java. So, let's turn it off.
A nice consequence of this is that in impurity analysis, we do not consider functions that call skipped library calls with object arguments as writing to their formals.
Reviewed By: skcho
Differential Revision: D19697110
fbshipit-source-id: 6e3a71f2a
Summary:
To emulate the `ThreadSafe` contract in C++/ObjC, reporting was gated behind a check that ensured a C++/ObjC class has a `std::mutex` member (plus other filters). This is reasonable, but it has some drawbacks
- other locks may be used, and therefore must be added to the member check;
- locking mechanisms that use the object itself as a monitor cannot be modelled (`synchronized` in ObjC)
RacerD already has `ThreadsDomain` which models our guess on whether a method is expected to run in a concurrent context, and which in C++/ObjC boils down to whether the method non-transitively acquires a lock. This should be a good enough indicator that the class should be checked regardless of whether the locks are member fields. This diff gates the C++/ObjC check on that abstract property.
Reviewed By: dulmarod
Differential Revision: D19558355
fbshipit-source-id: 229d7ff82
Summary: This diff removes a dead field, `is_cpp_nothrow` and `is_cpp_noexcept_method`.
Reviewed By: jvillard
Differential Revision: D19489417
fbshipit-source-id: 971a7f533
Summary:
The ProcLocker uses files as locks and relies on the guarantees of the `Unix.open_file` function when using `O_CREAT` and `O_EXCL` simultaneously.
- `setup`: creates a directory for the lock files inside `infe-out` and deletes its content if it already existed.
- `clean`: does nothing for now. Any file locks that may have been left unlocked are removed by the `setup` in the next run. This way the user can see what locks were taken if the program crashes.
- `lock_exn`: try to lock the `Procname` and if it can't releases all the locks that is currently holding.
- `unlock`: removes the corresponding file.
Reviewed By: ngorogiannis
Differential Revision: D19639402
fbshipit-source-id: e02f277ff
Summary:
Revert incomplete/incorrect translation of `synchronized` in ObjC.
The current translation is incomplete because
```
syncrhonized(foo){
return;
}
```
should be translated as
```
__set_locked_attribute(foo);
__delete_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
but instead we get
```
__set_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
The same applies for `break`/`continue` etc
Reviewed By: skcho
Differential Revision: D19718882
fbshipit-source-id: fc49ef529
Summary:
- Thread the two types into one instead of having a record where the `path` field doesn't always make sense (`Class` case).
- Improved pretty printing of class objects (java only).
- Move starvation-specific stuff out of `AbstractAddress` (eg `make_java_synchronized`).
- Slight optimisation of `apply_subst` for when a parameter is used without additional accesses inside a method (then, the substitution need not modify the term substituted for the parameter in any way).
Reviewed By: ezgicicek
Differential Revision: D19639922
fbshipit-source-id: 1cebecf5d
Summary:
`String` and `StringBuilder` both implement `CharSequence`. Let's generalize the model for `String` to `CharSequence` wherever possible and add missing models for
- `StringBuilder.append`
- `StringBuilder.toString`
Reviewed By: skcho
Differential Revision: D19558009
fbshipit-source-id: 0dfdb21af
Summary:
Java's String models were broken for
- initializing a String object with a locally defined constant string (which is an `Object*` in SIL).
- initializing a String object with a `char`/`byte` array
This diff fixes them and also adds models for `new String ()`.
Reviewed By: skcho
Differential Revision: D19662180
fbshipit-source-id: 23968d0aa
Summary:
When the expression resolving to a function to be called could not be
translated to either a proc name or at least an access path, the HIL
translation code would crash. However, this is perfectly possible. Moreover, no
one actually uses the payload of the `Indirect` datatype so no complication
arises from generilising its type to `HilExp.t`, as done in this diff.
Reviewed By: ngorogiannis
Differential Revision: D19691127
fbshipit-source-id: 4c0400ab7
Summary:
`replace_make_shared` is finding a constructor by iterating all fields of type structs, which may be
expensive if there are lots of fields or `replace_make_shared` is called many times with the same parameters.
Reviewed By: ngorogiannis
Differential Revision: D19446260
fbshipit-source-id: c5c279ca5
Summary: This diff fixes the array access checking function for nested global arrays. We had assumed that RHS of `store` statement in SIL does not include array access expression, but that is not true: for global arrays, SIL can have statements like `*LHS = GlobalArray[n][m]`.
Reviewed By: ezgicicek
Differential Revision: D19300153
fbshipit-source-id: 256325642
Summary:
This diff gives semantics of `std::make_shared` as simple constructor, i.e., it changes function
call of `std::make_chared<C>(i)` to the constructor `C(i)`.
Reviewed By: ngorogiannis
Differential Revision: D19432338
fbshipit-source-id: 0d838e555
Summary: This adds a check for when developers use weakSelf (and maybe strongSelf) in a block, when there is no need for it, because it won't cause a retain cycle. In general there is an annotation in Objective-C for methods that take blocks, NS_NOESCAPE, that means that the passed block won't leave the scope, i.e. is "no escaping". So we report when weakSelf is used and the block has the annotation.
Reviewed By: ngorogiannis
Differential Revision: D19662468
fbshipit-source-id: f5ac695aa
Summary:
This attribute is given to parameters of methods that take Objective-C blocks to show that they will be used only in the current context and won't "escape" the context.
We translate it here, with the goal to use it in a new check later. The check is about not using weakSelf in non-escaping blocks, because retain cycles are not possible.
The translation is a bit complex because the annotation comes in the parameter of a method, but in the checker we will need it in the block. So we pass it around in the frontend from the translation of the method call to the translation context and on to the block expression and the block declaration afterwards.
Reviewed By: ngorogiannis
Differential Revision: D19600377
fbshipit-source-id: dd49539bd
Summary:
The difference from default set is mostly in:
1. turning on warnings 32..39 `unused X`,
2. turning off warning 66 `unused open!` since `open! IStd` is present
in pretty much all the files.
3. Non-exhaustive pattern match is now treated as a compilation
error.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19646047
fbshipit-source-id: c84ba628a
Summary:
Refactor all occurences of `is_strict_mode` to use `NullsafeMode`
instead. This will allow introducing _local_ typechecking modes for
nullsafe in the follow up patches.
Reviewed By: ezgicicek
Differential Revision: D19639883
fbshipit-source-id: bdf535b66
Summary: Moving this big tuple to a record, because it's cleaner code, and I need to add another element in the next diff.
Reviewed By: ezgicicek
Differential Revision: D19640389
fbshipit-source-id: 86b1576a0
Summary:
Update from dune 1 to dune 2. The change was mostly straightforward
except that change to (modes ...) default to exe only was somewhat
unexpected.
Anyhow, with the community moving to dune 2 it's good to keep up.
Reviewed By: ngorogiannis
Differential Revision: D19605895
fbshipit-source-id: 1f9830de8
Summary: This will be eventually used in RacerD. Further diffs up the stack will migrate away the starvation-specific aspects.
Reviewed By: ezgicicek
Differential Revision: D19623392
fbshipit-source-id: a72650718
Summary:
Prevent returning a negative cost bound when calling `substring(begin_index, end_index)` when either is possible
- `begin_index < 0`
- `begin_index > end_index`
Instead, return unit cost since such cases either throw `IndexOutOfBoundsException ` at runtime or correspond to having two symbolic bounds that cannot be semantically compared.
Reviewed By: dulmarod
Differential Revision: D19619410
fbshipit-source-id: cf5e8cb7b
Summary:
The "access path" memory model (equal access paths iff equal object addresses) is suited to when aliasing occurs only at the roots (i.e. variables). When there is intentional aliasing in the middle of an access path, this model will miss the aliasing. For instance if `[x.f] == [y.g]`, then also `[x.f.h] == [y.g.h]`, but the latter access paths are unequal.
In Java, non-static inner classes consistently alias `this.this$0` inside an inner class, which points to the "parent" outer-class object. So if two inner-class objects (belonging to different inner classes) access `this(type:InnerClassA).this$0.f` and `this(type:InnerClassB).this$0.f` the equality will be missed (many other combinations exist). This isn't strictly due to the memory model -- any alias analysis would have to do some class invariant inference to detect this.
For this purpose `AccessPath.inner_class_normalize` exists (it replaces `this.this$0` with `this` of the appropriate type), but this breaks the invariant that we know which formal parameter is at the root (there may not even exist a `this` parameter if the method is static). So this was buggy.
Here we simply recursively remove the synthetic field prefix of the accesses list, while computing forwards the object type. This is only applied when we check aliasing across threads. This will also allow actuals/parameters substitutions (stacked diff) which normalisation was breaking.
Reviewed By: jberdine
Differential Revision: D19601455
fbshipit-source-id: 7e42667b6
Summary:
Rename DeclaredNonnull -> UncheckedNonnull, Nonnull -> StrictNonnull.
This is a preparatory step to introduce one more nullability option,
specifically CheckedNonnull to support local nullsafe mode.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19619289
fbshipit-source-id: 12a3ff814
Summary:
This will help adoption of non-transitive strictification modes.
The main use-case we are aiming with this function is:
- During strictification, a method is made nullable, but it is not feasible to fix all
callers straight away. Suppressing nullsafe with an appropriate task or
comment will make it easier to unblock the strictification and move
forward.
Reviewed By: artempyanykh
Differential Revision: D19599098
fbshipit-source-id: 3769bbd3d
Summary:
1. I can not imagine a useful usecase for strict node, lets not
complicate the code.
2. Add docs.
Reviewed By: artempyanykh
Differential Revision: D19599091
fbshipit-source-id: 1d37a717f
Summary:
Reorder assertNotNull and assumeNotNull:
1. More useful methods go first.
2. More useful methods explicitly recommended over less useful.
Reviewed By: artempyanykh
Differential Revision: D19599084
fbshipit-source-id: 1b9ad9ef6
Summary: The translation of closures includes a load instruction for the captured variable, then we add that corresponding id to the closure. This doesn't correspond to an actual "use" of the captured variable in the source program though, and causes false positives. Here we remove the ids from the domain when that id is being added to a closure.
Reviewed By: ngorogiannis
Differential Revision: D19557352
fbshipit-source-id: 52b426011
Summary:
- Add `Nullsafe` annotation as a general mechanism to specify
type-checking behaviour for nullsafe.
- Document annotation params and provide usage examples with
explanations.
- Add tests to demonstrate the behaviour with different type-checking
modes.
No implementation is added. This diff serves as an RFC to hash out the
details before I dive into code.
Reviewed By: ngorogiannis
Differential Revision: D19578329
fbshipit-source-id: b1a9f6162
Summary: Keep the type name of the class as the key in the map constructed from class names to their methods in a file. This will be used later, and also why string?
Reviewed By: dulmarod
Differential Revision: D19557707
fbshipit-source-id: aa8569581
Summary: D19496263 had refactored cost to its own subdirectory. In this process, it introduced a perf degradation by bundling of collection and computation of constraints which relies on imperative union find. This diff fixes that by separating them.
Reviewed By: ngorogiannis
Differential Revision: D19598610
fbshipit-source-id: 0e466522d
Summary: Add the interface that will be used to lock Procedures.
Reviewed By: ngorogiannis
Differential Revision: D19580578
fbshipit-source-id: b5e334b18
Summary: `String.split(regexp)` returns an array that is split by the given regexp. If the regexp doesn't match, the original string is returned. Hence, the resulting array's length must be in `[1, max(1, n_u -1)]` where`n_u` is the upper bound of the string's length.
Reviewed By: dulmarod
Differential Revision: D19578318
fbshipit-source-id: 675af7376
Summary:
Adding a new check as part of the SelfInBlock checker, for cases when a strong pointer to self, that is not self, is captured in the block.
This will cover the following wrong scenarios:
1. strongSelf is a local variable in a block as it should be, and then it's used in a sub-block, in which case it's a captured variable.
2. weakSelf is defined without the weak attribute by mistake.
Reviewed By: ngorogiannis
Differential Revision: D19538036
fbshipit-source-id: 151871745
Summary:
In practice, condition redundant is extremely noisy and low-signal
warning (hence it is turned off by default).
This diff does minor tweaks, without the intention to change anything
substantially:
1/ Change severity to advice
2/ Change "is" to "might be"
3/ Describe the reason in case the origin comes from a method.
The short term motivation is to use 3/ for specific use-case: running nullsafe on codebase and
identify most suspicious functions (that are not annotated by often
compared with null).
Reviewed By: artempyanykh
Differential Revision: D19553571
fbshipit-source-id: 2b43ea0af
Summary:
This is/can be useful for future changes that make the reporting more
precise based on the exact origin.
See the follow up diff as an example.
Reviewed By: artempyanykh
Differential Revision: D19553567
fbshipit-source-id: c2b2c28a1
Summary: Currently, empty summaries are passed to the cluster callbacks. This is pointless and could potentially lead to recomputing already analysed summaries. This change passes only procnames to the callback, and `Ondemand` is used to load the summary or analyse as needed.
Reviewed By: jvillard
Differential Revision: D19544043
fbshipit-source-id: 28ab642c3
Summary: There is no need to provide type environments to cluster analysers, since the execution environment can be used to retrieve those on demand.
Reviewed By: jvillard
Differential Revision: D19543561
fbshipit-source-id: f9b064011
Summary: We were reporting strongSelf Not Checked when the variable was checked in a conditional, this diff fixes that.
Reviewed By: jvillard
Differential Revision: D19535943
fbshipit-source-id: f8e64e1b7
Summary: I noticed when looking into a false positive of strongSelf Not Checked, that there were some inconsistencies in the translation of if statements with an and, with an extra redundant join only if using a method in the condition that returned an object. So I could repro the problem and investigate and found the place of the inconsistency in the translation. This diff fixes it without changing things too much.
Reviewed By: jvillard
Differential Revision: D19518368
fbshipit-source-id: 47a6a778c
Summary:
The order by which the scheduler visits odd and even methods here
will determine if there is any report at all. This is a bad test
so remove.
Reviewed By: fgasperij
Differential Revision: D19535537
fbshipit-source-id: 6b64b0de9
Summary: Adding reporting to strongSelf Not Checked when strongSelf is passed to a method in a not explicitly nullable position.
Reviewed By: ezgicicek
Differential Revision: D19330872
fbshipit-source-id: 95871a70a
Summary: After receiving feedback about this, I'm changing the reporting of strongSelf Not Checked to only in cases where it can cause a crash. Here I'm adding reporting for field access, and removing general reporting. In a next diff, I'll also add reporting for passing strongSelf to methods in not explicitly nullable positions.
Reviewed By: skcho
Differential Revision: D19329842
fbshipit-source-id: 35beb2aa3
Summary:
The RestartScheduler needs to know if the worker finished it's task
because:
1. there was no more work to do or
2. found that a needed Procname was already taken (this part is not yet implemented)
This need was addressed by (i) making the functions that the workers execute
return a value of task_result.t intead of unit and (ii) adding a
constructor to the worker_message.t (FinishedTask).
Reviewed By: ngorogiannis
Differential Revision: D19467783
fbshipit-source-id: a76b02b6c
Summary:
1. One should use either a writer or a stream to send a response, but not both.
2. A response should be forwarded only if it was commited.
Both properties are extracted from API comments on classes in the servlet API.
Reviewed By: jvillard
Differential Revision: D19514568
fbshipit-source-id: 79f0257ed
Summary:
If data comes from an outer OutputStream, then this outer OutputStream
needs to be flushed before getting the byte array.
Reviewed By: jvillard
Differential Revision: D19514569
fbshipit-source-id: e3e025394
Summary: We were lacking this kind of test where one interface refines the nullability of the other.
Reviewed By: ngorogiannis
Differential Revision: D19514245
fbshipit-source-id: fa3e781f3
Summary: `cost.ml` is huge. Let's split it to its logical parts (basically creating new files for the modules that were already in `cost.ml`) and move all cost related files into `\cost` directory. While we are at it, let's add `mli` files too.
Reviewed By: jvillard
Differential Revision: D19496263
fbshipit-source-id: 45096db4c
Summary: Deadlocks often result in two reports if not deduplicated (two traces), so there is some logic for doing that. Locks recently became an opaque type, with the `get_access_path` loophole supporting that deduplication ordering. Fix that here and remove `Lock.get_access_path`.
Reviewed By: jvillard
Differential Revision: D19465223
fbshipit-source-id: b597e3c65
Summary:
This is a common enough case to make error message specific.
Also let's ensure it's modelled.
Reviewed By: artempyanykh
Differential Revision: D19431899
fbshipit-source-id: f34459cb3
Summary:
The previous diff changes the message for params case, this one handles
return.
Reviewed By: artempyanykh
Differential Revision: D19430706
fbshipit-source-id: f897f0e56
Summary:
This diff gets global constant array values from their initializers. The `find_global_array` function is
added to memory domain, which finds values of global array locations during the ondemand value
generation.
Reviewed By: ngorogiannis
Differential Revision: D19300143
fbshipit-source-id: 7b0b84c42
Summary: Use more informative method names, and add comments explaining the logic behind each test. Correct two cases which are FPs instead of legitimate reports.
Reviewed By: artempyanykh
Differential Revision: D19465227
fbshipit-source-id: 29332e2b9
Summary: Change `MayBlock` and `StrictModeCall` constructors from taking a string to a `Procname.t`, which was the sole source of that string anyway.
Reviewed By: ezgicicek
Differential Revision: D19465226
fbshipit-source-id: e3ed6ef88
Summary:
If a race exists in two or more overloads of the same method and we use only the class and method name in the report text, then the current bug hashing algorithm will identify the two reports as duplicates.
To avoid this, the report had the class, method and list of type parameters. This is unreadable, however, and redundant (the report is already located within the method in question). So at the risk of duplicates, use only class+method names.
Also, fix a bug in `Procname.pp_simplified ~withclass` where `withclass` was ignored for C++/ObjC methods.
Now:
> Read/Write race. Non-private method `FrescoVitoImageSpec.onCreateInitialState(...)` indirectly reads with synchronization from `factory.AnimatedFactoryProvider.sImpl`. Potentially races with unsynchronized write in method `FrescoVitoImageSpec.onEnteredWorkingRange(...)`.@ [Litho components are required to be thread safe because of multi-threaded layout](https://fburl.com/background-layout). Reporting because current class is annotated `MountSpec`, so we assume that this method can run in parallel with other non-private methods in the class (including itself).
Before
> Read/Write race. Non-private method `void FrescoVitoImageSpec.onCreateInitialState(ComponentContext,StateValue,StateValue,Uri,MultiUri,ImageOptions,FrescoContext,Object,ImageListener)` indirectly reads with synchronization from `factory.AnimatedFactoryProvider.sImpl`. Potentially races with unsynchronized write in method `FrescoVitoImageSpec.onEnteredWorkingRange(...)`.@ [Litho components are required to be thread safe because of multi-threaded layout](https://fburl.com/background-layout). Reporting because current class is annotated `MountSpec`, so we assume that this method can run in parallel with other non-private methods in the class (including itself).
Reviewed By: artempyanykh
Differential Revision: D19462277
fbshipit-source-id: aebc20d89
Summary:
Currently, impurity analysis is oblivious to skipped functions which might e.g. return a non-deterministic value, write to memory or have some other side-effect. This diff fixes that by relying on Pulse's skipped functions to determine impurity. Any unknown function which is not modeled to be pure is assumed to be impure.
This is a heuristic. We could have assumed them to be pure by default as well.
Reviewed By: jvillard
Differential Revision: D19428514
fbshipit-source-id: 82efe04f9
Summary:
Let's collect the list of all skipped functions with a `proc_name` but no summary in Pulse's memory. This will be useful for the impurity analysis later (next diff).
Concretely, we extend Pulse's domain with a map from skipped calls to their respective traces. For efficiency, we only keep a single trace per skipped call.
For impurity analysis, tracking skipped calls in Pulse allows us to rely on Pulse's strong memory model to get rid of infeasible paths as opposed to creating an independent checker which wouldn't be able to do that.
Reviewed By: jvillard
Differential Revision: D19428426
fbshipit-source-id: 3c5e482c5
Summary:
As suggested by Ilya, the current message can be improved in a way that
it can contain more clear action. I also added artempyanykh's explanation at the
end of message to provide an additional justification from common sense
perspective.
But most importantly, the previous message was missing a space which is
eye bleeding, how come haven't I noticed this before, I can't stand it
OMG.
Reviewed By: artempyanykh
Differential Revision: D19430271
fbshipit-source-id: dd31f7adb
Summary:
Inferbo analyzed some program points unreachable incorrectly, because of unsound semantics of band
operator, which did not handle the case when given parameters are pointer values.
Reviewed By: ngorogiannis
Differential Revision: D19392705
fbshipit-source-id: dd590508c
Summary:
This diff revises the generation of unknown value. If the type of the unknown value generating is
int, it does not add the "Unknown" pointer/array value.
Reviewed By: ngorogiannis
Differential Revision: D19392696
fbshipit-source-id: e1b3c9a3a
Summary: In impurity analysis, pick up the pulse summary rather than re-analyzing. Re-order the checkers so that we first analyze pulse.
Reviewed By: jvillard
Differential Revision: D19448296
fbshipit-source-id: 2987fa848
Summary:
This diffs does: (1) move `get_formals` to `BufferOverrunUtils` (2) use separate `get_formals` in
`BufferOverrunChecker`, in order to simplify the following diff.
Reviewed By: jvillard
Differential Revision: D19432280
fbshipit-source-id: bfb4df118
Summary:
The restart scheduler will now have two phases:
1. Analyze all the procs obtained from the sources.
2. Run the FileScheduler on the sources.
The second step aims to analyze only the File level analyzers
requirements.
Reviewed By: ngorogiannis
Differential Revision: D19430244
fbshipit-source-id: b4f9ee69b
Summary: `dune build check` will compile all the ml files much quicker than `dune build`
Reviewed By: jvillard
Differential Revision: D19427864
fbshipit-source-id: 5221d32bc
Summary:
Introduce a new notion of equality for comparing abstract addresses in distinct threads:
```
(** Abstract address for a lock. There are two notions of equality:
- Equality for comparing two addresses within the same thread/process/trace. Under this,
identical globals and identical class objects compare equal. Locks represented by access paths
rooted at method parameters must have equal access paths to compare equal. Paths rooted at
locals are ignored.
- Equality for comparing two addresses in two distinct threads/traces. Globals and class objects
are compared in the same way, but locks represented by access paths rooted at parameters need
only have equal access lists (ie [x.f.g == y.f.g]). This allows demonically aliasing
parameters in *distinct* threads. This relation is used in [may_deadlock]. *)
```
Reviewed By: skcho
Differential Revision: D19347307
fbshipit-source-id: 9f338731b
Summary:
This diff avoids that `array_sizeof` returns bottom value when given Java enum values, which
introduced unreachable code inadvertently.
Reviewed By: ngorogiannis
Differential Revision: D19409077
fbshipit-source-id: 2816fd995
Summary: Access expressions can appear in casts, or sometimes other constructors, inside a `HilExp.t`. Extraction of the access expression can ignore those wrappers. Introduce a single function for doing that throughout the analyser.
Reviewed By: ezgicicek
Differential Revision: D19410673
fbshipit-source-id: a724cb466
Summary:
The property SkipAfterRemove already had a test, but not for
intra-procedural violations. This adds a test for that case.
Reviewed By: ngorogiannis
Differential Revision: D19330471
fbshipit-source-id: 1dd1c3ad7
Summary:
Now we can either disable it, or enable it only.
For integrations that disable it, this will allow to enable it for
NullsafeStrict classes without enabled it fully
Reviewed By: artempyanykh
Differential Revision: D19409131
fbshipit-source-id: a2b1fe650
Summary: This diff implements this for Field Not Initialized check
Reviewed By: artempyanykh
Differential Revision: D19393989
fbshipit-source-id: cf60e8d53
Summary: add subdirectories so that we can run each java file against its own topl properties
Reviewed By: rgrig
Differential Revision: D19347302
fbshipit-source-id: 562830774
Summary:
This diff does it for nullable dereference and assignment violations
rules which happen under NullsafeStrict case.
Follow up are to make the same for inheritance and field initializer
violations.
Possible follow up includes making error message more specific and
articulare this this is a nullsafe strict mode.
Reviewed By: artempyanykh
Differential Revision: D19392916
fbshipit-source-id: 2554ac7a7
Summary: In whole-program mode, analysing a method requires analysing first all constructors of the same class. This is not needed in normal mode, so gate that computation under `starvation_whole_program` for efficiency.
Reviewed By: artempyanykh
Differential Revision: D19393412
fbshipit-source-id: 2277e6b5e
Summary:
The RestartScheduler now uses it's own of_list function to build a
ProcessPool.TaskGenerator to be able to use a queue as the underlying
data structure.
Reviewed By: ngorogiannis
Differential Revision: D19390055
fbshipit-source-id: d57b493b7
Summary:
Previously, _override resolution_ considered only the number of
arguments. This led to many FPs in nullsafe's _Inconsistent Subclass
Annotation_ check.
Current version also checks that argument types match. However, we
still don't handle type parameters and erasure, so in this sense the
rules are incomplete.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19393201
fbshipit-source-id: a0c75b8dd
Summary:
That was used only by the now-defunct Java Buck integration without
genrules.
Reviewed By: ngorogiannis
Differential Revision: D19176545
fbshipit-source-id: 8253b614a
Summary:
You should use `--buck-java` instead, which uses the new "genrule
master" integration.
This diff makes it impossible to select the previous integration.
Upcoming diffs will clean up the resulting dead code.
This also make infer fail hard when no buck mode is specified in a buck
capture command, eg `infer -- buck build //foo:foo`. The reason is that
we need to choose between 3 incompatible integrations and making any of
them the default will confuse at least one person in the future.
Reviewed By: ngorogiannis
Differential Revision: D19176391
fbshipit-source-id: 707d18b50
Summary:
These changes introduce the RestartScheduler which for now is a copy of the FileScheduler:
- added it as a possible argument to the recently added `--scheduler` option.
- made the necessary changes in `InferAnalyze` to call it if it was chosen.
Reviewed By: ngorogiannis
Differential Revision: D19373505
fbshipit-source-id: 98f065057
Summary:
`diff` by default doesn't colorize its output, while `git diff`
does. Since we already depend on git it seems like a safe change and
brings some quality of life improvement while working with tests.
Reviewed By: ngorogiannis
Differential Revision: D19374691
fbshipit-source-id: 8872b527c
Summary:
`infer/tests/infer.make` unconditionally runs
```
$(call check_no_duplicates,$(INFER_OUT)/duplicates.txt)
```
as a part of `test` target. This requires the flag to be set to
produce `duplicates.txt`.
`clang.make` does this, but `java.make` was missing the flag which led
to an error on running `make tests` within
e.g. `tests/codetoanalyze/java/nullsafe-default`:
```
grep: infer-out/duplicates.txt: No such file or directory
```
Reviewed By: mityal
Differential Revision: D19373857
fbshipit-source-id: c2fcbe9dd
Summary:
This diff avoids that null-retuned path's abstract value ruins that of non-null-returned path.
What this diff does is: when joining two abstract states, one is null-return-path and the other is
non-null-return-path (`return obj;`), it keeps the method calls of `obj` from the
non-null-return-path.
While this design is unsound, I think it should work in practice.
Reviewed By: ezgicicek
Differential Revision: D19348313
fbshipit-source-id: cf5d0f3ff
Summary: This diff captures global initializers ondemand, like we do for functions defined in headers.
Reviewed By: ezgicicek
Differential Revision: D19346947
fbshipit-source-id: 05174e6a4
Summary: The type-name definition for Java can be potentially improved (eg increase sharing, or comparison speed, much like `QualifiedCppName`) by switching away from `Mangled.t` which is essentially a string. First step is to abstract the type.
Reviewed By: jberdine
Differential Revision: D19087508
fbshipit-source-id: 91a81f63b
Summary: Now that we have the kind of lock stored (global/class obj/path rooted at parameter), use it for comparison/equality, while ignoring the root variable of the access path, which is only used for printing.
Reviewed By: skcho
Differential Revision: D19346801
fbshipit-source-id: c65661dc6
Summary: Add command line option that directs racerd to treat all return values from unknown code (including abstract methods) as owned objects. This is essentially treating return values with full angelicism
Reviewed By: artempyanykh
Differential Revision: D19368375
fbshipit-source-id: 6a10153fa
Summary:
Java treats switch on nullables in a non-obvious way (throws an NPE
surprise) so lets have a decidated test exactly for this.
Reviewed By: ngorogiannis
Differential Revision: D19371280
fbshipit-source-id: d9867b6d6
Summary:
As part of enabling substitution of arguments into lock names in summaries, and potentially changing the equality relation over locks (so that we can eventually make equal `x.f.g` and `this.f.g` for example), the lock type needs to be elaborated so that the root of an access path is classified into:
- global variables are clearly separated from all other classes (invariant to substitution)
- class objects are also separate (also invariant, but identified by type)
- all other parameters remember their positional index (so that substitution becomes easier)
For now the comparison/equality is kept identical, so as to make easy CI comparisons.
Reviewed By: skcho
Differential Revision: D19232577
fbshipit-source-id: 0cd6a43db
Summary: Before going to a new lock representation that will allow, eg, substitutions with arguments for parameters on method calls, make the interface of the lock type abstract to ease the transition.
Reviewed By: mityal
Differential Revision: D19309359
fbshipit-source-id: 5277357ee
Summary:
Demonstrate that the per-file type environments don't prevent
the deadlock report here. The fear was that when the analyser
tries to locate the methods of the endpoint class, it might fail to
do so because the types might be stored in different type
environments (per file).
Reviewed By: mityal
Differential Revision: D19225908
fbshipit-source-id: 097e4aeea
Summary: This diff use actuall call path in the cost results instead of `class name + method name`.
Reviewed By: ngorogiannis
Differential Revision: D19194969
fbshipit-source-id: b72018586
Summary:
Model array length in Java as returning an unknown interval [0, +inf] for now.
Ideally, we can deal with the size in a more precise manner in the future like in InferBo.
Reviewed By: skcho
Differential Revision: D19312123
fbshipit-source-id: 8c51059a4
Summary:
This diff does a refactoring on the function that gets summary from DB,
`get_proc_summary_and_formals`.
* It separates the function into `get_summary` and `get_formals`, and
* renames `Checker.get_proc_summary` to `get_checks_summary`
Reviewed By: ngorogiannis
Differential Revision: D19300136
fbshipit-source-id: d28eaf16d
Summary:
This diff creates missing result directories in its running.
The problem was that `infer-out/captured` and its sub-directories were not ready at the step 3 below, which crashed with exceptions.
1. `infer capture -- [target build]`
2. `infer analyze --merge`
3. `infer analyze --merge --debug --reanalyze --procedures-filter '.*foo.*'`
Reviewed By: ngorogiannis
Differential Revision: D19274672
fbshipit-source-id: af84000d7
Summary: Pulse doesn't care about exceptions yet. With Exceptional CFG, java analysis takes a lot of time due to having many disjuncts. Let's use Normal CFG for now.
Reviewed By: jvillard
Differential Revision: D19194479
fbshipit-source-id: f94bb6078
Summary:
Kill these tests as that mode is not supported anymore.
Note that :
- "buck_cross_module" was effectively already dead since it's not hooked up to `make test`
- "buck" is actually another copy of "genrule" so wasn't testing anything
Reviewed By: ngorogiannis
Differential Revision: D19176166
fbshipit-source-id: b5bb90448
Summary:
This changes how we select amongst our (currently) 4 Buck integrations
for Java and clang, as well as how the user's choice is reflected by the
Config module.
The old command line interface is still supported but is now deprecated.
The changes in how to select each integration are:
- clang via "flavors", activated with `--flavors`, now with `--buck-clang`
- clang via "compilation DB", activated with `--buck-compilation-database`, unchanged
- Java via "genrule", activated with `--genrule-master-mode`, now with `--buck-java`
- Java "without genrules", used to be activated by *not specifying any other Buck mode*, unchanged
Instead of various `Config` flags corresponding to the previous CLI that
are allowed in any combination of `flavors`,
`buck_compilation_database`, `genrule_master_mode`, `Config` now exposes
a single `buck_mode` datatype. This allows, eg, `flavors` to override
`buck_compilation_database` if needed. It will also make it easier to
get rid of the old "Java without genrules" integration in a later diff
(see inline comments).
Reviewed By: ngorogiannis
Differential Revision: D19175686
fbshipit-source-id: 29b3831be
Summary:
We cannot necessarily know if the previous capture phase was java or
clang, hence to be on the safe side we should always merge the global
tenvs when merging Buck targets.
Reviewed By: ngorogiannis
Differential Revision: D19175685
fbshipit-source-id: 8e7492e14
Summary:
This generates a lot of code (the datatype is not trivial) and
transitively requires other datatypes to have compare functions, just so
we can then ask whether the mode is "Analyze".
Reviewed By: ngorogiannis
Differential Revision: D19164589
fbshipit-source-id: be6a12b41
Summary:
Infer would crash when started in a context where it already had
children it didn't know about.
Reviewed By: martintrojer
Differential Revision: D19177589
fbshipit-source-id: 0c8831597
Summary:
In order to improve the impurity analysis, this diff adds models for
- `hasNext()` and - `Object.equals()` modeled as returning a non-deterministic value (havoc_id)
- `next()` modeled as `StdVector.get` with a fresh index
- `iterator` modeled as just returning the underlying list
Reviewed By: jvillard
Differential Revision: D19177392
fbshipit-source-id: 0babb037a
Summary:
This diff updates the relation between iterator (offset) and integer value not only at
assignments (`x += 1`), but also at function calls (`foo()`) that increase integer values by one in
their side effects.
Reviewed By: ezgicicek
Differential Revision: D19163214
fbshipit-source-id: 47e52f939
Summary: This diff extends the domain to express the relation between iterator's offset and integer value.
Reviewed By: ezgicicek
Differential Revision: D19143670
fbshipit-source-id: 6223bc934
Summary:
Old versions of sawja/javalib got the line numbers slightly wrong. The workaround was to do a regexp search in the source file for the right line.
My understanding is that this is no longer necessary. This diff removes it.
Reviewed By: jvillard
Differential Revision: D19033415
fbshipit-source-id: 2da19d66d
Summary:
From a set of classes [classes], JClasspath.load_program builds a
program. This program contains (implicitely) the set of active classes
and we should not keep using the set [classes] and the generated
program as the same time, because they may not be synched
anymore. (In particular, in a forthcoming diff load_program may add
new classes during program construction)
This patch forces this discipline. It adds a [in_classes]
query for programs and makes sure we use it everywhere we should.
Reviewed By: ngorogiannis
Differential Revision: D18833335
fbshipit-source-id: a522f320c
Summary:
This is an optimization. We ask the user to tell us which states are nondeterministic, and we
generate code that handle nondeterminism only for those states. It is common for only one state per
TOPL property to be nondeterministic. This speeds up the biabduction-analysis of the monitor by a
factor of ~10. But, using the monitor is only a little faster.
Facebook
Reviewed By: jvillard
Differential Revision: D19160286
fbshipit-source-id: 4dd39769a
Summary:
The re-execution phase uses only the preconditions found by the footprint phase. Thus, Infer should
not spend time optimizing POSTconditions. These are used only to find custom errors.
Reviewed By: jvillard
Differential Revision: D19035495
fbshipit-source-id: d52b82733
Summary:
Move most of IR/Sil.ml into a new file biabduction/Predicates.ml to
reflect the fact that they are only useful for the biabduction analysis.
Unfortunately this is a huge change.
I tried to keep the change to a minimum, it's mostly about doing
s/Sil/Predicates/ in lots of places but sometimes I used the trick of
specifying parameters or return value types to avoid specifying the
module altogether. This isn't done consistently because there were just
too many places to change for poor me.
Reviewed By: ngorogiannis
Differential Revision: D19158530
fbshipit-source-id: d6dbcfe72
Summary:
hmmhmmhmm
Also `Sil.hpred` is going to move out of IR/ in a few diffs, into
biabduction/ where it belongs.
Reviewed By: ngorogiannis
Differential Revision: D19158535
fbshipit-source-id: e2a889ee2
Summary:
Part of making Sil.ml about SIL only.
In order to not introduce a dependency istd/Pp -> base/Config, the
utilities in Pp don't know when to introduce "diff" colours. Fix it by
wrapping them in Sil using the Config option. (we may want to just kill
that option at some point).
Similarly, move stuff from Io_infer to Pp.
Reviewed By: ngorogiannis
Differential Revision: D19158534
fbshipit-source-id: 8110cb7f9
Summary:
In the previous code, it removed non-build-called method calls. For example, it was like
```
{non-build-called: {prop1, prop3}}
{build-called: {}}
b.build();
{non-build-called: {}}
{build-called: {prop1, prop3}}
```
However, this behavior introduced a false positive when there is multiple builders that point to the
same abstract object and `build` is called one by one.
This diff changes the semantics to keep the method calls of non-build-called at `build` calls.
Reviewed By: ezgicicek
Differential Revision: D19144525
fbshipit-source-id: e2ace127f
Summary:
At some point, code reformatting moved the comment
in a place where it didn't make sense.
Reviewed By: ngorogiannis
Differential Revision: D19035489
fbshipit-source-id: 146ad8a15
Summary:
This applies some simplifications that were previously
done after footprint (and therefore lost), and some
simplifications that require looking at both pre and
post.
Reviewed By: ngorogiannis
Differential Revision: D19035494
fbshipit-source-id: bad79534a
Summary:
This havocs event data, so that biabduction doesn't try to
track what was the last event processed by the monitor
(which is redundant as long as the state of the monitor
is tracked).
Reviewed By: ngorogiannis
Differential Revision: D19035491
fbshipit-source-id: a1c75daae
Summary:
Don't instrument SIL when we can determine statically that
biabduction symexec would be a no-op.
Reviewed By: ngorogiannis
Differential Revision: D19116849
fbshipit-source-id: 4d25462a3
Summary: It is confusing to report missing props at the the beginning of the method (especially when there are many components created or when the method has many lines). Let's report them at create methods to better contextualize. Also, make the missing prop bold.
Reviewed By: skcho
Differential Revision: D19141135
fbshipit-source-id: a23d2e7c9
Summary:
It used to recompute the number of arguments during
instrumentation, which is unnecessary.
Reviewed By: ngorogiannis
Differential Revision: D19035492
fbshipit-source-id: 0f58c8ae7
Summary:
In addition to
state1 -> state2: pattern
one can now also write
state1 -> state2: pattern if condition
where "condition" is some conjunction of comparisons (==,<,>) that
involve variables bound by "pattern", registers of the automaton, and
constants.
Reviewed By: ngorogiannis
Differential Revision: D19035496
fbshipit-source-id: 6f6e6a9be
Summary:
The one with Object as the second param is in `com.facebook.common.internal`.
The one with String as the second param is in `com.facebook.common.preconditions`.
(Don't ask :) )
Reviewed By: artempyanykh
Differential Revision: D19087334
fbshipit-source-id: ba02c9101
Summary:
According to Java semantics, they are always non-null.
Internally they are represented as static fields, so they have
DeclaredNonnull nullability, which means NullsafeStrict mode would
refuse to use them without strictification.
Lets teach nullsafe that these guys are non-nullables.
See also FN in test case.
Reviewed By: ngorogiannis
Differential Revision: D19024547
fbshipit-source-id: 8c120fa50
Summary:
Inferbo does not use the external relational domains, apron and elina. At some point, the parts of
inferbo using them were broken and they do not seem to be fixed easily in the near future. Let's
remove them and keep the code base cleaner.
Reviewed By: jvillard
Differential Revision: D19022905
fbshipit-source-id: e0eafe79f
Summary:
We do not have the create method in the trace which makes it difficult to understand
- inter-procedural issues where create and prop setting are in different methods
- there are multiple create-build chains in a method
Let's add the create to the beginning of the trace. Moreover, let's simplify the prop printing to make traces easier to understand.
Reviewed By: ngorogiannis
Differential Revision: D19020213
fbshipit-source-id: 7f8a5d4ec
Summary: The new domain is much better than the old one. Let's kill the old one (along with old litho tests) and simplify things.
Reviewed By: skcho
Differential Revision: D18959627
fbshipit-source-id: df77ae20e
Summary:
- Remove `to_flat_string` as there is `get_field_name` that unambiguously does the same thing.
- Make `pp` print only the field in all languages.
- Fix `to_full_string` so that it has unified behaviour across java/clang and so that it doesn't print `class Foo.x`, but rather `Foo.x`.
Reviewed By: ezgicicek
Differential Revision: D18963033
fbshipit-source-id: e2c803c7d
Summary:
Remove Clang and Java submodules of Typ.Fieldname. They are unnecessary and they reflect a fake dichotomy: there is only one fieldname type. To distinguish between fields of Java classes and other C constructs, there is a helper function provided, but the idea is simple: obtain the class type the field belongs to, and check if it's a Java class.
This diff still preserves behaviour, but removes as many functions as possible from the interface, to leave a small surface.
Reviewed By: mityal
Differential Revision: D18962423
fbshipit-source-id: ffe6933ee
Summary: Unify treatment of Java and Clang fieldnames. Now a field is a struct with a class type-name and a string-field name. This diff is still behaviour preserving.
Reviewed By: jvillard
Differential Revision: D18953549
fbshipit-source-id: 8cae0d104
Summary: This function allows any string, and in particular empty class names. As a first step eliminate it in favour of a function that forces the caller to specify distinct class and field names. It turns out that the frontend already has them, so it saves effort along the way.
Reviewed By: jvillard
Differential Revision: D18953136
fbshipit-source-id: ff3cdfda5
Summary:
In order to handle the example added:
changed domain of `MethodCalled`
from `CreatedLocation -> (IsBuildCalled X IsChecked X Set(MethodCall))`
to `(CreatedLocation X IsBuildCalled) -> (IsChecked X Set(MethodCall))`
This avoids joining of two method calls where one is build-called and the other is not, e.g.,
```
if(b) {
o.build();
} else {
// no build call
}
```
changed domain of `NewDomain`
from `Created X MethodCalled`
to `(Created X MethodCalled) X (Created X MethodCalled)`
One is for no returned memory and the other is returned memory. This keeps precision some join
points of branches, e.g.,
```
if(b) {
return;
} else {
// no return
}
```
Reviewed By: ezgicicek
Differential Revision: D18909768
fbshipit-source-id: c39d1a1ef
Summary:
A plus is a plus, no need to give up when +/- is about pointers. This
gets rid of some false positives involving pointer arithmetic.
However, the problem remains if we make things a bit more
inter-procedural. This is documented in an added test.
Reviewed By: ezgicicek
Differential Revision: D18932877
fbshipit-source-id: 4ad1cfe72
Summary: It is not used anywhere and there are no plans to revive it. Kill it!
Reviewed By: skcho
Differential Revision: D18934719
fbshipit-source-id: b9b069b96
Summary: Under the buck/java integration, the classpath is propagated to all infer command lines. Logging that leads to huge waste and full disks. Make the logging conditional to debug mode.
Reviewed By: skcho
Differential Revision: D18934088
fbshipit-source-id: 7e2f410f5
Summary:
The `Typ.FIeldname` module has many issues. Among those:
- It has 5 different string/printing functions and most of them do radically different things in Java and in Clang.
- There is no type safety: creating a Clang field and calling a Java function on it will lead to a crash (`rindex_exn` etc, there are usually no dots in Clang fields).
- It uses a single string for Java fields, containing the package, the class and the field, e.g., `java.lang.Object.field`. This is wasteful, because
- there is no sharing of strings for packages/classes, and,
- string operations need to be performed every time we need the field or the class or the package alone.
This diff preserves the behaviour of the module's interface, so the API problems remain.
However, by using a saner representation for Java fields we can get small performance and large memory gains (the type environment in Java is much smaller, about 30-40%).
In addition, many functions on clang fields would previously do string manipulations (look for `.` and split on it) before returning the final field unchanged -- now they use the type of the field for that.
Reviewed By: jvillard
Differential Revision: D18908864
fbshipit-source-id: a72d847cc
Summary:
That class does some complicated accounting of memory that depends on
whether the string is "small", "medium" or "large". In the latter case
it does its own ref-counting and copy-on-write to save memory, and that
trips up pulse. Pretending all strings are small avoids that issue.
Reviewed By: skcho
Differential Revision: D18909030
fbshipit-source-id: 1c14d909b
Summary:
Including the current call state is useful because the contradiction
sometimes refers to abstract values that have been materialised since
the last call state so we cannot make sense of them unless we print the
current call state.
Reviewed By: skcho
Differential Revision: D18908424
fbshipit-source-id: 297f397a6
Summary:
- Do most of the work of `solve_arithmetic_constraints` inside `subst_attribute` instead, since we need to re-use the latter function for post-conditions where the first function is not appropriate.
- When substituting arithmetic constraints, we refine arithmetic information (both concrete intervals and inferbo), which can lead to inconsistent states. Instead of recording the new arithmetic facts by returning a new current state, just act as a map on attributes. This is to enable doing the point above.
- All this lead to a somewhat messy refactoring...
- Rename `CannotApplyPre` to `Contradiction` since it's used for post-conditions as well now
Reviewed By: skcho
Differential Revision: D18889120
fbshipit-source-id: d81647143
Summary:
After passing a `PRUNE` instruction we can refine the current inferbo
intervals for the values involved.
Reviewed By: ezgicicek
Differential Revision: D18889103
fbshipit-source-id: b521046aa
Summary:
This exposes a more general interface to other modules, at the cost of
hiding the fact that it won't deal with all binary operations with equal
precision.
Reviewed By: ezgicicek
Differential Revision: D18908095
fbshipit-source-id: 0c0653bb6
Summary: Guava uses assertions to ensure a future can be gotten without blocking (this means that if the future is not done, the app will crash). This diff teaches the starvation analyser about a number of such assertions, by treating them as assumes (since we don't care about exceptions).
Reviewed By: jvillard
Differential Revision: D18893427
fbshipit-source-id: 4d26a202b
Summary: A future is guaranteed not to block if `isDone()` has returned true first. Add logic for supporting that by remembering the objects that we have called `isDone` on and by making `assume` do the right thing with that knowledge. All this is achieved with the attribute domain.
Reviewed By: ezgicicek
Differential Revision: D18833901
fbshipit-source-id: 7f4ea0cd1
Summary: The formals of callees will be used in the following diff.
Reviewed By: ezgicicek
Differential Revision: D18878044
fbshipit-source-id: 6c01af826
Summary:
When retrieving a value from a container, we previously had an arbitrary hack which would
- In java, give no ownership to the returned object (trying to be sound)
- In C++ give conditional ownership to the current method's first argument (trying to be complete, but doing it badly, as the first argument may not be the `this` object in a static method, or we might be accessing it through another parameter altogether).
Harmonise both by using the existing ownership of the container as ownership value for the returned object (leaning towards completeness).
Reviewed By: jvillard
Differential Revision: D18882800
fbshipit-source-id: f98f8d315
Summary:
Every time we add an arithmetic information, add the corresponding
inferbo one.
Reviewed By: skcho
Differential Revision: D18888863
fbshipit-source-id: ab4afd372
Summary:
Pointers are hard... The previous test had no chance of doing
initialisation of the pointer by reference and was in fact a false
negative (and still is, fix incoming). Renamed functions to stress the
false negative and added a test that is really (potentially) doing
pointer initialisation by reference.
Reviewed By: skcho
Differential Revision: D18888008
fbshipit-source-id: 1e72408c7
Summary:
This reverts commit 4fd6165d190bab32544f9f040b777565432c15b2.
We don't need to check for reporting each node anymore. It suffices to just check per function.
Reviewed By: skcho
Differential Revision: D18883833
fbshipit-source-id: 2591b3af3
Summary:
Making `MethodCalled` an inverted map from created location to method calls results in not being able to track a builder that is created in two different branches of a conditional with different types. Instead, we can make `MethodCalled` simply a map and also change `Created` to be a map from access paths to a set of created locations.
To deal with the case of setting a prop only in one branch, we need to ensure that whenever we call a create method, we add a binding to `MethodCalled` with an empty list of methods so that its intersection with a non-empty one is empty.
Reviewed By: skcho
Differential Revision: D18883097
fbshipit-source-id: b3464ca20
Summary: As long as the types match, it should be possible to call build on two components that are created at different locations.
Reviewed By: skcho
Differential Revision: D18881740
fbshipit-source-id: 356f9e168
Summary:
Finally use information from the inferbo intervals in pulse's domain to
make decisions about whether conditionals are feasible or not.
Reviewed By: skcho
Differential Revision: D18811193
fbshipit-source-id: d80a28657
Summary:
Refine the type of inferbo intervals attributes to "pure" (non-bottom)
ones. This is because were we to get a Bottom value from inferbo we
should stop the abstract execution instead of recording it in the state.
Reviewed By: ezgicicek
Differential Revision: D18811165
fbshipit-source-id: fff8664b7
Summary:
Similarly to binops, let inferbo evaluate unary operations and record
the results.
Reviewed By: ezgicicek
Differential Revision: D18811146
fbshipit-source-id: 8f9e16bbd
Summary: Add a FN that is detected by the old domain but not the new one
Reviewed By: ngorogiannis
Differential Revision: D18854389
fbshipit-source-id: 9bdc90a6b
Summary: The map from `CreatedLocation` to `MethodCalls` already takes care of the association from create methods to their set props. `MethodCall` comparison should be oblivious the the receiver, otherwise, we risk mistakenly considering two props set at different locations as different.
Reviewed By: skcho
Differential Revision: D18829388
fbshipit-source-id: b5a0d628d
Summary: This diff check and report on every nodes. Problem of the previous design is that it has to report alarms only with the abstract memory of the exit node. However, the new abstract value becomes imprecise at every join points on the path to the exit node, since it is using inverted map, i.e., under-approximation on collecting called methods. As a solution, this diff report on every nodes where `.build` is called with the abstract memory at that node.
Reviewed By: ezgicicek
Differential Revision: D18809449
fbshipit-source-id: 4fd6165d1
Summary: Current method call comparison is too strong. As exemplified with the new test, one can also set the required prop by calling a version which contains the suffixes. The domain should take care of such cases now.
Reviewed By: skcho
Differential Revision: D18808869
fbshipit-source-id: 9f7672e75
Summary:
Sometimes clang9 does not return a boxing method (a name of function to apply), e.g., [@("str")].
To solve the issue, this diff uses "unknownSelector:" instead of giving up the translation.
Reviewed By: dulmarod
Differential Revision: D18831844
fbshipit-source-id: b9324ba39
Summary: This enables us to quickly pick up the Component typ when checking which required props to check.
Reviewed By: skcho
Differential Revision: D18807925
fbshipit-source-id: 47e407394
Summary: This diff checks litho condition on the new abstract value. This is triggered with `--new-litho-domain`, but it is intra-procedural as of now.
Reviewed By: ezgicicek
Differential Revision: D18783203
fbshipit-source-id: 98570104e
Summary: This diff adds a map from an object to a set of methods that has been called.
Reviewed By: ezgicicek
Differential Revision: D18781461
fbshipit-source-id: 47c10adaa
Summary: This adds a map from access expressions to locations where specific object is created by `create` method call.
Reviewed By: ezgicicek
Differential Revision: D18779985
fbshipit-source-id: f327450cd
Summary: This diff stack is to revise overall domain and semantics of litho checker. Some of the following diffs will not have visible changes until we replace the current checker with the new one.
Reviewed By: ezgicicek
Differential Revision: D18779592
fbshipit-source-id: d210d9bd1
Summary:
This can happen for a number of reasons that are not errors (mostly the exit node being unreachable) so isn't actionable, and definitely not worth showing the user. It could be a debug message but I don't think that's even worth it. Other checkers don't warn in similar circumstances.
With OCaml 4.08 we started actually seeing these error messages (differences in flushing behaviour?) so their annoying nature was revealed.
Reviewed By: ngorogiannis
Differential Revision: D18808460
fbshipit-source-id: a47a1dcb4
Summary:
Also add logic for recognising excessive timeouts. Refactor the code
around timeouts a little.
Reviewed By: artempyanykh
Differential Revision: D18807836
fbshipit-source-id: df5a1b566
Summary:
`Object.wait()` must be called on a locked monitor and it releases the
lock immediately, as far as other threads are concerned
(it also magically re-takes the lock when the monitor is `notified`).
Starvation can only occur if the UI thread is waiting
a lock that is distinct to that being waited on.
The check present was over-approximate in that it was checking that there exists a lock held by the UI thread and the thread issuing the `wait`, but did not make sure that lock was *not* the one waited on.
Amusingly, the e2e test was correct, but the reporting code wasn't.
Reviewed By: dulmarod
Differential Revision: D18782919
fbshipit-source-id: b3b98239e
Summary: The clang plugin exports C++ mangled names in hashed form for perf reasons, so we need to hash the incoming mangled names in the profiler samples, so that we can compare them.
Reviewed By: skcho
Differential Revision: D18761842
fbshipit-source-id: 3072b5e33
Summary: Similar to constructor established attributes, we do the same here for static initializers. That is, attributes of static properties are injected into the initial state of every method.
Reviewed By: ezgicicek
Differential Revision: D18763192
fbshipit-source-id: 3879a27c5
Summary:
For now lets support the simplest form: "//"-style comments.
Having comments is useful:
1. It serves as documentation for API.
2. It serves as justification of why such and such method or param is
nullable or not (so the future readers can understand the reasoning of
the person who added the signature).
Reviewed By: artempyanykh
Differential Revision: D18762289
fbshipit-source-id: 90c515ea8
Summary: Raises an error when weakSelf is used multiple times in a block. The issue is that there could be unexpected behaviour. Even if `weakSelf` is not nil in the first use, it could be nil in the following uses since the object that `weakSelf` points to could be freed anytime.
Reviewed By: skcho
Differential Revision: D18765493
fbshipit-source-id: 37fc652e3
Summary:
This diff enables parsing and auto-formatting documentation
comments (aka docstrings).
I have looked at this entire diff and manually made some changes to
improve the formatting. In some cases it looked like it would take too
much time, or benefit from someone more familiar with the code doing
it, and I instead disabled auto-formatting docstrings in those files.
Also, there are some source files where the docstrings are invalid,
and some where the structure detected by the parser appears not to
match what was intended. Auto-formatting has been disabled for these
files.
Reviewed By: ezgicicek
Differential Revision: D18755888
fbshipit-source-id: 68d72465d
Summary:
RacerD is angelic in the sense that when a method has no summary, no accesses are added to the symbolic state when we call that method. However, when the method returns an object we then proceed to access, this leads to non-angelic behaviour: if the object is assumed to be un-owned, then the accesses may lead to a race.
This manifests itself on Litho components, which are generated code without sources and thus RacerD has no CFG to analyse, and therefore produces no summary. The `Builder` patterns used in these classes are ubiquitous, and full of spurious races due to the fact that the returned objects, even though freshly allocated, are un-owned as far as RacerD knows.
Here, instead of going full-angelic and assuming that every method without a summary returns an owned object (which is a bit extreme), we try to model the `Builder` pattern wrt ownership. That is, static `create` methods returning `Builder` types are assumed to return full ownership; and, non-static methods of a `Builder` class which return the same type as their receiver are assumed to return conditional ownership.
Reviewed By: ezgicicek
Differential Revision: D18748423
fbshipit-source-id: bd53d4b67
Summary: This diff extends the bound domain to express multiplication of bounds in some simple cases.
Reviewed By: ezgicicek
Differential Revision: D18745246
fbshipit-source-id: 4f2dcb42c
Summary:
One standard way to schedule work is by starting a thread. We treat this by
- Treating invocations of `start` on a receiver with the `Runnable _` attribute as scheduling that runnable for parallel execution in the background (as opposed to on the UI thread).
- If `start` is used on an object of a subclass of `Thread` everything already works thanks to the `get_exp_attributes` function which will implicitly ascribe to an expression the attribute `Runnable _` if the expression points to an object with a known `run` method. This will even take care of some degree of dynamic dispatch, yay!
- If `start` is used on a `Thread` object which has been created with a constructor call provided with a `Runnable` argument, we have to appropriately model that constructor call, which is what is done in `do_call`.
Reviewed By: jvillard
Differential Revision: D18726676
fbshipit-source-id: 0bd83c28e
Summary:
A current blind spot is when object construction stores specific executors / runnables to object fields, which are then never mutated and accessed from normal methods. IOW the attributes established in the constructor are necessary to report properly inside a normal method (assuming these attributes are not invalidated by method code).
To achieve this, first we retain a subset of the final state attributes in the summary (only those that affect instance variables, in constructor methods). Then, when we analyse a non-constructor method:
- we analyse all constructors
- remove all attributes from the attribute map whose key is not an expression of the form `this.x. ...`
- re-localise all remaining keys so that they appear as rooted in the `this` local variable of the current procedure
- join (intersect) all such attribute maps
- use the result in place of initial state as far as the attribute map is concerned for the analysis of the current procedure, which can now start.
This means we can catch idioms that use side-effectful initialisation for configuring certain object fields like executors or runnables.
Reviewed By: jvillard
Differential Revision: D18707890
fbshipit-source-id: 42ac6108f
Summary: Another way to schedule work in android is by posting it to a `Handler`. A handler can be constructed out of the main looper, which forces it to schedule work on the UI thread. To model all this, we add syntactic models for getting the main looper and for creating handlers, and dataflow attributes for tracking that an expression is a looper/the main looper, or a handler constructed out of a looper.
Reviewed By: skcho
Differential Revision: D18706768
fbshipit-source-id: 7c46e6913
Summary:
We already have a number of `[pkg].Preconditions.checkNotNull`
modelled, but the one from `androidx` is missing yet widely used.
Reviewed By: mityal
Differential Revision: D18748550
fbshipit-source-id: 83d3317ae
Summary:
The introduction of inferbo intervals as pulse attributes creates the
first relational attributes. To make sense of inferbo intervals
appearing in summaries when in a caller context, we need to substitute
the abstract values they contain in the callee with the abstract values
they correspond to in the caller.
This has a significant consequence: we have to delay the check that
arithmetic constraints in the callee are satisfiable at the call-site
until *after* we have discovered all the relationships between callee
values and caller values from the heap. To solve this, we now run an
arithmetic constraints check *after* having materialised all the
addresses.
We also need to translate the abstract values in the attributes in the
post before recording them in the caller, for the same reasons.
Quite some code in this diff is concerned with substituting pulse values
inside inferbo intervals. There is a complication there too: even after
having discovered relationships between caller and callee abstract
values induced by the heap shapes, there could be abstract values in the
callee's attributes that we haven't seen yet. We need to make up new
values for these in the caller, so this substitution has to return a
potentially extended substitution.
Reviewed By: skcho
Differential Revision: D18745695
fbshipit-source-id: 077ae7670
Summary:
New syntax f1 AND-WITH-WITNESSES f2 : predicate_on_both_witnesses()
This is needed for a linter to check that a macro is present, see the test.
Reviewed By: skcho
Differential Revision: D18735988
fbshipit-source-id: a3be75c5e
Summary:
This gets rid of false positives when something invalid (eg null) is
passed by reference to an initialisation function. Havoc'ing what the
contents of the pointer to results in being optimistic about said
contents in the future.
Also surprisingly gets rid of some FNs (which means it can also
introduce FPs) in the `std::atomic` tests because a path condition
becomes feasible with havoc'ing.
There's a slight refinement possible where we don't havoc pointers to
const but that's more involved and left as future work.
Reviewed By: skcho
Differential Revision: D18726203
fbshipit-source-id: 264b5daeb
Summary:
It's a well-known fact that pulse should know too. To avoid splitting
the abstract state systematically, only act if we know the pointer is
exactly 0 to avoid reporting a nullptr dereference on `free(x)`.
Reviewed By: ezgicicek
Differential Revision: D18708575
fbshipit-source-id: 1cc3f6908
Summary:
Turns out code uses atomics in important places, modelling it removes
FPs.
The tests are copied from biabduction and adapted and extended a bit. I
didn't implement compare_exchange primitives for now (plus, giving them
a sequential semantics like in biabduction is probably a bit cheeky).
Reviewed By: skcho
Differential Revision: D18708576
fbshipit-source-id: a3581b8a4
Summary: This extends semantics of binary operator for BoItv. If there is no known interval value for a pulse value, it returns a symbolic value of the pulse value.
Reviewed By: jvillard
Differential Revision: D18726768
fbshipit-source-id: ed8ecf78b
Summary:
This diff adds inferbo's interval values to pulse's attributes. The added values will be used to
filter out infeasible passes in the following diffs.
Reviewed By: jvillard
Differential Revision: D18726667
fbshipit-source-id: c1125ac6e
Summary:
This will make our analysis more precise. E.g. in the future we can classify
"condition redundand" warnings by origin, and warn only for
InferredNonnull, or autogenerate fixes based on the origin.
Error messaging does not currently take into account origin for nonnull
types, but this can be changed in the future
Reviewed By: ngorogiannis
Differential Revision: D18685304
fbshipit-source-id: 6a1f263f5
Summary: The debugger in the middle of the evaluation makes working on that file difficult. Separating modules a bit, so that we can change the code easier. This is in preparation to trying to add aliases and witnesses to formulas in a next diff.
Reviewed By: jvillard
Differential Revision: D18708542
fbshipit-source-id: 523f30fc9
Summary: The tableaux evaluation was an experiment and it was turned off because of bad perf. Let's kill it to clear up the code.
Reviewed By: jvillard
Differential Revision: D18708388
fbshipit-source-id: 099f5a3d3
Summary:
This diff fixes the model of substring.
Problem: The cost model of the substring function was to return `size of string - start index` as a
cost. However, sometimes this was a negative number, because of state abstractions on paths, array
elements, call contexts, etc, which caused an exception inadvertently.
This diff changes the model to return just `size of string`, when it cannot say `size of string` is
bigger than `start index`.
Reviewed By: ezgicicek
Differential Revision: D18707954
fbshipit-source-id: 63f27e461
Summary: Rather than repeatedly matching actuals, let's use `ProcnameDispatcher.ModeledCall` to pick up the actual arguments with their corresponding values. This simplifies the models.
Reviewed By: jvillard
Differential Revision: D18685855
fbshipit-source-id: 7788bd8bb
Summary: This diff removes `'markers` and `'captured_types` from the procname dispatcher. They are for checking an integrity when a type is captured from template parameters then it is used to match in parameters. However, we have not used that feature, so which simply complicates the types in the dispatcher without any gain at the moment.
Reviewed By: jvillard
Differential Revision: D18706254
fbshipit-source-id: f493778d7
Summary: Preperation diff to use `ProcnameDispatcher` for Pulse: it changes function arguments, i.e. `ProcnameDispatcher.Call.FuncArg`, to a record in order to track the value of arguments. To do that, it changes `ProcnameDispatcher.Call` into a functor so that we can parametrize over the type of the value without making changes upwards.
Reviewed By: jvillard
Differential Revision: D18590224
fbshipit-source-id: 6a13fbc1a
Summary:
This will allow us tune nullsafe behavior in a more fine-grained way.
See e.g. a follow up diff when we report errors more clearly.
Reviewed By: ngorogiannis
Differential Revision: D18683747
fbshipit-source-id: 7b5c42a03
Summary:
We were wrongly not supressing third-party calls in
case third party repo is provided: there was a bug in the function is_modelled_externally.
because of this we accidentally stopped supressing third party and
started to advertise it in non-strict mode.
we do plan doing that, but little bit further ahead :)
So let's add a param guiding this.
Reviewed By: artempyanykh
Differential Revision: D18658271
fbshipit-source-id: 703f2675b
Summary:
We will migrate to the new representation in the future, but for now
let's keep both active.
Reviewed By: artempyanykh
Differential Revision: D18657955
fbshipit-source-id: 9deb03f1f
Summary:
Instead of trying to figure out what runnable is directly passed to an executor,
use attributes to track the flow of a runnable. This has several advantages:
- Can track runnables across function return values.
- Can somewhat overcome the information loss under dynamic dispatch.
- Unifies handling with other attributes.
Reviewed By: skcho
Differential Revision: D18672676
fbshipit-source-id: a06a0e6ff
Summary:
- Unify treatment of modelled and annotated executors by making things go through attributes.
- Add a return attribute to summaries, so that we can track flows of thread guards/executors/future stuff through returned values.
- Dispatch modeled functions to model summaries.
This will help in following diffs where runnables will also go through attributes.
Reviewed By: skcho
Differential Revision: D18660185
fbshipit-source-id: e26b1083e
Summary:
The infer/src/c_stubs directory contains a dune-project file. This
causes ocamlformat to conclude that the root of the project is
infer/src/c_stubs, and there is no .ocamlformat file there, so it does
not apply to the Fnv64Hash.ml file.
(There is currently a bug where it does apply anyway, but it is fixed
in master.)
Reviewed By: dulmarod
Differential Revision: D18684831
fbshipit-source-id: bcb53ce94
Summary:
Some field types of structs are missing in Java. The reason is:
* When capture, empty struct types are added without their fields.
* The empty struct types are overwritten to the global tenv when merging all tenvs.
As a fix, this diff add a boolean field, `dummy`, in `Typ.Struct.t`, then avoids that non-dummy types are replaced by dummy types.
Reviewed By: ngorogiannis
Differential Revision: D18657323
fbshipit-source-id: 4a263f8e7
Summary:
This is a better home for knowing whether a function has sentinel args
according to its prototype declaration.
Reviewed By: dulmarod, artempyanykh
Differential Revision: D18573919
fbshipit-source-id: 13f58eaa2
Summary: Another dead flag that one could mistakenly think is accurate.
Reviewed By: dulmarod
Differential Revision: D18573925
fbshipit-source-id: 129a9cff5
Summary:
This was never set to true except in a wrong way in the Java frontend
(see previous diff).
Reviewed By: dulmarod
Differential Revision: D18573927
fbshipit-source-id: 4c9d1a855
Summary:
This seems just wrong: the assume statement can return if the expression
is true.
Reviewed By: skcho
Differential Revision: D18573921
fbshipit-source-id: 47a5b0ea0
Summary:
A plugin update allows infer to know when a function doesn't return
according to its attributes. This propagates this info all the way to
the attributes of each function, and then use this information in a new
pre-analysis that cuts the links to successor nodes of each `Call`
instruction to a function that does not return.
NOTE: The "no_return" `CallFlag.t` was dead code, following diffs deal
with that (by removing it).
Reviewed By: dulmarod
Differential Revision: D18573922
fbshipit-source-id: 85ec64eca
Summary:
This also prints the CFGs *after* pre-analysis for individual procedures
in infer-out/captured/<filename>/<proc>.dot. One can also look up the
CFGs before pre-analysis in infer-out/captured/proc_cfgs_frontend.dot.
Context: I want to add a pre-analysis that needs to look at proc
attributes inter-procedurally. For this to make sense it has to happen
*after* all of capture, and before analysis.
Thus, this diff brings back the lazy running of the pre-analysis like in
D15803492, except that we still make sure to run the pre-analyses
systematically regardless of the checkers being run by running the
pre-analysis from ondemand.ml. Also we don't need to re-introduce the
"did_preanalysis" proc attribute for the same reason that the
pre-analysis is now run once and for all by ondemand.ml (instead of each
individual checker back in the days).
This has the benefit of running the pre-analysis only when needed, and
the drawback that several concurrent processes analysing the same proc
descs will duplicate work. Since pre-analyses are supposed to be very
fast I assume that neither is a big deal. If they become more expensive
then the benefit gets bigger and the drawback is just the same as with
regular analyses.
Reviewed By: skcho
Differential Revision: D18573920
fbshipit-source-id: de350eaef
Summary: This diff get static value with `EMPTY` field from class initializer.
Reviewed By: ngorogiannis
Differential Revision: D18616588
fbshipit-source-id: 26414c9b2
Summary: When we see a call to schedule some work on an executor and we don't have evidence that it is on some specific thread (UI/BG), instead of dropping the work, assign it `UnknownThread` and treat it as running on the background by default.
Reviewed By: jvillard
Differential Revision: D18615649
fbshipit-source-id: e8bad64b6
Summary:
This allows us to move the CFG rendering to IR/.
The parts of that file concerning CFGs and those concerning Biabduction
specs were entirely disjoint, it turns out, so that was easy.
Reviewed By: jberdine
Differential Revision: D18573924
fbshipit-source-id: 0a5ab6478
Summary: Following D18351867, this diff adds more size alias: when initial array size is one.
Reviewed By: ezgicicek
Differential Revision: D18530598
fbshipit-source-id: 26d57fe30
Summary:
- more flexible API
- less error-prone thanks to named parameters
- also takes care of adjusting predecessors of the previous successors!
This fixes some (probably harmless) bugs in the frontends.
Reviewed By: dulmarod
Differential Revision: D18573923
fbshipit-source-id: ad97b3607
Summary: The transition HOLDS-NEXT WITH-TRANSITION Parameters was only implemented for ObjC methods, now it also works on function calls.
Reviewed By: jvillard
Differential Revision: D18572760
fbshipit-source-id: 06e615cbe
Summary:
Now we point to the root cause of the problem, and also provide
actionable way to solve the issue
Reviewed By: artempyanykh
Differential Revision: D18575650
fbshipit-source-id: ba4884fe1
Summary:
Two goals:
1. Be less assertive when speaking about third party code (it might be
written with different conventions).
2. Point to third party signatures folder so the users know how to
proceed
Reviewed By: artempyanykh
Differential Revision: D18571514
fbshipit-source-id: 854d6e746
Summary:
1/ We now support messaging for third-party: show file name and line
number
2/ We did not show information about internal models in case of param
calls
3/ Small change: we don't specify "modelTables.ml" anymore: no need to
expose implementation details
Reviewed By: artempyanykh
Differential Revision: D18569790
fbshipit-source-id: 28586c8ff
Summary:
In the follow up diff we will use it to render more informative
error message when type error is relevant to the fact that the code is
third-party
Reviewed By: artempyanykh
Differential Revision: D18569783
fbshipit-source-id: 4f6dcc404
Summary:
Let the functions return the modified storage, not unit.
It will play nicely with the next diff, which will add more data to
storage which will be non-mutable.
Reviewed By: artempyanykh
Differential Revision: D18569768
fbshipit-source-id: ade5685be
Summary:
Whole bunch of changes aimed to make error messages more clear and
concise.
1/ Wording and language is unified. We make errors sound more like a
type system violations, rather than linter reccomendations.
Particularly, we refrain from saying things like "may be null" - this is
a linter-style statement that may provoke discussions (what if the
developer knows it can not be null in this particular case).
Instead, we refer to declared nullability and nullability of actual values. This way, it is more clear that this is not a heuristic, this is how rules of a type-system work.
2/ Additionally, we drop things like field class in places when the
context should be clear by who looks at the error. We expect the user
sees the code and the error caption. So e.g. we don't repeat the word "field"
twice.
3/ In cases when we are able to retrieve formal param name, we include it for
usability.
4/ For Field not initialized error, we refer to Initializer methods:
this is a non-obvious but important nullsafe feature.
Reviewed By: artempyanykh
Differential Revision: D18569762
fbshipit-source-id: 9221d7102
Summary:
It make the message bit less heavy, and also it is kind of obvious that
it is origin.
In follow up diffs we will change the text so it is hopefully even more
obvious.
Reviewed By: artempyanykh
Differential Revision: D18527695
fbshipit-source-id: a305d547b
Summary:
1. We don't want to teach the users to ignore noise origin because
sometimes we are going to render something useful for them.
2. It just looks not cool.
Reviewed By: artempyanykh
Differential Revision: D18527694
fbshipit-source-id: 0ea248122
Summary:
By default `install` will always overwrite the destination, which in
particular makes the target looks newer to `make`. This was causing the
models to be rebuilt on every `make`.
Passing `-C` helps with that:
-C, --compare
compare each pair of source and destination files, and in some cases, do not modify the destination at all
Reviewed By: martintrojer
Differential Revision: D18574558
fbshipit-source-id: 40758d689
Summary: Android may spontaneously call these methods on the UI thread, so recognize the fact.
Reviewed By: ezgicicek
Differential Revision: D18530477
fbshipit-source-id: a8a798779
Summary:
First step towards a global analysis. A new command line flag activates the step in `Driver`.
The whole-program analysis is a simple, quadratic (inefficient-as-yet), iteration over all domain elements. However, it is restricted to those elements that are explicitly scheduled to run.
Reviewed By: skcho
Differential Revision: D17787441
fbshipit-source-id: 9fecd766c
Summary:
This diff also removes clumsy typeErr.origin_desc and improves interface
of TypeOrigin.
This will allow to render smarter error message, depending on the
context (see follow up diff).
Reviewed By: ngorogiannis
Differential Revision: D18527692
fbshipit-source-id: b7eb11db8
Summary:
Note: Disabled by default.
Having some support for values, we can report when a null or constant
value is being dereferenced. The particularity here is that we don't
report when 0 is a possible value for the address, or even if we know
that the value of the address can only be 0 in that branch! Instead, we
allow ourselves to report only when we the address has been *set* to
NULL (or any constant).
This is in line with how pulse deals with other issues: only report when
1. we see an address become invalid, and
2. we see the same address be used later on
Reviewed By: skcho
Differential Revision: D17665468
fbshipit-source-id: f1ccf94cf
Summary:
This was causing loads of false positives later in the stack.
Invalidating the address of the object seems to be enough here as it
doesn't break any tests.
Reviewed By: ezgicicek
Differential Revision: D18246090
fbshipit-source-id: 2ef9a6a5c
Summary:
This diff avoids unqualified variables by `ItvUpdatedBy` are qualified later. For example,
```
z = x & y;
z = z + 1;
```
While `z` should not be selected as a control variable, it wasn't, because it was qualified by the addition. This pattern introduces FPs in many cases.
Reviewed By: ngorogiannis
Differential Revision: D18505894
fbshipit-source-id: 13aec3008
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
We used to use default for this, which is both buggy and hard to
diagnose. Now we explicitly document nullsafe behaviour in this case.
Reviewed By: artempyanykh
Differential Revision: D18481220
fbshipit-source-id: 0b0cf8b38
Summary:
This diff excludes integer variables from control variables when their values are calculated by
binary operators.
Reviewed By: ezgicicek
Differential Revision: D18505826
fbshipit-source-id: 710533d4c
Summary: Adding a trace that includes when strongSelf was assigned to, and when it's used without a check.
Reviewed By: skcho
Differential Revision: D18506113
fbshipit-source-id: 778c4b086
Summary: Let's make UI/Cold start parts of the message bold to get more attention.
Reviewed By: jvillard
Differential Revision: D18504857
fbshipit-source-id: b0f199f55
Summary:
The variable strongSelf, because it is equal to a weak captured pointer, needs to be checked for null before being used, otherwise it could be null by the time the block is executed.
Added this check to the SelfInBlock checker, and removed it from biabduction.
We want to migrate all the objc checks from biabduction, so it will be easier to change and faster and more reliable. Moreover, this check is more general, it will flag any use of unchecked strongSelf, not just a dereference.
Reviewed By: skcho
Differential Revision: D18403849
fbshipit-source-id: a9cf5d80b
Summary:
There was a precision loss during the substitution of array block. For example:
Callee's abstract memory includes an array block as follows, where `a` is a parameter.
```
a.elements -> { a.elements[*] with a.elements.size }
```
Callers' abstract memory includes a pointer that may point to multiple array blocks.
```
c -> { x, y }
x.elements -> { x.elements[*] with x.elements.size }
y.elements -> { y.elements[*] with y.elements.size }
```
When the callee is called with the parameter `c`, the callees memory is substituted to:
```
x.elements -> { x.elements[*] with top , y.elements[*] with top }
y.elements -> { x.elements[*] with top , y.elements[*] with top }
```
because `a.elements[*]` was substituted to `{ x.elements[*] , y.elements[*] }`
and `a.elements.size` was substituted to `top ( = x.elements.size join y.elements.size )`.
This diff tries to keep the precision in the specific case, not to join the sizes of array blocks.
So now the same callee's abstract memory is substituted to:
```
x.elements -> { x.elements[*] with x.elements.size }
y.elements -> { y.elements[*] with y.elements.size }
```
Reviewed By: ngorogiannis
Differential Revision: D18480585
fbshipit-source-id: b70e63c22
Summary: Due to the weakness of the analysis which can't detect side-effecting prop setting (e.g. as in `builder.prop1(..)`), we currently have many broken chains that do do have any `create` method in their prefixes. Let's not report on these broken chains.
Reviewed By: skcho
Differential Revision: D18503523
fbshipit-source-id: 7506e34b7
Summary:
In some apps executors are obtained by calling standard framework methods (and not by using DI with annotations).
To treat this style, we need to
- Detect calls that return such executors (`do_executor_effect`) and tag the return result with an `Attribute` indicating that it is now an executor, plus what thread it uses.
- Use that information when calling `execute`, to resolve the executor, if any, and its thread (in `do_work_scheduling` via `AttributeDomain.get_executor_constraint`).
- All this requires a new domain component, mapping variables to attributes. This extends the component previously used for remembering whether a variable is the result of a check on whether we run on the UI thread.
At the same time, I un-nested some functions from the transfer function for readability.
Reviewed By: skcho
Differential Revision: D18476122
fbshipit-source-id: bc39b5c2f
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
This method was the last escape hatch for InferredNullability to
misreport about its origin.
Now each time we change nullability we provide origin that is consistent
with the nullability itself.
Reviewed By: artempyanykh
Differential Revision: D18453567
fbshipit-source-id: 3d9b7fa2e
Summary:
This is one of key diff in this refactoring stack.
Previously, we used to set nullability independently of type origin,
which opened doors for inconsistent states, bugs, and overcomplicated
code.
Now we have just two method left:
1. `create` -> the main one, yay
2. `set_nonnull` -> will get rid of this in the next diff.
Reviewed By: artempyanykh
Differential Revision: D18451950
fbshipit-source-id: edc485709
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
In this diff, we merely rename the value to clearly reflect its currect
usage.
In follow up diffs we are going to make usages of this instance more
restricted and concentrate them in several places in the code.
Reviewed By: artempyanykh
Differential Revision: D18451290
fbshipit-source-id: cf3773364
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
1. `Formal` was bit cryptic name.
2. Splitting method param and this makes a lot of sense. It is almost an
implementation detail that hey happen to come from "param" in the
method's signature.
3. Apart from others, this diff fixes a minor bug - we used to treat
this as DeclaredNonnull, which (in future) means suppressing legit warnings
like condition redundant. This would be an issue if we were to start
showing "high confidence" condition redundant warnings.
Reviewed By: ngorogiannis
Differential Revision: D18451294
fbshipit-source-id: acc295e3f
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
1. Make Field origin a record: consistent with Proc; more readable; will
add more elements in follow up diffs.
2. `Proc` is bit ambiguous: it is unclear if this is a method call result or param.
Let's make it clear that it is the result of a method call.
Reviewed By: ngorogiannis
Differential Revision: D18450188
fbshipit-source-id: c5abae3ad
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
Using `New` was definitely not a good idea here, we need a specific
data type value here.
Reviewed By: artempyanykh
Differential Revision: D18450174
fbshipit-source-id: 7ce7cc7e8
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
Introducing more specific origins will allow us to automatically infer
nullability based on type origin (in follow up diffs).
Reviewed By: ngorogiannis
Differential Revision: D18450165
fbshipit-source-id: 9dc0d25c0
Summary:
This diff uses `of_java_array_alloc` instead of `of_c_array_alloc`. Java's array does not have
offset and stride. While this simplifies abstract memories in Java, there is not visible changes I can think of.
Reviewed By: ezgicicek
Differential Revision: D18453474
fbshipit-source-id: 36bdf3daf
Summary:
We were not tracking static `create ` and `build` methods intra-procedurally in the analysis and we just looked up their summaries. This resulted in not having static `create` methods in the trace. Let's fix that by pushing static check into `satisfy_heuristic`.
This results in a few FPs, but such cases are not very common in the codebase.
Reviewed By: skcho
Differential Revision: D18451955
fbshipit-source-id: d4ccf46f5
Summary:
We consider Java collections to be like c++ std::vectors and add models for
- `Collections.get(..)`
- `__cast`
Reviewed By: skcho
Differential Revision: D18449607
fbshipit-source-id: 448206c84
Summary: `equals1` and `equals2` in `SafeInvertedMap.join` are references that indicate whether given parameters and the result is physically equal or not. This diff fixes a missing update of them.
Reviewed By: ezgicicek
Differential Revision: D18450680
fbshipit-source-id: bae19cbe9
Summary:
`getThis` is an idiom for allowing Builder sub-classes to jump through the hoops of covariance plus java generics with self types. It's declared as abstract in the (generic) inner `Builder` class of a root class, and subclasses declare generic `Builder`s that inherit from the generic root `Builder` and trivially implement this method by returning `this`. Obviously, this returns conditional ownership (if `this` is owned, then the return value is owned).
The way it's typically used is
```
T foo() {
...
return getThis();
}
```
However, because abstract methods need dynamic dispatch for proper summarisation, we miss all that. A workaround was been implemented in D8947992 (see that for context), but it was buggy -- it required that the LHS type in the assignment
```
lhs = this.getThis();
```
is the same as the type of `this`, but this is too strict (eg, when using casts).
Here, the condition is changed to requiring that the return type of the method is the same as the type of `this`.
We also avoid asking for the `procdesc` as everything needed is in the attributes.
Reviewed By: jberdine
Differential Revision: D18450737
fbshipit-source-id: e67f0495c
Summary:
It returns non-top value when one of the parameters of band is positive, i.e., `x & 255` returns
`[0, 255]` instead of top.
Reviewed By: ezgicicek
Differential Revision: D18448614
fbshipit-source-id: aaa298a66
Summary:
Let's introduce a set of new cost analysis issue types that are raised when the function is statically determined to run on the UI thread. For this, we rely on the existing `runs_on_ui_thread` check that is developed for RacerD. We also update the cost summary and `jsonbug.cost_item` to include whether a method is on the ui thread so that we don't repeatedly compute this at diff time for complexity increase issues.
Note that `*_UI_THREAD` cost issues are assumed to be more strict than `*_COLD_START` reports at the moment. Next, we can also consider adding a new issue type that combines both such as `*_UI_THREAD_AND_COLD_START` (i.e. for methods that are both on cold start and run on ui thread).
Reviewed By: ngorogiannis
Differential Revision: D18428408
fbshipit-source-id: f18805716
Summary:
This diff tries more narrowing during analysis in order to get preciser results on nested loops.
In the widening phase, it does narrowing a loop right after its widening, for each loops. In general, this may make the widening phase non-terminating because it keeps the abstract state from monotonely increasing to the fixed point in a finite number of iterations. To avoid that situation, this diff applies the narrowing only when the first visit of the loop in the widening phase.
Reviewed By: ngorogiannis
Differential Revision: D18400631
fbshipit-source-id: cc76f7e85
Summary: Sometimes there is a code like `for(int i = 1; i < x; i++){ l.add(); }`, where the first element in a list is addressed specifically. This case was not analyzed precisely, because the alias value is added only when `i` is initialized by 0 by heuristic. This diff extends the heuristic, so it adds a size alias between `i` and `l.size()` when `i` is initialized by 0 or 1.
Reviewed By: ezgicicek
Differential Revision: D18351867
fbshipit-source-id: e7d19a4ec
Summary:
This diff adds semantics of Java function calls of enum `values` inside class initializers.
* Java class initializer function initializes a specific field `$VALUES`, which points to the list
of enum values.
* The `values` function of enum class returns the value of `$VALUES`.
The problem is when the `values` function is called inside the class initializer, for example:
```
enum Color {
RED,
GREEN,
BLUE;
static {
for (Color c : Color.values()) {}
}
}
```
This introduces a recursive dependency: the class initializer calls `Color.values` and the function
returns `Color.$VALUES` the value of which should be initialized in the class initializer.
To address the problem, this diff finds the value of `$VALUES` in its abstract memory when
`values` is called inside the class initializer.
Reviewed By: ezgicicek
Differential Revision: D18349281
fbshipit-source-id: 21766c20f
Summary:
The order was wrong: the map from procnames to node-ids was cleaned first, but to clean the map from node-ids to nodes, we need the id and we have already removed it.
The symptom was that effectively no new leaves are created by "removing" a node, so the call graph scheduler quickly devolves to the file-based one.
Reviewed By: skcho
Differential Revision: D18448209
fbshipit-source-id: f272a8112
Summary:
There was some over-general treatment of reachability, in anticipation of changes that didn't happen.
In particular, we only need to flag/remove single nodes, as they must be leaves to be scheduled,
therefore we never need to traverse their successors, because there aren't any.
Reviewed By: jvillard
Differential Revision: D18425905
fbshipit-source-id: b86490542
Summary:
- Convert `task_generator` into a module of `ProcessPool` and collect inside the two combinators which were in semi-random places.
- Make `SyntacticCallGraph` export a `task_generator` as opposed to a call-graph builder.
- Separate `target` type and put it in its own module to avoid dependency cycles.
Reviewed By: skcho
Differential Revision: D18425718
fbshipit-source-id: 7957edac8
Summary:
`typecheck_instr` is a function that essentially does most of nullsafe typechecking
work: it pattern matches the instruction and handles most of business logic (e.g. knowledge about builtins, rules of PropagatesNullable, flow-sensitive rule for condition branches etc etc etc.
This is a gigantic function consisting of huge amount of nested
function. No way it can fit in someone's head and it is super hard to
read. It also captures many things like node and node'.
This diff does fairly shallow work:
1. Moves most of nested subfunctions into top-level functions. It also
means that what was previously hiddenly captured is now made an explicit
dependency.
2. Some (minority) of them are renamed to better indicate the context.
3. Some (minority) of params are renamed to clear indicate the
difference.
4. Some params are made named params, especially booleans and params
that have the same types so could be confused.
5. In some placed, added bit more clear comments.
This diff DOES NOT do:
1. Tries to fully resolve confusion between cryptic names. In couple of places, instead of node'
and node we use node and original_node, which is barely better. But we
still have ugly things like typestate and typestate1 leaving together.
Baby steps!
2. Tries to create better or consistent names in functions. I did it for
most obvious cases, that's it.
3. Order or params is arbitrary. I just used the order that was easy to
come up with, no consistency here.
Reviewed By: artempyanykh
Differential Revision: D18425965
fbshipit-source-id: 7a775f08d
Summary:
This diff extends bound domain to express Min/Max of another bounds, so it can keep some more
precision in `Math.min/max`.
limitation: `MinMaxB`, the constructor of the bound, can contain only linear expressions or
previous min/max expressions.
Reviewed By: ezgicicek
Differential Revision: D18395365
fbshipit-source-id: fc90d27fd
Summary:
We still have few remaining places which we have a logic not encoded in
as a Nullability type. Ignoring third party param calls is one of them.
We don't want this behavior for strict mode.
Reviewed By: artempyanykh
Differential Revision: D18396639
fbshipit-source-id: dbfedc769
Summary:
The upcoming whole-program analysis will need to log reports in different source files simultaneously.
Here, the data structure containing the reports, and in charge of deduplication is generalised to
a map from source file to what it was previously. This way, writing out the reports is possible even
with multiple source files.
Reviewed By: skcho
Differential Revision: D18394994
fbshipit-source-id: 5f5ecc27c
Summary:
This is a dirty hack to make nullsafe behave consistently for internal
and external models.
Medium term this code needs to be rewritten in a better way, so that we
pass all information in annotated signature (either in form of Inferred
nullability, or type origin, which tracks third party calls).
As a follow up, we will make reporting more clear.
Reviewed By: artempyanykh
Differential Revision: D18372660
fbshipit-source-id: 12c2449e1
Summary: Capture locations where work is scheduled to run in parallel (here, just Executors). Also add a test file with cases the upcoming whole-program analysis for starvation should catch.
Reviewed By: dulmarod
Differential Revision: D18346880
fbshipit-source-id: 57411b052
Summary:
Ability to accept relative path is convenient for testing and local
debugging.
Ability to accept absolute paths is needed for buck integration - buck
knows absolute path (to properly invalidate cache when the content of
the folder is changed) and passes it to infer.
Reviewed By: artempyanykh
Differential Revision: D18370407
fbshipit-source-id: be7f12ae1
Summary: Follow ups will include error messaging that makes the choice clear
Reviewed By: artempyanykh
Differential Revision: D18347664
fbshipit-source-id: b6f005726
Summary:
In this diff, we just load the info from the storage. Next diff will be
actually using this information to infer nullability.
`ThirdPartyAnnotationGlobalRepo.get_repo` will be used in the next diff,
hence #skipdeadcode
Reviewed By: artempyanykh
Differential Revision: D18347647
fbshipit-source-id: 82a9223c6
Summary:
In next diffs, we will:
1/ teach nullsafe to read nullability information from the 3rd party
annotation folder
2/ use this storage in addition to our hard-coded
models to respect nullability during type-checking
Reviewed By: artempyanykh
Differential Revision: D18247538
fbshipit-source-id: ee45bc80e
Summary:
This diff extends the alias domain, so each variable can have multiple aliases.
It changed `KeyLhs` can be mapped to multiple alias targets in the `AliasMap` domain:
```
before : KeyLhs.t -> KeyRhs.t * AliasTarget.t
after : KeyLhs.t -> KeyRhs.t -> AliasTarget.t
```
Reviewed By: ezgicicek
Differential Revision: D18062178
fbshipit-source-id: b325a6055
Summary:
It extracts RHS of alias from `AliasTarget.t`, so it changes the `AliasMap` domain:
```
before : KeyLhs.t -> AliasTarget.t // AliasTarget.t includes KeyRhs.t
after : KeyLhs.t -> KeyRhs.t * AliasTarget.t
```
Reviewed By: ezgicicek
Differential Revision: D18299537
fbshipit-source-id: 1446580a8
Summary: We only care about expensive invariant calls. Let's disable the usual one since it is not used and there is no point filling up our dbs.
Reviewed By: dulmarod
Differential Revision: D18298530
fbshipit-source-id: 7a933c8da
Summary:
More rigid format for 3rd party annotation repo
To simplify external processing by tools, lets not accept any divergence from a
format, which is:
<fully qualified class name>#<method_name>(<params>)[ Nullable]
<params> = <list of param separated by ", ">
<param> = [Nullable ]<fully qualified type>
Particularly:
- Dont accept spaces in arbitrary places
- Require a single space after each comma separating params
- Require a single space between closing parethesis and nullable
annotation, if present
- Require at least one '.' in a class name (to prevent errors when
somebody did not specify a package).
This diff also makes error messaging more user-friendly, because now it
is easy to do a minor mistake like forget a space after comma.
Reviewed By: asm89
Differential Revision: D18297382
fbshipit-source-id: 91aab6823
Summary:
This diff tries to make less imprecise division by constants results.
For example, the results of the division `[l, u] / c`, where `c` is a positive constant, are:
1. If `l/c` or `u/c` is representable in the bound domain, it uses the precise bounds, i.e., `[l/c, u/c]`.
2. If it is not representable, it tries to make conservative results:
if `0<=l<=u`, it returns `[0, u]` because `0 <= [l/c, u/c] <= u`
if `l<=u<=0`, it returns `[l, 0]` because `l <= [l/c, u/c] <= 0`
if `l<=0<=u`, it returns `[l, u]` because `l <= [l/c, u/c] <= u`
3. otherwise, it returns top, `[-oo, +oo]`
Reviewed By: ezgicicek
Differential Revision: D18270380
fbshipit-source-id: 8fb14c0e4
Summary:
Add precision to analysis by elaborating the thread-status domain. This is done by having unknown (bottom), UI, BG or Any (both/top) elements in the lattice. This way, when we branch on thread-identity (if I am on UI thread do this, otherwise do that), we know that in one branch we are on UI thread and on the other we are *not* on the UI thread (BG thread), where previously the other branch would just go to top.
With this knowledge we can throw away pairs that come from callees which run on a thread that is impossible, given the current caller thread identity. This can happen when annotations are used incorrectly, and since this is the purview of annot-reachability, we just drop those pairs entirely.
Reviewed By: skcho
Differential Revision: D18202175
fbshipit-source-id: be604054e
Summary:
Now that we have two similar functions, it becomes confusing, because `Pp.to_string` and `Pp.string_of_pp` can seem to do the same stuff, while in reality they do the opposite.
Well, it is still bit confusing, because the proper names would be
`Pp.pp_of_to_string` and `Pp.to_string_of_pp`, but I think this high
level order names are not necessary given that in most cases they will
be used as concrete functions.
I think `Pp.of_string` captures such usages better than `to_string` used to do: you need to pp stuff,
but you have a string (or, technically, a function that returns a string), so you pretty print OF that string, aren't you?
Reviewed By: jvillard
Differential Revision: D18245876
fbshipit-source-id: fd4b6ab68
Summary:
This is a helper module for reading info from a 3rd party nullability repository.
Next diffs are going to use it for reading nullability repository from
disk.
Reviewed By: artempyanykh
Differential Revision: D18225473
fbshipit-source-id: 06a2dc97e
Summary:
Steal a page from RacerD (and improve interface of) on using certain calls to assert
execution on a particular thread. Reduces FPs and FNs too.
Reviewed By: dulmarod
Differential Revision: D18199843
fbshipit-source-id: 5bdff0dfe
Summary:
The zero cost of node does not make sense especially when the abstract memory is non-bottom. This
resulted in unreasonable zero cost results sometimes, e.g. when the checker could not find
appropriate control varaibles having interval values of iteration. This diff fixes this, so sets
the minimum basic cost as 1, if the abstract memory at the node is non-bottom.
Reviewed By: ezgicicek
Differential Revision: D18199291
fbshipit-source-id: b215d10e5
Summary:
When reporting null dereference it is useful to know where the null came
from.
Reviewed By: skcho
Differential Revision: D18206459
fbshipit-source-id: 0c8e6781b
Summary:
This simplifies the code overall. It also makes accessing the action of
a "trace" (which is now stored alongside it instead of deep inside it)
constant time instead of linear in the number of nested calls.
Reviewed By: skcho
Differential Revision: D18206460
fbshipit-source-id: 9546ff36f
Summary:
This adds a more interesting value domain to pulse: concrete intervals.
There are still two main limitations:
1. arithmetic operations are all over-approximated: any assignment involving arithmetic operations is replaced by non-determinism
2. abstract values that are discovered to be equal are not merged into one
Reviewed By: skcho
Differential Revision: D18058972
fbshipit-source-id: 0492a590f
Summary:
This does several things because it was hard to split it more:
1. Split most of the arithmetic reasoning to PulseArithmetic.ml. This
doesn't need to be reviewed thoroughly because an upcoming diff
changes the domain from just `EqualTo of Const.t` to an interval domain!
2. When going through a prune node intra-procedurally, abduce arithmetic
facts to the pre (instead of just propagating them). This is the "assume
as assert" trick used by biabduction 1.0 too and allows to propagate
arithmetic constraints to callers.
3. Use 2 when applying summaries by pruning specs whose preconditions
have un-satisfiable arithmetic constraints.
This changes one of the tests! Pulse now does a bit more work to find
the false positive, as can be seen in the longer trace.
Reviewed By: skcho
Differential Revision: D18117160
fbshipit-source-id: af3b2c8c0
Summary:
Instead of checking that each address in the pre that must be valid is
not invalid in the caller (and error out if it turns out it is invalid)
as we discover them, save these checks for after we are sure that the
precondition can be applied. It is in fact a bug that we can report an
error when trying to apply a precondition that is actually not
satisfiable in the current state for other reasons than lifetime issues.
We still want to skip calls in case of weird issues like mismatch in
number of formals vs actuals.
This will have more obvious effects later when we also check that
arithmetic facts in preconditions are satisfied at the call site: if a
pre mandates "x=1" and "y must be valid" and we have "x=0" and "y
invalid" then we shouldn't report an error.
Reviewed By: skcho
Differential Revision: D18115229
fbshipit-source-id: ad4ce72ff
Summary:
If a precondition cannot be applied, it means that this program path
somehow doesn't make sense for the caller and so should be pruned. Right
now we just treat this as skipping over the call instead.
This will become more important when specs start mentioning arithmetic
facts that must be satisfied at the call site. As it is we will only
stop if we discover aliasing in the pre not present at the call site or
vice versa.
Reviewed By: dulmarod
Differential Revision: D18115230
fbshipit-source-id: 4f1c7a583
Summary: The way `<=` is used in `AbstractDomain` prevents infix use and forces bracketing it everywhere. Replace with simple `leq`.
Reviewed By: jvillard
Differential Revision: D18201854
fbshipit-source-id: 8175224e4
Summary: Abstract state tracks stuff that is not needed for summaries, wasting space/time.
Reviewed By: jvillard
Differential Revision: D18171499
fbshipit-source-id: 25483ced9
Summary:
Primitive types are not annotated. Because of that, we used to implicitly derive
`DeclaredNonnull` type for them. This worked fine, but this leads to errors in Strict mode, which does
not believe DeclaredNonnull type.
Now lets offifically teach nullsafe that primitive types are
non-nullable.
Reviewed By: jvillard
Differential Revision: D18114623
fbshipit-source-id: 227217931
Summary:
This diff adds SafeInvertedMap, which is similar to InvertedMap but it
guarantees that there is no top elements in the tree of the map.
Reviewed By: ezgicicek
Differential Revision: D18062174
fbshipit-source-id: 2fbc51f31
Summary: `Str.regexp_string` should be used to find a method name instead of `Str.regexp`.
Reviewed By: ezgicicek
Differential Revision: D18136598
fbshipit-source-id: c4b56dd64
Summary:
The reporting phase would go through the critical pairs in the summary roughly three times, once for each major type of warning (starvation, lockless violation, deadlock). This is wasteful, and also led to some code duplication. Fix.
Also, use the more efficient annotation matcher in `ConcurrencyModels` and move some model matchers to `StarvationModels`.
Reviewed By: artempyanykh
Differential Revision: D18118149
fbshipit-source-id: ff4dc3d07
Summary: It is now possible to push the thread status into each critical pair. This leads to higher precision, because when code branches on whether it is on the UI thread, the final abstract state of the procedure will be `AnyThread`, but pairs created in the UI thread branch should know that their status is `UIThread`, not `AnyThread`.
Reviewed By: jvillard
Differential Revision: D18114273
fbshipit-source-id: cbb99b46f
Summary: This will be more useful later when adding another one.
Reviewed By: ezgicicek, jberdine
Differential Revision: D18115231
fbshipit-source-id: a0a01901a
Summary:
The business of translating `Top/True/False` to `true/false` can be
hidden more.
Reviewed By: skcho
Differential Revision: D18115228
fbshipit-source-id: 071fcbddf
Summary:
Warning 33 (unused open) is enabled but the module open is not really
unused, it's just also opened at the top of the file...
Reviewed By: skcho
Differential Revision: D18114385
fbshipit-source-id: 2a8f9512a
Summary:
This diff avoids making top values on unknown non-static function,
such as abstract function, calls. This is necessary because the
generated top values ruin the precision of the cost checker.
Reviewed By: ezgicicek
Differential Revision: D17418611
fbshipit-source-id: aeb759bdd
Summary:
The wrong function was used when we tried to see if the class is
annotated with NullsafeStrict. This made it work only for non-static
methods.
Now we use the proper way.
Reviewed By: ngorogiannis
Differential Revision: D18113848
fbshipit-source-id: 02b7555be
Summary:
Previously, we considered a function which modifies its parameters to be impure even though it might not be modifying the underlying value. This resulted in FPs like the following program in Java:
```
void fresh_pure(int[] a) {
a = new int[1];
}
```
Similarly, in C++, we considered the following program as impure because it was writing to `s`:
```
Simple* reassign_pure(Simple* s) {
s = new Simple{2};
return s;
}
```
This diff fixes that issue by starting the check for address equivalnce in pre-post not directly from the addresses of the stack variables, but from the addresses pointed to by these stack variables. That means, we only consider things to be impure if the actual values pointed by the parameters change.
Reviewed By: skcho
Differential Revision: D18113846
fbshipit-source-id: 3d7c712f3
Dulma Churchill
Sungkeun Cho
Mitya Lyubarskiy
Nikos Gorogiannis
Radu Grigore
Artem Pianykh
Fernando Gasperi Jabalera
Jules Villard
Ezgi Çiçek
Artem Pyanykh
David Pichardie
Martin Trojer
Kfir Schindelhaim
Josh Berdine