Summary:
Races in Nullsafe classes can undermine NPE safety despite the class passing the type checks.
This diff adds to the report text of THREAD_SAFETY_VIOLATION and GUARDEDBY_VIOLATION the following trailer:
> Data races in `Nullsafe` classes may still cause NPEs.
This only happens if the race is directly on a non-primitively-typed member field of the class.
It also uses distinct bug types (adds the suffix _NULLSAFE to the bug types above) for easier accounting.
Reviewed By: ezgicicek
Differential Revision: D26403274
fbshipit-source-id: 3cd6ca082
Summary:
We are getting lots of FPs due to modeling `Provider.get` as expensive. This is coming from Dependency Injection and Infer cannot statically determine the type of the provider and determine whether that provider is expensive (requires a global analysis and instrumentation).
Instead, we are downgrading this method to the default constant cost.
Reviewed By: skcho
Differential Revision: D26223978
fbshipit-source-id: 79f81c997
Summary:
Dear Infer team,
To contribute to Infer community, I would like to integrate infer#'s language agnostic layer into Infer.
Please help to review, discuss and consider to merge this feature.
Thanks,
Xiaoyu
Pull Request resolved: https://github.com/facebook/infer/pull/1361
Reviewed By: skcho
Differential Revision: D25928458
Pulled By: jvillard
fbshipit-source-id: 7726150b8
Summary:
**Existing heuristic**: If we have a call `foo(n)` that has no model and summary for `foo`, we underestimate its cost as constant[1].
However, if we have a model for `foo` (e.g.with modeled cost O(n)) but applying the model to arguments causes the cost to be Top (e.g if `n` has Top size), then we could have Top-poisoning where all the callers up the call chain will have Top costs [2].
To prevent these unintended Top-poisioning when adding models, this diff applies *the same heuristic* to modeled calls with Top cost and gives them constant cost. This way, when adding models, we wouldn't be introducing more Tops than if we were to have no models in the first place.
[1] This is problematic in itself and causes many FPs at diff time, but otherwise we would be getting Tops everywhere and would not be able to give any meaningful cost. E.g. for fblite, if we were to give unknown calls Top cost, #procedures with Top cost increases form 5% to 38% and #procedures with linear cost reduces by 99.75%.
[2] This was observed for `containsValue` for Instagram where %Tops increased by 88% :(
Reviewed By: skcho
Differential Revision: D26174644
fbshipit-source-id: 232354923
Summary: This diff adds an additional parameter of struct return type in ObjC's methods. The additional parameter had been supported only in C/C++ functions/methods for 5 years (D2865091 (ec80d40bdd)). If there is no specific reason not to do that, let's do it and fix the incorrect frontend translations.
Reviewed By: jvillard
Differential Revision: D26049748
fbshipit-source-id: 414b3011f
Summary:
- We hoist calculation of `loop_head_to_loop_nodes` to simplify `get_loop_control_map` and also to allow it to be used by inefficient keyset iterator without needing to compute exit maps unnecessarily.
- nit on comments
- `open Control` in `loop_control.ml`
- hoist bound map calculation in `cost.ml`
Reviewed By: ngorogiannis
Differential Revision: D25952592
fbshipit-source-id: ef6103497
Summary:
Developers complain when a function that used to only throw an exception has complexity increase in the updated revision. Let's suppress such issues by giving those functions 0 cost which is already suppressed by differential reporting.
One common case to the above throw pattern is Java methods that throw an unsupported implementation exception for a functionality that has not been implemented yet. When the developer adds the supported implementation, we don't want to warn them with complexity increase since they are adding new functionality.
This is a design choice/heuristic to prevent noisy results for now.
Reviewed By: skcho
Differential Revision: D25495151
fbshipit-source-id: 94a82b062
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:
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:
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 adds closure symbols to operation/allocation costs, when function pointer is called.
Reviewed By: ezgicicek
Differential Revision: D24308550
fbshipit-source-id: 6c5889d41
Summary:
This diff extended the polynomial domain to include symbols for closure calls.
When the closure symbol is added to the polynomial? Unknown closure is called inside a function
like,
```
foo() {
self->closure_field();
}
```
Thus, the cost of `foo` becomes `|self->flosure_field|`, rather than unknown. (Note that this
semantics is added only for autoreleasepool size at the moment.)
When the symbol is instantiated? `foo` is called with correct closure contexts.
```
goo() {
self->closure_field = ^(){ ... };
foo();
}
```
The summary of `goo` will have instantiated summary of the closure.
Reviewed By: ezgicicek
Differential Revision: D23992590
fbshipit-source-id: d1d228403
Summary:
This diff is a preparation to extend the polynomial domain. What I will do in the following diff is
to extend the polynomial domain to include a symbol representing "cost of function pointer call".
Then, the symbol will be instantiated to another polynomial cost at call sites.
The polynomial domain is defined as a sum of
* non negative insteger
* map from `bound` to polynomail domain
```
type poly = {const: NonNegativeInt.t; terms: poly M.t}
```
The problem is all `bound`s are adderessed as integer values, rather than polynomial, thus it
does not fit to include the symbol for the "cost of function pointer call".
In this diff, it introduces a `Key` module for the map `M`, the type of `Key.t` is the same to the previous key of `M`. The actual extension of the key type will be in the following diff.
Reviewed By: ezgicicek
Differential Revision: D23991401
fbshipit-source-id: cec64347f
Summary: Dispatch & function call mechanism was so jumbled up together. Let's refactor it to be cleaner.
Reviewed By: skcho
Differential Revision: D24049889
fbshipit-source-id: 42a218016
Summary: We add a naive model for `forEach` idiom for Java's Iterable and Maps. The model is naive because it doesn't take the cost of the lambda into account. This will be fixed later.
Reviewed By: da319
Differential Revision: D23868203
fbshipit-source-id: 37d169c6f
Summary:
This diff keeps closure parameters in closure-specializated procedures.
What the closure-specialization is doing is a propagation of concrete closures. For example, it
translates:
```
foo(block b) {
b();
}
goo() {
foo(^{...});
}
```
to
```
foo_new() {
(^{...})();
}
goo() {
foo_new();
}
```
However, if `foo` addresses `b` as a normal value like
```
foo(block b) {
block c = b;
}
```
this is translated to
```
foo_new() {
block c = b;
}
```
Note that the closure parameter of `foo` is removed, thus `b` becomes a free variable. Not good.
To avoid the situation, this diff keeps the closure parameters intact.
Reviewed By: da319
Differential Revision: D23905580
fbshipit-source-id: 014989fbf
Summary:
This diff add a model of `NSArray.index_of_object_passing_test` in
autoreleasepool size model.
Worst case autorelease cost is "array size x cost of given test function (lambda)".
Reviewed By: ezgicicek
Differential Revision: D23785165
fbshipit-source-id: 95ec9700a
Summary:
This diff simply changes the type of autorelease size models as `CostUtils.model` instead of
`unit`. In the following diff, it will add a model that has more complicated semantics that does not return the unit cost.
Reviewed By: ezgicicek
Differential Revision: D23785159
fbshipit-source-id: 300808574
Summary: Also, fix some leftover meaningless logging, enable all cost issue types on diff testing so that we can see the unreachable cost issues
Reviewed By: skcho
Differential Revision: D23784563
fbshipit-source-id: 1b4a2b582
Summary: ObjC objects can be added to autorelease pool by `CFAutorelease`.
Reviewed By: ezgicicek
Differential Revision: D23625632
fbshipit-source-id: 694e5bffb
Summary:
This diff regards some methods of NSKeyedUnarchiver as autorelease, because they may call
autorelease.
Reviewed By: ezgicicek
Differential Revision: D23600232
fbshipit-source-id: a9fc1cc56
Summary:
This diff increases autoreleasepool size when
* caller is non-ARC-compiled
* callee is ARC-compiled
* return type is a pointer to objc object
To distinguish non-ARC-/ARC-compiled:
* extended `translation_unit_decl_info` to have a boolean field `is_objc_arc_on`
* then copied it to `ProcAttributes.t` for each procedures.
Reviewed By: ezgicicek
Differential Revision: D23565003
fbshipit-source-id: dee22ea82
Summary: This diff adds trace field for autoreleasepool size. Unlike to the other checkers, eg inferbo and operation cost, the autoreleasepool size checker should have traces for constants.
Reviewed By: ezgicicek
Differential Revision: D23678084
fbshipit-source-id: 35e6cf5f5
Summary:
We defined cost's plus as "zero+unreachable = unreachable" for the operation cost. The meaning of
the zero cost is that no statment is analyzed yet, and the unreachable(bottom) cost means a program
point is analyzed as unreachable. We thought "zero+unreachable" happens in very specific cases we
need to check, or due to an analyzer bug. For debugging purpose, we defined it to return more
specific unreachable cost.
However, in allocation/autoreleasepool costs, the zero cost is not very special value. If there is
no alloc/autorelease calls, they can have the zero cost. "zero+unreachable = unreachable" doesn't
make sense there.
This diff changes the plus as "zero+unreachable = zero" for the non-operation costs.
Reviewed By: ezgicicek
Differential Revision: D23578869
fbshipit-source-id: 5392eca1c
Summary:
This diff adds a new experimental checker for detecting size of objects in autorelease pool in ObjC. The basic mechanism is almost the same with the previous cost calculation:
* Autorelease pool size is increased at explicit `autorelease` call
* Autorelease pool size is set as zero by the `autoreleasepool` block.
While it only supports the explicit calls as of now, we will extend the checker to handle more cases in the following diffs.
Reviewed By: ezgicicek
Differential Revision: D23473145
fbshipit-source-id: 416488176
Summary: Most of the time, when the procdesc of a callee is requested, all that is really required is the procedure attributes. However, requesting the procdesc may return `None` when the procedure is undefined (in Java, and soon for Clang too). So, change all callsites to using attributes instead, where possible.
Reviewed By: jvillard
Differential Revision: D23539422
fbshipit-source-id: 3b1a52d48
Summary: We don't report and IO Cost and are not planning to do so in the near future. Let's remove it from cost kinds.
Reviewed By: ngorogiannis
Differential Revision: D23499160
fbshipit-source-id: 7281da3af
Summary:
When implementing iterator, we find out that because some semantics of inferbo is Java-specific, we cannot simply use Java's `Collection` model for `NSCollection`.
So this diff writing `NSCollection` model separately.
This diff also extracts the common parts of `NSCollection` and `Collection` into `AbstractCollection` to combine the duplicate the parts.
Reviewed By: ezgicicek
Differential Revision: D22975159
fbshipit-source-id: daed3f99f
Summary:
This is needed to make dune auto-updating of unit tests introduced in
the next diff cohabit peacefully with our tests to make sure code stays
correctly formatted wrt ocamlformat.
Also, more auto-formatting = better.
Reviewed By: da319
Differential Revision: D22865004
fbshipit-source-id: 91c47ab08
Summary:
Pausing the experiment in favour of new PulseFormula. Can be resurrected
later.
Reviewed By: skcho
Differential Revision: D22576274
fbshipit-source-id: 76529d767
Summary:
In Cost/Inferbo checkers, it tried to find a subclass with heuristics when a method of interface or
abstract class is called. While it makes preciser analysis results in general, sometimes introduced
tricky FPs since the behavior of the heuristics depends on targets. This diff revert the heuristics
to suppress the FPs.
Reviewed By: ngorogiannis
Differential Revision: D22924485
fbshipit-source-id: 9f151231f
Summary:
We check for supertypes in Java. Why not ObjC?
Would be good to get dulmarod's input here.
Reviewed By: roro47
Differential Revision: D22817126
fbshipit-source-id: 52c1c3f3c
Summary: This diff refactors Java specific `PatternMatch` functions into its own module. When `PatternMatch.ml` was originally created, it was mainly for Java but now it also supports ObjC. Let's refactor it to reflect the Java/ObjC separation: move all functions that operate on Java procnames into Java submodule.
Reviewed By: jvillard
Differential Revision: D22816504
fbshipit-source-id: ff6b64b29
Summary: Add cost model for most common `NSString` functions in cost analysis
Reviewed By: skcho
Differential Revision: D22433005
fbshipit-source-id: 2f57bbda9
Summary: This diff prints where the cost becomes top by calling `html_debug_new_node_session`. This will print them in the start node of the procedure in html. There are already printing functions in `get_instr_node_cost_record`.
Reviewed By: ezgicicek
Differential Revision: D22547578
fbshipit-source-id: 257e957c0
Nikos Gorogiannis
Ezgi Çiçek
Xiaoyu Liu
Sungkeun Cho
Qianyi Shu
Jules Villard