use type= to register enum options (pantsbuild#7304)

### Problem

Resolves pantsbuild#7233. We want to make it easy and natural to register enum options, and one way to improve that is to automatically convert options registered as enums to instances of those enums.

### Solution

- Convert hyphenated enum values to underscored `classproperty`s, e.g. `'zinc-only'` becomes `.zinc_only`.
- Remove `register_enum_option()` and use `type=` to automatically convert option values to enum instances.
- Make `__new__` for `enum` classes accept an instance of the enum class and pass it through so options parsing can assume the constructor is idempotent.
- Wrap the `type` checking area in options parsing in a `try` and convert to a `ParseError` with more information about the invalid option.
- Extract `.all_variants` from a `type` arg and use it for `choices` if not explicitly specified, allowing enum `type` options to automatically populate the `choices` argument to `register()`.
- Extract `datatype` and `enum` logic into `DatatypeMixin` (to do special-case logic in `stable_json_sha1()`) and `ChoicesMixin` (to do the special-case logic to get `choices`).

### Result

Registering options as enum classes is much more natural, and has a better error message!

contrib/codeanalysis/src/python/pants/contrib/codeanalysis/tasks/indexable_java_targets.py

@@ -4,6 +4,8 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+from builtins import str
+
 from pants.backend.jvm.targets.jvm_target import JvmTarget
 from pants.build_graph.target_scopes import Scope
 from pants.subsystem.subsystem import Subsystem

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

@@ -118,9 +118,9 @@ def classify_target(cls, target):
     if target.has_sources('.java') or \
       isinstance(target, JUnitTests) or \
       (isinstance(target, ScalaLibrary) and tuple(target.java_sources)):
-      target_type = _JvmCompileWorkflowType('zinc-only')
+      target_type = _JvmCompileWorkflowType.zinc_only
     elif target.has_sources('.scala'):
-      target_type = _JvmCompileWorkflowType('rsc-then-zinc')
+      target_type = _JvmCompileWorkflowType.rsc_then_zinc
     else:
       target_type = None
     return target_type

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

@@ -14,8 +14,7 @@
 from pants.java.nailgun_executor import NailgunExecutor, NailgunProcessGroup
 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
+from pants.util.objects import enum
 
 
 class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
@@ -30,11 +29,11 @@ class ExecutionStrategy(enum([NAILGUN, SUBPROCESS, HERMETIC])): pass
   @classmethod
   def register_options(cls, register):
     super(NailgunTaskBase, cls).register_options(register)
-    register_enum_option(
-      register, cls.ExecutionStrategy, '--execution-strategy', 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. '
-           'Hermetic execution is an experimental subprocess execution framework.')
+    register('--execution-strategy',
+             default=cls.ExecutionStrategy.nailgun, type=cls.ExecutionStrategy,
+             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,
@@ -47,12 +46,9 @@ def register_options(cls, register):
                                           rev='0.9.1'),
                           ])
 
-  @memoized_property
+  @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(self.get_options().execution_strategy)
+    return self.get_options().execution_strategy
 
   @classmethod
   def subsystem_dependencies(cls):

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

@@ -12,7 +12,7 @@
 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, register_enum_option
+from pants.util.objects import enum
 
 
 class ToolchainVariant(enum(['gnu', 'llvm'])): pass
@@ -37,22 +37,16 @@ def register_options(cls, register):
              help='The default for the "compiler_option_sets" argument '
                   'for targets of this language.')
 
