From 6b156f71febdfaf49c7e036c4d258122b906e748 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Tue, 24 Jul 2018 05:48:57 -0700 Subject: [PATCH] [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 --- infer/src/concurrency/RacerD.ml | 79 ++++++++++++------- .../java/racerd/AbstractOwnership.java | 59 ++++++++++++++ 2 files changed, 109 insertions(+), 29 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/racerd/AbstractOwnership.java diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index 864c7b4ee..91099d459 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -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 diff --git a/infer/tests/codetoanalyze/java/racerd/AbstractOwnership.java b/infer/tests/codetoanalyze/java/racerd/AbstractOwnership.java new file mode 100644 index 000000000..b262697e9 --- /dev/null +++ b/infer/tests/codetoanalyze/java/racerd/AbstractOwnership.java @@ -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> { + 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 { + static ColumnBuilder create() { + return new ColumnBuilder(); + } + + @Override + ColumnBuilder getThis() { + return this; + } + + @Override + Column build() { + return new Column(); + } + } +}