[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
master
Sam Blackshear 7 years ago committed by Facebook Github Bot
parent c98bc53ede
commit 4545f36875

@ -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, _) ->

@ -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 *)

@ -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"

@ -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

@ -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 *)
let receiver = Domain.LocalAccessPath.make receiver_ap caller_pname in
if is_graphql_getter callee_procname summary || Domain.mem receiver astate then
(* TODO: we should probably track all calls rooted in formals as well *)
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

@ -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 ->

@ -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

@ -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

@ -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 {
}
}

@ -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());
}
}

@ -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()]

Loading…
Cancel
Save