Fix: Introduce reportDuplicateAliasesAsErrors option.

1. Report duplicate `@Binds` as a warning unconditionally. 2. If the option is enabled, report as an error.
yandex · Apr 6, 2023 · c081199 · c081199
1 parent 4e2a9aa
commit c081199
Show file tree

Hide file tree

Showing 11 changed files with 106 additions and 9 deletions.
diff --git a/api/dynamic/src/main/kotlin/Yatagan.kt b/api/dynamic/src/main/kotlin/Yatagan.kt
@@ -71,6 +71,21 @@ public object Yatagan {
             params.isStrictMode = enabled
         }
 
+        /**
+         * Whether to report duplicate `@Binds` as errors instead of warnings.
+         *
+         * `false` by default.
+         *
+         * @see <a href="https://github.com/yandex/yatagan/issues/48">issue #48</a>
+         */
+        @Deprecated(
+            "Will be unconditionally enabled, and the switch will be removed in the next major release",
+            level = DeprecationLevel.WARNING,
+        )
+        public fun reportDuplicateAliasesAsErrors(enabled: Boolean): Initializer = apply {
+            params.reportDuplicateAliasesAsErrors = enabled
+        }
+
         /**
          * If `true`, then Yatagan will try to locate and load compiled implementation in the classpath first.
          * Then, if no required classes found, Yatagan will proceed to utilizing reflection backend.
@@ -200,6 +215,7 @@ public object Yatagan {
         override var maxIssueEncounterPaths: Int = 5,
         override var isStrictMode: Boolean = true,
         override var logger: Logger? = null,
+        override var reportDuplicateAliasesAsErrors: Boolean = false,
         var useCompiledImplementationIfAvailable: Boolean = false,
     ) : RuntimeEngine.Params
 }
diff --git a/core/graph/impl/src/main/kotlin/BindingGraph.kt b/core/graph/impl/src/main/kotlin/BindingGraph.kt
@@ -22,9 +22,13 @@ import com.yandex.yatagan.core.model.ComponentModel
 /**
  * Creates [BindingGraph] instance given the root component.
  */
