Prevent option shadowing.

It was unused (at least deliberately, see below...), confusing, and
required some complicated logic to support.

Highlights:

- Simplifies `OptionValueContainer`. We still need some magic to support
  ranked values, but we no longer need option forwarding magic, which
  was a big part of the complication in that file. Now, instead of its
  own logic being largely (and complicatedly) implemented in terms of
  hasattr/setattr/getattr, it simply wraps a key->rankedvalue dict,
  and there's a small wrapper of `__getattr__`/`__setattr__` magic, just to
  support direct attribute access.

- Also removes the ability to access an option's value by any of its registered
  aliases. This wasn't used in practice, and complicated the code needlessly.
  So for example, in the rare case where we register an option with two names, say
  `-f`, `--foo`, we can now only access the value as `opts.foo`, not `opts.f`. But
  we have almost no such options anyway, so this is hardly important.  Now you
  must access the option by its 'dest' name, as determined by argparse, namely
  the first name in the registration list with len(name) > 1.

- We now detect attempts to shadow options, and error out on them.
  Unfortunately, we had quite a few unintentional shadowed options in
  the codebase (and some of these were quite confusing, so it's a good
  thing we got rid of them). Each such case was fixed by one of the
  following means:

    * Renaming the option.

    * Deferring to the global option of the same name (in some cases
    making that option recursive). This meant expanding the meaning of
    the global option, but in a sensible way.  E.g., previously the
    global `--fail-fast` meant "fail fast on parsing", but now it means
    "fail fast in whatever way makes sense to the code using the option".

    * Removing the inner option.  E.g., the filter task had a `--tag` option that
    wasn't necessary: the global `--tag` option already filters targets
    by tags, and the task had a more general `--tag-regex` option already.

    * Removing the outer option.  Specifically, since so many tasks register
    a `--version` option, it made no sense for the global scope to hog that
    option name for itself. So now only `-V` and `--pants-version` work for
    getting the version of pants itself.  Seems reasonable, especially since
    you already had to use `pants_version` in config, due to some special-casing.

    * Changing task registration names. E.g., the `JvmBundle` task was
    registered as `bundle` under goal `bundle`, meaning that its
    options scope was `bundle.bundle`, elided to just `bundle`.
    Which meant that any other task in the `bundle` goal could shadow its options.
    Renaming the task to `bundle.jvm` fixes this issue, and is also
    a good idea anyway. There's no reason to privilege JVM in this way.

  Note that unfortunately it was not feasible to do a deprecation cycle
  on these option changes, as they interact with the code in subtle ways,
  and getting deprecation right would have been a lot of work.
  However they were mostly config-only options, and `migrate_config.py` should
  deal with them.  When this rolls out, repo owners are going to have
  to pay extra attention to potential issues in their repos, but that
  was going to be true anyway, no matter how this change shaped up.

Testing Done:
CI passes: https://travis-ci.org/pantsbuild/pants/builds/87215317

Reviewed at https://rbcommons.com/s/twitter/r/3035/

build-support/bin/ci.sh

@@ -123,7 +123,7 @@ if [[ "${skip_bootstrap:-false}" == "false" ]]; then
     ./pants ${PANTS_ARGS[@]} ${bootstrap_compile_args[@]} binary \
       src/python/pants/bin:pants_local_binary && \
     mv dist/pants_local_binary.pex pants.pex && \
-    ./pants.pex --version && \
+    ./pants.pex -V && \
     ./pants.pex --pants-version
   ) || die "Failed to bootstrap pants."
 fi

examples/src/python/example/pants_publish_plugin/extra_test_jar_example.py

@@ -20,10 +20,11 @@
 ## for tips on how to adapt this example task for your own custom publishing needs.
 ##
 class ExtraTestJarExample(JarTask):
