Summary:
To emulate the `ThreadSafe` contract in C++/ObjC, reporting was gated behind a check that ensured a C++/ObjC class has a `std::mutex` member (plus other filters). This is reasonable, but it has some drawbacks
- other locks may be used, and therefore must be added to the member check;
- locking mechanisms that use the object itself as a monitor cannot be modelled (`synchronized` in ObjC)
RacerD already has `ThreadsDomain` which models our guess on whether a method is expected to run in a concurrent context, and which in C++/ObjC boils down to whether the method non-transitively acquires a lock. This should be a good enough indicator that the class should be checked regardless of whether the locks are member fields. This diff gates the C++/ObjC check on that abstract property.
Reviewed By: dulmarod
Differential Revision: D19558355
fbshipit-source-id: 229d7ff82
Summary: This diff removes a dead field, `is_cpp_nothrow` and `is_cpp_noexcept_method`.
Reviewed By: jvillard
Differential Revision: D19489417
fbshipit-source-id: 971a7f533
Summary:
The ProcLocker uses files as locks and relies on the guarantees of the `Unix.open_file` function when using `O_CREAT` and `O_EXCL` simultaneously.
- `setup`: creates a directory for the lock files inside `infe-out` and deletes its content if it already existed.
- `clean`: does nothing for now. Any file locks that may have been left unlocked are removed by the `setup` in the next run. This way the user can see what locks were taken if the program crashes.
- `lock_exn`: try to lock the `Procname` and if it can't releases all the locks that is currently holding.
- `unlock`: removes the corresponding file.
Reviewed By: ngorogiannis
Differential Revision: D19639402
fbshipit-source-id: e02f277ff
Summary:
Revert incomplete/incorrect translation of `synchronized` in ObjC.
The current translation is incomplete because
```
syncrhonized(foo){
return;
}
```
should be translated as
```
__set_locked_attribute(foo);
__delete_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
but instead we get
```
__set_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
The same applies for `break`/`continue` etc
Reviewed By: skcho
Differential Revision: D19718882
fbshipit-source-id: fc49ef529
Summary:
- Thread the two types into one instead of having a record where the `path` field doesn't always make sense (`Class` case).
- Improved pretty printing of class objects (java only).
- Move starvation-specific stuff out of `AbstractAddress` (eg `make_java_synchronized`).
- Slight optimisation of `apply_subst` for when a parameter is used without additional accesses inside a method (then, the substitution need not modify the term substituted for the parameter in any way).
Reviewed By: ezgicicek
Differential Revision: D19639922
fbshipit-source-id: 1cebecf5d
Summary:
`String` and `StringBuilder` both implement `CharSequence`. Let's generalize the model for `String` to `CharSequence` wherever possible and add missing models for
- `StringBuilder.append`
- `StringBuilder.toString`
Reviewed By: skcho
Differential Revision: D19558009
fbshipit-source-id: 0dfdb21af
Summary:
Java's String models were broken for
- initializing a String object with a locally defined constant string (which is an `Object*` in SIL).
- initializing a String object with a `char`/`byte` array
This diff fixes them and also adds models for `new String ()`.
Reviewed By: skcho
Differential Revision: D19662180
fbshipit-source-id: 23968d0aa
Summary:
When the expression resolving to a function to be called could not be
translated to either a proc name or at least an access path, the HIL
translation code would crash. However, this is perfectly possible. Moreover, no
one actually uses the payload of the `Indirect` datatype so no complication
arises from generilising its type to `HilExp.t`, as done in this diff.
Reviewed By: ngorogiannis
Differential Revision: D19691127
fbshipit-source-id: 4c0400ab7
Summary:
`replace_make_shared` is finding a constructor by iterating all fields of type structs, which may be
expensive if there are lots of fields or `replace_make_shared` is called many times with the same parameters.
Reviewed By: ngorogiannis
Differential Revision: D19446260
fbshipit-source-id: c5c279ca5
Summary: This diff fixes the array access checking function for nested global arrays. We had assumed that RHS of `store` statement in SIL does not include array access expression, but that is not true: for global arrays, SIL can have statements like `*LHS = GlobalArray[n][m]`.
Reviewed By: ezgicicek
Differential Revision: D19300153
fbshipit-source-id: 256325642
Summary:
This diff gives semantics of `std::make_shared` as simple constructor, i.e., it changes function
call of `std::make_chared<C>(i)` to the constructor `C(i)`.
Reviewed By: ngorogiannis
Differential Revision: D19432338
fbshipit-source-id: 0d838e555
Summary: This adds a check for when developers use weakSelf (and maybe strongSelf) in a block, when there is no need for it, because it won't cause a retain cycle. In general there is an annotation in Objective-C for methods that take blocks, NS_NOESCAPE, that means that the passed block won't leave the scope, i.e. is "no escaping". So we report when weakSelf is used and the block has the annotation.
Reviewed By: ngorogiannis
Differential Revision: D19662468
fbshipit-source-id: f5ac695aa
Summary:
This attribute is given to parameters of methods that take Objective-C blocks to show that they will be used only in the current context and won't "escape" the context.
We translate it here, with the goal to use it in a new check later. The check is about not using weakSelf in non-escaping blocks, because retain cycles are not possible.
The translation is a bit complex because the annotation comes in the parameter of a method, but in the checker we will need it in the block. So we pass it around in the frontend from the translation of the method call to the translation context and on to the block expression and the block declaration afterwards.
Reviewed By: ngorogiannis
Differential Revision: D19600377
fbshipit-source-id: dd49539bd
Summary:
The difference from default set is mostly in:
1. turning on warnings 32..39 `unused X`,
2. turning off warning 66 `unused open!` since `open! IStd` is present
in pretty much all the files.
3. Non-exhaustive pattern match is now treated as a compilation
error.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19646047
fbshipit-source-id: c84ba628a
Summary:
Refactor all occurences of `is_strict_mode` to use `NullsafeMode`
instead. This will allow introducing _local_ typechecking modes for
nullsafe in the follow up patches.
Reviewed By: ezgicicek
Differential Revision: D19639883
fbshipit-source-id: bdf535b66
Summary: Moving this big tuple to a record, because it's cleaner code, and I need to add another element in the next diff.
Reviewed By: ezgicicek
Differential Revision: D19640389
fbshipit-source-id: 86b1576a0
Summary:
Update from dune 1 to dune 2. The change was mostly straightforward
except that change to (modes ...) default to exe only was somewhat
unexpected.
Anyhow, with the community moving to dune 2 it's good to keep up.
Reviewed By: ngorogiannis
Differential Revision: D19605895
fbshipit-source-id: 1f9830de8
Summary: This will be eventually used in RacerD. Further diffs up the stack will migrate away the starvation-specific aspects.
Reviewed By: ezgicicek
Differential Revision: D19623392
fbshipit-source-id: a72650718
Summary:
Prevent returning a negative cost bound when calling `substring(begin_index, end_index)` when either is possible
- `begin_index < 0`
- `begin_index > end_index`
Instead, return unit cost since such cases either throw `IndexOutOfBoundsException ` at runtime or correspond to having two symbolic bounds that cannot be semantically compared.
Reviewed By: dulmarod
Differential Revision: D19619410
fbshipit-source-id: cf5e8cb7b
Summary:
The "access path" memory model (equal access paths iff equal object addresses) is suited to when aliasing occurs only at the roots (i.e. variables). When there is intentional aliasing in the middle of an access path, this model will miss the aliasing. For instance if `[x.f] == [y.g]`, then also `[x.f.h] == [y.g.h]`, but the latter access paths are unequal.
In Java, non-static inner classes consistently alias `this.this$0` inside an inner class, which points to the "parent" outer-class object. So if two inner-class objects (belonging to different inner classes) access `this(type:InnerClassA).this$0.f` and `this(type:InnerClassB).this$0.f` the equality will be missed (many other combinations exist). This isn't strictly due to the memory model -- any alias analysis would have to do some class invariant inference to detect this.
For this purpose `AccessPath.inner_class_normalize` exists (it replaces `this.this$0` with `this` of the appropriate type), but this breaks the invariant that we know which formal parameter is at the root (there may not even exist a `this` parameter if the method is static). So this was buggy.
Here we simply recursively remove the synthetic field prefix of the accesses list, while computing forwards the object type. This is only applied when we check aliasing across threads. This will also allow actuals/parameters substitutions (stacked diff) which normalisation was breaking.
Reviewed By: jberdine
Differential Revision: D19601455
fbshipit-source-id: 7e42667b6
Summary:
Rename DeclaredNonnull -> UncheckedNonnull, Nonnull -> StrictNonnull.
This is a preparatory step to introduce one more nullability option,
specifically CheckedNonnull to support local nullsafe mode.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19619289
fbshipit-source-id: 12a3ff814
Summary:
This will help adoption of non-transitive strictification modes.
The main use-case we are aiming with this function is:
- During strictification, a method is made nullable, but it is not feasible to fix all
callers straight away. Suppressing nullsafe with an appropriate task or
comment will make it easier to unblock the strictification and move
forward.
Reviewed By: artempyanykh
Differential Revision: D19599098
fbshipit-source-id: 3769bbd3d
Summary:
1. I can not imagine a useful usecase for strict node, lets not
complicate the code.
2. Add docs.
Reviewed By: artempyanykh
Differential Revision: D19599091
fbshipit-source-id: 1d37a717f
Summary:
Reorder assertNotNull and assumeNotNull:
1. More useful methods go first.
2. More useful methods explicitly recommended over less useful.
Reviewed By: artempyanykh
Differential Revision: D19599084
fbshipit-source-id: 1b9ad9ef6
Summary: The translation of closures includes a load instruction for the captured variable, then we add that corresponding id to the closure. This doesn't correspond to an actual "use" of the captured variable in the source program though, and causes false positives. Here we remove the ids from the domain when that id is being added to a closure.
Reviewed By: ngorogiannis
Differential Revision: D19557352
fbshipit-source-id: 52b426011
Summary:
- Add `Nullsafe` annotation as a general mechanism to specify
type-checking behaviour for nullsafe.
- Document annotation params and provide usage examples with
explanations.
- Add tests to demonstrate the behaviour with different type-checking
modes.
No implementation is added. This diff serves as an RFC to hash out the
details before I dive into code.
Reviewed By: ngorogiannis
Differential Revision: D19578329
fbshipit-source-id: b1a9f6162
Summary: Keep the type name of the class as the key in the map constructed from class names to their methods in a file. This will be used later, and also why string?
Reviewed By: dulmarod
Differential Revision: D19557707
fbshipit-source-id: aa8569581
Summary: D19496263 had refactored cost to its own subdirectory. In this process, it introduced a perf degradation by bundling of collection and computation of constraints which relies on imperative union find. This diff fixes that by separating them.
Reviewed By: ngorogiannis
Differential Revision: D19598610
fbshipit-source-id: 0e466522d
Summary: Add the interface that will be used to lock Procedures.
Reviewed By: ngorogiannis
Differential Revision: D19580578
fbshipit-source-id: b5e334b18
Summary: `String.split(regexp)` returns an array that is split by the given regexp. If the regexp doesn't match, the original string is returned. Hence, the resulting array's length must be in `[1, max(1, n_u -1)]` where`n_u` is the upper bound of the string's length.
Reviewed By: dulmarod
Differential Revision: D19578318
fbshipit-source-id: 675af7376
Summary:
Adding a new check as part of the SelfInBlock checker, for cases when a strong pointer to self, that is not self, is captured in the block.
This will cover the following wrong scenarios:
1. strongSelf is a local variable in a block as it should be, and then it's used in a sub-block, in which case it's a captured variable.
2. weakSelf is defined without the weak attribute by mistake.
Reviewed By: ngorogiannis
Differential Revision: D19538036
fbshipit-source-id: 151871745
Summary:
In practice, condition redundant is extremely noisy and low-signal
warning (hence it is turned off by default).
This diff does minor tweaks, without the intention to change anything
substantially:
1/ Change severity to advice
2/ Change "is" to "might be"
3/ Describe the reason in case the origin comes from a method.
The short term motivation is to use 3/ for specific use-case: running nullsafe on codebase and
identify most suspicious functions (that are not annotated by often
compared with null).
Reviewed By: artempyanykh
Differential Revision: D19553571
fbshipit-source-id: 2b43ea0af
Summary:
This is/can be useful for future changes that make the reporting more
precise based on the exact origin.
See the follow up diff as an example.
Reviewed By: artempyanykh
Differential Revision: D19553567
fbshipit-source-id: c2b2c28a1
Summary: Currently, empty summaries are passed to the cluster callbacks. This is pointless and could potentially lead to recomputing already analysed summaries. This change passes only procnames to the callback, and `Ondemand` is used to load the summary or analyse as needed.
Reviewed By: jvillard
Differential Revision: D19544043
fbshipit-source-id: 28ab642c3
Summary: There is no need to provide type environments to cluster analysers, since the execution environment can be used to retrieve those on demand.
Reviewed By: jvillard
Differential Revision: D19543561
fbshipit-source-id: f9b064011
Summary: We were reporting strongSelf Not Checked when the variable was checked in a conditional, this diff fixes that.
Reviewed By: jvillard
Differential Revision: D19535943
fbshipit-source-id: f8e64e1b7
Summary: I noticed when looking into a false positive of strongSelf Not Checked, that there were some inconsistencies in the translation of if statements with an and, with an extra redundant join only if using a method in the condition that returned an object. So I could repro the problem and investigate and found the place of the inconsistency in the translation. This diff fixes it without changing things too much.
Reviewed By: jvillard
Differential Revision: D19518368
fbshipit-source-id: 47a6a778c
Summary:
The order by which the scheduler visits odd and even methods here
will determine if there is any report at all. This is a bad test
so remove.
Reviewed By: fgasperij
Differential Revision: D19535537
fbshipit-source-id: 6b64b0de9
Summary: Adding reporting to strongSelf Not Checked when strongSelf is passed to a method in a not explicitly nullable position.
Reviewed By: ezgicicek
Differential Revision: D19330872
fbshipit-source-id: 95871a70a
Summary: After receiving feedback about this, I'm changing the reporting of strongSelf Not Checked to only in cases where it can cause a crash. Here I'm adding reporting for field access, and removing general reporting. In a next diff, I'll also add reporting for passing strongSelf to methods in not explicitly nullable positions.
Reviewed By: skcho
Differential Revision: D19329842
fbshipit-source-id: 35beb2aa3
Summary:
The RestartScheduler needs to know if the worker finished it's task
because:
1. there was no more work to do or
2. found that a needed Procname was already taken (this part is not yet implemented)
This need was addressed by (i) making the functions that the workers execute
return a value of task_result.t intead of unit and (ii) adding a
constructor to the worker_message.t (FinishedTask).
Reviewed By: ngorogiannis
Differential Revision: D19467783
fbshipit-source-id: a76b02b6c
Summary:
1. One should use either a writer or a stream to send a response, but not both.
2. A response should be forwarded only if it was commited.
Both properties are extracted from API comments on classes in the servlet API.
Reviewed By: jvillard
Differential Revision: D19514568
fbshipit-source-id: 79f0257ed
Nikos Gorogiannis
Sungkeun Cho
Fernando Gasperi Jabalera
Ezgi Çiçek
Jules Villard
Dulma Churchill
Artem Pianykh
Mitya Lyubarskiy
Radu Grigore