Validate that option scopes and target aliases don't collide. (Cherry…

…-pick of #11004) (#11015)

[ci skip-rust]

src/python/pants/build_graph/build_configuration.py

@@ -3,30 +3,90 @@
 
 import logging
 import typing
+from collections import defaultdict
 from collections.abc import Iterable
 from dataclasses import dataclass, field
-from typing import Any, Dict, Type, Union
+from enum import Enum
+from typing import Any, DefaultDict, Dict, Set, Type, Union, cast
 
 from pants.build_graph.build_file_aliases import BuildFileAliases
+from pants.engine.goal import GoalSubsystem
 from pants.engine.rules import Rule, RuleIndex
 from pants.engine.target import Target
 from pants.engine.unions import UnionRule
+from pants.goal.run_tracker import RunTracker
+from pants.option.global_options import GlobalOptions
 from pants.option.optionable import Optionable
+from pants.option.scope import normalize_scope
+from pants.reporting.reporting import Reporting
 from pants.util.ordered_set import FrozenOrderedSet, OrderedSet
+from pants.vcs.changed import Changed
 
 logger = logging.getLogger(__name__)
 
 
+# No goal or target_type can have a name from this set, so that `./pants help <name>`
+# is unambiguous.
+_RESERVED_NAMES = {"global", "targets", "goals"}
+
+
+# Subsystems used outside of any rule.
+_GLOBAL_SUBSYSTEMS: FrozenOrderedSet[Type[Optionable]] = FrozenOrderedSet(
+    {GlobalOptions, Reporting, RunTracker, Changed}
+)
+
+
 @dataclass(frozen=True)
 class BuildConfiguration:
     """Stores the types and helper functions exposed to BUILD files."""
 
     registered_aliases: BuildFileAliases
-    optionables: FrozenOrderedSet[Optionable]
+    optionables: FrozenOrderedSet[Type[Optionable]]
     rules: FrozenOrderedSet[Rule]
     union_rules: FrozenOrderedSet[UnionRule]
     target_types: FrozenOrderedSet[Type[Target]]
 
+    @property
+    def all_optionables(self) -> FrozenOrderedSet[Type[Optionable]]:
+        """Return all optionables in the system: global and those registered via rule usage."""
+        return _GLOBAL_SUBSYSTEMS | self.optionables
+
+    def __post_init__(self) -> None:
+        class Category(Enum):
+            goal = "goal"
+            reserved_name = "reserved name"
+            subsystem = "subsystem"
+            target_type = "target type"
+
+        name_to_categories: DefaultDict[str, Set[Category]] = defaultdict(set)
+        normalized_to_orig_name: Dict[str, str] = {}
+
+        for opt in self.all_optionables:
+            scope = cast(str, opt.options_scope)
+            normalized_scope = normalize_scope(scope)
+            name_to_categories[normalized_scope].add(
+                Category.goal if issubclass(opt, GoalSubsystem) else Category.subsystem
+            )
+            normalized_to_orig_name[normalized_scope] = scope
+        for tgt_type in self.target_types:
+            name_to_categories[normalize_scope(tgt_type.alias)].add(Category.target_type)
+        for reserved_name in _RESERVED_NAMES:
+            name_to_categories[normalize_scope(reserved_name)].add(Category.reserved_name)
+
+        found_collision = False
+        for name, cats in name_to_categories.items():
+            if len(cats) > 1:
+                scats = sorted(cat.value for cat in cats)
+                cats_str = ", ".join(f"a {cat}" for cat in scats[:-1]) + f" and a {scats[-1]}."
+                colliding_names = "`/`".join(
+                    sorted({name, normalized_to_orig_name.get(name, name)})
+                )
+                logger.error(f"Naming collision: `{colliding_names}` is registered as {cats_str}")
+                found_collision = True
+
+        if found_collision:
+            raise TypeError("Found naming collisions. See log for details.")
+
     @dataclass
     class Builder:
         _exposed_object_by_alias: Dict[Any, Any] = field(default_factory=dict)
@@ -42,8 +102,8 @@ def registered_aliases(self) -> BuildFileAliases:
             These returned aliases aren't so useful for actually parsing BUILD files.
             They are useful for generating online documentation.
 
