From c9f3e20fc4a21dc9a04ede945391ccc2ae55f92d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Wed, 9 Oct 2019 03:14:07 -0700 Subject: [PATCH] [required-props] Add more tests showing the ineffectiveness of callee heuristic Summary: As a heuristic, litho library calls on non-litho callers are not tracked. This is very imprecise and results in FPs and FNs as exemplified by newly added tests. Instead, we should check to see if the summary contains a `build()` method as will be done in the next diff. This diff adds these tests and refactors the test code. Reviewed By: skcho Differential Revision: D17809536 fbshipit-source-id: 6dff1868c --- .../codetoanalyze/java/litho/MyComponent.java | 54 +++++ .../java/litho/MyLithoComponent.java | 43 ++++ .../java/litho/MyTreeComponent.java | 38 +++ .../java/litho/RequiredProps.java | 229 ++---------------- .../java/litho/ResPropComponent.java | 69 ++++++ .../java/litho/VarArgPropComponent.java | 83 +++++++ .../tests/codetoanalyze/java/litho/issues.exp | 4 +- 7 files changed, 313 insertions(+), 207 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/litho/MyComponent.java create mode 100644 infer/tests/codetoanalyze/java/litho/MyLithoComponent.java create mode 100644 infer/tests/codetoanalyze/java/litho/MyTreeComponent.java create mode 100644 infer/tests/codetoanalyze/java/litho/ResPropComponent.java create mode 100644 infer/tests/codetoanalyze/java/litho/VarArgPropComponent.java diff --git a/infer/tests/codetoanalyze/java/litho/MyComponent.java b/infer/tests/codetoanalyze/java/litho/MyComponent.java new file mode 100644 index 000000000..6bb4e7c86 --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/MyComponent.java @@ -0,0 +1,54 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.litho; + +import com.facebook.litho.Component; +import com.facebook.litho.annotations.Prop; + +public class MyComponent extends Component { + @Prop Object prop1; // implicitly non-optional + + @Prop(optional = true) + Object prop2; // explicitly optional + + @Prop(optional = false) + Object prop3; // explicitly non-optional + + Object nonProp; + + public Builder create() { + return new Builder(); + } + + public 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; + } + + @Override + public Builder getThis() { + return this; + } + } +} diff --git a/infer/tests/codetoanalyze/java/litho/MyLithoComponent.java b/infer/tests/codetoanalyze/java/litho/MyLithoComponent.java new file mode 100644 index 000000000..a4acb10e5 --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/MyLithoComponent.java @@ -0,0 +1,43 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package com.facebook.litho; + +import com.facebook.litho.annotations.Prop; + +public class MyLithoComponent extends Component { + @Prop Object prop1; // implicitly non-optional + + @Prop(optional = false) + Object prop2; // explicitly non-optional + + public Builder create() { + return new Builder(); + } + + public static class Builder extends Component.Builder { + MyLithoComponent mMyLithoComponent; + + public Builder prop1(Object o) { + this.mMyLithoComponent.prop1 = o; + return this; + } + + public Builder prop2(Object o) { + this.mMyLithoComponent.prop2 = o; + return this; + } + + public MyLithoComponent build() { + return mMyLithoComponent; + } + + @Override + public Builder getThis() { + return this; + } + } +} diff --git a/infer/tests/codetoanalyze/java/litho/MyTreeComponent.java b/infer/tests/codetoanalyze/java/litho/MyTreeComponent.java new file mode 100644 index 000000000..addc1d849 --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/MyTreeComponent.java @@ -0,0 +1,38 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.litho; + +import com.facebook.litho.Component; +import com.facebook.litho.annotations.TreeProp; + +class MyTreeComponent extends Component { + @TreeProp Object prop1; // implicitly non-optional + + Object nonProp; + + public Builder create() { + return new Builder(); + } + + static class Builder extends Component.Builder { + MyTreeComponent mMyTreeComponent; + + public Builder prop1(Object o) { + this.mMyTreeComponent.prop1 = o; + return this; + } + + public MyTreeComponent build() { + return mMyTreeComponent; + } + + @Override + public Builder getThis() { + return this; + } + } +} diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java index b6218feba..6b0713b9f 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -9,217 +9,13 @@ package codetoanalyze.java.litho; import com.facebook.litho.Column; import com.facebook.litho.Component; -import com.facebook.litho.annotations.Prop; -import com.facebook.litho.annotations.ResType; -import com.facebook.litho.annotations.TreeProp; +import com.facebook.litho.MyLithoComponent; import java.util.ArrayList; -import java.util.List; - -class MyComponent extends Component { - @Prop Object prop1; // implicitly non-optional - - @Prop(optional = true) - Object prop2; // explicitly optional - - @Prop(optional = false) - Object prop3; // explicitly non-optional - - 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; - } - - @Override - public Builder getThis() { - return this; - } - } -} - -class MyTreeComponent extends Component { - @TreeProp Object prop1; // implicitly non-optional - - Object nonProp; - - public Builder create() { - return new Builder(); - } - - static class Builder extends Component.Builder { - MyTreeComponent mMyTreeComponent; - - public Builder prop1(Object o) { - this.mMyTreeComponent.prop1 = o; - return this; - } - - public MyTreeComponent build() { - return mMyTreeComponent; - } - - @Override - public Builder getThis() { - return this; - } - } -} - -/** - * using @Prop(resType = ..) allows you to set the Prop with any of .propname, .propnameRes, or - * .propnameAttr - */ -class ResPropComponent extends Component { - - @Prop(resType = ResType.SOME) - Object prop; // implicitly non-optional with resType - - public Builder create() { - return new Builder(); - } - - static class Builder extends Component.Builder { - - ResPropComponent mResPropComponent; - - public Builder prop(Object o) { - this.mResPropComponent.prop = o; - return this; - } - - public Builder propRes(Object o) { - this.mResPropComponent.prop = o; - return this; - } - - public Builder propAttr(Object o) { - this.mResPropComponent.prop = o; - return this; - } - - public Builder propDip(Object o) { - this.mResPropComponent.prop = o; - return this; - } - - public Builder propPx(Object o) { - this.mResPropComponent.prop = o; - return this; - } - - public Builder propSp(Object o) { - this.mResPropComponent.prop = o; - return this; - } - - public ResPropComponent build() { - return mResPropComponent; - } - - @Override - public Builder getThis() { - return this; - } - } -} - -/** 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; - } - - @Override - public Builder getThis() { - return this; - } - } -} public class RequiredProps { public MyComponent mMyComponent; + public MyLithoComponent mMyLithoComponent; public ResPropComponent mResPropComponent; public VarArgPropComponent mVarArgPropComponent; @@ -383,6 +179,27 @@ public class RequiredProps { mVarArgPropComponent.create().build(); } + // can't track MyLithoComponent.build() because it is on litho, hence we miss the error + public Component buildPropLithoMissingBothBad_FN() { + return mMyLithoComponent.create().build(); + } + + // we pick up Component.build() rather than + // MyLithoComponent.build(). Hence, we can't detect that it has + // missing props at all. + public void buildPropLithoMissingOneBad_FN() { + Column.create() + .child(mMyLithoComponent.create().prop1(new Object()).commonProp(new Object())) + .build(); + } + + // We can't track prop1 and prop2 because they are on litho + public Component buildPropLithoOK_FP() { + Component.Builder layoutBuilder = + mMyLithoComponent.create().prop1(new Object()).prop2(new Object()); + return layoutBuilder.build(); + } + public class NonRequiredTreeProps { public MyTreeComponent mMyTreeComponent; diff --git a/infer/tests/codetoanalyze/java/litho/ResPropComponent.java b/infer/tests/codetoanalyze/java/litho/ResPropComponent.java new file mode 100644 index 000000000..4e429018e --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/ResPropComponent.java @@ -0,0 +1,69 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.litho; + +import com.facebook.litho.Component; +import com.facebook.litho.annotations.Prop; +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 { + + @Prop(resType = ResType.SOME) + Object prop; // implicitly non-optional with resType + + public Builder create() { + return new Builder(); + } + + static class Builder extends Component.Builder { + + ResPropComponent mResPropComponent; + + public Builder prop(Object o) { + this.mResPropComponent.prop = o; + return this; + } + + public Builder propRes(Object o) { + this.mResPropComponent.prop = o; + return this; + } + + public Builder propAttr(Object o) { + this.mResPropComponent.prop = o; + return this; + } + + public Builder propDip(Object o) { + this.mResPropComponent.prop = o; + return this; + } + + public Builder propPx(Object o) { + this.mResPropComponent.prop = o; + return this; + } + + public Builder propSp(Object o) { + this.mResPropComponent.prop = o; + return this; + } + + public ResPropComponent build() { + return mResPropComponent; + } + + @Override + public Builder getThis() { + return this; + } + } +} diff --git a/infer/tests/codetoanalyze/java/litho/VarArgPropComponent.java b/infer/tests/codetoanalyze/java/litho/VarArgPropComponent.java new file mode 100644 index 000000000..89a696619 --- /dev/null +++ b/infer/tests/codetoanalyze/java/litho/VarArgPropComponent.java @@ -0,0 +1,83 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.litho; + +import com.facebook.litho.Component; +import com.facebook.litho.annotations.Prop; +import java.util.ArrayList; +import java.util.List; + +/** 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; + } + + @Override + public Builder getThis() { + return this; + } + } +} diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index 0bc4ce95d..d07720a5f 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -1,11 +1,13 @@ codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.FP_setRequiredOnBothBranchesOk(boolean):com.facebook.litho.Component, 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(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoOK_FP():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 MyLithoComponent$Builder.build()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoOK_FP():com.facebook.litho.Component, 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 MyLithoComponent$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 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 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$Builder.prop1(Object),calls Component Component$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithout1Bad():com.facebook.litho.Component, 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(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithout3Bad():com.facebook.litho.Component, 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$Builder.prop1(Object),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.callBuildSuffixWithoutRequiredBad():com.facebook.litho.Component, 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(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] -codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.generalTypeForgot3Bad():java.lang.Object, 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$Builder.prop1(Object),calls MyComponent MyComponent$Builder.build()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.generalTypeForgot3Bad():java.lang.Object, 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$Builder.prop1(Object),calls Component MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setProp3InCalleeButForgetProp1Bad():com.facebook.litho.Component, 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(),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnOneBranchBad(boolean):com.facebook.litho.Component, 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(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/ShouldUpdate.java, com.facebook.graphql.model.LithoTest.onCreateLayout():void, 0, GRAPHQL_FIELD_ACCESS, no_bucket, ERROR, [story.getActors()]