From 2a0f5240bc8c711d8e45db0e5426517f8bfbf49a Mon Sep 17 00:00:00 2001 From: Aziz Murtazaev <aziz@fb.com> Date: Wed, 12 Jun 2019 10:06:46 -0700 Subject: [PATCH] Fix CachedValue bug when OnCalculateCachedValue has no params Summary: When no param is passed to method annotated with `OnCalculateCachedValue` we were not generating `hashCode` for the equivalent input class. Some context about how `CachedValue` works: We generate a plain data class for each method annotated with `OnCalculateCachedValue` that encapsulates params and use that as key for cachedValues map in `ComponentTree`. If there was no param, we were skipping generating `hashCode`, therefore we couldn't retrieve cached value in other lifecycle methods, therefore always recreating new one. Fix is to generate hashcode with class type when method has no param. Reviewed By: topwu Differential Revision: D15781618 fbshipit-source-id: 656bd7d81f006aa80f98e2d4a07a19babfdebce1 --- .../model/CachedValueGeneratorTest.java | 199 ++++++++++++++++++ .../generator/CachedValueGenerator.java | 25 +-- 2 files changed, 212 insertions(+), 12 deletions(-) create mode 100644 litho-it/src/test/java/com/facebook/litho/specmodels/model/CachedValueGeneratorTest.java diff --git a/litho-it/src/test/java/com/facebook/litho/specmodels/model/CachedValueGeneratorTest.java b/litho-it/src/test/java/com/facebook/litho/specmodels/model/CachedValueGeneratorTest.java new file mode 100644 index 00000000000..6508769aad0 --- /dev/null +++ b/litho-it/src/test/java/com/facebook/litho/specmodels/model/CachedValueGeneratorTest.java @@ -0,0 +1,199 @@ +/* + * Copyright 2014-present Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.litho.specmodels.model; + +import static org.assertj.core.api.Java6Assertions.assertThat; + +import androidx.annotation.Nullable; +import com.facebook.litho.Component; +import com.facebook.litho.Row; +import com.facebook.litho.annotations.LayoutSpec; +import com.facebook.litho.annotations.OnCalculateCachedValue; +import com.facebook.litho.annotations.OnCreateLayout; +import com.facebook.litho.annotations.OnEvent; +import com.facebook.litho.annotations.OnUpdateState; +import com.facebook.litho.annotations.Param; +import com.facebook.litho.annotations.Prop; +import com.facebook.litho.annotations.State; +import com.facebook.litho.annotations.TreeProp; +import com.facebook.litho.specmodels.generator.CachedValueGenerator; +import com.facebook.litho.specmodels.internal.RunMode; +import com.facebook.litho.specmodels.processor.LayoutSpecModelFactory; +import com.google.testing.compile.CompilationRule; +import java.util.List; +import java.util.Set; +import javax.annotation.processing.Messager; +import javax.lang.model.element.TypeElement; +import javax.lang.model.util.Elements; +import javax.lang.model.util.Types; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +/** Tests {@link CachedValueGenerator} */ +@RunWith(JUnit4.class) +public class CachedValueGeneratorTest { + @Rule public CompilationRule mCompilationRule = new CompilationRule(); + @Mock private Messager mMessager; + + private final LayoutSpecModelFactory mLayoutSpecModelFactory = new LayoutSpecModelFactory(); + + private SpecModel mLayoutSpecModel; + + @LayoutSpec + static class CachedValueTestSpec { + + @OnCalculateCachedValue(name = "expensiveValue") + static String onCreateExpensiveValue() { + return "ABC"; + } + + @OnCalculateCachedValue(name = "moreExpensiveValue") + static String onCreateMoreExpensiveValue(@Prop boolean arg0, @State int arg1) { + return "DEF"; + } + + @OnCreateLayout + public void testDelegateMethod( + @Prop boolean arg0, + @Prop @Nullable Component arg1, + @Prop List<Component> arg2, + @Prop List<String> arg3, + @State int arg4, + @Param Object arg5, + @TreeProp long arg6, + @TreeProp Set<List<Row>> arg7, + @TreeProp Set<Integer> arg8) {} + + @OnEvent(Object.class) + public void testEventMethod( + @Prop boolean arg0, + @Prop @Nullable Component arg1, + @State int arg2, + @Param Object arg3, + @TreeProp long arg4) {} + + @OnUpdateState + public void testUpdateStateMethod() {} + } + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + Elements elements = mCompilationRule.getElements(); + Types types = mCompilationRule.getTypes(); + TypeElement typeElement = + elements.getTypeElement( + CachedValueGeneratorTest.CachedValueTestSpec.class.getCanonicalName()); + mLayoutSpecModel = + mLayoutSpecModelFactory.create( + elements, types, typeElement, mMessager, RunMode.normal(), null, null); + } + + @Test + public void testGenerateInputsClassNoParam() { + final List<SpecMethodModel<DelegateMethod, Void>> models = + SpecModelUtils.getMethodModelsWithAnnotation( + mLayoutSpecModel, OnCalculateCachedValue.class); + final SpecMethodModel<DelegateMethod, Void> specMethodModel = + models.stream() + .filter(m -> m.name.toString().equals("onCreateExpensiveValue")) + .findFirst() + .get(); + final String expensiveValueInputsClass = + CachedValueGenerator.createInputsClass(mLayoutSpecModel, specMethodModel, "expensiveValue") + .toString(); + assertThat(expensiveValueInputsClass) + .isEqualTo( + "private static class ExpensiveValueInputs {\n" + + " ExpensiveValueInputs() {\n" + + " }\n" + + "\n" + + " @java.lang.Override\n" + + " public int hashCode() {\n" + + " return com.facebook.litho.CommonUtils.hash(getClass());\n" + + " }\n" + + "\n" + + " @java.lang.Override\n" + + " public boolean equals(java.lang.Object other) {\n" + + " if (this == other) {\n" + + " return true;\n" + + " }\n" + + " if (other == null || !(other instanceof ExpensiveValueInputs)) {\n" + + " return false;\n" + + " }\n" + + " ExpensiveValueInputs cachedValueInputs = (ExpensiveValueInputs) other;\n" + + " return true;\n" + + " }\n" + + "}\n"); + } + + @Test + public void testGenerateInputsClassWithParam() { + final List<SpecMethodModel<DelegateMethod, Void>> models = + SpecModelUtils.getMethodModelsWithAnnotation( + mLayoutSpecModel, OnCalculateCachedValue.class); + final SpecMethodModel<DelegateMethod, Void> specMethodModel = + models.stream() + .filter(m -> m.name.toString().equals("onCreateMoreExpensiveValue")) + .findFirst() + .get(); + final String expensiveValueInputsClass = + CachedValueGenerator.createInputsClass( + mLayoutSpecModel, specMethodModel, "moreExpensiveValue") + .toString(); + assertThat(expensiveValueInputsClass) + .isEqualTo( + "private static class MoreExpensiveValueInputs {\n" + + " private final boolean arg0;\n" + + "\n" + + " private final int arg1;\n" + + "\n" + + " MoreExpensiveValueInputs(boolean arg0, int arg1) {\n" + + " this.arg0 = arg0;\n" + + " this.arg1 = arg1;\n" + + " }\n" + + "\n" + + " @java.lang.Override\n" + + " public int hashCode() {\n" + + " return com.facebook.litho.CommonUtils.hash(arg0, arg1);\n" + + " }\n" + + "\n" + + " @java.lang.Override\n" + + " public boolean equals(java.lang.Object other) {\n" + + " if (this == other) {\n" + + " return true;\n" + + " }\n" + + " if (other == null || !(other instanceof MoreExpensiveValueInputs)) {\n" + + " return false;\n" + + " }\n" + + " MoreExpensiveValueInputs cachedValueInputs = (MoreExpensiveValueInputs) other;\n" + + " if (arg0 != cachedValueInputs.arg0) {\n" + + " return false;\n" + + " }\n" + + " if (arg1 != cachedValueInputs.arg1) {\n" + + " return false;\n" + + " }\n" + + " return true;\n" + + " }\n" + + "}\n"); + } +} diff --git a/litho-processor/src/main/java/com/facebook/litho/specmodels/generator/CachedValueGenerator.java b/litho-processor/src/main/java/com/facebook/litho/specmodels/generator/CachedValueGenerator.java index 8a91018225c..c99f8f543bd 100644 --- a/litho-processor/src/main/java/com/facebook/litho/specmodels/generator/CachedValueGenerator.java +++ b/litho-processor/src/main/java/com/facebook/litho/specmodels/generator/CachedValueGenerator.java @@ -165,7 +165,7 @@ private static MethodSpec createGetterMethod( return methodSpec.build(); } - private static TypeSpec createInputsClass( + public static TypeSpec createInputsClass( SpecModel specModel, SpecMethodModel<DelegateMethod, Void> onCalculateCachedValueMethod, String cachedValueName) { @@ -187,15 +187,15 @@ private static TypeSpec createInputsClass( typeSpec.addMethod(constructor.build()); final int paramSize = onCalculateCachedValueMethod.methodParams.size(); + MethodSpec.Builder hashCodeMethod = + MethodSpec.methodBuilder("hashCode") + .addAnnotation(Override.class) + .returns(TypeName.INT) + .addModifiers(Modifier.PUBLIC); + + CodeBlock.Builder codeBlock = + CodeBlock.builder().add("return $T.hash(", ClassNames.COMMON_UTILS); if (paramSize > 0) { - MethodSpec.Builder hashCodeMethod = - MethodSpec.methodBuilder("hashCode") - .addAnnotation(Override.class) - .returns(TypeName.INT) - .addModifiers(Modifier.PUBLIC); - - CodeBlock.Builder codeBlock = - CodeBlock.builder().add("return $T.hash(", ClassNames.COMMON_UTILS); for (int i = 0; i < paramSize; i++) { if (i < paramSize - 1) { codeBlock.add("$L, ", onCalculateCachedValueMethod.methodParams.get(i).getName()); @@ -203,10 +203,11 @@ private static TypeSpec createInputsClass( codeBlock.add("$L);\n", onCalculateCachedValueMethod.methodParams.get(i).getName()); } } - - hashCodeMethod.addCode(codeBlock.build()); - typeSpec.addMethod(hashCodeMethod.build()); + } else { + codeBlock.add("getClass());\n"); } + hashCodeMethod.addCode(codeBlock.build()); + typeSpec.addMethod(hashCodeMethod.build()); MethodSpec.Builder equalsMethod = MethodSpec.methodBuilder("equals")