From c081199c86f55cd0bde5e69cfd0b55c5a639d53d Mon Sep 17 00:00:00 2001 From: Fedor Ihnatkevich Date: Thu, 6 Apr 2023 15:42:20 +0300 Subject: [PATCH] Fix: Introduce `reportDuplicateAliasesAsErrors` option. 1. Report duplicate `@Binds` as a warning unconditionally. 2. If the option is enabled, report as an error. --- api/dynamic/src/main/kotlin/Yatagan.kt | 16 +++++++++++ .../impl/src/main/kotlin/BindingGraph.kt | 6 ++++- .../impl/src/main/kotlin/BindingGraphImpl.kt | 4 +++ .../src/main/kotlin/GraphBindingsManager.kt | 17 +++++++++--- core/graph/impl/src/main/kotlin/Options.kt | 27 +++++++++++++++++++ processor/common/src/main/kotlin/Options.kt | 1 + processor/common/src/main/kotlin/process.kt | 8 +++++- rt/engine/src/main/kotlin/RuntimeEngine.kt | 14 ++++++++-- .../test/kotlin/CoreBindingsFailureTest.kt | 12 ++++++++- .../conflicting-bindings.golden.txt | 8 ++++++ .../format/src/main/kotlin/reporting.kt | 2 ++ 11 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 core/graph/impl/src/main/kotlin/Options.kt diff --git a/api/dynamic/src/main/kotlin/Yatagan.kt b/api/dynamic/src/main/kotlin/Yatagan.kt index 34a3edea..59034aff 100644 --- 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 issue #48 + */ + @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 } \ No newline at end of file diff --git a/core/graph/impl/src/main/kotlin/BindingGraph.kt b/core/graph/impl/src/main/kotlin/BindingGraph.kt index 58bdc369..f9ab1b78 100644 --- 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, ) } \ No newline at end of file diff --git a/core/graph/impl/src/main/kotlin/BindingGraphImpl.kt b/core/graph/impl/src/main/kotlin/BindingGraphImpl.kt index 3ea05dda..02d103aa 100644 --- 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() override val localBindings = mutableMapOf() override val localConditionLiterals = mutableMapOf() override val localAssistedInjectFactories = mutableSetOf() @@ -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 index 949a3de7..ae840ccb 100644 --- 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, ) : MayBeInvalid, WithParents 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 new file mode 100644 index 00000000..0a3057e8 --- /dev/null +++ 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, +) \ No newline at end of file diff --git a/processor/common/src/main/kotlin/Options.kt b/processor/common/src/main/kotlin/Options.kt index dcaacb05..eda75f85 100644 --- 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 index 37b1d629..0764f123 100644 --- 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 process( val logger = LoggerDecorator(delegate.logger) val plugins = loadServices() 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 index 15e01cc1..1c9dfbe3 100644 --- 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

( val maxIssueEncounterPaths: Int val isStrictMode: Boolean val logger: Logger? + val reportDuplicateAliasesAsErrors: Boolean } fun destroy() { @@ -79,7 +81,8 @@ class RuntimeEngine

( "$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

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

( } } + private fun createGraphOptions() = Options( + reportDuplicateAliasesAsErrors = params.reportDuplicateAliasesAsErrors, + ) + private companion object { val pluginProviders: List = loadServices() } diff --git a/testing/tests/src/test/kotlin/CoreBindingsFailureTest.kt b/testing/tests/src/test/kotlin/CoreBindingsFailureTest.kt index 31573f8b..d0efac5e 100644 --- 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 val sub: SubComponent.Builder val string: String + val api: Api @Component.Builder interface Builder { diff --git a/testing/tests/src/test/resources/golden/CoreBindingsFailureTest/conflicting-bindings.golden.txt b/testing/tests/src/test/resources/golden/CoreBindingsFailureTest/conflicting-bindings.golden.txt index 0792abc4..6753902b 100644 --- a/testing/tests/src/test/resources/golden/CoreBindingsFailureTest/conflicting-bindings.golden.txt +++ b/testing/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 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ \ No newline at end of file diff --git a/validation/format/src/main/kotlin/reporting.kt b/validation/format/src/main/kotlin/reporting.kt index 9ff5d359..7c4ee9d3 100644 --- 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,