-    register_enum_option(
-      register, ToolchainVariant, '--toolchain-variant', advanced=True, default='gnu',
-      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('--toolchain-variant', advanced=True,
+             default=ToolchainVariant.gnu, type=ToolchainVariant,
+             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)
 
   def get_toolchain_variant_for_target(self, target):
-    # TODO(#7233): convert this option into an enum instance using the `type` argument in option
-    # registration!
-    enum_or_value = self.get_target_mirrored_option('toolchain_variant', target)
-    if isinstance(enum_or_value, ToolchainVariant):
-      return enum_or_value
-    else:
-      return ToolchainVariant(enum_or_value)
+    return self.get_target_mirrored_option('toolchain_variant', target)
 
   @classproperty
   def get_compiler_option_sets_enabled_default_value(cls):

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

@@ -9,7 +9,7 @@
 from future.utils import text_type
 
 from pants.base.deprecated import warn_or_error
-from pants.base.hash_utils import stable_json_hash
+from pants.base.hash_utils import stable_json_sha1
 from pants.base.payload import Payload
 from pants.base.payload_field import PayloadField
 from pants.base.validation import assert_list
@@ -22,7 +22,7 @@
 class ConanRequirementSetField(tuple, PayloadField):
 
   def _compute_fingerprint(self):
-    return stable_json_hash(tuple(hash(req) for req in self))
+    return stable_json_sha1(tuple(hash(req) for req in self))
 
 
 class ConanRequirement(datatype([

src/python/pants/base/hash_utils.py

@@ -13,6 +13,7 @@
 
 from pants.base.deprecated import deprecated
 from pants.util.collections_abc_backport import Iterable, Mapping, OrderedDict, Set
+from pants.util.objects import DatatypeMixin
 from pants.util.strutil import ensure_binary
 
 
@@ -89,6 +90,13 @@ def default(self, o):
                         .format(cls=type(self).__name__, val=o))
       # Set order is arbitrary in python 3.6 and 3.7, so we need to keep this sorted() call.
       return sorted(self.default(i) for i in o)
+    elif isinstance(o, DatatypeMixin):
+      # datatype objects will intentionally raise in the __iter__ method, but the Iterable abstract
+      # base class will match any class with any superclass that has the attribute __iter__ in the
+      # __dict__ (see https://docs.python.org/2/library/abc.html), so we need to check for it
+      # specially here.
+      # TODO: determine if the __repr__ should be some abstractmethod on DatatypeMixin!
+      return self.default(repr(o))
     elif isinstance(o, Iterable) and not isinstance(o, (bytes, list, str)):
       return list(self.default(i) for i in o)
     return o

src/python/pants/core_tasks/explain_options_task.py

@@ -11,6 +11,7 @@
 from packaging.version import Version
 
 from pants.option.arg_splitter import GLOBAL_SCOPE
+from pants.option.options_fingerprinter import CoercingOptionEncoder
 from pants.option.ranked_value import RankedValue
 from pants.task.console_task import ConsoleTask
 from pants.version import PANTS_SEMVER
@@ -174,4 +175,4 @@ def console_output(self, targets):
           if self.is_json():
             inner_map["history"] = history_list
     if self.is_json():
-      yield json.dumps(output_map, indent=2, sort_keys=True)
+      yield json.dumps(output_map, indent=2, sort_keys=True, cls=CoercingOptionEncoder)

src/python/pants/engine/rules.py

@@ -232,8 +232,7 @@ def _get_starting_indent(source):
 def _make_rule(output_type, input_selectors, for_goal=None, cacheable=True):
   """A @decorator that declares that a particular static function may be used as a TaskRule.
 
-  :param Constraint output_type: The return/output type for the Rule. This may be either a
-    concrete Python type, or an instance of `Exactly` representing a union of multiple types.
+  :param type output_type: The return/output type for the Rule. This must be a concrete Python type.
   :param list input_selectors: A list of Selector instances that matches the number of arguments
     to the @decorated function.
   :param str for_goal: If this is a @console_rule, which goal string it's called for.
@@ -348,8 +347,8 @@ def get_some_union_type(x):
 
 
 class UnionRule(datatype([
-    ('union_base', type),
-    ('union_member', type),
+    ('union_base', _type_field),
+    ('union_member', _type_field),
 ])):
   """Specify that an instance of `union_member` can be substituted wherever `union_base` is used."""
 
@@ -487,8 +486,8 @@ def add_type_transition_rule(union_rule):
         add_rule(rule)
       else:
         raise TypeError("""\
-Unexpected rule type: {}. Rules either extend Rule or UnionRule, or are static functions decorated \
-with @rule.""".format(type(entry)))
+Rule entry {} had an unexpected type: {}. Rules either extend Rule or UnionRule, or are static \
+functions decorated with @rule.""".format(entry, type(entry)))
 
     return cls(serializable_rules, serializable_roots, union_rules)
 

src/python/pants/goal/run_tracker.py

@@ -28,12 +28,27 @@
 from pants.goal.artifact_cache_stats import ArtifactCacheStats
 from pants.goal.pantsd_stats import PantsDaemonStats
 from pants.option.config import Config
+from pants.option.options_fingerprinter import CoercingOptionEncoder
 from pants.reporting.report import Report
 from pants.stats.statsdb import StatsDBFactory
 from pants.subsystem.subsystem import Subsystem
+from pants.util.collections_abc_backport import OrderedDict
 from pants.util.dirutil import relative_symlink, safe_file_dump
 
 
+class RunTrackerOptionEncoder(CoercingOptionEncoder):
+  """Use the json encoder we use for making options hashable to support datatypes.
+
+  This encoder also explicitly allows OrderedDict inputs, as we accept more than just option values
+  when encoding stats to json.
+  """
+
+  def default(self, o):
+    if isinstance(o, OrderedDict):
+      return o
+    return super(RunTrackerOptionEncoder, self).default(o)
+
+
 class RunTracker(Subsystem):
   """Tracks and times the execution of a pants run.
 
@@ -341,7 +356,7 @@ def error(msg):
     # TODO(benjy): The upload protocol currently requires separate top-level params, with JSON
     # values.  Probably better for there to be one top-level JSON value, namely json.dumps(stats).
     # But this will first require changing the upload receiver at every shop that uses this.
-    params = {k: json.dumps(v) for (k, v) in stats.items()}
+    params = {k: cls._json_dump_options(v) for (k, v) in stats.items()}
     cookies = Cookies.global_instance()
     auth_provider = auth_provider or '<provider>'
 
@@ -371,13 +386,17 @@ def do_post(url, num_redirects_allowed):
     except Exception as e:  # Broad catch - we don't want to fail the build over upload errors.
       return error('Error: {}'.format(e))
 
+  @classmethod
+  def _json_dump_options(cls, stats):
+    return json.dumps(stats, cls=RunTrackerOptionEncoder)
+
   @classmethod
   def write_stats_to_json(cls, file_name, stats):
     """Write stats to a local json file.
 
     :return: True if successfully written, False otherwise.
     """
-    params = json.dumps(stats)
+    params = cls._json_dump_options(stats)
     if PY2:
       params = params.decode('utf-8')
     try:
@@ -412,7 +431,7 @@ def store_stats(self):
     stats_file = os.path.join(get_pants_cachedir(), 'stats',
                               '{}.json'.format(self.run_info.get_info('id')))
     mode = 'w' if PY3 else 'wb'
-    safe_file_dump(stats_file, json.dumps(stats), mode=mode)
+    safe_file_dump(stats_file, self._json_dump_options(stats), mode=mode)
 
     # Add to local stats db.
     StatsDBFactory.global_instance().get_db().insert_stats(stats)

src/python/pants/init/engine_initializer.py

@@ -263,9 +263,7 @@ def setup_legacy_graph(native, options_bootstrapper, build_configuration):
       options_bootstrapper,
       build_configuration,
       native=native,
-      # TODO(#7233): convert this into an enum instance using the `type` argument in option
-      # registration!
-      glob_match_error_behavior=GlobMatchErrorBehavior(bootstrap_options.glob_expansion_failure),
+      glob_match_error_behavior=bootstrap_options.glob_expansion_failure,
       build_ignore_patterns=bootstrap_options.build_ignore,
       exclude_target_regexps=bootstrap_options.exclude_target_regexp,
       subproject_roots=bootstrap_options.subproject_roots,

src/python/pants/init/plugin_resolver.py

@@ -10,7 +10,6 @@
 import site
 from builtins import object, open
 
-from future.utils import PY3
 from pex import resolver
 from pex.base import requirement_is_exact
 from pkg_resources import Requirement
@@ -22,6 +21,7 @@
 from pants.util.contextutil import temporary_dir
 from pants.util.dirutil import safe_mkdir, safe_open
 from pants.util.memo import memoized_property
+from pants.util.strutil import ensure_text
 from pants.version import PANTS_SEMVER
 
 
@@ -99,7 +99,7 @@ def _resolve_exact_plugin_locations(self):
       tmp_plugins_list = resolved_plugins_list + '~'
       with safe_open(tmp_plugins_list, 'w') as fp:
         for plugin in self._resolve_plugins():
-          fp.write(plugin.location if PY3 else plugin.location.decode('utf-8'))
+          fp.write(ensure_text(plugin.location))
           fp.write('\n')
       os.rename(tmp_plugins_list, resolved_plugins_list)
     with open(resolved_plugins_list, 'r') as fp:

src/python/pants/java/nailgun_client.py

@@ -17,6 +17,7 @@
 from pants.java.nailgun_protocol import ChunkType, NailgunProtocol
 from pants.util.osutil import safe_kill
 from pants.util.socket import RecvBufferedSocket
+from pants.util.strutil import ensure_binary
 
 
 logger = logging.getLogger(__name__)
@@ -62,7 +63,7 @@ def _write_flush(self, fd, payload=None):
     """Write a payload to a given fd (if provided) and flush the fd."""
     try:
       if payload:
-        fd.write(payload)
+        fd.write(ensure_binary(payload))
       fd.flush()
     except (IOError, OSError) as e:
       # If a `Broken Pipe` is encountered during a stdio fd write, we're headless - bail.

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, register_enum_option
+from pants.util.objects import datatype, enum
 
 
 class GlobMatchErrorBehavior(enum(['ignore', 'warn', 'error'])):
@@ -195,12 +195,10 @@ 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_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('--glob-expansion-failure', advanced=True,
+             default=GlobMatchErrorBehavior.warn, type=GlobMatchErrorBehavior,
+             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.')

src/python/pants/option/parser.py

@@ -8,7 +8,7 @@
 import os
 import re
 import traceback
-from builtins import next, object, open
+from builtins import next, object, open, str
 from collections import defaultdict
 
 import six
@@ -442,7 +442,13 @@ def to_value_type(val_str):
       elif kwargs.get('type') == bool:
         return self._ensure_bool(val_str)
       else:
-        return self._wrap_type(kwargs.get('type', str))(val_str)
+        type_arg = kwargs.get('type', str)
+        try:
+          return self._wrap_type(type_arg)(val_str)
+        except TypeError as e:
+          raise ParseError(
+            "Error applying type '{}' to option value '{}', for option '--{}' in {}: {}"
+            .format(type_arg.__name__, val_str, dest, self._scope_str(), e))
 
     # Helper function to expand a fromfile=True value string, if needed.
     def expand(val_str):
@@ -551,6 +557,13 @@ def record_option(value, rank, option_details=None):
     def check(val):
       if val is not None:
         choices = kwargs.get('choices')
+        # If the `type` argument has an `all_variants` attribute, use that as `choices` if not
+        # already set. Using an attribute instead of checking a subclass allows `type` arguments
+        # which are functions to have an implicit fallback `choices` set as well.
+        if choices is None and 'type' in kwargs:
+          type_arg = kwargs.get('type')
+          if hasattr(type_arg, 'all_variants'):
+            choices = list(type_arg.all_variants)
         if choices is not None and val not in choices:
           raise ParseError('`{}` is not an allowed value for option {} in {}. '
                            'Must be one of: {}'.format(val, dest, self._scope_str(), choices))