-fun BindingGraph(root: ComponentModel): BindingGraph {
+fun BindingGraph(
+    root: ComponentModel,
+    options: Options,
+): BindingGraph {
     require(root.isRoot) { "Not reached: can't use non-root component as a root of a binding graph" }
     return BindingGraphImpl(
         component = root,
+        options = options,
     )
 }
diff --git a/core/graph/impl/src/main/kotlin/BindingGraphImpl.kt b/core/graph/impl/src/main/kotlin/BindingGraphImpl.kt
@@ -50,6 +50,7 @@ import com.yandex.yatagan.validation.format.reportError
 
 internal class BindingGraphImpl(
     private val component: ComponentModel,
+    internal val options: Options,
     override val parent: BindingGraphImpl? = null,
     override val conditionScope: ConditionScope = ConditionScope.Unscoped,
     private val factoryMethodInParent: ComponentFactoryModel? = null,
@@ -119,6 +120,7 @@ internal class BindingGraphImpl(
         },
     )
 
+    internal val localNodes = mutableSetOf<NodeModel>()
     override val localBindings = mutableMapOf<Binding, BindingUsageImpl>()
     override val localConditionLiterals = mutableMapOf<ConditionModel, LiteralUsage>()
     override val localAssistedInjectFactories = mutableSetOf<AssistedInjectFactoryModel>()
@@ -152,6 +154,7 @@ internal class BindingGraphImpl(
             .map { (childComponent, childConditionScope) ->
                 BindingGraphImpl(
                     component = childComponent,
+                    options = options,
                     parent = this,
                     conditionScope = conditionScope and childConditionScope,
                     factoryMethodInParent = component.subComponentFactoryMethods.find {
@@ -186,6 +189,7 @@ internal class BindingGraphImpl(
                 }
                 override fun visitBinding(binding: Binding) = binding
             }
+            localNodes.add(dependency.node)
             // MAYBE: employ local alias resolution cache
             val nonAlias = binding.accept(AliasMaterializeVisitor())
             if (nonAlias.owner == this) {

diff --git a/core/graph/impl/src/main/kotlin/GraphBindingsManager.kt b/core/graph/impl/src/main/kotlin/GraphBindingsManager.kt
@@ -16,7 +16,6 @@
 
 package com.yandex.yatagan.core.graph.impl
 
-import com.yandex.yatagan.core.graph.BindingGraph
 import com.yandex.yatagan.core.graph.Extensible
 import com.yandex.yatagan.core.graph.WithParents
 import com.yandex.yatagan.core.graph.bindings.AliasBinding
@@ -73,11 +72,13 @@ import com.yandex.yatagan.validation.MayBeInvalid
 import com.yandex.yatagan.validation.RichString
 import com.yandex.yatagan.validation.Validator
 import com.yandex.yatagan.validation.format.Strings
+import com.yandex.yatagan.validation.format.demoteToWarning
 import com.yandex.yatagan.validation.format.modelRepresentation
 import com.yandex.yatagan.validation.format.reportError
+import com.yandex.yatagan.validation.format.reportWarning
 
 internal class GraphBindingsManager(
-    private val graph: BindingGraph,
+    private val graph: BindingGraphImpl,
     subcomponents: Map<ComponentModel, ComponentFactoryModel?>,
 ) : MayBeInvalid, WithParents<GraphBindingsManager> by hierarchyExtension(graph, GraphBindingsManager) {
     init {
@@ -328,9 +329,8 @@ internal class GraphBindingsManager(
     }
 
     override fun validate(validator: Validator) {
-        val locallyRequestedNodes = graph.localBindings.map { (binding, _) -> binding.target }.toHashSet()
         for ((node, bindings) in localAndParentExplicitBindings) {
-            if (node !in locallyRequestedNodes) {
+            if (node !in graph.localNodes) {
                 // Check duplicates only for locally requested bindings - no need to report parent duplicates.
                 // As a side effect, if duplicates are present for an unused binding - we don't care.
                 continue
@@ -353,6 +353,15 @@ internal class GraphBindingsManager(
                         continue
                     }
 
+                    if (!graph.options.reportDuplicateAliasesAsErrors && distinct.all { it is AliasBinding }) {
+                        validator.reportWarning(Strings.Errors.conflictingBindings(`for` = node).demoteToWarning()) {
+                            distinct.forEach { binding ->
+                                addNote(Strings.Notes.duplicateBinding(binding))
+                            }
+                        }
+                        continue
+                    }
+
                     validator.reportError(Strings.Errors.conflictingBindings(`for` = node)) {
                         distinct.forEach { binding ->
                             addNote(Strings.Notes.duplicateBinding(binding))

diff --git a/core/graph/impl/src/main/kotlin/Options.kt b/core/graph/impl/src/main/kotlin/Options.kt
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2023 Yandex LLC
+ *
+ * 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.yandex.yatagan.core.graph.impl
+
+/**
+ * Options, customizing graph building behavior in various ways.
+ */
+data class Options(
+    /**
+     * Whether to report duplicate aliases as errors instead of warnings.
+     */
+    val reportDuplicateAliasesAsErrors: Boolean,
+)
diff --git a/processor/common/src/main/kotlin/Options.kt b/processor/common/src/main/kotlin/Options.kt
@@ -51,6 +51,7 @@ enum class BooleanOption(
 
     StrictMode("yatagan.enableStrictMode", default = true),
     UsePlainOutput("yatagan.usePlainOutput", default = false),
+    ReportDuplicateAliasesAsErrors("yatagan.experimental.reportDuplicateAliasesAsErrors", default = false),
 
     ;
 

diff --git a/processor/common/src/main/kotlin/process.kt b/processor/common/src/main/kotlin/process.kt
@@ -22,6 +22,7 @@ import com.yandex.yatagan.codegen.impl.ComponentGeneratorFacade
 import com.yandex.yatagan.core.graph.BindingGraph
 import com.yandex.yatagan.core.graph.childrenSequence
 import com.yandex.yatagan.core.graph.impl.BindingGraph
+import com.yandex.yatagan.core.graph.impl.Options
 import com.yandex.yatagan.core.model.impl.ComponentModel
 import com.yandex.yatagan.validation.ValidationMessage.Kind.Error
 import com.yandex.yatagan.validation.ValidationMessage.Kind.MandatoryWarning
@@ -48,7 +49,12 @@ fun <Source> process(
         val logger = LoggerDecorator(delegate.logger)
         val plugins = loadServices<ValidationPluginProvider>()
         for (rootModel in rootModels) {
-            val graphRoot = BindingGraph(root = rootModel)
+            val graphRoot = BindingGraph(
+                root = rootModel,
+                options = Options(
+                    reportDuplicateAliasesAsErrors = delegate.options[BooleanOption.ReportDuplicateAliasesAsErrors]
+                )
+            )
             val allMessages = buildList {
                 addAll(validate(graphRoot))
                 if (plugins.isNotEmpty()) {

diff --git a/rt/engine/src/main/kotlin/RuntimeEngine.kt b/rt/engine/src/main/kotlin/RuntimeEngine.kt
@@ -22,6 +22,7 @@ import com.yandex.yatagan.base.ObjectCacheRegistry
 import com.yandex.yatagan.base.loadServices
 import com.yandex.yatagan.core.graph.BindingGraph
 import com.yandex.yatagan.core.graph.impl.BindingGraph
+import com.yandex.yatagan.core.graph.impl.Options
 import com.yandex.yatagan.core.model.impl.ComponentModel
 import com.yandex.yatagan.lang.InternalLangApi
 import com.yandex.yatagan.lang.LangModelFactory
@@ -55,6 +56,7 @@ class RuntimeEngine<P : RuntimeEngine.Params>(
         val maxIssueEncounterPaths: Int
         val isStrictMode: Boolean
         val logger: Logger?
+        val reportDuplicateAliasesAsErrors: Boolean
     }
 
     fun destroy() {
@@ -79,7 +81,8 @@ class RuntimeEngine<P : RuntimeEngine.Params>(
                 "$componentClass is not a root Yatagan component"
             }
             val graph = BindingGraph(
-                root = componentModel
+                root = componentModel,
+                options = createGraphOptions(),
             )
             val promise = doValidate(graph)
             val factory = promise.awaitOnError {
@@ -112,7 +115,10 @@ class RuntimeEngine<P : RuntimeEngine.Params>(
                 )
             }
 
-            val graph = BindingGraph(componentModel)
+            val graph = BindingGraph(
+                root = componentModel,
+                options = createGraphOptions(),
+            )
             val promise = doValidate(graph)
             builder = RuntimeAutoBuilder(
                 componentClass = componentClass,
@@ -158,6 +164,10 @@ class RuntimeEngine<P : RuntimeEngine.Params>(
         }
     }
 
+    private fun createGraphOptions() = Options(
+        reportDuplicateAliasesAsErrors = params.reportDuplicateAliasesAsErrors,
+    )
+
     private companion object {
         val pluginProviders: List<ValidationPluginProvider> = loadServices()
     }

diff --git a/testing/tests/src/test/kotlin/CoreBindingsFailureTest.kt b/testing/tests/src/test/kotlin/CoreBindingsFailureTest.kt
@@ -505,6 +505,10 @@ class CoreBindingsFailureTest(
                 val myString: String
             }
             
+            interface Api
+            class Impl1 @Inject constructor(): Api
+            class Impl2 @Inject constructor(): Api
+            
             @Module
             class MyModule {
                 @MyQualifier(Named("hello"))
@@ -518,7 +522,12 @@ class CoreBindingsFailureTest(
                 @Provides fun dep(): Dependency = throw AssertionError()
             }
             
-            @Module(includes = [MyModule::class], subcomponents = [SubComponent::class])
+            @Module interface MyBindsModule {
+                @Binds fun api1(i: Impl1): Api
+                @Binds fun api2(i: Impl2): Api
+            }
+            
+            @Module(includes = [MyModule::class, MyBindsModule::class], subcomponents = [SubComponent::class])
             class MyModule2 {
                 @Provides @IntoList fun one(): Number = 1L
                 @Provides @IntoList fun three(): Number = 3f
@@ -545,6 +554,7 @@ class CoreBindingsFailureTest(
                 val numbers: List<Number>
                 val sub: SubComponent.Builder
                 val string: String
+                val api: Api
 
                 @Component.Builder
                 interface Builder {

diff --git a/...g/tests/src/test/resources/golden/CoreBindingsFailureTest/conflicting-bindings.golden.txt b/...g/tests/src/test/resources/golden/CoreBindingsFailureTest/conflicting-bindings.golden.txt
@@ -68,6 +68,14 @@ Encountered:
 error: Conflicting bindings for `test.SubComponent.Builder`
 NOTE: Conflicting binding: `provision test.MyModule2::builder()`
 NOTE: Conflicting binding: `child-component-factory component-creator test.SubComponent.Builder`
+Encountered:
+  here: graph for root-component test.MyComponent2
+        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+warning: Conflicting bindings for `test.Api`
+NOTE: Conflicting binding: `alias test.MyBindsModule::api1(test.Impl1)`
+NOTE: Conflicting binding: `alias test.MyBindsModule::api2(test.Impl2)`
 Encountered:
   here: graph for root-component test.MyComponent2
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/validation/format/src/main/kotlin/reporting.kt b/validation/format/src/main/kotlin/reporting.kt
@@ -68,6 +68,8 @@ inline fun Validator.reportMandatoryWarning(message: WarningMessage, block: Vali
     report(buildMandatoryWarning(message = message, block = block))
 }
 
+fun ErrorMessage.demoteToWarning(): WarningMessage = WarningMessage(text)
+
 @PublishedApi
 internal class ValidationMessageBuilderImpl(
     private val kind: ValidationMessage.Kind,