Summary:
I... kinda forgot about attributes in D25092158 (ab2813e355), which is probably why
impurity was angry that attributes were sometimes missing. Repare this
by adding together the attributes of all the values that are equal.
Reviewed By: skcho
Differential Revision: D25428405
fbshipit-source-id: e5d55b782
Summary:
Address a long-standing embarassing TODO in a minimal way: array indices
are values and when applying a summary we didn't actually bother
translating callee values to caller values. Fix that in a simple way by
just using the current mapping between callee and caller values and
otherwise freshen callee values to avoid clashes with caller values.
Reviewed By: ezgicicek
Differential Revision: D25424013
fbshipit-source-id: 03ca59b9f
Summary:
I wrote an entire diff trying to fix the "bug" that this wasn't needed
so I think this warrants a comment ;)
Reviewed By: ezgicicek
Differential Revision: D25423958
fbshipit-source-id: 414038e40
Summary: The Ondemand entry point `analyze_proc_desc` exists purely to support specialisation under biabduction. After fixing the storing of specialised `proc_desc`s for java it suffices to use `analyze_proc_name` which will work just fine in its place.
Reviewed By: jvillard
Differential Revision: D25421763
fbshipit-source-id: b162feec3
Summary: Whenever the interface functions are called, there is always an execution environment present, so it is safer and better to get rid of the setter/getter reference thing.
Reviewed By: jvillard
Differential Revision: D25421335
fbshipit-source-id: 7110c932b
Summary: This diff gives semantics of dispatch_sync to call the closure parameter.
Reviewed By: jvillard
Differential Revision: D25423175
fbshipit-source-id: a45309073
Summary:
This diff supports inter-procedural uninit analysis in pulse.
* Added `MustBeInitialized` attribute to pre state when an address is read
* Remove `Uninitialized` attribute when callee has `WrittenTo` for the
same address
Reviewed By: jvillard
Differential Revision: D25368492
fbshipit-source-id: cbc74d4dc
Summary:
Skipping the analysis of `std::vector::empty()` caused false positives: in the case where `std::vector::empty()` was called several times ("returning" different values each time), we were not able to prune infeasible paths.
Model `std::vector::empty()` as returning the same value every time it is called.
Reviewed By: ezgicicek
Differential Revision: D23904704
fbshipit-source-id: 52e8a2451
Summary:
Since D20736043 (d84fea52ae) is adding edges from the noreturn function node to exit node, analyzers should
handle the state differently to normal states.
Reviewed By: ezgicicek
Differential Revision: D25402576
fbshipit-source-id: a98e41b0c
Summary:
This diff adds uninitialized value check in pulse. For now, it supports only simple cases,
- declared variables with a type of integer, float, void, and pointer
- malloced pointer variables that points to integer, float, void, and pointer
TODOs: I will add more cases in the following diffs.
- declared/malloced array
- declared/malloced struct
- inter-procedural checking
Reviewed By: jvillard
Differential Revision: D25269073
fbshipit-source-id: 317df9a85
Summary:
This diff adds the ability to skip translation with `... && neg ( pattern)` logic so that we can skip translation of some files if the source does not contain a pattern.
Note that `skip-translation` expects a list of patterns as disjunctions:
https://www.internalfb.com/intern/diffusion/INFER/browse/master/infer/src/IR/inferconfig.ml?commit=76ae5fa0d3376573f6d04814e47ff6b5a9dd9746&lines=74
whereas we want the ability to have conjuctions inside.
## Context
Immutability analysis requires analyzing generated code which might have `Immutable` annotations. When analysing fbandroid, we skip all generated code:
```
"skip-translation": [
{
"source_contains": "generated",
"language": "Java"
}
],
```
However, rather than analyzing all generated code (which might be expensive across all targets) by removing the above, with this diff, we only analyze generated code that doesn't contain e.g. `Immutable` and skip all other generated code as before:
```
"skip-translation": [
{
"source_contains": "generated",
"source_not_contains": "Immutable",
"language": "Java"
}
],
```
Reviewed By: ngorogiannis
Differential Revision: D25328931
fbshipit-source-id: 3ae6ae92a
Summary:
D17710123 (ec62fbefb2) introduced locking to protect the shared pipe to the
originator of the process pool.
D20158845 (a154c8c328) changed the situation by creating a private pipe to the
originator for each worker, so should have removed the locks.
Reviewed By: ezgicicek
Differential Revision: D25370445
fbshipit-source-id: e5f3e4b00
Summary:
See comments added in the code: there's always a chance some unsat
states make it to the end of the execution of an instructions. Before
this diff they would get propagated and executed until some code path
actually bothers to check their satifiability. After this diff we throw
them out at the end of the execution of the first instruction they get
generated in.
An alternative design would be to return Unsat explicitly everywhere we
currently might return `false_`. This would be good too but there's
still a chance we'd generate `false_` and so even if we did that more
significant refactoring, the detection in this diff would still be a
good last line of defense.
Reviewed By: ezgicicek
Differential Revision: D25336042
fbshipit-source-id: a24693596
Summary: When using the restart scheduler incrementing the analyzed count before the analysis itself gives wrong results.
Reviewed By: jvillard
Differential Revision: D25367787
fbshipit-source-id: aed22cc68
Summary:
The frontend of ObjC regarding to captured variable was incorrect: it set capturing mode as
by-reference always, but it actually translaged as if all captured variables were passed with
by-value. This diff fixes this based on the document.
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Blocks/Articles/bxVariables.html
* global variable: by reference
* local variable: by value
* `static` local variable: by reference
* `__block` local variable: by reference
* parameter: by value
Reviewed By: jvillard
Differential Revision: D25306122
fbshipit-source-id: ec499d705
Summary:
When we know `v1 = v2`, canonicalize `v2 -> v1 * v3 -> v2` to
`v1 -> v1 * v3 -> v1`. Only do this when creating summaries
(and so also when reporting errors) for now.
This only takes into account the equality relation between variables
for now. It needs to be extended to take into account other ways
variables can be equal, eg when two variables are equal to the same
constant or the same term.
Reviewed By: skcho
Differential Revision: D25092158
fbshipit-source-id: 9e589b631
Summary:
Made `AbductiveDomain.summary_of_post` return a Sat/Unsat to make sure
callers filter unsat summaries. Also made `ExitProgram` take a summary
instead of a non-normalized abstract state, which was wrong (mostly
could litter the disjuncts with infeasible paths).
Reviewed By: skcho
Differential Revision: D25277565
fbshipit-source-id: 72dacb944
Summary:
Use the new module to represent both Sat/Unsat from Pulse formulas, and
FeasiblePath/InfeasiblePath from PulseReport.
Reviewed By: jberdine
Differential Revision: D25277566
fbshipit-source-id: 9f8412ca9
Summary:
Until then we mostly ignored aliasing constraints added by callees,
except some of the cases where the aliasing was incompatible with the
current heap. But, we should add `v_caller = v_caller'` any time both
of these (caller) variables are equal to the same callee variable.
These situations are hard to create at the moment since all values in
the pre-condition heap are created distinct and never change. The next
diff introduces canonicalisation of states and merges equal variables,
thus needs this change.
Reviewed By: skcho
Differential Revision: D25092213
fbshipit-source-id: 9fa7b8b53
Summary:
This diff uses Pvar.t in CapturedVar.t, so that
* it can include additional info in Pvar.t
* it can avoid some `Pvar.mk` calls when using the captured variables
Reviewed By: jvillard
Differential Revision: D25331763
fbshipit-source-id: 4e0c2ab4a
Summary:
- Eliminate dead `source` field in `field_data` record.
- Use a 2-level hashtable structure (`procname` -> `source_file` and `source_file` -> `file_data`) to help potentially using an LRU cache for layer 1.
- Use laziness instead of mutability.
- Fixed stale comments
Reviewed By: jvillard
Differential Revision: D25303360
fbshipit-source-id: 68a22d299
Summary:
Models are currently not specialised*. Remove the ability entirely, to allow further changes.
*this is proved by the fact that Infer doesn't crash, which it would if it tried to specialise models, because their procdesc is the already pre-analysed one, and pre-analysing twice a procdesc will crash.
Reviewed By: jvillard
Differential Revision: D25027698
fbshipit-source-id: 76ae5fa0d
Summary:
The code related to "pruned" Topl constraints was scattered in
PulseTopl. Now it's grouped in PulseTopl.Constraint.
Reviewed By: jvillard
Differential Revision: D25273820
fbshipit-source-id: 5d2d0765b
Summary:
When extracting summaries, ask PulseFormula to work harder to prove that
path-conditions are unsat. This reduces the number of false positives.
Reviewed By: jvillard
Differential Revision: D25270609
fbshipit-source-id: 61ef5e8ac
Summary:
Added a topl-max-disjuncts, which is analogous to pulse-max-disjuncts.
Note, however, that the maximum number of states tracked will be the
product of the two limits.
Added also topl-max-conjuncts, which drops Topl states that became too
complex.
Reviewed By: jvillard
Differential Revision: D25240386
fbshipit-source-id: 588c90390
Summary: Tests for the LRU cache now expect a fixed order of key-value pairs in the cache where previously only the set of key-value pairs was considered.
Reviewed By: jvillard
Differential Revision: D25301133
fbshipit-source-id: 0d8077950
Summary: In Java, public class name should be the same to file name.
Reviewed By: ezgicicek
Differential Revision: D25245194
fbshipit-source-id: 49fd16748
Summary:
This diff adds a new issue type for reporting modifications to immutable fields (when `report-immutable-modifications` is enabled).
The underlying analysis depends on impurity analysis which itself is based on post-processing of pulse's summaries.
Reviewed By: skcho
Differential Revision: D25216637
fbshipit-source-id: 42e843793
Summary:
Previously, impurity analysis only collected one access for a single modification but not all other modifying accesses. This diff
- changes the impurity domain to collect all modifying accesses
- tracks and prints all the accesses seen to reach the modification, improving readability&debugging
Recording all accesses are needed in the next diff to determine if a method modifies any immutable fields. To determine that, we need to know all modifications, not just a single one.
Reviewed By: skcho
Differential Revision: D25186516
fbshipit-source-id: 43ceb3cd8
Summary: The main point here is to ignore owned interfaces when considering whether to warn about non-thread-safe calls.
Reviewed By: ezgicicek
Differential Revision: D25187775
fbshipit-source-id: c2a7ce89c
Summary: There are no users of this, and it stands in the way of refactoring.
Reviewed By: ezgicicek
Differential Revision: D25241940
fbshipit-source-id: 5e653341a
Summary: To look for captured variable address escape we should only check the validity of the addresses captured by reference. Checking the validity of the address captured by value can cause nullptr dereference false positives.
Reviewed By: jvillard
Differential Revision: D25219347
fbshipit-source-id: faf6f2b00
Summary:
For a long sequence of calls nop();...;nop() the runtime was quadratic
because formals and actuals were bound via equalities. Now,
substitutions are used, when easy.
Reviewed By: jvillard
Differential Revision: D25211504
fbshipit-source-id: 696e3dcdf
Summary:
If f() calls g() and g() violates a property, there used to be two
traces (one for f() and one for g()) even if all that f() has to do with
the property is that it calls g(). Now the error is reported only in
g().
Reviewed By: jvillard
Differential Revision: D25210007
fbshipit-source-id: 68ea57e71
Summary:
A "large step" is a call, and it is "trivial" if it does not affect the
automaton state; i.e., if it is irrelevant to the property being
tracked.
Reviewed By: jvillard
Differential Revision: D25209670
fbshipit-source-id: bf3e594b3
Summary:
Makes sure that topl summaries don't repeat. Previously this happened
when two posts led to the same summary when procedure-local variables
were killed. Such repeated summaries quickly lead to exponential
explosion. (For example, the added test -- `ManyLoops.java` -- didn't
finish in any reasonable time.)
Reviewed By: jvillard
Differential Revision: D25209623
fbshipit-source-id: 04b1a3e12
Summary:
Now one can use the pattern #ArrayWrite(A,I) to match on a write at
index I in array A. This only works in the Pulse variant of Topl (not in
the one based on SIL instrumentation).
Reviewed By: jvillard
Differential Revision: D25202768
fbshipit-source-id: 479f434e3
Summary:
PulseTopl.large_step is now implemented
All active tests are migrated now to topl-in-pulse.
Reviewed By: jvillard
Differential Revision: D25179556
fbshipit-source-id: dc1136bab
Summary:
When running the deep-Pulse version of Topl, it now produces and reports
traces.
Reviewed By: jvillard
Differential Revision: D25177139
fbshipit-source-id: 6955ee0cd
Summary:
A Topl "small step" is a call to a method that is of interest to the
automaton. When such a call of interest is made, the topl component of
PulseAbductiveDomain.t is updated. This means that intra-procedural
Topl should now work entirely inside Pulse, without instrumenting Sil.
Main TODOs:
- add error extraction
- implement inter-procedural (PulseTopl.large_step)
Reviewed By: jvillard
Differential Revision: D25028286
fbshipit-source-id: e31a96d13
Summary:
When a procedure is called, we must evolve the topl component of the
PulseAbductiveDomain. This commit just inserts a call to a dummy
PulseTopl.large_step in the right place. The [large_step] function still
needs to be done.
Reviewed By: jvillard
Differential Revision: D24980825
fbshipit-source-id: 0eb280145
Summary:
Put hooks into Pulse for a faster Topl:
- done: PulseAbductiveDomain now tracks a Topl state
- todo: PulseTopl needs some transfer function (now they're dummies)
Reviewed By: jvillard
Differential Revision: D23815497
fbshipit-source-id: f3f0cf9ef
Summary: Ocaml doesn't have extensible records so the workaround I have found is to wrap the inferbo model env into another record.
Reviewed By: skcho
Differential Revision: D25244745
fbshipit-source-id: 87f53d5e5
Summary: We always add Pvars to impurity domain. So let's simplify the domain to make it explicit.
Reviewed By: ngorogiannis
Differential Revision: D25220214
fbshipit-source-id: 4dc9bce4c
Summary:
Currently, we don't issue warnings for third party return value in
non-@Nullsafe modes.
For some integrations, this feature is useful.
This diff repurposes the existing param to suit this goal.
Reviewed By: artempyanykh
Differential Revision: D25186043
fbshipit-source-id: 308101841
Summary:
This diff makes the issue to be rendered more clearly. Before, we used to report
weirdly looking unconventional mode names like NullsafeLocal, even when
exact mode name was irrelevant.
Reviewed By: artempyanykh
Differential Revision: D25186041
fbshipit-source-id: 2619bcbd2
Summary:
This diff adds trace of closure in autoreleasepool checker. We introduce a symbolic trace value for closure variable.
* It is added to the trace when closure variable is called
* and is substituted to concrete one when actual closure is given later.
Reviewed By: ezgicicek
Differential Revision: D25025883
fbshipit-source-id: a6e246be7
Summary: At a function call, an access performed by a callee must be processed in various ways before it's added to the accesses of the caller, and several of these steps may throw away the access. Previously, this was done by effectively doing a bit of transformation, creating a new set of accesses, then folding over that to add to the caller's. This is inefficient and somewhat confusing, as this can be done with one fold and a sequence of `Option.map`s.
Reviewed By: skcho
Differential Revision: D24885117
fbshipit-source-id: 4ab61eab9
Summary:
Existing closure substitution only supported direct block calls to formals.
The following didn't work since the domain was only keeping track of loads/calls from formals, but didn't support stores.
```
void foo(dispatch_block_t block1){
dispatch_block_t local_block = block1;
local_block(); // we don't substitute the call here
}
```
This diff adds support for assigning a block to a local variable so that we can specialize the above example.
We now have a pair domain
- existing mapping from ids to block vars
- a new mapping from mangled to block specializations
the latter allows us to update the mapping in local block assignment (via store).
Reviewed By: skcho
Differential Revision: D25030234
fbshipit-source-id: 3f172341c
Summary:
Specialized closure substitution was broken for conditionals:
```
void foo(dispatch_block_t block1){
if (x){
block1(); // not replaced with specialized implementation
}
}
```
The problem was that when substituting function calls, it only used memory state at the exit node, rather than at each program point.
We could solve this by
- reverting the domain change in D24418560 (c47911359a), i.e. collecting all possible mappings conservatively (e.g. switch the domain back to `Map`)
- pass the `invariant_map` for substitutions at each program point.
We go with the second option here.
The closure substitution is still somewhat broken as exemplified by the following example:
```
void foo(dispatch_block_t block1){
dispatch_block_t local_block = block1;
local_block(); // we don't substitute the call here
}
```
Reviewed By: skcho
Differential Revision: D24993962
fbshipit-source-id: ebadddb58
Summary:
A minor but persistent annoyance: named argument of a non-ambiguous type
of almost the same name. No.
Reviewed By: skcho
Differential Revision: D24991673
fbshipit-source-id: c806f9cec
Summary:
This was left as a TODO before: where to place calls to destructors for
C++ temporaries that are only conditionally creating when evaluating an
expression. This can happen inside the branches of a conditional
operation `b?e:f` or in potentially-short-circuited conditions on the
righ-hand side of `&&` and `||` operators.
Following the compilation scheme of clang (observed by looking at the
generated LLVM bitcode), we instrument the program with "marker"
variables, so that for instance `X x = true?X():y;` becomes (following
the execution on the true branch):
```
marker1 = 0; // initialize all markers to 0
PRUNE(true) // entering true branch
X::X(&temporary); // create temporary...
marker1 = 1; // ...triggers setting its marker to 1
X::X(&x, &temporary); // finish expression
if (marker1) {
X::~X(&temporary); // conditionally destroy the temporary
}
```
In this diff, you'll find code for:
- associating markers to temporaries that need them
- code to initialize markers to 0 before full-expressions
- code to conditionally destroy temporaries based on the values of the
markers once the full-expression has finished evaluating
Reviewed By: da319
Differential Revision: D24954070
fbshipit-source-id: cf15df7f7
Summary:
The translation of `switch` cases needs to insert nodes around the
translation of each `case` sub-statement, so we need to force node
creation in these sub-statements so the nodes around it can be connected
to the translation of the sub-statements.
Also added more logging I found useful when debugging that.
Reviewed By: da319
Differential Revision: D24991455
fbshipit-source-id: d3a622142
Summary:
The current source parser is based on ocamllex only.
In order to track field declaration locations, we propose a
new parser using ocamllex/menhir. This is a more ambitious
project that closely follows the official Java syntax.
Reviewed By: jvillard
Differential Revision: D24858280
fbshipit-source-id: 22d6766e5
Summary:
In all other places, we index params from 0, but accidentally recorded
the wrong number in json. It was because of the confusion between index
and user-visible param position that we show for the user message.
This diff fixes it: now we use 0-based indices internally (but of course still
report 1-based ones in the error message).
Reviewed By: artempyanykh
Differential Revision: D24916878
fbshipit-source-id: 45532c5ff
Summary:
Sometimes there are annotations that don't correspond to the user facing
code.
Previously we would fail, now process them gracefully.
Reviewed By: artempyanykh
Differential Revision: D24890895
fbshipit-source-id: e64a866ec
Summary:
Split the translation of return more aggressively between:
1. the instruction that has to happen before the translation of the sub-expr
2. the sub-expr
3. the instruction that has to happen after the sub-expr
This is needed for the next diff which creates potentially large CFGs in
(2).
Reviewed By: da319
Differential Revision: D24954071
fbshipit-source-id: a7e7e2527
Summary: Model `folly::Optional::get_pointer` which returns an address to a value if exists or `nullptr` if empty.
Reviewed By: jvillard
Differential Revision: D24935677
fbshipit-source-id: 9d990fe07
Summary:
We deliberately stopped as soon as an error was detected when applying a
function call. This is not good as other pre/posts of the function may
apply cleanly, which would allow us to cover more behaviours of the
code.
Went on a bit of a refactoring tangeant while fixing this, to clarify
the `Ok None`/`Ok Some _`/`Error _` datatype returned by PulseInterproc.
Now we report errors as soon as we find them during function calls but
continue accumulating specs afterwards.
Reviewed By: da319
Differential Revision: D24888768
fbshipit-source-id: d5f2c29d7
Summary:
In the next diff we need to know when a destructor is needed for sure
before calling some of these auxiliary methods.
Reviewed By: da319
Differential Revision: D24832078
fbshipit-source-id: 6d4e17de2
Summary:
Communicate new facts from the arithmetic domain to the memory domain to
detect contradictions between the two.
Reviewed By: jberdine
Differential Revision: D24832079
fbshipit-source-id: 2caf8e9af
Summary:
This is several inter-connected changes together to keep the tests
happy.
The ConditionalOperator `b?t:e` is translated by first creating a
placeholder variable to temporarily store the result of the evaluation
in each branch, then the real thing we want to assign to reads that
variable. But, there are situations where that changes the semantics of
the expression, namely when the value created is a struct on the stack
(eg, a C++ temporary). This is because in SIL we cannot assign the
*address* of a program variable, only its contents, so by the time we're
out of the conditional operator we cannot set the struct value
correctly anymore: we can only set its content, which we did, but that
results in a "shifted" struct value that is one dereference away from
where it should be.
So a batch of changes concern `conditionalOperator_trans`:
- instead of systematically creating a temporary for the conditional,
use the `trans_state.var_exp_typ` provided from above if available
when translating `ConditionalOperator`
- don't even set anything if that variable was already initialized by
merely translating the branch expression, eg when it's a constructor
- fix long-standing TODO to propagate these initialization facts
accurately for ConditionalOperator (used by `init_expr_trans` to also
figure out if it should insert a store to the variable being
initialised or not)
The rest of the changes adapt some relevant other constructs to deal
with conditionalOperator properly now that it can set the current
variable itself, instead of storing stuff inside a temp variable. This
change was a problem because some constructs, eg a variable declaration,
will insert nodes that set up the variable before calling its
initialization, and now the initialization happens *before* that setup,
in the translation of the inner conditional operator, which naturally
creates nodes above the current one.
- add a generic helper to force a sequential order between two
translation results, forcing node creation if necessary
- use that in `init_expr_trans` and `cxxNewExpr_trans`
- adjust many places where `var_exp_typ` was incorrectly not reset when translating sub-expressions
The sequentiality business creates more nodes when used, and the
conditionalOperator business uses fewer temporary variables, so the
frontend results change quite a bit.
Note that biabduction tests were invaluable in debugging this. There
could be other constructs to adjust similarly to cxxNewExpr that were
not covered by the tests though.
Added tests in pulse that exercises the previous bug.
Reviewed By: da319
Differential Revision: D24796282
fbshipit-source-id: 0790c8d17
Summary: In cpp, lambda's operator() name includes line and column numbers which were not ignore in proc name when computing bug hash.
Reviewed By: ngorogiannis
Differential Revision: D24890545
fbshipit-source-id: 95e6735f3
Summary:
As per title, plus de-quadratic-ify substitution of actuals into formals.
Also, fix a bug in treatment of callee summaries where the caller lock state was updated first and then used to process accesses in the callee (so should only take into account the original caller state).
Reviewed By: jvillard
Differential Revision: D24857922
fbshipit-source-id: 07ce6999c
Summary:
Otherwise this gets serialized in an unconventional way; `nullable` serializes it as a
standard `null` or string value.
Reviewed By: artempyanykh
Differential Revision: D24827593
fbshipit-source-id: a5a9afc80
Summary: As per title, plus minor improvements in interfaces and a couple of FIXMEs.
Reviewed By: skcho
Differential Revision: D24836125
fbshipit-source-id: f7a4dc196
Summary:
The starvation domain keeps a domain element per distinct pair of lock object and source location. This was used to counteract the imprecision of implicit Quandary-style traces. Starvation has used explicit traces for a long time now, so keeping all these elements is expensive (in fact, in some cases exponential) and of no value. Now, lock object identity is the only distinguishing feature of a domain element.
Also, fix some pretty printing for debugging purposes.
Reviewed By: jvillard
Differential Revision: D24829306
fbshipit-source-id: 22e12f9c1
Summary:
This diff fixes `degree_with_term` to ignore function pointer symbols. `degree_with_term` does
* calculate the degree
* simplify the polynomial only for printing them to users
thus, there is no problem to ignore the function pointer symbols always, ie which does not affect semantics or summary values.
Reviewed By: ezgicicek
Differential Revision: D24596479
fbshipit-source-id: 1e29d2de0
Summary: We recently introduced a more precise model for constructing an optional from a value by making a shallow copy. However, this introduced Use After Delete false positives. For now, we go back to a less precise model by creating a fresh value. A proper model would be to either make a deep copy or call the copy constructor for a value. We will address this in the following diff.
Reviewed By: jvillard
Differential Revision: D24826749
fbshipit-source-id: 3e5e4edeb
Summary: Refactor `folly::Optional` models to make them easier to reuse for `std::optional`
Reviewed By: jvillard
Differential Revision: D24760053
fbshipit-source-id: f665e84c8
Summary:
- log trans_state for each instruction
- create boxes to indent logs
- hunt down "@." that would prematurely close the boxes
- improve messages
Reviewed By: ngorogiannis
Differential Revision: D24794798
fbshipit-source-id: 80d51a8c5
Summary:
They are indirectly encoded in "procedure" field already, but:
1/ To extract name and params one needs to parse the procedure on the
client side
2/ We already have class, package, and field: method and its params is a
consistent change
Reviewed By: artempyanykh
Differential Revision: D24731064
fbshipit-source-id: fae478e7e
Summary:
If the issue one of:
- Field Not Nullable
- Field Not Initialized
- Field Overannotated,
we record field_name to .json result.
NoTE: Design choice for representation. For Field Not Initialized and Field Overannotated
this is always internal (relative to the class) field, but for Field Not
Nullable it can be either internal or external. We could have a
structured output, or always output full name. I preferred to output
short name for convenience of the main usacase I am anticipating.
NOTE: not to be confused with the case where the field is nullable but
we e.g. try to dereference it. This is indirectly related to the issue
(can be several such fields for starters) and if we one day output it,
it will be provided in a separate way (similarly to how we output
nullable_methods).
Reviewed By: artempyanykh
Differential Revision: D24730320
fbshipit-source-id: c995ec221
Summary:
We never tested params dependent on things (tested only things dependend
on params).
Reviewed By: artempyanykh
Differential Revision: D24726858
fbshipit-source-id: a0861cfc3
Summary:
Since we report issues types without a prefix (e.g. ERADICATE_) and
with spaces we should also allow for prefix-less issue types in
SuppressLint, so both should work
- SuppressLint("eradicate-field-not-initialized")
- SuppressLint("Field Not Initialized")
Reviewed By: jvillard
Differential Revision: D24760341
fbshipit-source-id: 1590cf6d0
Summary:
There is a feature in Nullsafe that is interfering with "annotation
graph" feature. Because of this we would not detect provisional
violations for misuses of params of equals() (They will be recorded
as user facing rather than provisional issues).
This diff turns this feature off for annotation graph mode.
Reviewed By: artempyanykh
Differential Revision: D24726655
fbshipit-source-id: 4b7577667
Summary:
Virtual "this" invisible param exists in the annotated signature, but
does not exist in some other places, which causes a lot of annoyance in
different places.
This diff does not intend to solve all this, but makes one step forward.
1/ AnnotatedNullability now explicitly distincts normal params and
VirtualThis.
2/ AnnotatedSignature accounts for this via: a) not having redundant
fake annotation points b) having corrent param indices (those were off
by 1 in non-virtul methods).
Reviewed By: artempyanykh
Differential Revision: D24726480
fbshipit-source-id: fdb8bb0fb
Summary: This is a complex enough feature so iterating on it in a safe manner will be useful.
Reviewed By: artempyanykh
Differential Revision: D24725406
fbshipit-source-id: 81b247143
Summary: `folly::Optional::value()` returns a reference, hence an error was shown when the actual value was being accessed. Since `value()` throws an exception in case of `folly::none`, we want to show the error message at the call site of `value()`. We do this by dereferencing the result of `value()` in the model.
Reviewed By: jvillard
Differential Revision: D24702875
fbshipit-source-id: ca9f30349
Summary:
This is needed for `infer reportdiff` to report issues that are in files
in the same workspace but were captured under a different project root,
but it's also legit to do so in general as "under project root" really
means "this file belongs to the project under analysis".
Reviewed By: martintrojer
Differential Revision: D24755592
fbshipit-source-id: fa8fab127
Summary: In cpp, lambda's `operator()` name includes line and column numbers which were not ignore when computing bug hash.
Reviewed By: jberdine
Differential Revision: D24649125
fbshipit-source-id: 7a235fd3e
Summary:
The problem in Reporting.ml:log_issue_from_summary is that it merely
checks the presence of `SuppressLint` annotation on method's body to
decide whether to log or not the issue. This means that regardless of
issue types specified in `SuppressLint`, all issues on such method will
get blocked.
Here we fix that.
Reviewed By: ngorogiannis, mityal
Differential Revision: D24726604
fbshipit-source-id: c9cae3833
Summary:
This diff glues the previous work together.
The ClassLevelAnalysis finds list of provisional violation, builds the
graph based on them, and outputs this graph as a separate issue.
Reviewed By: artempyanykh
Differential Revision: D24682802
fbshipit-source-id: 8174da91a
Summary:
This change is what makes the annotation graph a _graph_.
Now we can detect places when adding one annotation causes an issue that can be
fixed by adding another annotation.
Reviewed By: artempyanykh
Differential Revision: D24650979
fbshipit-source-id: b8452b822
Summary:
In the previous diff we introduced
AnnotatedNullability.is_annotatable_as_nullable method.
Lets now use it in all places where we issue ProvisionallyNullable
annotation.
This will lead to more precise nullability graph (without attempts to
annotate primitives).
Reviewed By: artempyanykh
Differential Revision: D24650951
fbshipit-source-id: 8ea0bb97d
Summary:
This change is on par with the logic that we already have for methods
and method params.
In this logic, we explicitly distinct class under analysis & the
external class when fetching AnnotatedNullability (we needed this to support
various Nullsafe modes as well).
Here we use the same approach for fields.
NOTE: this is not intended to work with nested classes just yet.
Reviewed By: artempyanykh
Differential Revision: D24650934
fbshipit-source-id: f555bcc3f
Summary:
:
New flag (equivalent to --clang-biniou-file) to pass in AST to
frontend.
Passing in json files has the big advantage of emitting line numbers
on frontend AST errors.
Reviewed By: jvillard
Differential Revision: D23814358
fbshipit-source-id: 0ad0452ff
Summary: Cleanup `Typ` by moving all constant types to `StdTyp`. Also remove `Typ.typ` as it's just `Typ.t` now.
Reviewed By: jberdine
Differential Revision: D24620397
fbshipit-source-id: 4764f87ef
Summary:
Now we have all pieces of information needed to build the annotation
graph (to be finished as a follow up).
This diff utilizes the same approach that is done when we calculate node
promotions:
1/ AssignmentRule and DereferenceRule know that some issues are related
to provisional annotations and expose this information.
2/ ClassLevelAnalysis collects all the issues, extract violations from
them, and outputs them.
Next step will be changing the output to the meta-issue printing the
annotation graph in json.
Reviewed By: artempyanykh
Differential Revision: D24621410
fbshipit-source-id: c240da3aa
Summary:
In the previous diffs, we introduced
AnnotatedNullability.ProvisionallyNullable. This is a symmetric change
introducing the same for Nullability.t
Note how ProvisionallyNullable is made `is_nonnullish`. This means that
if we run nullsafe in --nullsafe-annotation-graph mode, the following
should start happening:
1/ A lot of new issues will be recorded - every time a local method or
non-null param is dereferenced etc.
2/ But all of those new issues will be filtered by corresponding Rules
(e.g. AssignmentRule). So those issues will not manifest to user-visible
errors. However, now we are capable to see and analyse those hidden
issues in ClassLevelAnalysis. This is exactly what we will be doing in
follow up diffs.
Note that the place of ProvisionallyNullable in the hierarchy is
somewhat arbitraily. It is important to make it less pri then Nullable -
but when it comes to comparing with other "nonnullish" modes - we can
tweak the behavior later on.
Reviewed By: artempyanykh
Differential Revision: D24621411
fbshipit-source-id: 3b99e5e55
Summary:
The fact that we did not preserve all joins (and used only the first
one) was a shorcut since the origin is used only for reporting, and
reporting the info about left joinee is enough for practical purposes.
Hovewer, to support annotation graph, we need all joinees.
This diff is a no-op since we use only the first one currently.
Reviewed By: artempyanykh
Differential Revision: D24621412
fbshipit-source-id: 8fe1174e7
Summary:
This was perhaps needed at some point, but now there is no need in this.
See the next diff that refactors InferredNullability so we store all
joinee results there.
Reviewed By: artempyanykh
Differential Revision: D24621413
fbshipit-source-id: d6f406b87
Summary:
This is the basic building blocks for the future annotation graph. This
diff introduces the first abstraction to be iterated on.
Reviewed By: artempyanykh
Differential Revision: D24621414
fbshipit-source-id: 19acdf216
Summary:
Before we were creating a fresh internal value when we were constructing `folly::Optional`. This diff models `folly::Optional` constructor more precisely by copying the given value.
There was also a missing dereference in the model of `value_or`
Reviewed By: jvillard
Differential Revision: D24621016
fbshipit-source-id: c86d3c157
Summary:
The global type environment is loaded in all analysis workers, so it is best to minimise its memory footprint. `MaxSharing` does a great job at this but
- it is very slow
- it may be involved in observed segfaults, since it uses potentially unsafe operations
This diff replaces `MaxSharing` with a post-facto poor-man's hashcons pass through the parts involved in a java type environment. It is not as efficient as `MaxSharing` at reducing footprint, but it's much faster.
Reviewed By: jberdine
Differential Revision: D24590400
fbshipit-source-id: c37100325
Summary:
Namely `enumerateObjectsUsingBlock` which takes a collection and a block as an arg and the iterates over the collection and applies the block to each element of the collection.
This is a common way to iterate over Objc collections, so let's add a model for it.
Reviewed By: skcho
Differential Revision: D24604590
fbshipit-source-id: 1ceeb4b40
Summary:
The autorelease pool size checker increases the size when an non-arc-compiled function calls
arc-compiled function. However, if the callee is declared in arc-compiled code and its body is not
actually captured or compiled, we should not increase the autorelease pool size.
Reviewed By: ezgicicek
Differential Revision: D24569661
fbshipit-source-id: 2fd707096
Summary:
This diff revises the translation of message expression's arguments in ObjC frontend. In the
frontend, it massages the arguments when calling a static method, so the class or object value is
not given to the static method as the first parameter.
The problem is that it used a raise-exception-and-catch way to detect where we remove the first
parameter. This way of using an exception is not only hard to understand, but also incorrectly
removed the first parameter, with breaking abstract semantics sometimes. (See the added test.) This diff
avoids using the exception.
Reviewed By: jvillard
Differential Revision: D24565513
fbshipit-source-id: 0a84ca394
Summary:
The `make clean` did not remove objects and dot files, so
```
infer/tests/codetoanalyze/objc/frontend$ make test
infer/tests/codetoanalyze/objc/frontend$ make clean
infer/tests/codetoanalyze/objc/frontend$ make test
```
the second `make test` did nothing. This diff adds additional regular
expressions to clean all objects and dot files generated.
Reviewed By: ngorogiannis
Differential Revision: D24566169
fbshipit-source-id: b8c50c922
Summary: This diff fixes on-demand symbolic value generation of a class that inherits NSEnumerator.
Reviewed By: ngorogiannis
Differential Revision: D24504955
fbshipit-source-id: bcb20e8aa
Summary:
This diff replaces overridden method calls in ObjC when possible, ie the first parameter of the
method has a sub-class type of the method's class. For example,
when `MyEnumerator` is a sub-class of `NSEnumerator` and there is overridden `nextObject`,
```
[my_enumerator nextObject]
```
in Sil, it was translated to like
```
NSEnumerator.nextObject(my_enumerator : MyEnumerator*)
```
and the analyzer missed the overridden method. This diff replaces the function call to
```
MyEnumerator.nextObject(my_enumerator : MyEnumerator*)
```
Reviewed By: ezgicicek
Differential Revision: D24477290
fbshipit-source-id: 6842a76f8
Summary: Model `folly::Optional::value_or(default)` to return value if not-empty and `default` if empty.
Reviewed By: jvillard
Differential Revision: D24539456
fbshipit-source-id: cc9e176cc
Summary: Can be useful, especially to dump all the summaries as json.
Reviewed By: skcho
Differential Revision: D24504253
fbshipit-source-id: 845e7d657
Summary:
Emit the crucial parts of Pulse summaries as json to enable
post-processing by external tools. Stop somewhat arbitrarily at some
datatypes that are just emitted as "opaque" values.
For example:
```
$ infer debug --procedures --procedures-summary-json --select 0
[[["pulse",[["ContinueProgram",{"post":{"heap":[["v3",[[["Dereference"],["v4","_"]]]],["v7",[[["Dereference"],["v3","_"]]]]],"stack":[[["ProgramVar",{"plain":"return","mangled":null}],["v7","_"]]],"attrs":"_"},"pre":{"heap":[],"stack":[],"attrs":"_"},"skipped_calls":"_","path_condition":"_"}],["ContinueProgram",{"post":{"heap":[["v3",[[["Dereference"],["v4","_"]]]],["v8",[[["Dereference"],["v3","_"]]]]],"stack":[[["ProgramVar",{"plain":"return","mangled":null}],["v8","_"]]],"attrs":"_"},"pre":{"heap":[],"stack":[],"attrs":"_"},"skipped_calls":"_","path_condition":"_"}]]]]]
```
Reviewed By: ezgicicek
Differential Revision: D24503387
fbshipit-source-id: 9bd08e93b
Summary:
Output summaries in json format, so that other tools can exploit the
results of infer without having to be written inside infer itself.
For now the json for a summary is just one line saying "opaque" :)
Set up the infra to generate (yo)json automatically using
ppx_yojson_conv. See it in action in the next diff.
Reviewed By: ezgicicek
Differential Revision: D24503343
fbshipit-source-id: e24a2fff3
Summary:
- output the "menu" of the interactive mode on stderr instead of stdout
so that we can pipe the results, eg
`infer debug --procedures --procedures-summary | cat`
This will be more useful when we add an option to output json, as
otherwise the menu pollutes the json.
- Allow "--select" to work for infer-debug too:
`infer debug --procedures --procedures-summary --select 0`
Reviewed By: da319
Differential Revision: D24503301
fbshipit-source-id: d7fb4b713
Summary:
This diff revises `nextObject` model to handle multiple symbolic enumerators. Instead joining the
symbolic offsets of them, which sometimes introduces top, it sums the offsets. This is a sound &
conservative semantics since they are all non-negative integers.
Reviewed By: ezgicicek
Differential Revision: D24474513
fbshipit-source-id: 6707aa907
Summary:
This diff revises memory model of enumerator in ObjC to enable passing it as a parameter.
The cost checker was not able to analyze a function precisely when it gets an enumerator as a
parameter because the offset of an enumerator was available only when the analyzer knew the correct
relation between the enumerator and an array.
This diff simplifies the enumerator to have a similar value with `array->elements`, so its offset can
be taken without the relation between enumerator and array to get them.
Reviewed By: ezgicicek
Differential Revision: D24446574
fbshipit-source-id: 27cdc051e
Jules Villard
Nikos Gorogiannis
Sungkeun Cho
Daiva Naudziuniene
Ezgi Çiçek
Joshua Selbo
Radu Grigore
Mitya Lyubarskiy
David Pichardie
Joscha Benz
Martin Trojer
Artem Pianykh
Josh Berdine