From 4545f36875cca2383bede4cc197087b8986afeea Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 23 Jan 2018 10:37:49 -0800 Subject: [PATCH] [litho] prototype of required `@Prop` checker Summary: At each call to `Component$Builder.build()`, checks that the required props for `Component` have been set via prior calls to the `Builder`. Does not yet handle `Prop(optional = true)`, but will address that in a follow-up. Reviewed By: jeremydubreil Differential Revision: D6735524 fbshipit-source-id: 0c812fd --- infer/src/IR/Typ.ml | 2 +- infer/src/IR/Typ.mli | 2 +- infer/src/base/IssueType.ml | 2 + infer/src/base/IssueType.mli | 2 + infer/src/checkers/Litho.ml | 131 ++++++++++--- infer/src/checkers/LithoDomain.ml | 2 - infer/src/checkers/annotations.ml | 4 + infer/src/checkers/annotations.mli | 2 + .../codetoanalyze/java/litho/Component.java | 16 ++ .../java/litho/RequiredProps.java | 179 ++++++++++++++++++ .../tests/codetoanalyze/java/litho/issues.exp | 7 + 11 files changed, 319 insertions(+), 30 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/litho/Component.java create mode 100644 infer/tests/codetoanalyze/java/litho/RequiredProps.java diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index 235535633..1c24cc93f 100644 --- a/infer/src/IR/Typ.ml +++ b/infer/src/IR/Typ.ml @@ -409,7 +409,7 @@ module Name = struct let split_typename typename = split_classname (name typename) - let get_parent_class class_name = + let get_outer_class class_name = let package_name, class_name_no_package = split_typename class_name in match String.rsplit2 ~on:'$' class_name_no_package with | Some (parent_class, _) -> diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index d11a23be1..7d4b92a88 100644 --- a/infer/src/IR/Typ.mli +++ b/infer/src/IR/Typ.mli @@ -163,7 +163,7 @@ module Name : sig (** Given a package.class_name string, look for the latest dot and split the string in two (package, class_name). *) - val get_parent_class : t -> t option + val get_outer_class : t -> t option (** Given an inner classname like C$Inner1$Inner2, return Some C$Inner1. If the class is not an inner class, return None *) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 6a3d73d86..e2e9b8c85 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -267,6 +267,8 @@ let memory_leak = from_string "MEMORY_LEAK" let missing_fld = from_string "Missing_fld" ~hum:"Missing Field" +let missing_required_prop = from_string "MISSING_REQUIRED_PROP" + let null_dereference = from_string "NULL_DEREFERENCE" let null_test_after_dereference = from_string ~enabled:false "NULL_TEST_AFTER_DEREFERENCE" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 1931b03f7..fdde249bb 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -184,6 +184,8 @@ val memory_leak : t val missing_fld : t +val missing_required_prop : t + val null_dereference : t val null_test_after_dereference : t diff --git a/infer/src/checkers/Litho.ml b/infer/src/checkers/Litho.ml index 07c7fdf1b..336a26497 100644 --- a/infer/src/checkers/Litho.ml +++ b/infer/src/checkers/Litho.ml @@ -22,11 +22,37 @@ module Summary = Summary.Make (struct let read_payload (summary: Specs.summary) = summary.payload.litho end) +module LithoFramework = struct + let is_component_builder procname tenv = + match procname with + | Typ.Procname.Java java_procname -> + PatternMatch.is_subtype_of_str tenv + (Typ.Procname.Java.get_class_type_name java_procname) + "com.facebook.litho.Component$Builder" + | _ -> + false + + + let is_component_build_method procname tenv = + match Typ.Procname.get_method procname with + | "build" -> + is_component_builder procname tenv + | _ -> + false + + + let is_on_create_layout = function + | Typ.Procname.Java java_pname -> ( + match Typ.Procname.Java.get_method java_pname with "onCreateLayout" -> true | _ -> false ) + | _ -> + false +end + module TransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG module Domain = Domain - type extras = ProcData.no_extras + type extras = Specs.summary let is_graphql_getter procname summary = Option.is_none summary @@ -67,26 +93,95 @@ module TransferFunctions (CFG : ProcCfg.S) = struct assert false else None in - Domain.substitute ~f_sub summary + Domain.substitute ~f_sub summary |> Domain.join astate | None -> astate + let get_required_props typename tenv = + match Tenv.lookup tenv typename with + | Some {fields} -> + List.filter_map + ~f:(fun (fieldname, _, annot) -> + if Annotations.ia_is_prop annot then Some (Typ.Fieldname.Java.get_field fieldname) + else None ) + fields + | None -> + [] + + + let report_missing_required_prop summary prop_string loc = + let message = + F.asprintf "@Prop %s is required, but not set before the call to build()" prop_string + in + let exn = + Exceptions.Checkers (IssueType.missing_required_prop, Localise.verbatim_desc message) + in + let ltr = [Errlog.make_trace_element 0 loc message []] in + Reporting.log_error summary ~loc ~ltr exn + + + let check_required_props receiver_ap astate callee_procname caller_procname tenv summary loc = + match callee_procname with + | Typ.Procname.Java java_pname + -> ( + (* Here, we'll have a type name like MyComponent$Builder in hand. Truncate the $Builder + part from the typename, then look at the fields of MyComponent to figure out which + ones are annotated with @Prop *) + let typename = Typ.Procname.Java.get_class_type_name java_pname in + match Typ.Name.Java.get_outer_class typename with + | Some outer_class_typename -> + let required_props = get_required_props outer_class_typename tenv in + let receiver = Domain.LocalAccessPath.make receiver_ap caller_procname in + let method_call = Domain.MethodCall.make receiver callee_procname in + let f _ prop_setter_calls = + (* See if there's a required prop whose setter wasn't called *) + let prop_set = + List.fold prop_setter_calls + ~f:(fun acc pname -> String.Set.add acc (Typ.Procname.get_method pname)) + ~init:String.Set.empty in + List.iter + ~f:(fun required_prop -> + if not (String.Set.mem prop_set required_prop) then + report_missing_required_prop summary required_prop loc ) + required_props + in + (* check every chain ending in [build()] to make sure that required props are passed + on all of them *) + Domain.iter_call_chains_with_suffix ~f method_call astate + | None -> + () ) + | _ -> + () + + let exec_instr astate (proc_data: extras ProcData.t) _ (instr: HilInstr.t) : Domain.astate = let caller_pname = Procdesc.get_proc_name proc_data.pdesc in match instr with | Call ( (Some return_base as ret_opt) - , Direct callee_procname + , Direct (Typ.Procname.Java java_callee_procname as callee_procname) , ((HilExp.AccessPath receiver_ap) :: _ as actuals) , _ - , _ ) -> + , loc ) -> + if LithoFramework.is_component_build_method callee_procname proc_data.tenv then + (* call to Component$Builder.build(); check that all required props are passed *) + (* TODO: only check when the root of the call chain is <: Component? otherwise, + methods that call build() but implicitly have a precondition of "add a required prop" + will be wrongly flagged *) + check_required_props receiver_ap astate callee_procname caller_pname proc_data.tenv + proc_data.extras loc ; let summary = Summary.read_summary proc_data.pdesc callee_procname in - (* track the call if the callee is a graphql getter *or* the receiver is already tracked *) - (* TODO: we should probably track all formals as well *) + (* TODO: we should probably track all calls rooted in formals as well *) let receiver = Domain.LocalAccessPath.make receiver_ap caller_pname in - if is_graphql_getter callee_procname summary || Domain.mem receiver astate then - let receiver = Domain.LocalAccessPath.make receiver_ap caller_pname in + if ( LithoFramework.is_component_builder callee_procname proc_data.tenv + (* track Builder's in order to check required prop's *) + || is_graphql_getter callee_procname summary + || (* track GraphQL getters in order to report graphql field accesses *) + Domain.mem receiver astate + (* track anything called on a receiver we're already tracking *) ) + && not (Typ.Procname.Java.is_static java_callee_procname) + then let return_access_path = Domain.LocalAccessPath.make (return_base, []) caller_pname in let return_calls = (try Domain.find return_access_path astate with Not_found -> Domain.CallSet.empty) @@ -116,23 +211,7 @@ end module Analyzer = LowerHil.MakeAbstractInterpreter (ProcCfg.Exceptional) (TransferFunctions) -let report access_path call_chain summary = - let call_strings = List.map ~f:(Typ.Procname.to_simplified_string ~withclass:false) call_chain in - let call_string = String.concat ~sep:"." call_strings in - let message = F.asprintf "%a.%s" AccessPath.pp access_path call_string in - let exn = Exceptions.Checkers (IssueType.graphql_field_access, Localise.verbatim_desc message) in - let loc = Specs.get_loc summary in - let ltr = [Errlog.make_trace_element 0 loc message []] in - Reporting.log_error summary ~loc ~ltr exn - - -let should_report proc_desc = - match Procdesc.get_proc_name proc_desc with - | Typ.Procname.Java java_pname -> ( - match Typ.Procname.Java.get_method java_pname with "onCreateLayout" -> true | _ -> false ) - | _ -> - false - +let should_report proc_desc = LithoFramework.is_on_create_layout (Procdesc.get_proc_name proc_desc) let report_graphql_getters summary access_path call_chain = let call_strings = List.map ~f:(Typ.Procname.to_simplified_string ~withclass:false) call_chain in @@ -151,7 +230,7 @@ let postprocess astate proc_desc : Domain.astate = let checker {Callbacks.summary; proc_desc; tenv} = - let proc_data = ProcData.make_default proc_desc tenv in + let proc_data = ProcData.make proc_desc tenv summary in match Analyzer.compute_post proc_data ~initial:Domain.empty with | Some post -> ( if should_report proc_desc then diff --git a/infer/src/checkers/LithoDomain.ml b/infer/src/checkers/LithoDomain.ml index e494ff3b6..fcfce3918 100644 --- a/infer/src/checkers/LithoDomain.ml +++ b/infer/src/checkers/LithoDomain.ml @@ -63,7 +63,6 @@ let substitute ~(f_sub: LocalAccessPath.t -> LocalAccessPath.t option) astate = add access_path' call_set' acc ) astate empty - let iter_call_chains_with_suffix ~f call_suffix astate = let max_depth = cardinal astate in let rec unroll_call_ ({receiver; procname}: MethodCall.t) (acc, depth) = @@ -84,7 +83,6 @@ let iter_call_chains_with_suffix ~f call_suffix astate = in unroll_call_ call_suffix ([], 0) - let iter_call_chains ~f astate = iter (fun _ call_set -> diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 62bbd5bde..c4f364e35 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -75,6 +75,8 @@ let performance_critical = "PerformanceCritical" let present = "Present" +let prop = "Prop" + let propagates_nullable = "PropagatesNullable" let returns_ownership = "ReturnsOwnership" @@ -166,6 +168,8 @@ let ia_is_nullable ia = ia_ends_with ia nullable || ia_is_propagates_nullable ia let ia_is_present ia = ia_ends_with ia present +let ia_is_prop ia = ia_ends_with ia prop + let ia_is_nonnull ia = List.exists ~f:(ia_ends_with ia) [nonnull; notnull; camel_nonnull] let ia_is_false_on_null ia = ia_ends_with ia false_on_null diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index ea23bd15d..c5addf393 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -94,6 +94,8 @@ val ia_is_on_bind : Annot.Item.t -> bool val ia_is_on_mount : Annot.Item.t -> bool +val ia_is_prop : Annot.Item.t -> bool + val ia_is_on_unbind : Annot.Item.t -> bool val ia_is_on_unmount : Annot.Item.t -> bool diff --git a/infer/tests/codetoanalyze/java/litho/Component.java b/infer/tests/codetoanalyze/java/litho/Component.java new file mode 100644 index 000000000..cfdc71fe9 --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/Component.java @@ -0,0 +1,16 @@ +/* + * Copyright (c) 2018 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ +package com.facebook.litho; + +public class Component { + + public abstract static class Builder { + + } +} diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java new file mode 100644 index 000000000..557e2633f --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -0,0 +1,179 @@ +/* + * Copyright (c) 2018 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ +import com.facebook.litho.Component; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +enum ResType { + SOME, + NONE +} + +@Target({ ElementType.PARAMETER, ElementType.FIELD }) +@Retention(RetentionPolicy.CLASS) +@interface Prop { + ResType resType(); + boolean optional() default false; +} + +class MyComponent extends Component { + @Prop(resType = ResType.NONE, optional = false) + Object prop1; + + @Prop(resType = ResType.NONE, optional = true) + Object prop2; + + @Prop(resType = ResType.SOME, optional = false) + Object prop3; + + Object nonProp; + + public Builder create() { + return new Builder(); + } + + static class Builder extends Component.Builder { + MyComponent mMyComponent; + + public Builder prop1(Object o) { + this.mMyComponent.prop1 = o; + return this; + } + + public Builder prop2(Object o) { + this.mMyComponent.prop2 = o; + return this; + } + + public Builder prop3(Object o) { + this.mMyComponent.prop3 = o; + return this; + } + + public MyComponent build() { + return mMyComponent; + } + + } + +} + +public class RequiredProps { + + public MyComponent mMyComponent; + + public MyComponent buildWithAllOk() { + return + mMyComponent + .create() + .prop1(new Object()) + .prop2(new Object()) + .prop3(new Object()) + .build(); + } + + // need to parse optional boolean for this to work + public MyComponent FP_buildWithRequiredOk() { + return + mMyComponent + .create() + .prop1(new Object()) + .prop3(new Object()) + .build(); + } + + public MyComponent buildWithout1Bad() { + return + mMyComponent + .create() + .prop2(new Object()) + .prop3(new Object()) + .build(); + } + + public MyComponent buildWithout2Bad() { + return + mMyComponent + .create() + .prop1(new Object()) + .prop3(new Object()) + .build(); + } + + private static MyComponent.Builder setProp1(MyComponent.Builder builder) { + return builder.prop1(new Object()); + } + + private static MyComponent.Builder setProp3(MyComponent.Builder builder) { + return builder.prop3(new Object()); + } + + public MyComponent setProp1InCalleeOk() { + return + setProp1( + mMyComponent + .create() + .prop2(new Object())) + .prop3(new Object()) + .build(); + } + + public MyComponent setProp3InCalleeOk() { + return + setProp3( + mMyComponent + .create() + .prop1(new Object()) + .prop2(new Object())) + .build(); + } + + public MyComponent setProp3InCalleeButForgetProp1Bad() { + return + setProp3(mMyComponent.create()) + .prop2(new Object()) + .build(); + } + + public MyComponent setRequiredOnOneBranchBad(boolean b) { + MyComponent.Builder builder = mMyComponent.create(); + if (b) { + builder = builder.prop1(new Object()); + } + return builder.prop2(new Object()).prop3(new Object()).build(); + } + + public MyComponent FP_setRequiredOnBothBranchesOk(boolean b) { + MyComponent.Builder builder = mMyComponent.create(); + if (b) { + builder = builder.prop1(new Object()); + } else { + builder = builder.prop1(new Object()); + } + return builder.prop2(new Object()).prop3(new Object()).build(); + } + + // we report every time we see [build()], need to be a bit smarter in general + private MyComponent FP_buildSuffix(MyComponent.Builder builder) { + return builder.prop2(new Object()).prop3(new Object()).build(); + } + + // shouldn't report here + public MyComponent callBuildSuffixWithRequiredOk() { + return FP_buildSuffix(mMyComponent.create().prop1(new Object())); + } + + // should report here + public MyComponent FN_callBuildSuffixWithoutRequiredBad() { + return FP_buildSuffix(mMyComponent.create()); + } + +} diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index 474bbe8b6..088889184 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -1,3 +1,10 @@ +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.FP_buildSuffix(MyComponent$Builder), 1, MISSING_REQUIRED_PROP, [@Prop prop1 is required, but not set before the call to build()] +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.FP_buildWithRequiredOk(), 6, MISSING_REQUIRED_PROP, [@Prop prop2 is required, but not set before the call to build()] +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.FP_setRequiredOnBothBranchesOk(boolean), 7, MISSING_REQUIRED_PROP, [@Prop prop1 is required, but not set before the call to build()] +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.buildWithout1Bad(), 6, MISSING_REQUIRED_PROP, [@Prop prop1 is required, but not set before the call to build()] +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.buildWithout2Bad(), 6, MISSING_REQUIRED_PROP, [@Prop prop2 is required, but not set before the call to build()] +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.setProp3InCalleeButForgetProp1Bad(), 4, MISSING_REQUIRED_PROP, [@Prop prop1 is required, but not set before the call to build()] +codetoanalyze/java/litho/RequiredProps.java, MyComponent RequiredProps.setRequiredOnOneBranchBad(boolean), 5, MISSING_REQUIRED_PROP, [@Prop prop1 is required, but not set before the call to build()] codetoanalyze/java/litho/ShouldUpdate.java, void LithoTest.onCreateLayout(), 0, GRAPHQL_FIELD_ACCESS, [&story.getActors().get(...)] codetoanalyze/java/litho/ShouldUpdate.java, void LithoTest.onCreateLayout(), 0, GRAPHQL_FIELD_ACCESS, [&story.getActors().get(...).toString()] codetoanalyze/java/litho/ShouldUpdate.java, void LithoTest.onCreateLayout(), 0, GRAPHQL_FIELD_ACCESS, [&story.getActors().size()]