-            :returns: A new BuildFileAliases instance containing this BuildConfiguration's registered alias
-                      mappings.
+            :returns: A new BuildFileAliases instance containing this BuildConfiguration's
+                      registered alias mappings.
             """
             return BuildFileAliases(
                 objects=self._exposed_object_by_alias.copy(),
@@ -94,13 +154,12 @@ def _register_exposed_context_aware_object_factory(
                 alias
             ] = context_aware_object_factory
 
-        def register_optionables(self, optionables):
-            """Registers the given subsystem types.
-
-            :param optionables: The Optionable types to register.
-            :type optionables: :class:`collections.Iterable` containing
-                               :class:`pants.option.optionable.Optionable` subclasses.
-            """
+        # NB: We expect the parameter to be Iterable[Type[Optionable]], but we can't be confident
+        # in this because we pass whatever people put in their `register.py`s to this function;
+        # I.e., this is an impure function that reads from the outside world. So, we use the type
+        # hint `Any` and perform runtime type checking.
+        def register_optionables(self, optionables: Union[typing.Iterable[Type[Optionable]], Any]):
+            """Registers the given subsystem types."""
             if not isinstance(optionables, Iterable):
                 raise TypeError("The optionables must be an iterable, given {}".format(optionables))
             optionables = tuple(optionables)
@@ -137,17 +196,18 @@ def register_rules(self, rules):
                 rule.output_type for rule in self._rules if issubclass(rule.output_type, Optionable)
             )
 
-        # NB: We expect the parameter to be Iterable[Type[Target]], but we can't be confident in this
-        # because we pass whatever people put in their `register.py`s to this function; i.e., this is
-        # an impure function that reads from the outside world. So, we use the type hint `Any` and
-        # perform runtime type checking.
+        # NB: We expect the parameter to be Iterable[Type[Target]], but we can't be confident in
+        # this because we pass whatever people put in their `register.py`s to this function;
+        # I.e., this is an impure function that reads from the outside world. So, we use the type
+        # hint `Any` and perform runtime type checking.
         def register_target_types(
             self, target_types: Union[typing.Iterable[Type[Target]], Any]
         ) -> None:
             """Registers the given target types."""
             if not isinstance(target_types, Iterable):
                 raise TypeError(
-                    f"The entrypoint `target_types` must return an iterable. Given {repr(target_types)}"
+                    f"The entrypoint `target_types` must return an iterable. "
+                    f"Given {repr(target_types)}"
                 )
             bad_elements = [
                 tgt_type

src/python/pants/build_graph/build_configuration_test.py

@@ -1,66 +1,118 @@
 # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
+from typing import Type
 
-import unittest
+import pytest
 
 from pants.build_graph.build_configuration import BuildConfiguration
 from pants.build_graph.build_file_aliases import BuildFileAliases
+from pants.engine.goal import GoalSubsystem
+from pants.engine.target import Target
 from pants.engine.unions import UnionRule, union
+from pants.option.optionable import Optionable
+from pants.option.subsystem import Subsystem
 from pants.util.frozendict import FrozenDict
 from pants.util.ordered_set import FrozenOrderedSet
 
 
-class BuildConfigurationTest(unittest.TestCase):
-    def setUp(self) -> None:
-        self.bc_builder = BuildConfiguration.Builder()
+@pytest.fixture
+def bc_builder() -> BuildConfiguration.Builder:
+    return BuildConfiguration.Builder()
 
-    def _register_aliases(self, **kwargs) -> None:
-        self.bc_builder.register_aliases(BuildFileAliases(**kwargs))
 
-    def test_register_bad(self) -> None:
-        with self.assertRaises(TypeError):
-            self.bc_builder.register_aliases(42)
+def _register_aliases(bc_builder, **kwargs) -> None:
+    bc_builder.register_aliases(BuildFileAliases(**kwargs))
 
-    def test_register_exposed_object(self):
-        self._register_aliases(objects={"jane": 42})
-        aliases = self.bc_builder.create().registered_aliases
-        assert FrozenDict() == aliases.context_aware_object_factories
-        assert FrozenDict(jane=42) == aliases.objects
 
-    def test_register_exposed_context_aware_object_factory(self):
-        def caof_function(parse_context):
-            return parse_context.rel_path
+def test_register_bad(bc_builder: BuildConfiguration.Builder) -> None:
+    with pytest.raises(TypeError):
+        bc_builder.register_aliases(42)
 
-        class CaofClass:
-            def __init__(self, parse_context):
-                self._parse_context = parse_context
 
-            def __call__(self):
-                return self._parse_context.rel_path
+def test_register_exposed_object(bc_builder: BuildConfiguration.Builder) -> None:
+    _register_aliases(bc_builder, objects={"jane": 42})
+    aliases = bc_builder.create().registered_aliases
+    assert FrozenDict() == aliases.context_aware_object_factories
+    assert FrozenDict(jane=42) == aliases.objects
+
+
+def test_register_exposed_context_aware_object_factory(
+    bc_builder: BuildConfiguration.Builder,
+) -> None:
+    def caof_function(parse_context):
+        return parse_context.rel_path
+
+    class CaofClass:
+        def __init__(self, parse_context):
+            self._parse_context = parse_context
+
+        def __call__(self):
+            return self._parse_context.rel_path
+
+    _register_aliases(
+        bc_builder, context_aware_object_factories={"func": caof_function, "cls": CaofClass}
+    )
+    aliases = bc_builder.create().registered_aliases
+    assert FrozenDict() == aliases.objects
+    assert (
+        FrozenDict({"func": caof_function, "cls": CaofClass})
+        == aliases.context_aware_object_factories
+    )
 
-        self._register_aliases(
-            context_aware_object_factories={"func": caof_function, "cls": CaofClass}
-        )
-        aliases = self.bc_builder.create().registered_aliases
-        assert FrozenDict() == aliases.objects
-        assert (
-            FrozenDict({"func": caof_function, "cls": CaofClass})
-            == aliases.context_aware_object_factories
-        )
 
-    def test_register_union_rules(self) -> None:
-        @union
-        class Base:
-            pass
+def test_register_union_rules(bc_builder: BuildConfiguration.Builder) -> None:
+    @union
+    class Base:
+        pass
 
-        class A:
-            pass
+    class A:
+        pass
 
-        class B:
-            pass
+    class B:
+        pass
 
-        union_a = UnionRule(Base, A)
-        union_b = UnionRule(Base, B)
-        self.bc_builder.register_rules([union_a])
-        self.bc_builder.register_rules([union_b])
-        assert self.bc_builder.create().union_rules == FrozenOrderedSet([union_a, union_b])
+    union_a = UnionRule(Base, A)
+    union_b = UnionRule(Base, B)
+    bc_builder.register_rules([union_a])
+    bc_builder.register_rules([union_b])
+    assert bc_builder.create().union_rules == FrozenOrderedSet([union_a, union_b])
+
+
+def test_validation(caplog, bc_builder: BuildConfiguration.Builder) -> None:
+    def mk_dummy_opt(_options_scope: str, goal: bool = False) -> Type[Optionable]:
+        class DummyOptionable(GoalSubsystem if goal else Subsystem):  # type: ignore[misc]
+            options_scope = _options_scope
+
+        return DummyOptionable
+
+    def mk_dummy_tgt(_alias: str) -> Type[Target]:
+        class DummyTarget(Target):
+            alias = _alias
+            core_fields = tuple()  # type: ignore[var-annotated]
+
+        return DummyTarget
+
+    bc_builder.register_optionables(
+        (
+            mk_dummy_opt("foo"),
+            mk_dummy_opt("Bar-bar"),
+            mk_dummy_opt("baz"),
+            mk_dummy_opt("qux", goal=True),
+            mk_dummy_opt("global"),
+        )
+    )
+    bc_builder.register_target_types(
+        (mk_dummy_tgt("bar_bar"), mk_dummy_tgt("qux"), mk_dummy_tgt("global"))
+    )
+    with pytest.raises(TypeError) as e:
+        bc_builder.create()
+    assert (
+        "Naming collision: `Bar-bar`/`bar_bar` is registered as a subsystem and a "
+        "target type." in caplog.text
+    )
+    assert "Naming collision: `qux` is registered as a goal and a target type." in caplog.text
+    assert (
+        "Naming collision: `global` is registered as a reserved name, a subsystem "
+        "and a target type." in caplog.text
+    )
+    assert "Found naming collisions" in str(e)

src/python/pants/init/options_initializer.py

@@ -12,7 +12,6 @@
 from pants.base.exceptions import BuildConfigurationError
 from pants.build_graph.build_configuration import BuildConfiguration
 from pants.init.extension_loader import load_backends_and_plugins
-from pants.init.global_subsystems import GlobalSubsystems
 from pants.init.plugin_resolver import PluginResolver
 from pants.option.global_options import GlobalOptions
 from pants.option.options_bootstrapper import OptionsBootstrapper
@@ -79,9 +78,10 @@ def _construct_options(options_bootstrapper, build_configuration):
 
         :returns: An Options object representing the full set of runtime options.
         """
