Canonicalize enum pattern matching for execution strategy, platform, …

…and elsewhere (#7226)

### Problem

In #7092 we added [`NailgunTask#do_for_execution_strategy_variant()`](https://github.com/cosmicexplorer/pants/blob/70977ef064305b78406a627e07f4dae3a60e4ae4/src/python/pants/backend/jvm/tasks/nailgun_task.py#L31-L43), which allowed performing more declarative execution strategy-specific logic in nailgunnable tasks. Further work with rsc will do even more funky things with our nailgunnable task logic, and while we will eventually have a unified story again for nailgun and subprocess invocations with the v2 engine (see #7079), for now having this check that we have performed the logic we expect all execution strategy variants is very useful.

This PR puts that pattern matching logic into `enum()`: https://github.com/pantsbuild/pants/blob/84cf9a75dbf68cf7126fe8372ab9b2f48720464d/src/python/pants/util/objects.py#L173-L174, among other things.

**Note:** `TypeCheckError` and other exceptions are moved up from further down in `objects.py`.

### Solution

- add `resolve_for_enum_variant()` method to `enum` which does the job of the previous `do_for_execution_strategy_variant()`
- make the native backend's `Platform` into an enum.
- stop silently converting a `None` argument to the enum's `create()` classmethod into its`default_value`.
- add `register_enum_option()` helper method to register options based on enum types.

### Result

We have a low-overhead way to convert potentially-tricky conditional logic into a checked pattern matching-style interface with `enum()`, and it is easier to register enum options.

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py

@@ -201,11 +201,11 @@ def _nailgunnable_combined_classpath(self):
 
   # Overrides the normal zinc compiler classpath, which only contains zinc.
   def get_zinc_compiler_classpath(self):
-    return self.do_for_execution_strategy_variant({
+    return self.execution_strategy_enum.resolve_for_enum_variant({
       self.HERMETIC: lambda: super(RscCompile, self).get_zinc_compiler_classpath(),
       self.SUBPROCESS: lambda: super(RscCompile, self).get_zinc_compiler_classpath(),
       self.NAILGUN: lambda: self._nailgunnable_combined_classpath,
-    })
+    })()
 
   def register_extra_products_from_contexts(self, targets, compile_contexts):
     super(RscCompile, self).register_extra_products_from_contexts(targets, compile_contexts)
@@ -861,15 +861,15 @@ def _runtool_nonhermetic(self, parent_workunit, classpath, main, tool_name, args
   def _runtool(self, main, tool_name, args, distribution,
                tgt=None, input_files=tuple(), input_digest=None, output_dir=None):
     with self.context.new_workunit(tool_name) as wu:
-      return self.do_for_execution_strategy_variant({
+      return self.execution_strategy_enum.resolve_for_enum_variant({
         self.HERMETIC: lambda: self._runtool_hermetic(
           main, tool_name, args, distribution,
           tgt=tgt, input_files=input_files, input_digest=input_digest, output_dir=output_dir),
         self.SUBPROCESS: lambda: self._runtool_nonhermetic(
           wu, self.tool_classpath(tool_name), main, tool_name, args, distribution),
         self.NAILGUN: lambda: self._runtool_nonhermetic(
           wu, self._nailgunnable_combined_classpath, main, tool_name, args, distribution),
-      })
+      })()
 
   def _run_metai_tool(self,
                       distribution,

src/python/pants/backend/jvm/tasks/nailgun_task.py

@@ -15,6 +15,7 @@
 from pants.process.subprocess import Subprocess
 from pants.task.task import Task, TaskBase
 from pants.util.memo import memoized_property
+from pants.util.objects import enum, register_enum_option
 
 
 class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
@@ -24,30 +25,16 @@ class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
   SUBPROCESS = 'subprocess'
   HERMETIC = 'hermetic'
 
-  class InvalidExecutionStrategyMapping(Exception): pass
-
-  _all_execution_strategies = frozenset([NAILGUN, SUBPROCESS, HERMETIC])
-
-  def do_for_execution_strategy_variant(self, mapping):
-    """Invoke the method in `mapping` with the key corresponding to the execution strategy.
-
-    `mapping` is a dict mapping execution strategy -> zero-argument lambda.
-    """
-    variants = frozenset(mapping.keys())
-    if variants != self._all_execution_strategies:
-      raise self.InvalidExecutionStrategyMapping(
-        'Must specify a mapping with exactly the keys {} (was: {})'
-        .format(self._all_execution_strategies, variants))
-    method_for_variant = mapping[self.execution_strategy]
-    # The methods need not return a value, but we pass it along if they do.
-    return method_for_variant()
+  class ExecutionStrategy(enum([NAILGUN, SUBPROCESS, HERMETIC])): pass
 
   @classmethod
   def register_options(cls, register):
     super(NailgunTaskBase, cls).register_options(register)
-    register('--execution-strategy', choices=[cls.NAILGUN, cls.SUBPROCESS, cls.HERMETIC], default=cls.NAILGUN,
-             help='If set to nailgun, nailgun will be enabled and repeated invocations of this '
-                  'task will be quicker. If set to subprocess, then the task will be run without nailgun.')
+    register_enum_option(
+      register, cls.ExecutionStrategy, '--execution-strategy',
+      help='If set to nailgun, nailgun will be enabled and repeated invocations of this '
+           'task will be quicker. If set to subprocess, then the task will be run without nailgun. '
+           'Hermetic execution is an experimental subprocess execution framework.')
     register('--nailgun-timeout-seconds', advanced=True, default=10, type=float,
              help='Timeout (secs) for nailgun startup.')
     register('--nailgun-connect-attempts', advanced=True, default=5, type=int,
@@ -60,6 +47,13 @@ def register_options(cls, register):
                                           rev='0.9.1'),
                           ])
 
+  @memoized_property
+  def execution_strategy_enum(self):
+    # TODO: This .create() call can be removed when the enum interface is more stable as the option
+    # is converted into an instance of self.ExecutionStrategy via the `type` argument through
+    # register_enum_option().
+    return self.ExecutionStrategy.create(self.get_options().execution_strategy)
+
   @classmethod
   def subsystem_dependencies(cls):
     return super(NailgunTaskBase, cls).subsystem_dependencies() + (Subprocess.Factory,)
@@ -76,9 +70,10 @@ def __init__(self, *args, **kwargs):
     self._executor_workdir = os.path.join(self.context.options.for_global_scope().pants_workdir,
                                           *id_tuple)
 
-  @memoized_property
+  # TODO: eventually deprecate this when we can move all subclasses to use the enum!
+  @property
   def execution_strategy(self):
-    return self.get_options().execution_strategy
+    return self.execution_strategy_enum.value
 
   def create_java_executor(self, dist=None):
     """Create java executor that uses this task's ng daemon, if allowed.

src/python/pants/backend/native/config/environment.py

@@ -9,37 +9,14 @@
 
 from pants.engine.rules import SingletonRule
 from pants.util.meta import AbstractClass
-from pants.util.objects import datatype
+from pants.util.objects import datatype, enum
 from pants.util.osutil import all_normalized_os_names, get_normalized_os_name
 from pants.util.strutil import create_path_env_var
 
 
-class Platform(datatype(['normalized_os_name'])):
+class Platform(enum('normalized_os_name', all_normalized_os_names())):
 
-  class UnsupportedPlatformError(Exception):
-    """Thrown if pants is running on an unrecognized platform."""
-
-  @classmethod
-  def create(cls):
-    return Platform(get_normalized_os_name())
-
-  _NORMALIZED_OS_NAMES = frozenset(all_normalized_os_names())
-
-  def resolve_platform_specific(self, platform_specific_funs):
-    arg_keys = frozenset(platform_specific_funs.keys())
-    unknown_plats = self._NORMALIZED_OS_NAMES - arg_keys
-    if unknown_plats:
-      raise self.UnsupportedPlatformError(
-        "platform_specific_funs {} must support platforms {}"
-        .format(platform_specific_funs, list(unknown_plats)))
-    extra_plats = arg_keys - self._NORMALIZED_OS_NAMES
-    if extra_plats:
-      raise self.UnsupportedPlatformError(
-        "platform_specific_funs {} has unrecognized platforms {}"
-        .format(platform_specific_funs, list(extra_plats)))
-
-    fun_for_platform = platform_specific_funs[self.normalized_os_name]
-    return fun_for_platform()
+  default_value = get_normalized_os_name()
 
 
 class Executable(AbstractClass):
@@ -89,9 +66,9 @@ def as_invocation_environment_dict(self):
 
     :rtype: dict of string -> string
     """
-    lib_env_var = self._platform.resolve_platform_specific({
-      'darwin': lambda: 'DYLD_LIBRARY_PATH',
-      'linux': lambda: 'LD_LIBRARY_PATH',
+    lib_env_var = self._platform.resolve_for_enum_variant({
+      'darwin': 'DYLD_LIBRARY_PATH',
+      'linux': 'LD_LIBRARY_PATH',
     })
     return {
       'PATH': create_path_env_var(self.path_entries),

src/python/pants/backend/native/subsystems/binaries/gcc.py

@@ -44,9 +44,9 @@ def path_entries(self):
 
   @memoized_method
   def _common_lib_dirs(self, platform):
-    lib64_tuples = platform.resolve_platform_specific({
-      'darwin': lambda: [],
-      'linux': lambda: [('lib64',)],
+    lib64_tuples = platform.resolve_for_enum_variant({
+      'darwin': [],
+      'linux': [('lib64',)],
     })
     return self._filemap(lib64_tuples + [
       ('lib',),

src/python/pants/backend/native/subsystems/binaries/llvm.py

@@ -80,16 +80,13 @@ def _filemap(self, all_components_list):
   def path_entries(self):
     return self._filemap([('bin',)])
 
-  _PLATFORM_SPECIFIC_LINKER_NAME = {
-    'darwin': lambda: 'ld64.lld',
-    'linux': lambda: 'lld',
-  }
-
   def linker(self, platform):
     return Linker(
       path_entries=self.path_entries,
-      exe_filename=platform.resolve_platform_specific(
-        self._PLATFORM_SPECIFIC_LINKER_NAME),
+      exe_filename=platform.resolve_for_enum_variant({
+        'darwin': 'ld64.lld',
+        'linux': 'lld',
+      }),
       library_dirs=[],
       linking_library_dirs=[],
       extra_args=[],

src/python/pants/backend/native/subsystems/native_build_step.py

@@ -10,14 +10,10 @@
 from pants.subsystem.subsystem import Subsystem
 from pants.util.memo import memoized_property
 from pants.util.meta import classproperty
-from pants.util.objects import enum
+from pants.util.objects import enum, register_enum_option
 
 
-class ToolchainVariant(enum('descriptor', ['gnu', 'llvm'])):
-
-  @property
-  def is_gnu(self):
-    return self.descriptor == 'gnu'
+class ToolchainVariant(enum(['gnu', 'llvm'])): pass
 
 
 class NativeBuildStep(CompilerOptionSetsMixin, MirroredTargetOptionMixin, Subsystem):
@@ -39,11 +35,10 @@ def register_options(cls, register):
              help='The default for the "compiler_option_sets" argument '
                   'for targets of this language.')
 
-    register('--toolchain-variant', type=str, fingerprint=True, advanced=True,
-             choices=ToolchainVariant.allowed_values,
-             default=ToolchainVariant.default_value,
-             help="Whether to use gcc (gnu) or clang (llvm) to compile C and C++. Currently all "
-                  "linking is done with binutils ld on Linux, and the XCode CLI Tools on MacOS.")
+    register_enum_option(
+      register, ToolchainVariant, '--toolchain-variant', advanced=True,
+      help="Whether to use gcc (gnu) or clang (llvm) to compile C and C++. Currently all "
+           "linking is done with binutils ld on Linux, and the XCode CLI Tools on MacOS.")
 
   def get_compiler_option_sets_for_target(self, target):
     return self.get_target_mirrored_option('compiler_option_sets', target)

src/python/pants/backend/native/subsystems/native_toolchain.py

@@ -87,10 +87,11 @@ class LLVMCppToolchain(datatype([('cpp_toolchain', CppToolchain)])): pass
 
 @rule(LibcObjects, [Select(Platform), Select(NativeToolchain)])
 def select_libc_objects(platform, native_toolchain):
-  paths = platform.resolve_platform_specific({
+  # We use lambdas here to avoid searching for libc on osx, where it will fail.
+  paths = platform.resolve_for_enum_variant({
     'darwin': lambda: [],
     'linux': lambda: native_toolchain._libc_dev.get_libc_objects(),
-  })
+  })()
   yield LibcObjects(paths)
 
 
@@ -343,8 +344,12 @@ class ToolchainVariantRequest(datatype([
 @rule(CToolchain, [Select(ToolchainVariantRequest)])
 def select_c_toolchain(toolchain_variant_request):
   native_toolchain = toolchain_variant_request.toolchain
-  # TODO: make an enum exhaustiveness checking method that works with `yield Get(...)` statements!
-  if toolchain_variant_request.variant.is_gnu:
+  # TODO(#5933): make an enum exhaustiveness checking method that works with `yield Get(...)`!
+  use_gcc = toolchain_variant_request.variant.resolve_for_enum_variant({
+    'gnu': True,
+    'llvm': False,
+  })
+  if use_gcc:
     toolchain_resolved = yield Get(GCCCToolchain, NativeToolchain, native_toolchain)
   else:
     toolchain_resolved = yield Get(LLVMCToolchain, NativeToolchain, native_toolchain)
@@ -354,7 +359,12 @@ def select_c_toolchain(toolchain_variant_request):
 @rule(CppToolchain, [Select(ToolchainVariantRequest)])
 def select_cpp_toolchain(toolchain_variant_request):
   native_toolchain = toolchain_variant_request.toolchain
-  if toolchain_variant_request.variant.is_gnu:
+  # TODO(#5933): make an enum exhaustiveness checking method that works with `yield Get(...)`!
+  use_gcc = toolchain_variant_request.variant.resolve_for_enum_variant({
+    'gnu': True,
+    'llvm': False,
+  })
+  if use_gcc:
     toolchain_resolved = yield Get(GCCCppToolchain, NativeToolchain, native_toolchain)
   else:
     toolchain_resolved = yield Get(LLVMCppToolchain, NativeToolchain, native_toolchain)

src/python/pants/backend/native/targets/native_artifact.py

@@ -22,9 +22,9 @@ def alias(cls):
 
   def as_shared_lib(self, platform):
     # TODO: check that the name conforms to some format in the constructor (e.g. no dots?).
-    return platform.resolve_platform_specific({
-      'darwin': lambda: 'lib{}.dylib'.format(self.lib_name),
-      'linux': lambda: 'lib{}.so'.format(self.lib_name),
+    return platform.resolve_for_enum_variant({
+      'darwin': 'lib{}.dylib'.format(self.lib_name),
+      'linux': 'lib{}.so'.format(self.lib_name),
     })
 
   def _compute_fingerprint(self):

src/python/pants/backend/native/tasks/conan_fetch.py

@@ -124,9 +124,9 @@ def _conan_user_home(self, conan, in_workdir=False):
 
   @memoized_property
   def _conan_os_name(self):
-    return Platform.create().resolve_platform_specific({
-      'darwin': lambda: 'Macos',
-      'linux': lambda: 'Linux',
+    return Platform.create().resolve_for_enum_variant({
+      'darwin': 'Macos',
+      'linux': 'Linux',
     })
 
   @property

src/python/pants/backend/native/tasks/link_shared_libraries.py

@@ -142,11 +142,6 @@ def _make_link_request(self, vt, compiled_objects_product):
 
     return link_request
 
-  _SHARED_CMDLINE_ARGS = {
-    'darwin': lambda: ['-Wl,-dylib'],
-    'linux': lambda: ['-shared'],
-  }
-
   def _execute_link_request(self, link_request):
     object_files = link_request.object_files
 
@@ -163,7 +158,10 @@ def _execute_link_request(self, link_request):
     self.context.log.debug("resulting_shared_lib_path: {}".format(resulting_shared_lib_path))
     # We are executing in the results_dir, so get absolute paths for everything.
     cmd = ([linker.exe_filename] +
-           self.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS) +
+           self.platform.resolve_for_enum_variant({
+             'darwin': ['-Wl,-dylib'],
+             'linux': ['-shared'],
+           }) +
            linker.extra_args +
            ['-o', os.path.abspath(resulting_shared_lib_path)] +
            ['-L{}'.format(lib_dir) for lib_dir in link_request.external_lib_dirs] +

src/python/pants/backend/python/tasks/unpack_wheels.py

@@ -105,9 +105,9 @@ def _name_and_platform(whl):
 
   @memoized_classproperty
   def _current_platform_abbreviation(cls):
-    return NativeBackendPlatform.create().resolve_platform_specific({
-      'darwin': lambda: 'macosx',
-      'linux': lambda: 'linux',
+    return NativeBackendPlatform.create().resolve_for_enum_variant({
+      'darwin': 'macosx',
+      'linux': 'linux',
     })
 
   @classmethod

src/python/pants/engine/fs.py

@@ -57,8 +57,9 @@ def __new__(cls, include, exclude=(), glob_match_error_behavior=None, conjunctio
       cls,
       include=tuple(include),
       exclude=tuple(exclude),
-      glob_match_error_behavior=GlobMatchErrorBehavior.create(glob_match_error_behavior),
-      conjunction=GlobExpansionConjunction.create(conjunction))
+      glob_match_error_behavior=GlobMatchErrorBehavior.create(glob_match_error_behavior,
+                                                              none_is_default=True),
+      conjunction=GlobExpansionConjunction.create(conjunction, none_is_default=True))
 
 
 class PathGlobsAndRoot(datatype([('path_globs', PathGlobs), ('root', text_type)])):

src/python/pants/init/engine_initializer.py

@@ -346,7 +346,8 @@ def setup_legacy_graph_extended(
     rules = (
       [
         RootRule(Console),
-        SingletonRule.from_instance(GlobMatchErrorBehavior.create(glob_match_error_behavior)),
+        SingletonRule.from_instance(GlobMatchErrorBehavior.create(glob_match_error_behavior,
+                                                                  none_is_default=True)),
         SingletonRule.from_instance(build_configuration),
         SingletonRule(SymbolTable, symbol_table),
       ] +

src/python/pants/option/global_options.py

@@ -16,7 +16,7 @@
 from pants.option.optionable import Optionable
 from pants.option.scope import ScopeInfo
 from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin
-from pants.util.objects import datatype, enum
+from pants.util.objects import datatype, enum, register_enum_option
 
 
 class GlobMatchErrorBehavior(enum('failure_behavior', ['ignore', 'warn', 'error'])):
@@ -26,8 +26,6 @@ class GlobMatchErrorBehavior(enum('failure_behavior', ['ignore', 'warn', 'error'
   be aware of any changes to this object's definition.
   """
 
-  default_option_value = 'warn'
-
 
 class ExecutionOptions(datatype([
   'remote_store_server',
@@ -197,12 +195,12 @@ def register_bootstrap_options(cls, register):
              help='Paths to ignore for all filesystem operations performed by pants '
                   '(e.g. BUILD file scanning, glob matching, etc). '
                   'Patterns use the gitignore syntax (https://git-scm.com/docs/gitignore).')
-    register('--glob-expansion-failure', type=str,
-             choices=GlobMatchErrorBehavior.allowed_values,
-             default=GlobMatchErrorBehavior.default_option_value,
-             advanced=True,
-             help="Raise an exception if any targets declaring source files "
-                  "fail to match any glob provided in the 'sources' argument.")
+    register_enum_option(
+      # TODO: allow using the attribute `GlobMatchErrorBehavior.warn` for more safety!
+      register, GlobMatchErrorBehavior, '--glob-expansion-failure', default='warn',
+      advanced=True,
+      help="Raise an exception if any targets declaring source files "
+           "fail to match any glob provided in the 'sources' argument.")
 
     register('--exclude-target-regexp', advanced=True, type=list, default=[], daemon=False,
              metavar='<regexp>', help='Exclude target roots that match these regexes.')