Merge pull request #49 from yandex/wp/report-duplicate-aliases

Fix: Introduce reportDuplicateAliasesAsErrors option.
yandex · Apr 6, 2023 · 71c7718 · 71c7718
2 parents 4e2a9aa + 208f738
commit 71c7718
Show file tree

Hide file tree

Showing 15 changed files with 175 additions and 16 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/main/kotlin/CompileTestDriver.kt b/testing/tests/src/main/kotlin/CompileTestDriver.kt
@@ -16,6 +16,7 @@
 
 package com.yandex.yatagan.testing.tests
 
+import com.yandex.yatagan.processor.common.Option
 import com.yandex.yatagan.testing.source_set.SourceSet
 import org.junit.Rule
 import org.junit.rules.TestWatcher
@@ -29,6 +30,8 @@ interface CompileTestDriver : SourceSet {
         sources: SourceSet,
     )
 
+    fun <V : Any> givenOption(option: Option<V>, value: V)
+
     /**
      * Runs the test and validates the output against the golden output file.
      * The golden output file resource path is computed from the junit test name.

diff --git a/testing/tests/src/main/kotlin/CompileTestDriverBase.kt b/testing/tests/src/main/kotlin/CompileTestDriverBase.kt
@@ -22,6 +22,7 @@ import com.yandex.yatagan.generated.CompiledApiClasspath
 import com.yandex.yatagan.generated.DynamicApiClasspath
 import com.yandex.yatagan.processor.common.IntOption
 import com.yandex.yatagan.processor.common.LoggerDecorator
+import com.yandex.yatagan.processor.common.Option
 import com.yandex.yatagan.testing.source_set.SourceFile
 import com.yandex.yatagan.testing.source_set.SourceSet
 import org.junit.Assert
@@ -39,6 +40,10 @@ abstract class CompileTestDriverBase private constructor(
     private val mainSourceSet: SourceSet,
 ) : CompileTestDriver, SourceSet by mainSourceSet {
     private var precompiledModuleOutputDirs: List<File>? = null
+    private val options = mutableMapOf(
+        IntOption.MaxIssueEncounterPaths.key to "100",
+        IntOption.MaxSlotsPerSwitch.key to "100",
+    )
 
     protected constructor(
         apiType: ApiType = ApiType.Compiled,
@@ -95,6 +100,10 @@ abstract class CompileTestDriverBase private constructor(
 
     abstract fun generatedFilesSubDir(): String?
 
+    override fun <V : Any> givenOption(option: Option<V>, value: V) {
+        options[option.key] = value.toString()
+    }
+
     override fun compileRunAndValidate() {
         val goldenResourcePath = "golden/${testNameRule.testClassSimpleName}/${testNameRule.testMethodName}.golden.txt"
 
@@ -162,10 +171,7 @@ abstract class CompileTestDriverBase private constructor(
             "-opt-in=com.yandex.yatagan.VariantApi",
             "-P", "plugin:org.jetbrains.kotlin.kapt3:correctErrorTypes=true",
         ),
-        processorOptions = mapOf(
-            IntOption.MaxIssueEncounterPaths.key to "100",
-            IntOption.MaxSlotsPerSwitch.key to "100",
-        ),
+        processorOptions = options,
     )
 
     protected open fun createCompilationArguments() = createBaseCompilationArguments()

diff --git a/testing/tests/src/main/kotlin/DynamicCompileTestDriver.kt b/testing/tests/src/main/kotlin/DynamicCompileTestDriver.kt
@@ -20,8 +20,10 @@ import com.squareup.javapoet.ClassName
 import com.yandex.yatagan.Component
 import com.yandex.yatagan.lang.jap.asTypeElement
 import com.yandex.yatagan.lang.jap.isAnnotatedWith
+import com.yandex.yatagan.processor.common.IntOption
 import com.yandex.yatagan.processor.common.Logger
 import com.yandex.yatagan.processor.common.LoggerDecorator
+import com.yandex.yatagan.processor.common.Option
 import com.yandex.yatagan.testing.source_set.SourceFile
 import org.intellij.lang.annotations.Language
 import java.io.File
@@ -39,6 +41,9 @@ class DynamicCompileTestDriver(
     apiType: ApiType = ApiType.Dynamic,
 ) : CompileTestDriverBase(apiType) {
     private val accumulator = ComponentBootstrapperGenerator()
+    private val options = mutableMapOf<Option<*>, Any>(
+        IntOption.MaxIssueEncounterPaths to 100,
+    )
 
     private val runnerSource = SourceFile.java("RuntimeTestRunner", """
         import com.yandex.yatagan.Yatagan;
@@ -60,8 +65,25 @@ class DynamicCompileTestDriver(
         }
     """.trimIndent())
 
+    private fun formatOptions() = buildString {
+        for ((option, value) in options) {
+            val argument = when(value) {
+                is Boolean -> value.toString()
+                is Int -> value.toString()
+                else -> throw AssertionError("Unexpected option value type, please, support it explicitly")
+            }
+            append('.')
+            append(option.key.substringAfterLast('.'))
+            append('(').append(argument).append(')')
+        }
+    }
+
     override fun generatedFilesSubDir(): String? = null
 
+    override fun <V : Any> givenOption(option: Option<V>, value: V) {
+        options[option] = value
+    }
+
     override fun doCompile(): TestCompilationResult {
         val testCompilationResult = super.doCompile()
         check(testCompilationResult.success) {
@@ -136,7 +158,7 @@ class DynamicCompileTestDriver(
         return success
     }
 
-    private class ComponentBootstrapperGenerator : AbstractProcessor() {
+    private inner class ComponentBootstrapperGenerator : AbstractProcessor() {
         private val _bootstrapperNames: MutableSet<ClassName> = TreeSet()
         val bootstrapperNames: Set<ClassName> get() = _bootstrapperNames
 
@@ -196,10 +218,9 @@ class DynamicCompileTestDriver(
                             
                             Yatagan.setupReflectionBackend()
                                 .validation(delegate)
-                                .maxIssueEncounterPaths(100)
-                                .strictMode(true)
                                 .useCompiledImplementationIfAvailable(true)
                                 .logger(logger)
+                                ${formatOptions()}
                                 .apply();
                             
                             try {