-        optionables = {GlobalOptions, *GlobalSubsystems.get(), *build_configuration.optionables}
         known_scope_infos = [
-            si for optionable in optionables for si in optionable.known_scope_infos()
+            si
+            for optionable in build_configuration.all_optionables
+            for si in optionable.known_scope_infos()
         ]
         return options_bootstrapper.get_full_options(known_scope_infos)
 

src/python/pants/option/scope.py

@@ -10,6 +10,10 @@
 GLOBAL_SCOPE_CONFIG_SECTION = "GLOBAL"
 
 
+def normalize_scope(scope: str):
+    return scope.lower().replace("-", "_")
+
+
 @dataclass(frozen=True)
 class Scope:
     """An options scope."""

tests/python/pants_test/init/test_extension_loader.py

@@ -108,8 +108,6 @@ def setUp(self):
     def create_register(
         self,
         build_file_aliases=None,
-        register_goals=None,
-        global_subsystems=None,
         rules=None,
         target_types=None,
         module_name="register",
@@ -131,8 +129,6 @@ def register_entrypoint(function_name, function):
                     setattr(register_module, function_name, function)
 
             register_entrypoint("build_file_aliases", build_file_aliases)
-            register_entrypoint("global_subsystems", global_subsystems)
-            register_entrypoint("register_goals", register_goals)
             register_entrypoint("rules", rules)
             register_entrypoint("target_types", target_types)