From 073e4db9d75e48e29586012feabc244b19a9ad4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Mon, 30 Sep 2019 06:54:44 -0700 Subject: [PATCH] [required-props] Add support for checking varArgs Summary: `Prop(varArg = myProp) List myPropList` can also be set via `myPropList()` or `myProp()`. Add support for picking up the `varArg` and checking this form of required props. Reviewed By: ngorogiannis Differential Revision: D17571997 fbshipit-source-id: 7956cb972 --- infer/src/checkers/Litho.ml | 61 ++++++++++-- .../java/litho/RequiredProps.java | 92 +++++++++++++++++++ .../tests/codetoanalyze/java/litho/issues.exp | 1 + 3 files changed, 144 insertions(+), 10 deletions(-) diff --git a/infer/src/checkers/Litho.ml b/infer/src/checkers/Litho.ml index 2f162e814..d0c6501a5 100644 --- a/infer/src/checkers/Litho.ml +++ b/infer/src/checkers/Litho.ml @@ -91,6 +91,10 @@ module GraphQLGetters = struct end module RequiredProps = struct + (* VarProp is only for props that have a varArg parameter like + @Prop(varArg = "var_prop") whereas Prop is for everything except. *) + type required_prop = Prop of string | VarProp of {prop: string; var_prop: string} + let get_required_props typename tenv = let is_required annot_list = List.exists @@ -106,19 +110,47 @@ module RequiredProps = struct parameters) ) annot_list in + let get_var_args annot_list = + List.fold ~init:None + ~f:(fun acc (({Annot.parameters} as annot), _) -> + if Annotations.annot_ends_with annot Annotations.prop then + (* Pick up the parameter for varArg if it has the form + @Prop(varArg = myProp). *) + List.fold ~init:acc + ~f:(fun acc Annot.{name; value} -> + if Option.value_map name ~default:false ~f:(fun name -> String.equal "varArg" name) + then Some value + else acc ) + parameters + else acc ) + annot_list + in match Tenv.lookup tenv typename with | Some {fields} -> List.filter_map ~f:(fun (fieldname, _, annot) -> - if is_required annot then Some (Typ.Fieldname.Java.get_field fieldname) else None ) + if is_required annot then + let prop = Typ.Fieldname.Java.get_field fieldname in + let var_prop_opt = get_var_args annot in + Some + (Option.value_map var_prop_opt ~default:(Prop prop) ~f:(fun var_prop -> + VarProp {var_prop; prop} )) + else None ) fields | None -> [] - let report_missing_required_prop summary prop_string parent_typename loc = + let report_missing_required_prop summary prop parent_typename loc = let message = - F.asprintf "@Prop %s is required for component %s, but is not set before the call to build()" + let prop_string = + match prop with + | Prop prop -> + F.asprintf "@Prop %s" prop + | VarProp {var_prop; prop} -> + F.asprintf "Either @Prop %s or @Prop(varArg = %s)" prop var_prop + in + F.asprintf "%s is required for component %s, but is not set before the call to build()" prop_string (Typ.Name.name parent_typename) in let ltr = [Errlog.make_trace_element 0 loc message []] in @@ -148,13 +180,22 @@ module RequiredProps = struct let suffixes = String.Set.of_list ["Attr"; "Dip"; "Px"; "Res"; "Sp"] let has_prop prop_set prop = - String.Set.mem prop_set prop - (* @Prop(resType = ...) myProp can also be set via myProp(), myPropAttr(), myPropDip(), myPropPx(), myPropRes() or myPropSp(). - Our annotation parameter parsing is too primitive to identify resType, so just assume - 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) ) + let check prop = + String.Set.mem prop_set prop + || (* @Prop(resType = ...) myProp can also be set via myProp(), myPropAttr(), myPropDip(), myPropPx(), myPropRes() or myPropSp(). + Our annotation parameter parsing is too primitive to identify resType, so just assume + 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) ) + in + match prop with + | Prop prop -> + check prop + | VarProp {var_prop; prop} -> + (* @Prop(varArg = myProp) List myPropList can also be set + via myPropList() or myProp().*) + check var_prop || check prop let report astate tenv summary = diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java index 09efb32a1..d77a64972 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -13,6 +13,8 @@ import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.util.ArrayList; +import java.util.List; enum ResType { SOME, @@ -25,6 +27,8 @@ enum ResType { ResType resType() default ResType.NONE; boolean optional() default false; + + String varArg() default ""; } @Target({ElementType.PARAMETER, ElementType.FIELD}) @@ -46,6 +50,7 @@ class MyComponent extends Component { Object nonProp; + public Builder create() { return new Builder(); } @@ -150,10 +155,77 @@ class ResPropComponent extends Component { } } +/** varArg test */ +class VarArgPropComponent extends Component { + + @Prop(varArg = "prop") + List props; + + public Builder create() { + return new Builder(); + } + + static class Builder extends Component.Builder { + + VarArgPropComponent mVarArgPropComponent; + + public Builder prop(Object prop) { + if (prop == null) { + return this; + } + if (this.mVarArgPropComponent.props == null) { + this.mVarArgPropComponent.props = new ArrayList(); + } + this.mVarArgPropComponent.props.add(prop); + return this; + } + + public Builder propAttr(Object prop) { + if (prop == null) { + return this; + } + if (this.mVarArgPropComponent.props == null) { + this.mVarArgPropComponent.props = new ArrayList(); + } + this.mVarArgPropComponent.props.add(prop); + return this; + } + + public Builder propsAttr(List props) { + if (props == null) { + return this; + } + if (this.mVarArgPropComponent.props == null || this.mVarArgPropComponent.props.isEmpty()) { + this.mVarArgPropComponent.props = props; + } else { + this.mVarArgPropComponent.props.addAll(props); + } + return this; + } + + public Builder props(List props) { + if (props == null) { + return this; + } + if (this.mVarArgPropComponent.props == null || this.mVarArgPropComponent.props.isEmpty()) { + this.mVarArgPropComponent.props = props; + } else { + this.mVarArgPropComponent.props.addAll(props); + } + return this; + } + + public VarArgPropComponent build() { + return mVarArgPropComponent; + } + } +} + public class RequiredProps { public MyComponent mMyComponent; public ResPropComponent mResPropComponent; + public VarArgPropComponent mVarArgPropComponent; public MyComponent buildWithAllOk() { return mMyComponent @@ -275,6 +347,26 @@ public class RequiredProps { mResPropComponent.create().build(); } + public void buildPropVarArgNormalOk() { + mVarArgPropComponent.create().props(new ArrayList()).build(); + } + + public void buildPropVarArgElementOk() { + mVarArgPropComponent.create().prop(new Object()).build(); + } + + public void buildPropVarArgAttrElementOk() { + mVarArgPropComponent.create().propAttr(new Object()).build(); + } + + public void buildPropVarArgNormalAttrElementOk() { + mVarArgPropComponent.create().propsAttr(new ArrayList()).build(); + } + + public void buildPropVarArgMissingBad() { + mVarArgPropComponent.create().build(); + } + public class NonRequiredTreeProps { public MyTreeComponent mMyTreeComponent; diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index 00ed47514..5a3accddb 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -1,5 +1,6 @@ codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.FP_setRequiredOnBothBranchesOk(boolean):codetoanalyze.java.litho.MyComponent, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to 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()] +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()] 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()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithout1Bad():codetoanalyze.java.litho.MyComponent, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithout3Bad():codetoanalyze.java.litho.MyComponent, 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()]