Summary:
`to_reportable_violation` is responsible for identifying if the
violation is reportable in this mode or not.
This logic is higly coupled with other functions in
`ReportableViolation`, such as `get_description`: You can not have
sensible description on the violation that is NOT reportable in a given
mode.
So from logical perpsective, creation of `ReportableViolation.t` should
belong to this module itself, not to the parent `Rule` module.
This change will make design clearer.
Reviewed By: artempyanykh
Differential Revision: D20511756
fbshipit-source-id: ef27b5057
Summary:
This diff finishes work in D20491716.
We removed dependency on nullsafe mode for field initialization in
D20491716, so this diff just formalizes it.
Reviewed By: jvillard
Differential Revision: D20493164
fbshipit-source-id: 6ac612e78
Summary:
This diff continues work in D20491716.
This time for Inheritance Rule.
Reviewed By: jvillard
Differential Revision: D20492889
fbshipit-source-id: c4dfd95c3
Summary:
This diff continues work in D20491716.
This time for Dereference Rule.
Reviewed By: jvillard
Differential Revision: D20492296
fbshipit-source-id: ff7f824f9
Summary:
# Problem
In current design, Rules (assignment rule, dereference rule, inheritance
rule) decide, depending on the mode, wether the issue is legit or not.
If the issue is not actionable for the given mode, it won't be created
and registered.
For meta-issues, we want to be able to do smart things like:
- Identify if we can raise strictness of the mode without
introducing new issues
- Classify classes on "clean" vs "broken", taking into account issues
that are currently invisible.
# Solution
In the new design:
1. Rules are issuing violations independently of mode. This makes sense
semantically. Mode is "level of trust we have for suspicious things",
but the thing does not cease to be suspicious in any mode.
2. Each Rule decides if it is reportable or not in a given mode.
3. `nullsafe_mode` is passed to the function `register_error`, that 1)
adds error so it can be recorded in summary for file-level analysis
phase 2) reports some of them to the user.
# This diff
This diff converts only AssignmentRule, follow up will include
conversion of other rules, so no issue encapsutes the mode.
Reviewed By: jvillard
Differential Revision: D20491716
fbshipit-source-id: af17dd66d
Summary:
`make deadcode` is failing on master but our CI jobs didn't catch it :(
Let's fix existing deadcode for now.
Reviewed By: martintrojer
Differential Revision: D20510062
fbshipit-source-id: 4a5e5f849
Summary:
Previously, at each function call, we added a `WrittenTo` attribute for applying the address of the actuals. However, this results in mistakenly considering each function application that inspects its argument as impure. Instead, we should only propagate `WrittenTo` if the actuals have already `WrittenTo` attributes.
For instance, for the following functions
```
public static boolean is_null(Byte a) {
return a == null;
}
public static boolean call_is_null(Byte a) {
return is_null(a);
}
```
We used to get the following pulse summary for `call_is_null` (showing only one of the disjuncts):
```
#0: PRE:
{ roots={ &a=v1 };
mem ={ v1 -> { * -> v2 } };
attrs={ v1 -> { MustBeValid },
v2 -> { Arith =null, BoItv ([max(0, v2), min(0, v2)]) } };}
POST:
{ roots={ &a=v1, &return=v8 };
mem ={ v1 -> { * -> v2 }, v8 -> { * -> v4 } };
attrs={ v2 -> { Arith =null,
BoItv ([max(0, v2), min(0, v2)]),
WrittenTo-----------WRONG },
v4 -> { Arith =1,
BoItv (1),
Invalid ConstantDereference(is the constant 1),
WrittenTo-----------WRONG },
v8 -> { WrittenTo } };}
SKIPPED_CALLS: { }
```
where we mistakenly recorded a `WrittenTo` for `v2` (what `a` points to). As a result, we considered `call_is_null` as impure :( This diff fixes that since the callee `is_null` doesn't have any `WrittenTo` attributes for its parameter `a`. So, we don't propagate `WrittenTo` and get the following summary
```
#0: PRE:
{ roots={ &a=v1 };
mem ={ v1 -> { * -> v2 } };
attrs={ v1 -> { MustBeValid },
v2 -> { Arith =null, BoItv ([max(0, v2), min(0, v2)]) } };}
POST:
{ roots={ &a=v1, &return=v8 };
mem ={ v1 -> { * -> v2 }, v8 -> { * -> v4 } };
attrs={ v2 -> { Arith =null, BoItv ([max(0, v2), min(0, v2)]) },
v4 -> { Arith =1,
BoItv (1),
Invalid ConstantDereference(is the constant 1) },
v8 -> { WrittenTo } };}
SKIPPED_CALLS: { }
```
Reviewed By: skcho
Differential Revision: D20490102
fbshipit-source-id: 253d8ef64
Summary:
Make all arguments named and move function from `Procname.Java` to `Procname`, and making it return a `Procname.t` as opposed to `Procname.Java.t` (all callers want a `Procname` eventually).
Various other small fixes in the callers.
Reviewed By: skcho
Differential Revision: D20492305
fbshipit-source-id: e646cc799
Summary: These tests fail when seemingly unrelated changes are made to infer. In particular, it seems timeout limits have to be increased by 10x or more to make them succeed again. Disabling until we have a more stable replacement.
Reviewed By: ezgicicek
Differential Revision: D20489647
fbshipit-source-id: 9706b0807
Summary:
This diff naively models the following as `StdVector.push_back`:
- `StringBuilder.append`
- `String.replace`
- `Queue.poll`
It also adds a FN test for `Iterator.next`.
Reviewed By: skcho
Differential Revision: D20469786
fbshipit-source-id: 2d8e8d117
Summary:
This diff is doing three things:
1. Finishes work paved in D20115024, and applies it to nullsafe. In that diff, we hardened API for
file level analysis. Here we use this API in nullsafe, so now we can
analyze things on file-level, not only in proc-level like it was before!
2. Introduces a class-level analysis. For Nullsafe purposes, file is not
an interesting granularity, but we want to analyze a lot of things on
file level. Interesting part here is anonymous classes and how we link
them to their corresponding user-defined classes.
3. Introduces a first (yet to be improved) implementation of class-level
analysis. Namely it is "meta-issues" that tell what is going with class
on high level. For now these are two primitive issues, and we will
refine them in follow up diffs. They are disabled by default.
Follow ups include:
1. Refining semantics of meta-issues.
2. Adding other issues that we could not analyze before or analyzed not
user friendly. Most importantly, we will use it to improve reporting for
FIELD NOT INITIALIZED, which is not very user friendly exactly because
of lack of class-level aggregation.
Reviewed By: artempyanykh
Differential Revision: D20417841
fbshipit-source-id: 59ba7d2e3
Summary: The `FN_loop2` was not actually FN because infer analyzes its complexity as degree 1 correctly.
Reviewed By: dulmarod
Differential Revision: D20468367
fbshipit-source-id: 9e4c19415
Summary: The `iterate_over_mycollection_quad_FN` was not actually FN because infer analyzes its complexity as degree 2 correctly. So, this diff removed `_FN` from there.
Reviewed By: ezgicicek
Differential Revision: D20467398
fbshipit-source-id: b10340612
Summary: There has never been a sufficient formal basis for soundness nor completeness of reports on locals. This diff changes the domain to effectively concern only expressions rooted at formals or globals.
Reviewed By: ezgicicek
Differential Revision: D19769201
fbshipit-source-id: 36ae04d8c
Summary: `Object.clone` modeled as pure until the analysis can distinguish returning a fresh object vs. having no side-effects.
Reviewed By: skcho
Differential Revision: D20439998
fbshipit-source-id: 421054cfb
Summary:
As ngorogiannis pointed out, we never expect whitespaces in classname, so
stripping makes no sense here in best case, and hides a bug under rug in
worst case.
Reviewed By: jvillard
Differential Revision: D20417033
fbshipit-source-id: bc7449171
Summary: Let's also print skipped calls in `pp` to ease debugging both for summary and intermediate steps.
Reviewed By: jvillard
Differential Revision: D20417852
fbshipit-source-id: 7da03ae81
Summary:
There is a module and a module type in the file PulseAbductiveDomain.ml
with the same name. This is confusing and it's better to keep separate names.
Reviewed By: jvillard
Differential Revision: D20388769
fbshipit-source-id: bcfed436e
Summary:
Be a bit more careful about the difference between PrePost.t and
AbductiveDomain.t. It's needed in another diff where the types will be
different.
Reviewed By: ezgicicek
Differential Revision: D20393927
fbshipit-source-id: beaf80c90
Summary: In preparation for PulseArithmetic to be something else.
Reviewed By: ezgicicek
Differential Revision: D20393928
fbshipit-source-id: d93131e12
Summary:
`JavaSplitName` is used to represent Java types (in `Procname` in particular). The type itself is a pair of string (an optional package qualifier) and a "type name" (the quotes are there because it may contain array qualifiers).
For example `java.lang.Object[][]` should be represented as
```
{package=Some "java.lang"; typename="Object[][]"}
```
The constructor `make` was misused to construct instead types such as
```
{package=None; typename="java.lang.Object[][]"}`
```
This is evident when we print the return type of a `Procname` non-verbosely (the default), but we still see the package qualifier.
Obviously this is not just a pretty-printing bug, the values were themselves wrong.
The fix is to use the `of_string` constructor which will parse the package and separate it correctly. Another bug (in response to this one) had to be fixed in `Procname.is_vararg` to maintain behaviour in Nullsafe and Quandary.
Reviewed By: mityal
Differential Revision: D20394146
fbshipit-source-id: 4633902eb
Summary:
We will use it in follow up diffs.
From many perspectives, if the function belongs to an anonymous class,
it is useful to know the original user-defined class.
This function makes this distinction clear.
Thanks to ngorogiannis, whos work on refactoring `Typ.name` made this module
easy enough so we can introduce unit tests!
Reviewed By: ngorogiannis
Differential Revision: D20389311
fbshipit-source-id: 408d95660
Summary: `Procname.Java.get_return_typ` is buggy because whenever faced with an array of objects, it returns a type that implies the object is stored by value in the array (this is correct behaviour only when the element type is primitive, not when it's an object type).
Reviewed By: ezgicicek
Differential Revision: D20384403
fbshipit-source-id: d91322d3a
Summary:
We try to consolidate Java-specific stuff in JavaClassName.
Let's introduce the function in JavaClassName and make it clear that
its analog in Typ.Name.Java one throws if called on a wrong type.
Reviewed By: ngorogiannis
Differential Revision: D20386357
fbshipit-source-id: a1577ef8b
Summary:
Impurity domain was tracking all changes to variables (with a list of traces that containing all write/invalid accesses). This results in having long traces with multiple access events for the same variable. For instance,
```
void swap_impure(int[] array, int i, int j) {
int tmp = array[i];
array[i] = array[j]; \\ included in the trace
array[j] = tmp; \\ included in the trace
}
```
here we recorded both array accesses.
This diff changes the domain to include accesses so that we only keep track of a single trace per access. Array accesses are only recorded once.
Note that we want to record all unique accesses, not just the first one, because impurity will be used for hoisting/cost where we will invalidate impure arguments and consider all the rest as not changing.
Reviewed By: jvillard
Differential Revision: D20385745
fbshipit-source-id: d3647dad3
Summary:
D20362149 missed
- to pass the optional argument `include_value_history` to the recursive call in `PulseTrace.add_to_errlog`.
- to set `include_value_history=false` for skipped calls.
This diff fixes these issues.
Reviewed By: skcho
Differential Revision: D20385604
fbshipit-source-id: 176e4d010
Summary: No need to load the contents of the whole file(s) as a string first.
Reviewed By: ngorogiannis
Differential Revision: D20362602
fbshipit-source-id: 46fbc3693
Summary:
Next step in moving logic from make to dune. Since dune understands
build rules with multiple targets we don't need to introduce
artificial pipelining as in make. Rules are pretty straightforward,
albeit somewhat verbose.
The tricky part here was adjusting deadcode detection.
Reviewed By: jvillard
Differential Revision: D20322605
fbshipit-source-id: 688e5f96f
Summary:
Main changes are:
1. Dune can promote targets into the source tree as a part of build. This
allows us to **remove custom promotion/installation logic in src/Makefile**.
2. Dune promotion only works for path within workspace. This required
**moving dune-workspace one folder up**: from infer/infer/src to infer/infer.
But this is not bad, since it makes it possible to migrate tests under dune at some point.
3. `checkCopyright` now also promoted into `infer/infer/bin` instead of
`infer/scripts` partly for consistency and partly because of the
dune-workspace location.
4. `byte` mode was replaced with `byte_complete`. The latter takes
similar amount of time to build compared to `byte`, but produces
standalone binaries that don't require InferCStubs to be
installed. This allowed to remove `dune_exec_shim` and custom logic
around `dune build InferCStubs.install` when dealing with byte
targets.
All in all, `infer/src/Makefile` is not about 2/3 its previous size
with less custom logic in Makefiles/scripts and more encoded in dune
build files.
Reviewed By: jvillard
Differential Revision: D20303902
fbshipit-source-id: 9e4c65bd0
Summary:
With the introduction of environments and profiles we no longer need
to generate most dune files in libraries via make. Also, those dune
builds don't need to be in OCaml.
In addition to converting build files to plain sexp definitions, this
patch also:
- Adjusts copyrightCheck to work correctly with sexp-based dune files.
- Adds auto-formatting for sexp-based dune files.
Reviewed By: jvillard
Differential Revision: D20250208
fbshipit-source-id: 495aeaa99
Summary: With plain/sexp dune files we need to support lisp style comments in checkCopyright
Reviewed By: jvillard
Differential Revision: D20366314
fbshipit-source-id: 04db9e3c1
Summary:
With profiles and `(env ...)` stanza it's possible to consolidate
various ocamlc/ocamlopt/etc setups in a single place.
Where previously we needed to append `dune.common` to every dune file
and specify `flags` and `ocamlopt_flags` now the flags are specified
in `env` and applied accross the board.
This allows to
1. simplify build definitions,
2. avoid the need to generate dune files,
3. use plain sexps instead of OCaml and JBuilder plugin in build
files.
(I'll try to address 2 and 3 in the followup patches).
Existing `make` targets should continue working as before. Also, we
can use dune CLI like so:
```
infer/src$ dune printenv --profile opt # <- very useful for introspection
infer/src$ dune build check
infer/src$ dune build check --profile test
infer/src$ dune build infer.exe --profile dev
infer/src$ dune build infer.exe --profile opt
```
Also, with just 1 context something like `dune runtest` will run unit
tests only once instead of N times, where N is the number of contexts.
Now, there's one difference compared to the previous setup with
contexts:
- Previously, each context had its own build folder, and building infer
in opt context didn't invalidate any of the build artifacts in default
context. Therefore, alternating between `make` and `make opt` had low
overhead at the expense of having N copies of all build artifacts (1
for every context).
- Now, there's just 1 build folder and switching between profiles does
invalidate some artifacts (but not all) and rebuild takes a bit more
time.
So, if you're alternating like crazy between profiles your experience
may get worse (but not necessarily, more on that below). If you want
to trigger an opt build occasionally, you shouldn't notice much
difference.
For those who are concerned about slower build times when alternating
between different build profiles, there's a solution: [dune
cache](https://dune.readthedocs.io/en/stable/caching.html).
You can enable it by creating a file `~/.config/dune/config` with the
following contents:
```
(lang dune 2.0)
(cache enabled)
```
With cache enabled switching between different build profiles (but
also branches and commits) has ~0 overhead.
Dune cache works fine on Linux, but unfortunately there are [certain
problems with
MacOS](https://github.com/ocaml/dune/issues/3233) (hopefully, those
will be fixed soon and then there will be no downsides to using
profiles compared to contexts for our case).
Reviewed By: jvillard
Differential Revision: D20247864
fbshipit-source-id: 5f8afa0db
Summary:
What was happening in that case is that a .inferconfig file containing `{ "myflag":
false }` used to be translated by CommandLineOption as `--no-` (`--no-<long>`),
because it picked the *non-deprecated* form of the option (here `myflag` would
have to be a deprecated form of the option since its non-deprecated form is
empty `""`). Instead, pick a non-empty form of the option.
Reviewed By: jberdine
Differential Revision: D20368784
fbshipit-source-id: 8e761e684
Summary:
This was never quite finished and inferbo has a new way to do sort of
the same thing.
Reviewed By: skcho, ngorogiannis
Differential Revision: D20362619
fbshipit-source-id: 7c7935d47
Summary: Just for fun, and because killing InferPrint.ml is just so satisfying.
Reviewed By: ngorogiannis
Differential Revision: D20362643
fbshipit-source-id: 039cfec61
Summary:
Now that this module only prints report.json and costs_report.json, we
can dis-entangle the whole callback spaghetti.
Reviewed By: ngorogiannis
Differential Revision: D20362640
fbshipit-source-id: 56ffa4e08
Summary:
At this point in the stack, here's what's left in InferPrint.ml:
1. how to output report.json and costs_report.json
2. how to print summaries from the command line (`infer report`)
3. how to print --issues-tests stuff (`infer report --issues-tests`)
Keep only 1. in there. 2. goes to SpecsFiles.ml directly from infer.ml,
and 3. goes to its own module (also the fields to output in
--issues-tests get a non-poly variant).
1. does some extra stuff sometimes, eg in test-determinator mode. Keep
this for now.
Reviewed By: ngorogiannis
Differential Revision: D20362642
fbshipit-source-id: 0d9f0e8e2
Summary:
Make <infer-out>/report.json the default value for this option, as this
is what is used 99% of the time. Clean up test options using this.
Reviewed By: ngorogiannis
Differential Revision: D20362644
fbshipit-source-id: a1bb18757
Summary:
InferPrint hasn't been in charge of writing bugs.txt since forever.
This will be re-implemented as a post-processing of report.json instead
(like it is now, but in OCaml instead of python).
Reviewed By: ngorogiannis
Differential Revision: D20362641
fbshipit-source-id: 83d8cb53d
Summary:
I don't think anyone uses this. Meta-goal: cleaning up InferPrint.ml.
Measuring stats about summaries is good in principle, but we should do
it somewhere else instead of in the InferPrint callback hell. For
instance when we record each summary. Meanwhile, delete this.
Reviewed By: ngorogiannis
Differential Revision: D20362639
fbshipit-source-id: c73d431a5
Summary:
Adding a model for malloc: we add an attribute "Allocated". This can be used for implementing memory leaks: whenever the variables get out of scope, we can check that if the variable has an attribute Allocated, it also has an attribute Invalid CFree.
Possibly we will need more details in the Allocated attribute, to know if it's malloc, or other allocation function, but we can add that later when we know how it should look like.
Reviewed By: jvillard
Differential Revision: D20364541
fbshipit-source-id: 5e667a8c3
Summary:
This is not a full cleanup, but it is better than nothing.
At least `callback2` won't trigger me each time I see it anymore.
Reviewed By: jvillard
Differential Revision: D20364812
fbshipit-source-id: 8f25c24ad
Summary: Impurity traces are quite big due to recording values histories. Let's simplify the traces by removing pulse's value histories.
Reviewed By: skcho
Differential Revision: D20362149
fbshipit-source-id: 8a2a6115e
Summary: Type is not enough to say a function call of `Provider.get` is expensive or not.
Reviewed By: jvillard
Differential Revision: D20366206
fbshipit-source-id: 83d3e8741
Summary:
This diff uses a type parameter of `Provider.get` to decide whether assigning expensive cost to the
function call or not. For example, if the type is small one like `Provider<Integer>`, it be
evaluated to have a unit cost, otherwise a linear cost.
To get the return type of `Provider.get`, I added a simple analyzer that collects "casted" types
backwards. In Sil, while the function call statement loses the return type, e.g,
```
n$5=_fun_Object Provider.get()(n$3:javax.inject.Provider*);
```
the `n$5`'s value is usually casted to a specific type at some point later.
```
*&$irvar0:java.lang.Object*=n$5
n$8=*&$irvar0:java.lang.Object*
n$9=_fun___cast(n$8:java.lang.Object*,sizeof(t=java.lang.Integer;sub_t=( sub )(cast)):void)
```
So, the analyzer starts from the cast statements backward, collecting the types to cast for each
variables.
Reviewed By: ezgicicek
Differential Revision: D20345268
fbshipit-source-id: 704b42ec1
Summary:
The previous diff finally unblocks this important milestone.
Now we have only one place when `convert_complex_exp_to_pvar` needs to
modify the typestate (in Sil.Load typechecking).
Because the only case left is when `~is_assignment: false`, we also can
simplify logic of updating the typestate (remove dependency on
`is_assignment`, and rename the methods so they more clearly say what
they do (instead of vague "update_typestate").
This diff, in turn, unblocks further refactoring. Namely, remove this
remaining usage of
`convert_complex_exp_to_pvar_and_register_field_in_typestate` from
Sil.Load instruction so that there is no tricky field magic left!
We will try doing it in follow up diffs.
Reviewed By: artempyanykh
Differential Revision: D20334924
fbshipit-source-id: 20ce185ed
Summary:
When assigning to field `field = expr()` we need to compare actual
nullability of `expr()` (rhs) with formal nullability of field (lhs).
Formal nullability is based on field declaration.
Before this diff lhs nullability was calculated based on actual
nullability of lhs. This is an absolutely wrong thing to do!
E.g. `field = null` would update nullability of field in typestate, and
from this field will be considered as nullable, so next time field =
null is allowed...
Due to series of hacks pile on top of each other, this wrong behavior
actually works correctly; but this depends on completely wrong things
happening in other places of the program (e.g. typestate modifies in
wrong places to make this work!).
This diff unblocks refactoring that will clean up hacks mentioned above,
and simplify typechecking logic.
Reviewed By: artempyanykh
Differential Revision: D20334925
fbshipit-source-id: 8bbcbd33a
Summary: The macro is dead. It had been used when Inferbo had include-based C++ models.
Reviewed By: jvillard
Differential Revision: D20309031
fbshipit-source-id: bcfd8f923
Summary:
Warning: This might be a bit brutal.
PerfStats and EventLogger are pretty much subsumed by `ScubaLogging`.
It seems no one has been looking at the data they generate recently.
Let's delete them! If we need to re-implement some parts later on, let's
do that using `ScubaLogging`, which is better (eg, still produces data
when infer crashes).
Things we lose:
- errors in the clang frontend due to missing decl translation, etc.
- errors in biabduction due to timeouts, functions not found, etc.
We could also re-implement these using BackendStats and ScubaLogging
instead of brutally deleting everything.
Reviewed By: ngorogiannis
Differential Revision: D20343087
fbshipit-source-id: 90a3121ca
Summary:
At some point we thought disconnected CFGs (where some nodes are not
reachable from the initial node) were signs of bugs in our frontend, but
it turned out not to be the case. Thus, we compute the boolean "is
connected" for each procdesc for the only purpose of logging that
uninteresting piece of information.
Delete it.
Reviewed By: ngorogiannis
Differential Revision: D20342834
fbshipit-source-id: 3f9317003
Summary:
The goals are to have all the checker definitions and documentation in one
place (except how to actually run them, since that's not quite the same
concept; for example inferbo is one checker but several analyses depend on its
symbolic execution), and later on to be able to link issues reported by infer
back to the checker that generated them.
This makes apparent that the documentation of our checkers is lacking,
not touching that in this diff.
Not sure if "analysis" would be a better name than "checker" at this
point? For instance "Linters" is one of the checkers, which historically
at least we have not considered to be the case.
Reviewed By: mityal
Differential Revision: D20252386
fbshipit-source-id: fc611bfb7
Summary:
Some options are deprecated and this is indicated with `~long:""`. But for
boolean options this creates a spurious `"--no-"` flag. Detect when the option
has an empty flag and don't create the "no" version in that case.
Reviewed By: ngorogiannis
Differential Revision: D20286830
fbshipit-source-id: 9a2095ea8
Summary: This diff adds a model for Java's `Object.clone()` method (similar to existing shallow_copy).
Reviewed By: jvillard
Differential Revision: D20341073
fbshipit-source-id: 30ae40fe7
Summary:
Some (all?) of this is already tested in other tests, but this feature
is important enough (and the implementation is scattered accross the
whole code), so I found it useful to have a small test that ensures the
very basic things are working as expected.
See `NestedFieldAccess.java` that tests far more advances things, but
here we focus only very basic things: conditions, local variable
assignments, and explicit assignments.
Reviewed By: jvillard
Differential Revision: D20339056
fbshipit-source-id: a6cfd0043
Summary:
A java byte is not a short and a java character is not a C character. Fix those types by mapping them to what the spec ordains.
An extra advantage is that this establishes a bijection between java types and their project in the `sil` type system, so pretty printing in Java mode can actually be made to work.
Reviewed By: mityal
Differential Revision: D20330525
fbshipit-source-id: 00ac2294b
Summary:
- ignore `ConstantDereference` invalidations since they just represent assignments to constants.
- add `impurity.mli`
- split `extract_impurity` into several sub functions.
Reviewed By: jvillard
Differential Revision: D20335674
fbshipit-source-id: f0fd6b5f4
Summary:
1. Log cases when we fall back to default while typechecking (we aim to
gradually reduce their usage)
2. Log high level operations when checking field assignment
Reviewed By: ngorogiannis
Differential Revision: D20334923
fbshipit-source-id: 80c45b29e
Summary:
Experiment show that we did not rely on fact that this function could modify
typestate.
Reviewed By: artempyanykh
Differential Revision: D20285198
fbshipit-source-id: dc20f4b4b
Summary:
Previous diff introduced convert_complex_exp_to_pvar that does not
modify typestate.
Luckily enough, experiement shows that we don't need it in this place!
Reviewed By: artempyanykh
Differential Revision: D20284942
fbshipit-source-id: ea1471681
Summary:
This function is converting exp to pvar AND sometimes modifies a
typestate (even when ~is_assignment is false, which is something one
would not expect!).
We aim to reduce cases when it is not needed.
This diff splits the function into two: good and evil, and uses good
when it is a no-op.
Reviewed By: ngorogiannis
Differential Revision: D20284301
fbshipit-source-id: 0e5d65a65
Summary:
Extract `Typ.Name.Java.Split` into a standalone module.
- This module is really only used in `Procname` so no need to be in `Typ`.
- Remove some pointless conversions to strings and back.
- Reduce the interface of `Split` to the minimum ahead of possible removal in favour of normal types.
Reviewed By: mityal
Differential Revision: D20322887
fbshipit-source-id: 7963757cb
Summary:
- Use `Typ` constants instead of constructors for basic types.
- Nest single-use unexported functions at the point of use.
- Improve exception safety wrt `Not_found` exceptions by preferring `_opt` functions.
- Simplify deeply nested `match` statements by pushing conditionals to `when` clauses when possible.
Reviewed By: skcho
Differential Revision: D20304392
fbshipit-source-id: edbc08219
Summary:
- Remove dead argument in function in JMain.
- Document order of constructed classpath.
- Stop asking repeatedly for the cwd in `add_source_file`.
- Improve exception safety and readability in `load_from_verbose_output`.
Reviewed By: ezgicicek
Differential Revision: D20290519
fbshipit-source-id: 86c32dcdf
Summary:
Problem: `infer report <specs file name>` is called manually sometimes to see analysis results in CLI. However, giving the specs file name is sometimes annoying, because the specs file name may be quite long and include special characters sometimes.
This diff introduces `--procedures-summary` to lookup the summaries interactively in `infer explore`.
example1: There are 8 procedures that include "max" in their names, then I selected one of them by entering a number.
```
$ infer explore --procedures --procedures-filter '.*max.*' --procedures-summary
0: minmax_div_const2_Bad_FN
1: minmax_div_const_Good
2: use_int64_max_Bad
3: use_uint64_max_Good
4: use_int64_max_Good
5: minmax_div_const_Bad
6: minmax_div_const2_Good
7: use_uint64_max_Bad
Select one number (type 'a' for selecting all, 'q' for quit): 2
void use_int64_max_Bad()
Analyzed
ERRORS: BUFFER_OVERRUN_L1
WARNINGS:
FAILURE:NONE SYMOPS:0
BufferOverrunAnalysis: StackLocs: { } MemPure: { } Alias: { ret= }
BufferOverrunChecker: Safety conditions:
{ }
```
example2: If there is only one specs file that satisfies the given filter, it reports the summary of that procedure without an interaction.
```
$ infer explore --procedures --procedures-filter '.*add_in_loop_ok.*' --procedures-summary
Selected proc name: void ArrayListTest.add_in_loop_ok()
void void ArrayListTest.add_in_loop_ok()(ArrayListTest* this)
Analyzed
ERRORS:
WARNINGS:
FAILURE:NONE SYMOPS:0
BufferOverrunAnalysis: StackLocs: { } MemPure: { } Alias:
{ i=size(__new-390022197-0-1.elements), ret= }
LatestPrune: latest { i -> (5, { }, { }) by ((5, { }, { }) >= (5, { }, { })),
__new-390022197-0-1.elements -> (⊥, { }, { __new-390022197-1-1 -> length : 5 }) by ((5, { }, { }) >= (5, { }, { })) }
BufferOverrunChecker: Safety conditions:
{ }
```
Reviewed By: jvillard
Differential Revision: D20284052
fbshipit-source-id: 2131339f1
Summary:
Found a high number of `uname` syscalls while tracing infer. Figured it
must be the `Unix.gethostname` call. Stop continuously asking for the
hostname.
Reviewed By: jberdine
Differential Revision: D20309048
fbshipit-source-id: b8be1f2dc
Summary:
These are important enough operations to be tracked in logs.
This diff also tweaks logs for the main loop processing instuctions, so
that we log instr before processing it, not the reverse
Reviewed By: artempyanykh
Differential Revision: D20282443
fbshipit-source-id: 40b8b6627
Summary: The class map of biabduction models does not need to be part of the program type. This diff centralises all that in JModels module.
Reviewed By: jvillard
Differential Revision: D20283623
fbshipit-source-id: 5f95a7477
Summary:
- Factor out function that walks zip filename entries in jar file.
- Kill dead type (`classpath`) and record field (`classpath`).
- Make some string arguments named.
- Introduce function that folds over classnames in jar file and reduces occurrences of pattern "produce list, fold over it".
Reviewed By: artempyanykh
Differential Revision: D20245756
fbshipit-source-id: ccca47cdd
Summary: We don't need skipped calls for pre and post. Let's pull them out to `PulseAbductiveDomain`, next to pre and post.
Reviewed By: jvillard
Differential Revision: D20283589
fbshipit-source-id: 5cf970292
Summary: We forgot to take skipped calls into account for state comparison. This diff fixes that.
Reviewed By: skcho
Differential Revision: D20282739
fbshipit-source-id: 7b4d84bb0
Summary: Experiments suggest that infer does not take advantage of hardware threads and using more CPUs than the number of physical cores actually hurts performance. On Linux, `ProcessPool` now by default uses only the same number of workers as physical CPUs and pins them evenly.
Reviewed By: ngorogiannis
Differential Revision: D20193171
fbshipit-source-id: f8b9f55bf
Summary: Add the possibility of passing Scuba tagsets through the command line arguments.
Reviewed By: ngorogiannis
Differential Revision: D20262807
fbshipit-source-id: 9134cce8f
Summary:
It's a lot of code to maintain for something that no one ever uses
anymore.
Reviewed By: ngorogiannis
Differential Revision: D20282794
fbshipit-source-id: 28422c415
Summary: `PulseBaseDomain.leq` is never called but was there to satisfy the signature of `NoJoin` which itself was not needed. This diff removes `include NoJoin` and instead just adds signature for `pp` in `PulseBaseDomain`.
Reviewed By: jvillard
Differential Revision: D20280104
fbshipit-source-id: 8e3659280
Summary:
These were not used (and were actually activated byt the same config
param). They both are in experimental stage that never reached maturity.
Since the team does not have immediate plans to work on ObjC nullability
checker; and since "eradicate" (now known as nullsafe) is the main
solution for Java, removing it is sensible.
Reviewed By: jvillard
Differential Revision: D20279866
fbshipit-source-id: 79e64992b
Summary: The refactor from using 1 pipe for all worker-to-master communication to `n` (one per worker) introduced the possibility of starving workers because the master process read all the messages from one pipe (refreshing the file descriptors to read from with `Unix.select`) before moving to the next one. These changes aim to prevent that by reading one message from all available pipes before refreshing the file descriptors to read from.
Reviewed By: ngorogiannis
Differential Revision: D20194924
fbshipit-source-id: 91a0fbc47
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
Martin Trojer
Mitya Lyubarskiy
Ezgi Çiçek
Nikos Gorogiannis
Sungkeun Cho
Dulma Churchill
Jules Villard
Artem Pianykh
Alexander Mols
Fernando Gasperi Jabalera
Radu Grigore
Artem Pyanykh