From 1fb344289ecf47bee098f7c5ef8c0ad178820ee5 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Tue, 12 Nov 2019 14:30:32 -0800 Subject: [PATCH] [racerd] fix pattern matching for abstract methods returning conditional ownership Summary: `getThis` is an idiom for allowing Builder sub-classes to jump through the hoops of covariance plus java generics with self types. It's declared as abstract in the (generic) inner `Builder` class of a root class, and subclasses declare generic `Builder`s that inherit from the generic root `Builder` and trivially implement this method by returning `this`. Obviously, this returns conditional ownership (if `this` is owned, then the return value is owned). The way it's typically used is ``` T foo() { ... return getThis(); } ``` However, because abstract methods need dynamic dispatch for proper summarisation, we miss all that. A workaround was been implemented in D8947992 (see that for context), but it was buggy -- it required that the LHS type in the assignment ``` lhs = this.getThis(); ``` is the same as the type of `this`, but this is too strict (eg, when using casts). Here, the condition is changed to requiring that the return type of the method is the same as the type of `this`. We also avoid asking for the `procdesc` as everything needed is in the attributes. Reviewed By: jberdine Differential Revision: D18450737 fbshipit-source-id: e67f0495c --- infer/src/concurrency/RacerD.ml | 11 ----------- infer/src/concurrency/RacerDModels.ml | 14 ++++++++++++++ infer/src/concurrency/RacerDModels.mli | 3 +++ 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index 3cc99677e..4ab5b3476 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -186,17 +186,6 @@ module TransferFunctions (CFG : ProcCfg.S) = struct 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 = - Ondemand.get_proc_desc callee - |> Option.exists ~f:(fun callee_pdesc -> - (Procdesc.get_attributes callee_pdesc).ProcAttributes.is_abstract - && - match Procdesc.get_formals callee_pdesc with - | [(_, typ)] when Typ.equal typ (snd ret_base) -> - true - | _ -> - false ) - in if is_box callee_pname then match actuals with | HilExp.AccessExpression actual_access_expr :: _ -> diff --git a/infer/src/concurrency/RacerDModels.ml b/infer/src/concurrency/RacerDModels.ml index f1669aee7..9925e0ad3 100644 --- a/infer/src/concurrency/RacerDModels.ml +++ b/infer/src/concurrency/RacerDModels.ml @@ -462,3 +462,17 @@ let is_synchronized_container callee_pname (access_exp : HilExp.AccessExpression false ) | _ -> false + + +(** check that callee is abstract and accepts one argument. In addition, its argument type must be + equal to its return type. *) +let is_abstract_getthis_like callee = + attrs_of_pname callee + |> Option.exists ~f:(fun (attrs : ProcAttributes.t) -> + attrs.is_abstract + && + match attrs.formals with + | [(_, typ)] when Typ.equal typ attrs.ret_type -> + true + | _ -> + false ) diff --git a/infer/src/concurrency/RacerDModels.mli b/infer/src/concurrency/RacerDModels.mli index 678657dfb..fd8583fb9 100644 --- a/infer/src/concurrency/RacerDModels.mli +++ b/infer/src/concurrency/RacerDModels.mli @@ -52,3 +52,6 @@ val should_flag_interface_call : Tenv.t -> HilExp.t list -> CallFlags.t -> Typ.P val is_synchronized_container : Typ.Procname.t -> HilExp.AccessExpression.t -> Tenv.t -> bool (** is a call on an access expression to a method of a synchronized container? *) + +val is_abstract_getthis_like : Typ.Procname.t -> bool +(** is the callee an abstract method with one argument where argument type is equal to return type *)