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
Summary: This diff finds dead modules, i.e, .ml files that is not used in the binaries.
Reviewed By: ngorogiannis
Differential Revision: D20035984
fbshipit-source-id: 56ac2e817
Summary:
Lets not also describe what it does, but emphasize that it is
nullsafe-specific and also what are benefits of using it.
Reviewed By: artempyanykh
Differential Revision: D20030497
fbshipit-source-id: f28c803fd
Summary:
The `--continue-analysis` option enables continuing analysis after more targets are captured by
`--continue`. For example,
```
$ infer capture -- buck build tgt1
$ infer analyze --merge
$ infer capture --continue -- bucck build tgt2
$ infer analyze --merge --continue-analyze
```
In the last analysis, it reuses the analysis results of `tgt1` from the previous analysis. If
`tgt1` and `tgt2` have a same dependency to a library, the analysis results of the library is also
reused.
Reviewed By: dulmarod
Differential Revision: D19996598
fbshipit-source-id: bb6874a6f
Summary:
Introduction of `ThirdPartyNonnull` nullability broke nullability
refinement heuristic for enums. This diff fixes it and also adds tests
so that we hopefully avoid such issues in future.
Reviewed By: mityal
Differential Revision: D19975810
fbshipit-source-id: f9245f305
Summary:
We need to be able to differentiate `UncheckedNonnull`s in internal vs
third-party code. Previously, those were under one `UncheckedNonnull`
nullability which led to hacks for optmistic third-party parameter
checks in `eradicateChecks.ml` and lack of third-party enforcement in
`Nullsafe(LOCAL, trust=all)` mode (i.e. we want to trust internal
unchecked code, but don't want to trust unvetted third-party).
Now such values are properly modelled and can be accounted for
regularly within rules.
Also, various whitelists are refactored using
`Nullability.is_considered_nonnull ~nullsafe_mode nullability`.
`ErrorRenderingUtils` became a tad more convoluted, but oh well, one
step at a time.
Reviewed By: mityal
Differential Revision: D19977086
fbshipit-source-id: 8337a47b9
Summary:
Add support for nullsafe mode with `trust=all` and `trust=none` a case
with a specific trust list is not supported yet and needs to be
implemented separately.
Tests introduce one unexpected
`ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION` issue which
complains about `this` having incorrect nullability; it is a bug and
needs to be fixed separately.
Reviewed By: mityal
Differential Revision: D19662708
fbshipit-source-id: 3bc1e3952
Summary: Was broken by previous diff: I forgot to update issues.exp
Reviewed By: artempyanykh
Differential Revision: D20001233
fbshipit-source-id: 67f534349
Summary: In all other cases we have period at the end, which is inconsistent.
Reviewed By: artempyanykh
Differential Revision: D20001065
fbshipit-source-id: 85ec6d751
Summary:
This helps debug nullsafe. Before, we would only print the initial and
last state of a given node but now we can see all the intermediate steps
too.
Example before:
```
before:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
after:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
```
After:
```
before:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
instr: n$0=*&this:Toto* [line 10]
new state:
n$0 -> [this] [StrictNonnull] Toto*
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
...
instr: EXIT_SCOPE(n$0,n$1,this); [line 10]
new state:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
```
Reviewed By: mityal
Differential Revision: D19973278
fbshipit-source-id: bcea33f96
Summary:
Use a record of package, class name to store (qualified) Java class names. This saves the round trip of concatenating then splitting again, etc, as well as saves some memory in the type environment as now the package paths can be shared across classes of the same package (about 10% in tests).
Also remove some unfortunate APIs.
Reviewed By: jvillard
Differential Revision: D19969325
fbshipit-source-id: f7b7f5a55
Summary: Change the ProcLocker implementation to use symlinks instead of files. Tests have indicated that they may use less resources.
Reviewed By: ngorogiannis
Differential Revision: D19822048
fbshipit-source-id: 991ababf2
Summary:
The previos one was too broad and did not indicate the main intended
usage, which is currently the nullsafe typechecker.
Also it was misleading: Initializer methods should NOT be called inside
constructors.
Finally, it recommended using Initializer in Builder pattern, which is
a questionable idea, so it better to avoid mentioning builders.
Reviewed By: artempyanykh
Differential Revision: D19942675
fbshipit-source-id: 0eb1ce796
Summary: The way `Mangled.t` is used in `JavaClassName` means that it's always a plain string (we never have a "mangled" part). Remove the indirection and extra allocation. Also, simplify the API by throwing away one function that was used just once and wastefully.
Reviewed By: artempyanykh
Differential Revision: D19950672
fbshipit-source-id: b61fcba6e
Summary: Instead of converting the class type name of a java procedure to a string and then back to a type name, just get it directly.
Reviewed By: jvillard
Differential Revision: D19950528
fbshipit-source-id: dadf6d130
Summary: Rather than recomputing the `proc_name`, let's pass it around.
Reviewed By: skcho
Differential Revision: D19951461
fbshipit-source-id: 90b57dcc7
Summary: This diff suppresses integer overflow issues in functions that includes "hash" in its name.
Reviewed By: jvillard
Differential Revision: D19942654
fbshipit-source-id: d86fa4f00
Summary: In line with changes to loom query in D19903057, let's adjust Infer's processing of the results.
Reviewed By: martintrojer
Differential Revision: D19902933
fbshipit-source-id: 200b3a03e
Summary: Add the number of cores used and the scheduler type to the environment info printed before running.
Reviewed By: ngorogiannis
Differential Revision: D19941243
fbshipit-source-id: 576e9f610
Summary: Not needed any more as infer's concurrency isn't controlled via make.
Reviewed By: jvillard
Differential Revision: D19905712
fbshipit-source-id: f97ef4421
Summary:
Some annotation processors / transormers might generate artificial fields. These
are not expicitly written by code writer, hence non actionable.
We distinct this fields heuristically: if they start with "$", they are
surely not user-written.
Reviewed By: ezgicicek
Differential Revision: D19947988
fbshipit-source-id: a0a15fc23
Summary:
Add let*/+ syntax to `result` types to simplify all the applications of
`>>=`, `>>|` that are followed by a binding (eg `>>= fun x -> ...`) in
pulse.
Reviewed By: skcho
Differential Revision: D19940728
fbshipit-source-id: 4df159029
Summary:
We can already tell that a summary cannot be applied by raising
`Contradiction`, so use this mechanism to stop applying a summary if the
number of formals doesn't match the number of actuals provided.
Previously we would return an option type and `None` in case of
mismatch, on top of the `raise Contradiction` mechanism (used for
aliasing and arithmetic contradictions).
This changes the behaviour of pulse in this case: before we would skip
over the function call, but now we stop the analysis.
Reviewed By: dulmarod
Differential Revision: D19940729
fbshipit-source-id: 6def40cd6
Summary: Once we identify a weakSelf variable that is being used in a Noescape block, we want to report only the first occurrence.
Reviewed By: skcho
Differential Revision: D19941502
fbshipit-source-id: 2b6d4648b
Summary: For each variable that we identify as a captured strong self, we want to report only the first occurrence.
Reviewed By: skcho
Differential Revision: D19940031
fbshipit-source-id: f38f642c9
Summary: When we discovered that a strongSelf var was not checked for null, we then report in each occurrence which is spammy. Now we report only the first occurrence. To achieve that, we store a `reported` flag in the domain that gets set to true after we report once, and we only report if it's false.
Reviewed By: jvillard
Differential Revision: D19877218
fbshipit-source-id: c44109ae9
Summary:
This adds `let*/+`, `and*/+` operators for Option. See [the manual](https://caml.inria.fr/pub/docs/manual-ocaml/manual046.html) for more information.
Example usage:
```
let foo =
let open IOption.Let_syntax in
let* a = get_optional () in
let* b = get_another_optional () in
return (a + b)
```
Reviewed By: jvillard
Differential Revision: D19880033
fbshipit-source-id: c7998b0c6
Summary:
Previous implementation supported only stringy params (strings and
stringified bools). Current one exposes a proper variant `Annot.t`,
with support for all possible param values in Java except
numbers (more on that below).
This change is required for implementing `Nullsafe(LOCAL)` as the
annotation used to specify nullsafe behaviour has a more complex
structure than what we've dealt with before.
**Why support for number values was not added**: supporting numbers
requires using `int64`. Unfortunately, adding another variant `Vnum
int64` to `Annot.t` causes a runtime failure on assert in
`MaximumSharing.ml:133`. It seems that it might be enough to flip
`fail_on_nonstring` from `true` to `false`, but since this would
require additional testing and is not required for my case, I'll leave
checking this to whoever needs to use numeric annot params in future.
Reviewed By: ezgicicek
Differential Revision: D19855923
fbshipit-source-id: 878e33856
Summary: No reason to use a set when an integer will suffice. This further reduces GC churn.
Reviewed By: fgasperij
Differential Revision: D19888300
fbshipit-source-id: 9fc8c73f5
Summary: Queues are implemented using a circular array, so should be less GC-heavy than continually allocating/freeing list nodes.
Reviewed By: jberdine, fgasperij
Differential Revision: D18504104
fbshipit-source-id: 93d29c253
Summary:
Building the call graph should be done only in the scheduler process after having forked all workers. This was achieved by a lazy init pattern, whereby the first time `next` was called, it would build the call graph, on the assumption that `next` is only ever called in the scheduler after forking.
D19769741 made this compulsory regardless the scheduler by passing a thunk to `ProcessPool` which is called to obtain the actual scheduler, on the right process and after the fork.
This means we don't need the custom lazy init logic any more. In addition, that set up used a DB query to overapproximate the number of procedures to analyse, because this was supposed to be provided *before* forking. Now this is also not needed, and on top of that we can provide the exact number after building the call graph.
Reviewed By: ezgicicek
Differential Revision: D19833974
fbshipit-source-id: 7f6d51d93
Summary:
In Inferbo, the bottom memory is introduced when a node is unreachable by pruning, i.e.
`[[e]] <= [0,0]` on `prune(e)`. This diff distinguishes whether `[[e]]` is `[0,0]` (unreachable)
or bottom (it could not evaluate `e` by some unknown reasons).
Reviewed By: ezgicicek
Differential Revision: D19902046
fbshipit-source-id: 7706017d6
Summary:
More newer = more better.
This flips the Not_found -> Not_found_s switch, and forbids a bunch more
polymorphic comparisons (mostly turned into `int` comparisons for
convenience). Earlier diffs prepare for this so this diff is only about
breaking changes in the API, of which there are only a few.
Reviewed By: jberdine
Differential Revision: D19861583
fbshipit-source-id: fe54ce8f0
Summary:
Core v13 APIs stopped raising `Not_found` and instead raise
`Not_found_s`, which wreaks havoc in our codebase. Carefully inspect
each `Not_found` and add `Not_found_s` where needed (that way it's
compatible with both Core v12 and v13 for now).
Reviewed By: jberdine
Differential Revision: D19861585
fbshipit-source-id: 9a5361ae9
Summary:
The big one:
- stop using polymorphic `<>`, `<`, `>`, ..
- add `<>` to `PolyVariantEqual` escape hatch now that `<>` is as taboo as `=`
- Interestingly, there were a lot of uses of `Z.(x < y)`, which although
they seem to use `Z.lt` actually used polymorphic comparison. The actual
comparison infix operators of `Z` are cleverly hidden in `Z.Compare`
instead, which makes them impractical to use...
Reviewed By: jberdine
Differential Revision: D19861584
fbshipit-source-id: 5dce08ad9
Summary:
Polymorphic compare is bad, 'mkay? I also tried changing this to do what
the comment says the function does (keep the first term instead of the
"largest" term) but that resulted in test changes so I kept the original
behaviour instead.
Reviewed By: jberdine
Differential Revision: D19861587
fbshipit-source-id: d57fd4a02
Summary:
That trick briefly worked when it was introduced but I think a change in
merlin broke it again as I see "sexp_list", "sexp_option" in the types
shown by merlin again. Disgusting!
Remove useless hack. Also referring to types such as `sexp_list` is
deprecated in core v13 for some reason.
Reviewed By: jberdine
Differential Revision: D19861586
fbshipit-source-id: 1c4c3af13
Dulma Churchill
Mitya Lyubarskiy
Ezgi Çiçek
Sungkeun Cho
Martin Trojer
Jules Villard
Nikos Gorogiannis
Alexander Mols
Fernando Gasperi Jabalera
Radu Grigore
Artem Pianykh
Artem Pyanykh
David Pichardie