From 9c1bef41e7b1e7ee9fc2c57a192fd1eeb665778a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Thu, 5 Dec 2019 08:30:14 -0800 Subject: [PATCH] [litho] Make MethodCall comparison oblivious to prop suffixes Summary: Current method call comparison is too strong. As exemplified with the new test, one can also set the required prop by calling a version which contains the suffixes. The domain should take care of such cases now. Reviewed By: skcho Differential Revision: D18808869 fbshipit-source-id: 9f7672e75 --- infer/src/checkers/LithoDomain.ml | 34 ++++++++++++++++++- infer/src/checkers/LithoDomain.mli | 2 ++ infer/src/checkers/RequiredProps.ml | 4 +-- .../java/litho/RequiredProps.java | 20 +++++++++++ .../java/litho/ResPropComponent.java | 4 +-- .../tests/codetoanalyze/java/litho/issues.exp | 2 ++ 6 files changed, 60 insertions(+), 6 deletions(-) diff --git a/infer/src/checkers/LithoDomain.ml b/infer/src/checkers/LithoDomain.ml index 10d628aaa..e15c4fa89 100644 --- a/infer/src/checkers/LithoDomain.ml +++ b/infer/src/checkers/LithoDomain.ml @@ -30,6 +30,8 @@ module LocalAccessPath = struct let pp fmt t = AccessPath.pp fmt t.access_path end +let suffixes = String.Set.of_list ["Attr"; "Dip"; "Px"; "Res"; "Sp"] + module MethodCall = struct type t = {receiver: LocalAccessPath.t; procname: Typ.Procname.t; location: Location.t} [@@deriving compare] @@ -68,9 +70,39 @@ module NewDomain = struct module S = AbstractDomain.InvertedSet (struct include MethodCall + let compare_procname p1 p2 = + let chopped_opt pname = + let method_name = Typ.Procname.get_method pname in + String.Set.find_map suffixes ~f:(fun suffix -> String.chop_suffix method_name ~suffix) + in + let replace_both s p = + match p with + | Typ.Procname.Java java_pname -> + (* This is a bit of a hack to quickly compare based on only + method names *) + Typ.Procname.Java + ( Typ.Procname.Java.replace_method_name s java_pname + |> Typ.Procname.Java.replace_parameters [] ) + | _ -> + p + in + let p1, p2 = + match (chopped_opt p1, chopped_opt p2) with + | Some chopped1, Some chopped2 -> + (replace_both chopped1 p1, replace_both chopped2 p2) + | Some chopped1, None -> + (replace_both chopped1 p1, Typ.Procname.replace_parameters [] p2) + | None, Some chopped2 -> + (Typ.Procname.replace_parameters [] p1, replace_both chopped2 p2) + | _ -> + (p1, p2) + in + Typ.Procname.compare p1 p2 + + let compare x y = let r = LocalAccessPath.compare x.receiver y.receiver in - if r <> 0 then r else Typ.Procname.compare x.procname y.procname + if r <> 0 then r else compare_procname x.procname y.procname end) (* TODO: add return type && and do not add return method to the set *) diff --git a/infer/src/checkers/LithoDomain.mli b/infer/src/checkers/LithoDomain.mli index bb5b2ccc4..a1fe12678 100644 --- a/infer/src/checkers/LithoDomain.mli +++ b/infer/src/checkers/LithoDomain.mli @@ -21,6 +21,8 @@ module LocalAccessPath : sig val pp : F.formatter -> t -> unit end +val suffixes : String.Set.t + (** Called procedure & location + its receiver *) module MethodCall : sig type t = private {receiver: LocalAccessPath.t; procname: Typ.Procname.t; location: Location.t} diff --git a/infer/src/checkers/RequiredProps.ml b/infer/src/checkers/RequiredProps.ml index 2fc541deb..19d1542cb 100644 --- a/infer/src/checkers/RequiredProps.ml +++ b/infer/src/checkers/RequiredProps.ml @@ -92,8 +92,6 @@ let find_client_component_type call_chain = call_chain -let suffixes = String.Set.of_list ["Attr"; "Dip"; "Px"; "Res"; "Sp"] - let has_prop prop_set prop = let check prop = String.Set.mem prop_set prop @@ -102,7 +100,7 @@ let has_prop prop_set prop = that all @Prop's can be set any of these 6 ways. *) String.Set.exists prop_set ~f:(fun el -> String.chop_prefix el ~prefix:prop - |> Option.exists ~f:(fun suffix -> String.Set.mem suffixes suffix) ) + |> Option.exists ~f:(fun suffix -> String.Set.mem LithoDomain.suffixes suffix) ) in match prop with | Prop prop -> diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java index 34cb8ffa4..8161472cd 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -230,6 +230,26 @@ public class RequiredProps { mResPropComponent.create().build(); } + public void buildPropResInCondOk_FP(boolean b) { + ResPropComponent.Builder builder = mResPropComponent.create(); + if (b) { + builder.propAttr(new Object()); + } else { + builder.propDip(new Object()); + } + builder.build(); + } + + public void buildPropResInCondOneNormalOk_FP(boolean b) { + ResPropComponent.Builder builder = mResPropComponent.create(); + if (b) { + builder.propAttr(new Object()); + } else { + builder.prop(new Object()); + } + builder.build(); + } + public void buildPropVarArgNormalOk() { mVarArgPropComponent.create().props(new ArrayList()).build(); } diff --git a/infer/tests/codetoanalyze/java/litho/ResPropComponent.java b/infer/tests/codetoanalyze/java/litho/ResPropComponent.java index 4e429018e..2a93a1e98 100644 --- a/infer/tests/codetoanalyze/java/litho/ResPropComponent.java +++ b/infer/tests/codetoanalyze/java/litho/ResPropComponent.java @@ -14,7 +14,7 @@ import com.facebook.litho.annotations.ResType; * using @Prop(resType = ..) allows you to set the Prop with any of .propname, .propnameRes, or * .propnameAttr */ -class ResPropComponent extends Component { +public class ResPropComponent extends Component { @Prop(resType = ResType.SOME) Object prop; // implicitly non-optional with resType @@ -23,7 +23,7 @@ class ResPropComponent extends Component { return new Builder(); } - static class Builder extends Component.Builder { + public static class Builder extends Component.Builder { ResPropComponent mResPropComponent; diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index 280e1b526..09a7a18cb 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -2,6 +2,8 @@ codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredPr codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingBothBad():com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent MyLithoComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingOneBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent$Builder MyLithoComponent$Builder.prop1(Object),calls Component$Builder Component$Builder.commonProp(Object),calls Component Component$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingOneInLoopBad(int):void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent$Builder MyLithoComponent$Builder.prop1(Object),calls Component$Builder Component$Builder.commonProp(Object),calls Component Component$Builder.build()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropResInCondOk_FP(boolean):void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop is required for component codetoanalyze.java.litho.ResPropComponent, but is not set before the call to build(),calls ResPropComponent$Builder ResPropComponent.create(),calls ResPropComponent ResPropComponent$Builder.build()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropResInCondOneNormalOk_FP(boolean):void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop is required for component codetoanalyze.java.litho.ResPropComponent, but is not set before the call to build(),calls ResPropComponent$Builder ResPropComponent.create(),calls ResPropComponent ResPropComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropResMissingBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop is required for component codetoanalyze.java.litho.ResPropComponent, but is not set before the call to build(),calls ResPropComponent$Builder ResPropComponent.create(),calls ResPropComponent ResPropComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropVarArgMissingBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [Either @Prop props or @Prop(varArg = prop) is required for component codetoanalyze.java.litho.VarArgPropComponent, but is not set before the call to build(),calls VarArgPropComponent$Builder VarArgPropComponent.create(),calls VarArgPropComponent VarArgPropComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithColumnChildBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop3 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent.create(),calls MyComponent$Builder MyComponent$Builder.prop1(Object),calls Component Component$Builder.build()]