-  """
-  Example of a pants publish plugin. For every JarLibrary target in the build graph, this plugin
-  will create an 'example.txt' file, which will be placed in an additional jar. During publishing,
-  this additional jar will be published along with the target.
+  """Example of a pants publish plugin.
+
+  For every JarLibrary target in the build graph, this plugin will create an 'example.txt' file,
+  which will be placed in an additional jar. During publishing, this additional jar will be published
+  along with the target.
   """
 
   def __init__(self, context, workdir):

migrations/options/src/python/migrate_config.py

@@ -42,7 +42,7 @@
                                                                  'changed_targets_heuristic_limit'),
   ('scala-compile', 'warning_args'): ('compile.scala', 'warning_args'),
   ('scala-compile', 'no_warning_args'): ('compile.scala', 'no_warning_args'),
-  ('scala-compile', 'runtime-deps'): ('compile.scala', 'runtime-deps'),
+  ('scala-compile', 'runtime_deps'): ('compile.scala', 'runtime_deps'),
   ('scala-compile', 'use_nailgun'): ('compile.scala', 'use_nailgun'),
   ('scala-compile', 'args'): ('compile.scala', 'args'),
 
@@ -67,7 +67,7 @@
 
   ('scala-repl', 'args'): ('repl.scala', 'args'),
 
-  ('checkstyle', 'bootstrap-tools'): ('compile.checkstyle', 'bootstrap_tools'),
+  ('checkstyle', 'bootstrap_tools'): ('compile.checkstyle', 'bootstrap_tools'),
   ('checkstyle', 'configuration'): ('compile.checkstyle', 'configuration'),
   ('checkstyle', 'properties'): ('compile.checkstyle', 'properties'),
 
@@ -80,11 +80,11 @@
   ('jvm', 'debug_config'): ('DEFAULT', 'debug_config'),
   ('jvm', 'debug_port'): ('DEFAULT', 'debug_port'),
 
-  ('scala-compile', 'scalac-plugins'): ('compile.scala', 'plugins'),
-  ('scala-compile', 'scalac-plugin-args'): ('compile.scala', 'plugin_args'),
+  ('scala-compile', 'scalac_plugins'): ('compile.scala', 'plugins'),
+  ('scala-compile', 'scalac_plugin_args'): ('compile.scala', 'plugin_args'),
 
   ('markdown-to-html', 'extensions'): ('markdown', 'extensions'),
-  ('markdown-to-html', 'code-style'): ('markdown', 'code_style'),
+  ('markdown-to-html', 'code_style'): ('markdown', 'code_style'),
 
   # Note: This assumes that ConfluencePublish is registered as the only task in a
   #       goal called 'confluence'.  Adjust if this is not the case in your pants.ini.
@@ -93,25 +93,25 @@
   # JVM tool migrations.
   ('antlr-gen', 'javadeps'): ('gen.antlr', 'antlr3'),
   ('antlr4-gen', 'javadeps'): ('gen.antlr', 'antlr4'),
-  ('scrooge-gen', 'bootstrap-tools'): ('gen.scrooge', 'scrooge'),
-  ('thrift-linter', 'bootstrap-tools'): ('thrift-linter', 'scrooge_linter'),
-  ('wire-gen', 'bootstrap-tools'): ('gen.wire', 'wire_compiler'),
-  ('benchmark-run', 'bootstrap-tools'): ('bench', 'benchmark_tool'),
-  ('benchmark-run', 'agent-bootstrap-tools'): ('bench', 'benchmark_agent'),
-  ('compile.checkstyle', 'bootstrap-tools'): ('compile.checkstyle', 'checkstyle'),
-  ('ivy-resolve', 'bootstrap-tools'): ('resolve.ivy', 'xalan'),
-  ('jar-tool', 'bootstrap-tools'): ('DEFAULT', 'jar-tool'),
-  ('junit-run', 'junit-bootstrap-tools'): ('test.junit', 'junit'),
-  ('junit-run', 'emma-bootstrap-tools'): ('test.junit', 'emma'),
-  ('junit-run', 'cobertura-bootstrap-tools'): ('test.junit', 'cobertura'),
-  ('java-compile', 'jmake-bootstrap-tools'): ('compile.java', 'jmake'),
+  ('scrooge-gen', 'bootstrap_tools'): ('gen.scrooge', 'scrooge'),
+  ('thrift-linter', 'bootstrap_tools'): ('thrift-linter', 'scrooge_linter'),
+  ('wire-gen', 'bootstrap_tools'): ('gen.wire', 'wire_compiler'),
+  ('benchmark-run', 'bootstrap_tools'): ('bench', 'benchmark_tool'),
+  ('benchmark-run', 'agent_bootstrap_tools'): ('bench', 'benchmark_agent'),
+  ('compile.checkstyle', 'bootstrap_tools'): ('compile.checkstyle', 'checkstyle'),
+  ('ivy-resolve', 'bootstrap_tools'): ('resolve.ivy', 'xalan'),
+  ('jar-tool', 'bootstrap_tools'): ('DEFAULT', 'jar-tool'),
+  ('junit-run', 'junit_bootstrap_tools'): ('test.junit', 'junit'),
+  ('junit-run', 'emma_bootstrap_tools'): ('test.junit', 'emma'),
+  ('junit-run', 'cobertura_bootstrap_tools'): ('test.junit', 'cobertura'),
+  ('java-compile', 'jmake_bootstrap_tools'): ('compile.java', 'jmake'),
   ('java-compile', 'compiler-bootstrap-tools'): ('compile.java', 'java_compiler'),
   # Note: compile-bootstrap-tools is not a typo.
-  ('scala-compile', 'compile-bootstrap-tools'): ('compile.scala', 'scalac'),
-  ('scala-compile', 'zinc-bootstrap-tools'): ('compile.scala', 'zinc'),
-  ('scala-compile', 'scalac-plugin-bootstrap-tools'): ('compile.scala', 'plugin_jars'),
-  ('scala-repl', 'bootstrap-tools'): ('repl.scala', 'scala_repl'),
-  ('specs-run', 'bootstrap-tools'): ('test.specs', 'specs'),
+  ('scala-compile', 'compile_bootstrap_tools'): ('compile.scala', 'scalac'),
+  ('scala-compile', 'zinc_bootstrap_tools'): ('compile.scala', 'zinc'),
+  ('scala-compile', 'scalac_plugin_bootstrap_tools'): ('compile.scala', 'plugin_jars'),
+  ('scala-repl', 'bootstrap_tools'): ('repl.scala', 'scala_repl'),
+  ('specs-run', 'bootstrap_tools'): ('test.specs', 'specs'),
 
   # Artifact cache spec migration.
   ('dx-tool', 'read_artifact_caches'): ('dex', 'read_artifact_caches'),
@@ -140,7 +140,7 @@
 
   ('backend', 'python-path'): ('DEFAULT', 'pythonpath'),
 
-  ('python-ipython', 'entry-point'): ('repl.py', 'ipython_entry_point'),
+  ('python-ipython', 'entry_point'): ('repl.py', 'ipython_entry_point'),
   ('python-ipython', 'requirements'): ('repl.py', 'ipython_requirements'),
 
   ('jar-publish', 'restrict_push_branches'): ('publish.jar', 'restrict_push_branches'),
@@ -224,7 +224,7 @@
   ('scrooge-gen', 'java'): ('gen.scrooge', 'service_deps'),
 
   # jar-tool subsystem.
-  ('jar-tool', 'bootstrap-tools'): ('jar-tool', 'jar-tool'),
+  ('jar-tool', 'bootstrap_tools'): ('jar-tool', 'jar_tool'),
   ('jar-tool', 'jvm_args'): ('jar-tool', 'jvm_options'),
 
   # Technically 'indices' and 'indexes' are both acceptable plural forms of 'index'. However
@@ -335,6 +335,51 @@
   ('compile.zinc', 'changed-targets-heuristic-limit'): None,
   ('compile.zinc', 'partition-size-hint'): None,
   ('compile.zinc', 'strategy'): None,
+
+  # Jmake and Apt removal.
+  ('compile.apt', 'args'): ('compile.zinc', 'args'),
+  ('compile.apt', 'jvm_options'): ('compile.zinc', 'jvm_options'),
+  ('compile.java', 'args'): ('compile.zinc', 'args'),
+  ('compile.java', 'jvm_options'): ('compile.zinc', 'jvm_options'),
+
+  # Renaming JarCreate's scope.  It was previously privileged to be called 'jar'. That might seem reasonable,
+  # except that adding anything to the 'jar' goal (e.g., see extra_test_jar_example.py) creates the potential
+  # for option shadowing.
+  # Note: We probably did the "eliding a task with the same name as its goal" thing wrong. Since options are
+  # inherited from outer scopes, this creates lots of potential for option shadowing. We should probably not
+  # have elided the option scope (e.g., the scope should have remained jar.jar, not jar, internally), but rather
+  # done the elision early, when interpreting scopes in config/cmd-line args etc.
+  # TODO: Fix this?  I don't think it would be that difficult, but there might be unintended consequences.
+  # For now we simply avoid it in cases where there's any likelihood of options collision occuring.
+  # In practice this won't be a problem with cases where there really is only one sensible task in a goal,
+  # e.g., the various ConsoleTasks.
+  ('jar', 'use_nailgun'): ('jar.create', 'use_nailgun'),
+  ('jar', 'nailgun_timeout_seconds'): ('jar.create', 'nailgun_timeout_seconds'),
+  ('jar', 'nailgun_connect_attempts'): ('jar.create', 'nailgun_connect_attempts'),
+  ('jar', 'nailgun_server'): ('jar.create', 'nailgun_server'),
+
+  # Renaming JvmBinaryCreate's scope.  It was previously privileged to be called 'binary'.
+  ('binary', 'use_nailgun'): ('binary.jvm', 'use_nailgun'),
+  ('binary', 'nailgun_timeout_seconds'): ('binary.jvm', 'nailgun_timeout_seconds'),
+  ('binary', 'nailgun_connect_attempts'): ('binary.jvm', 'nailgun_connect_attempts'),
+  ('binary', 'nailgun_server'): ('binary.jvm', 'nailgun_server'),
+
+  # Renaming JvmBundleCreate's scope.  It was previously privileged to be called 'bundle'.
+  ('bundle', 'use_nailgun'): ('bundle.jvm', 'use_nailgun'),
+  ('bundle', 'nailgun_timeout_seconds'): ('bundle.jvm', 'nailgun_timeout_seconds'),
+  ('bundle', 'nailgun_connect_attempts'): ('bundle.jvm', 'nailgun_connect_attempts'),
+  ('bundle', 'nailgun_server'): ('bundle.jvm', 'nailgun_server'),
+  ('bundle', 'deployjar'): ('bundle.jvm', 'deployjar'),
+  ('bundle', 'archive'): ('bundle.jvm', 'archive'),
+  ('bundle', 'archive_prefix'): ('bundle.jvm', 'archive_prefix'),
+
+  # Preventing shadowing of global options.
+  ('compile.zinc', 'plugins'): ('compile.zinc', 'scalac_plugins'),
+  ('compile.zinc', 'plugin_args'): ('compile.zinc', 'scalac_plugin_args'),
+  ('gen.protoc', 'plugins'): ('compile.zinc', 'protoc_plugins'),
+
+  # Superceded by the global --tags option.
+  ('filter', 'tags'): None
 }
 
 jvm_global_strategy_removal = ('The JVM global compile strategy was removed in favor of the '
@@ -530,11 +575,10 @@
   ('compile.zinc', 'partition-size-hint'): jvm_global_strategy_removal,
   ('compile.zinc', 'strategy'): jvm_global_strategy_removal,
 
-  # Jmake and Apt removal.
-  ('compile.apt', 'args'): ('compile.zinc', 'args'),
-  ('compile.apt', 'jvm-options'): ('compile.zinc', 'jvm-options'),
-  ('compile.java', 'args'): ('compile.zinc', 'args'),
-  ('compile.java', 'jvm-options'): ('compile.zinc', 'jvm-options'),
+  ('bootstrap.bootstrap-jvm-tools', 'jvm-options'): 'This previously shadowed the same option '
+                                                    'registered by IvyTaskMixin.  If you want to '
+                                                    'specifically configure the shader, use '
+                                                    'shader-jvm-options.'
 }
 
 

src/docs/install.md

@@ -24,7 +24,7 @@ run it.  It's recommended though, that you pin the version of pants.  To do this
 version of pants you just installed:
 
       :::bash
-      ./pants --version
+      ./pants -V
       0.0.42
 
 Then add an entry like so to pants.ini with that version:

src/python/pants/backend/codegen/tasks/protobuf_gen.py

@@ -47,7 +47,7 @@ def register_options(cls, register):
                   '--pants-bootstrapdir.  When changing this parameter you may also need to '
                   'update --javadeps.',
              default='2.4.1')
-    register('--plugins', advanced=True, fingerprint=True, action='append',
+    register('--protoc-plugins', advanced=True, fingerprint=True, action='append',
              help='Names of protobuf plugins to invoke.  Protoc will look for an executable '
                   'named protoc-gen-$NAME on PATH.',
              default=[])

src/python/pants/backend/core/tasks/filter.py

@@ -39,9 +39,6 @@ def register_options(cls, register):
              help='Filter on targets that these targets depend on.')
     register('--regex', action='append', metavar='[+-]regex1,regex2,...',
              help='Filter on target addresses matching these regexes.')
-    # TODO: Do we need this now that we have a global --tag flag? Deprecate this if not.
-    register('--tag', action='append', metavar='[+-]tag1,tag2,...',
-             help='Filter on targets with these tags.')
     register('--tag-regex', action='append', metavar='[+-]regex1,regex2,...',
              help='Filter on targets with tags matching these regexes.')
 
@@ -98,10 +95,6 @@ def filter_for_tag_regex(tag_regex):
       return lambda target: any(map(regex.search, map(str, target.tags)))
     self._filters.extend(create_filters(self.get_options().tag_regex, filter_for_tag_regex))
 
-    def filter_for_tag(tag):
-      return lambda target: tag in map(str, target.tags)
-    self._filters.extend(create_filters(self.get_options().tag, filter_for_tag))
-
   def console_output(self, _):
     wrapped_filter = wrap_filters(self._filters)
     filtered = set()

src/python/pants/backend/jvm/register.py

@@ -155,13 +155,13 @@ def register_goals():
   task(name='scaladoc', action=ScaladocGen).install('doc')
 
   # Bundling.
-  task(name='jar', action=JarCreate).install('jar')
+  task(name='create', action=JarCreate).install('jar')
   detect_duplicates = task(name='dup', action=DuplicateDetector)
 
-  task(name='binary', action=BinaryCreate).install().with_description('Create a runnable binary.')
+  task(name='jvm', action=BinaryCreate).install('binary').with_description('Create a runnable binary.')
   detect_duplicates.install('binary')
 
-  task(name='bundle', action=BundleCreate).install().with_description(
+  task(name='jvm', action=BundleCreate).install('bundle').with_description(
       'Create an application bundle from binary targets.')
   detect_duplicates.install('bundle')
 

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

@@ -76,7 +76,9 @@ def product_types(cls):
   @classmethod
   def register_options(cls, register):
     super(BootstrapJvmTools, cls).register_options(register)
-    register('--jvm-options', action='append', metavar='<option>...',
+    # Must be registered with the shader- prefix, as IvyTaskMixin already registers --jvm-options.
+    # TODO: IvyTaskMixin should probably add an ivy- prefix; there's no reason to privilege it.
+    register('--shader-jvm-options', action='append', metavar='<option>...',
              help='Run the tool shader with these extra jvm options.')
 
   @classmethod

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

@@ -31,8 +31,6 @@ def _isdir(name):
   @classmethod
   def register_options(cls, register):
     super(DuplicateDetector, cls).register_options(register)
-    register('--fail-fast', default=False, action='store_true',
-             help='Fail fast if duplicate classes/resources are found.')
     register('--excludes', default=EXCLUDED_FILES, action='append',
              help='Case insensitive filenames (without directory) to exclude from duplicate check. '
                   'Filenames can be specified in a comma-separated list or by using multiple '

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

@@ -61,8 +61,6 @@ class JUnitRun(TestTaskMixin, JvmToolTaskMixin, JvmTask):
   @classmethod
   def register_options(cls, register):
     super(JUnitRun, cls).register_options(register)
-    register('--fail-fast', action='store_true',
-             help='Fail fast on the first test failure in a suite.')
     register('--batch-size', advanced=True, type=int, default=sys.maxint,
              help='Run at most this many tests in a single test process.')
     register('--test', action='append',

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py

@@ -96,9 +96,9 @@ def get_no_warning_args_default(cls):
   @classmethod
   def register_options(cls, register):
     super(ZincCompile, cls).register_options(register)
-    register('--plugins', advanced=True, action='append', fingerprint=True,
+    register('--scalac-plugins', advanced=True, action='append', fingerprint=True,
              help='Use these scalac plugins.')
-    register('--plugin-args', advanced=True, type=dict_option, default={}, fingerprint=True,
+    register('--scalac-plugin-args', advanced=True, type=dict_option, default={}, fingerprint=True,
              help='Map from plugin name to list of arguments for that plugin.')
     # TODO: disable by default because it breaks dependency parsing:
     #   https://github.com/pantsbuild/pants/issues/2224

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

@@ -71,10 +71,9 @@ def register_options(cls, register):
                   '(relative to the repo root) matches any of these regexes.')
     register('--jvm-options', action='append', metavar='<option>...', advanced=True,
              help='Run scalastyle with these extra jvm options.')
+    # TODO: Use the task's log level instead of this separate verbosity knob.
     register('--verbose', action='store_true', default=False,
              help='Enable verbose scalastyle output.')
-    register('--quiet', action='store_true', default=False,
-             help='Silence scalastyle error messages.')
     cls.register_jvm_tool(register, 'scalastyle')
 
   @classmethod
@@ -131,7 +130,7 @@ def execute(self):
 
       scalastyle_config = self.validate_scalastyle_config()
       scalastyle_verbose = self.get_options().verbose
-      scalastyle_quiet = self.get_options().quiet
+      scalastyle_quiet = self.get_options().quiet or False
       scalastyle_excluder = self.create_file_excluder()
 
       self.context.log.debug('Non synthetic scala targets to be checked:')

src/python/pants/binaries/binary_util.py

@@ -82,10 +82,6 @@ class NoBaseUrlsError(TaskError):
     """Indicates that no urls were specified in pants.ini."""
     pass
 
-  class MissingBinaryUtilOptionsError(Exception):
-    """Internal error. --supportdir and --version must be registered in register_options()"""
-    pass
-
   def _select_binary_base_path(self, supportdir, version, name, uname_func=None):
     """Calculate the base path.
 

