Optimizes ViewEnvironment and ViewRegistry operators

Take care to use the existing instances when combining with empty ones, and add tests to preserve that optimization.

Author: rjrjr

workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/ViewEnvironment.kt

@@ -27,8 +27,11 @@ constructor(
     ViewEnvironment(map + pair)
 
   @Suppress("DEPRECATION")
-  public operator fun plus(other: ViewEnvironment): ViewEnvironment =
-    ViewEnvironment(map + other.map)
+  public operator fun plus(other: ViewEnvironment): ViewEnvironment {
+    if (other.map.isEmpty()) return this
+    if (this.map.isEmpty()) return other
+    return ViewEnvironment(map + other.map)
+  }
 
   override fun toString(): String = "ViewEnvironment($map)"
 

workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/ViewRegistry.kt

@@ -113,15 +113,19 @@ public operator fun ViewRegistry.plus(entry: Entry<*>): ViewRegistry =
 
 /** @throws IllegalArgumentException if other has redundant entries. */
 @WorkflowUiExperimentalApi
-public operator fun ViewRegistry.plus(other: ViewRegistry): ViewRegistry =
-  CompositeViewRegistry(this, other)
+public operator fun ViewRegistry.plus(other: ViewRegistry): ViewRegistry {
+  if (other.keys.isEmpty()) return this
+  if (this.keys.isEmpty()) return other
+  return CompositeViewRegistry(this, other)
+}
 
 /**
  * Replaces the existing [ViewRegistry] of the receiver with [registry]. Use
  * [ViewEnvironment.merge] to combine them instead.
  */
 @WorkflowUiExperimentalApi
 public operator fun ViewEnvironment.plus(registry: ViewRegistry): ViewEnvironment {
+  if (registry.keys.isEmpty()) return this
   return this + (ViewRegistry to registry)
 }
 
@@ -147,6 +151,8 @@ public infix fun ViewRegistry.merge(other: ViewRegistry): ViewRegistry {
  */
 @WorkflowUiExperimentalApi
 public infix fun ViewEnvironment.merge(registry: ViewRegistry): ViewEnvironment {
+  if (registry.keys.isEmpty()) return this
+
   val merged = this[ViewRegistry] merge registry
   return this + merged
 }
@@ -159,6 +165,7 @@ public infix fun ViewEnvironment.merge(registry: ViewRegistry): ViewEnvironment
 @WorkflowUiExperimentalApi
 public infix fun ViewEnvironment.merge(other: ViewEnvironment): ViewEnvironment {
   if (other.map.isEmpty()) return this
+  if (this.map.isEmpty()) return other
 
   val oldReg = this[ViewRegistry]
   val newReg = other[ViewRegistry]

workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/ViewEnvironmentTest.kt

@@ -1,6 +1,7 @@
 package com.squareup.workflow1.ui
 
 import com.google.common.truth.Truth.assertThat
+import com.squareup.workflow1.ui.ViewEnvironment.Companion.EMPTY
 import org.junit.Test
 
 @OptIn(WorkflowUiExperimentalApi::class)
@@ -23,11 +24,11 @@ internal class ViewEnvironmentTest {
   }
 
   @Test fun defaults() {
-    assertThat(ViewEnvironment.EMPTY[DataHint]).isEqualTo(DataHint())
+    assertThat(EMPTY[DataHint]).isEqualTo(DataHint())
   }
 
   @Test fun put() {
-    val environment = ViewEnvironment.EMPTY +
+    val environment = EMPTY +
       (StringHint to "fnord") +
       (DataHint to DataHint(42, "foo"))
 
@@ -36,23 +37,23 @@ internal class ViewEnvironmentTest {
   }
 
   @Test fun `map equality`() {
-    val env1 = ViewEnvironment.EMPTY +
+    val env1 = EMPTY +
       (StringHint to "fnord") +
       (DataHint to DataHint(42, "foo"))
 
-    val env2 = ViewEnvironment.EMPTY +
+    val env2 = EMPTY +
       (StringHint to "fnord") +
       (DataHint to DataHint(42, "foo"))
 
     assertThat(env1).isEqualTo(env2)
   }
 
   @Test fun `map inequality`() {
-    val env1 = ViewEnvironment.EMPTY +
+    val env1 = EMPTY +
       (StringHint to "fnord") +
       (DataHint to DataHint(42, "foo"))
 
-    val env2 = ViewEnvironment.EMPTY +
+    val env2 = EMPTY +
       (StringHint to "fnord") +
       (DataHint to DataHint(43, "foo"))
 
@@ -68,19 +69,29 @@ internal class ViewEnvironmentTest {
   }
 
   @Test fun override() {
-    val environment = ViewEnvironment.EMPTY +
+    val environment = EMPTY +
       (StringHint to "able") +
       (StringHint to "baker")
 
     assertThat(environment[StringHint]).isEqualTo("baker")
   }
 
   @Test fun `keys of the same type`() {
-    val environment = ViewEnvironment.EMPTY +
+    val environment = EMPTY +
       (StringHint to "able") +
       (OtherStringHint to "baker")
 
     assertThat(environment[StringHint]).isEqualTo("able")
     assertThat(environment[OtherStringHint]).isEqualTo("baker")
   }
+
+  @Test fun `preserve this when merging empty`() {
+    val environment = EMPTY + (StringHint to "able")
+    assertThat(environment + EMPTY).isSameInstanceAs(environment)
+  }
+
+  @Test fun `preserve other when merging to empty`() {
+    val environment = EMPTY + (StringHint to "able")
+    assertThat(EMPTY + environment).isSameInstanceAs(environment)
+  }
 }

workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/TypedViewRegistryTest.kt renamed to workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/ViewRegistryTest.kt

@@ -1,14 +1,15 @@
 package com.squareup.workflow1.ui
 
 import com.google.common.truth.Truth.assertThat
+import com.squareup.workflow1.ui.ViewEnvironment.Companion.EMPTY
 import com.squareup.workflow1.ui.ViewRegistry.Entry
 import org.junit.Test
 import kotlin.reflect.KClass
 import kotlin.test.assertFailsWith
 import kotlin.test.assertTrue
 
 @OptIn(WorkflowUiExperimentalApi::class)
-internal class TypedViewRegistryTest {
+internal class ViewRegistryTest {
 
   @Test fun `keys from bindings`() {
     val factory1 = TestEntry(FooRendering::class)
@@ -62,21 +63,61 @@ internal class TypedViewRegistryTest {
   @Test fun `merge into ViewEnvironment prefers right side`() {
     val factory1 = TestEntry(FooRendering::class)
     val factory2 = TestEntry(FooRendering::class)
-    val merged = (ViewEnvironment.EMPTY + ViewRegistry(factory1)) merge ViewRegistry(factory2)
+    val merged = (EMPTY + ViewRegistry(factory1)) merge ViewRegistry(factory2)
 
     assertThat(merged[ViewRegistry][FooRendering::class]).isSameInstanceAs(factory2)
   }
 
   @Test fun `merge of ViewEnvironments prefers right side`() {
     val factory1 = TestEntry(FooRendering::class)
     val factory2 = TestEntry(FooRendering::class)
-    val e1 = ViewEnvironment.EMPTY + ViewRegistry(factory1)
-    val e2 = ViewEnvironment.EMPTY + ViewRegistry(factory2)
+    val e1 = EMPTY + ViewRegistry(factory1)
+    val e2 = EMPTY + ViewRegistry(factory2)
     val merged = e1 + e2
 
     assertThat(merged[ViewRegistry][FooRendering::class]).isSameInstanceAs(factory2)
   }
 
+  @Test fun `plus of empty returns this`() {
+    val reg = ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(reg + ViewRegistry()).isSameInstanceAs(reg)
+  }
+
+  @Test fun `plus to empty returns other`() {
+    val reg = ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(ViewRegistry() + reg).isSameInstanceAs(reg)
+  }
+
+  @Test fun `merge of empty reg returns this`() {
+    val reg = ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(reg merge ViewRegistry()).isSameInstanceAs(reg)
+  }
+
+  @Test fun `merge to empty reg returns other`() {
+    val reg = ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(ViewRegistry() merge reg).isSameInstanceAs(reg)
+  }
+
+  @Test fun `env merge of empty reg returns this env`() {
+    val env = EMPTY + ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(env merge ViewRegistry()).isSameInstanceAs(env)
+  }
+
+  @Test fun `env merge of empty env returns other env`() {
+    val env = EMPTY + ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(env merge EMPTY).isSameInstanceAs(env)
+  }
+
+  @Test fun `env merge to empty env returns other env`() {
+    val env = EMPTY + ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(EMPTY merge env).isSameInstanceAs(env)
+  }
+
+  @Test fun `env plus empty reg returns env`() {
+    val env = EMPTY + ViewRegistry(TestEntry(FooRendering::class))
+    assertThat(env + ViewRegistry()).isSameInstanceAs(env)
+  }
+
   private class TestEntry<T : Any>(
     override val type: KClass<in T>
   ) : Entry<T>

workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/container/EnvironmentScreenTest.kt

@@ -3,6 +3,7 @@ package com.squareup.workflow1.ui.container
 import com.google.common.truth.Truth.assertThat
 import com.squareup.workflow1.ui.Screen
 import com.squareup.workflow1.ui.ViewEnvironment
+import com.squareup.workflow1.ui.ViewEnvironment.Companion.EMPTY
 import com.squareup.workflow1.ui.ViewEnvironmentKey
 import com.squareup.workflow1.ui.ViewRegistry
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
@@ -46,7 +47,7 @@ internal class EnvironmentScreenTest {
     val fooFactory = TestFactory(FooScreen::class)
     val viewRegistry = ViewRegistry(fooFactory)
     val envScreen = FooScreen.withEnvironment(
-      ViewEnvironment.EMPTY + viewRegistry + TestValue("foo")
+      EMPTY + viewRegistry + TestValue("foo")
     )
 
     assertThat(envScreen.viewEnvironment[ViewRegistry][FooScreen::class])
@@ -78,11 +79,11 @@ internal class EnvironmentScreenTest {
     val barFactory = TestFactory(BarScreen::class)
 
     val left = FooScreen.withEnvironment(
-      ViewEnvironment.EMPTY + ViewRegistry(fooFactory1, barFactory) + TestValue("left")
+      EMPTY + ViewRegistry(fooFactory1, barFactory) + TestValue("left")
     )
 
     val union = left.withEnvironment(
-      ViewEnvironment.EMPTY + ViewRegistry(fooFactory2) + TestValue("right")
+      EMPTY + ViewRegistry(fooFactory2) + TestValue("right")
     )
 
     assertThat(union.viewEnvironment[ViewRegistry][FooScreen::class])
@@ -92,4 +93,9 @@ internal class EnvironmentScreenTest {
     assertThat(union.viewEnvironment[TestValue])
       .isEqualTo(TestValue("right"))
   }
+
+  @Test fun `keep existing instance on vacuous merge`() {
+    val left = FooScreen.withEnvironment(EMPTY + TestValue("whatever"))
+    assertThat(left.withEnvironment()).isSameInstanceAs(left)
+  }
 }