[racerd] special-case for Litho's getThis

Summary:
Litho uses an idiom as follows.  `Component.Builder` has an abstract method `T getThis()` which is supposed to be implemented as `{return this;}`.
I believe the reason it's there is to have covariant return types in subclasses, though this doesn't really matter.
We don't do dynamic dispatch, so instead of summarising `getThis` as having a return ownership of `OwnedIf({ 0 })` we just return `Unowned`.
This is generating many false positives, which are really hard to debug.
Here I'm special-casing any abstract method whose return type is equal to that of it's only argument, to return conditional ownership.

Reviewed By: da319

Differential Revision: D8947992

fbshipit-source-id: 33c7e952d
master
Nikos Gorogiannis 6 years ago committed by Facebook Github Bot
parent ee7f07a1a9
commit 6b156f71fe

@ -315,6 +315,54 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
AccessDomain.fold update_callee_access callee_accesses caller_astate.accesses
let call_without_summary get_proc_desc callee_pname ret_access_path call_flags actuals astate =
let open RacerDConfig in
let open RacerDDomain in
let should_assume_returns_ownership (call_flags: CallFlags.t) actuals =
not call_flags.cf_interface && List.is_empty actuals
in
let is_abstract_getthis_like callee =
get_proc_desc callee
|> Option.value_map ~default:false ~f:(fun callee_pdesc ->
(Procdesc.get_attributes callee_pdesc).ProcAttributes.is_abstract
&&
match Procdesc.get_formals callee_pdesc with
| [(_, typ)] when Typ.equal typ (ret_access_path |> fst |> snd) ->
true
| _ ->
false )
in
if Models.is_box callee_pname then
match actuals with
| HilExp.AccessExpression actual_access_expr :: _ ->
let actual_ap = AccessExpression.to_access_path actual_access_expr in
if AttributeMapDomain.has_attribute actual_ap Functional astate.attribute_map then
(* TODO: check for constants, which are functional? *)
let attribute_map =
AttributeMapDomain.add_attribute ret_access_path Functional astate.attribute_map
in
{astate with attribute_map}
else astate
| _ ->
astate
else if should_assume_returns_ownership call_flags actuals then
(* assume non-interface methods with no summary and no parameters return ownership *)
let ownership =
OwnershipDomain.add ret_access_path OwnershipAbstractValue.owned astate.ownership
in
{astate with ownership}
else if is_abstract_getthis_like callee_pname then
(* assume abstract, single-parameter methods whose return type is equal to that of the first
formal return conditional ownership -- an example is getThis in Litho *)
let ownership =
OwnershipDomain.add ret_access_path
(OwnershipAbstractValue.make_owned_if 0)
astate.ownership
in
{astate with ownership}
else astate
let exec_instr (astate: Domain.astate) ({ProcData.tenv; extras; pdesc} as proc_data) _
(instr: HilInstr.t) =
let open Domain in
@ -431,35 +479,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
in
{locks; threads; accesses; ownership; attribute_map; wobbly_paths}
| None ->
let should_assume_returns_ownership (call_flags: CallFlags.t) actuals =
(* assume non-interface methods with no summary and no parameters return
ownership *)
not call_flags.cf_interface && List.is_empty actuals
in
if Models.is_box callee_pname then
match actuals with
| HilExp.AccessExpression actual_access_expr :: _ ->
let actual_ap = AccessExpression.to_access_path actual_access_expr in
if
AttributeMapDomain.has_attribute actual_ap Functional
astate.attribute_map
then
(* TODO: check for constants, which are functional? *)
let attribute_map =
AttributeMapDomain.add_attribute ret_access_path Functional
astate.attribute_map
in
{astate with attribute_map}
else astate
| _ ->
astate
else if should_assume_returns_ownership call_flags actuals then
let ownership =
OwnershipDomain.add ret_access_path OwnershipAbstractValue.owned
astate.ownership
in
{astate with ownership}
else astate
call_without_summary extras callee_pname ret_access_path call_flags actuals
astate
in
let add_if_annotated predicate attribute attribute_map =
if PatternMatch.override_exists predicate tenv callee_pname then

@ -0,0 +1,59 @@
/*
* Copyright (c) 2016-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
package codetoanalyze.java.checkers;
import com.facebook.infer.annotation.ReturnsOwnership;
import javax.annotation.concurrent.ThreadSafe;
// no races should be reported here
// abstract getThis should get a default summary returning conditional ownership
@ThreadSafe
abstract class Component {
abstract static class Builder<T extends Builder<T>> {
abstract T getThis();
private int i;
public T set(int i) {
this.i = i;
return getThis();
}
public T background() {
return getThis();
}
@ReturnsOwnership
abstract Component build();
}
}
@ThreadSafe
class Column extends Component {
static Component onCreateLayoutOk() {
Component.Builder<?> builder = ColumnBuilder.create().background();
return builder.set(0).build();
}
static class ColumnBuilder extends Component.Builder<ColumnBuilder> {
static ColumnBuilder create() {
return new ColumnBuilder();
}
@Override
ColumnBuilder getThis() {
return this;
}
@Override
Column build() {
return new Column();
}
}
}
Loading…
Cancel
Save