src/python/pants/option/global_options.py

@@ -42,8 +42,8 @@ def register_bootstrap_options(cls, register):
     logging.addLevelName(logging.WARNING, 'WARN')
     register('-l', '--level', choices=['debug', 'info', 'warn'], default='info', recursive=True,
              help='Set the logging level.')
-    register('-q', '--quiet', action='store_true',
-             help='Squelches all console output apart from errors.')
+    register('-q', '--quiet', action='store_true', recursive=True,
+             help='Squelches most console output.')
     # Not really needed in bootstrap options, but putting it here means it displays right
     # after -l and -q in help output, which is conveniently contextual.
     register('--colors', action='store_true', default=True, recursive=True,
@@ -55,9 +55,7 @@ def register_bootstrap_options(cls, register):
     # registration serves in part as documentation of the dependency.
     # TODO(John Sirois): Move pantsbuild.pants bootstrapping into pants itself and have it use this
     # version option directly.
-    register('-V', '--pants-version', '--version',  # NB: pants_version is the 1st long option
-                                                    # since that's the one read from pants.ini;
-                                                    # the version form only works from the CLI.
+    register('-v', '-V', '--pants-version',
              nargs='?',  # Allows using the flag with no args on the CLI to print version as well
                          # as setting the version in pants.ini
              default=pants_version(),  # Displays the current version correctly in `./pants -h`.
@@ -139,9 +137,9 @@ def register_options(cls, register):
              default=[register.bootstrap.pants_workdir],
              help='Ignore these paths when evaluating the command-line target specs.  Useful with '
                   '::, to avoid descending into unneeded directories.')
-    register('--fail-fast', advanced=True, action='store_true',
-             help='When parsing specs, will stop on the first erronous BUILD file encountered. '
-                  'Otherwise, will parse all builds in a spec and then display an error.')
+    register('--fail-fast', advanced=True, action='store_true', recursive=True,
+             help='Exit as quickly as possible on error, rather than attempting to continue '
+                  'to process the non-erroneous subset of the input.')
     register('--cache-key-gen-version', advanced=True, default='200', recursive=True,
              help='The cache key generation. Bump this to invalidate every artifact for a scope.')
     register('--max-subprocess-args', advanced=True, type=int, default=100, recursive=True,