Tighten up checkstyle plugin subsystem option passing. (pantsbuild#6648)

Ensure we only try to pass options declared by checkstyle plugins.

Fixes pantsbuild#6645

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/plugin_subsystem_base.py

@@ -6,26 +6,50 @@
 
 import json
 
+from pants.option.parser import Parser
 from pants.subsystem.subsystem import Subsystem
 from pants.util.memo import memoized
 
+from pants.contrib.python.checks.checker.common import CheckstylePlugin
+
 
 class PluginSubsystemBase(Subsystem):
   @classmethod
   def plugin_type(cls):
+    """Returns the type of the plugin this subsystem configures."""
     raise NotImplementedError('Subclasses must override and return the plugin type the subsystem '
                               'configures.')
 
+  _option_names = None
+
   @classmethod
   def register_options(cls, register):
-    super(PluginSubsystemBase, cls).register_options(register)
+    option_names = []
+
+    def recording_register(*args, **kwargs):
+      option_names.append(Parser.parse_dest(*args, **kwargs))
+      register(*args, **kwargs)
+
+    super(PluginSubsystemBase, cls).register_options(recording_register)
+
     # All checks have this option.
-    register('--skip', type=bool,
-             help='If enabled, skip this style checker.')
+    recording_register('--skip', type=bool, help='If enabled, skip this style checker.')
+
+    cls.register_plugin_options(recording_register)
+    cls._option_names = frozenset(option_names)
+
+  @classmethod
+  def register_plugin_options(cls, register):
+    """Register options for the corresponding plugin."""
+    raise NotImplementedError('Subclasses must override instead of `register_options`.')
 
   def options_blob(self):
+    assert self._option_names is not None, (
+      'Expected `register_options` to be called before any attempt to read the `options_blob`.'
+    )
     options = self.get_options()
-    options_dict = {option: options.get(option) for option in options}
+    options_dict = {option: options.get(option)
+                    for option in options if option in self._option_names}
     return json.dumps(options_dict) if options_dict else None
 
 
@@ -40,9 +64,14 @@ def default_subsystem_for_plugin(plugin_type):
   :type: :class:`pants.contrib.python.checks.checker.common.CheckstylePlugin`
   :rtype: :class:`pants.contrib.python.checks.tasks.checkstyle.plugin_subsystem_base.PluginSubsystemBase`
   """
+  if not issubclass(plugin_type, CheckstylePlugin):
+    raise ValueError('Can only create a default plugin subsystem for subclasses of {}, given: {}'
+                     .format(CheckstylePlugin, plugin_type))
+
   return type(str('{}Subsystem'.format(plugin_type.__name__)),
               (PluginSubsystemBase,),
               {
                 str('options_scope'): 'pycheck-{}'.format(plugin_type.name()),
-                str('plugin_type'): classmethod(lambda _: plugin_type),
+                str('plugin_type'): classmethod(lambda cls: plugin_type),
+                str('register_plugin_options'): classmethod(lambda cls, register: None),
               })

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/pycodestyle_subsystem.py

@@ -45,8 +45,7 @@ class PyCodeStyleSubsystem(PluginSubsystemBase):
   )
 
   @classmethod
-  def register_options(cls, register):
-    super(PyCodeStyleSubsystem, cls).register_options(register)
+  def register_plugin_options(cls, register):
     register('--ignore', fingerprint=True, type=list, default=cls.DEFAULT_IGNORE_CODES,
              help='Prevent test failure but still produce output for problems.')
     register('--max-length', fingerprint=True, type=int, default=100,

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/pyflakes_subsystem.py

@@ -12,8 +12,7 @@ class FlakeCheckSubsystem(PluginSubsystemBase):
   options_scope = 'pycheck-pyflakes'
 
   @classmethod
-  def register_options(cls, register):
-    super(FlakeCheckSubsystem, cls).register_options(register)
+  def register_plugin_options(cls, register):
     register('--ignore', fingerprint=True, type=list, default=[],
              help='List of warning codes to ignore.')
 

contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_plugin_subsystem_base.py

@@ -0,0 +1,65 @@
+# coding=utf-8
+# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from __future__ import absolute_import, division, print_function, unicode_literals
+
+import json
+import unittest
+
+from pants_test.subsystem import subsystem_util
+
+from pants.contrib.python.checks.checker.common import CheckstylePlugin
+from pants.contrib.python.checks.tasks.checkstyle.plugin_subsystem_base import (PluginSubsystemBase,
+                                                                                default_subsystem_for_plugin)
+
+
+class Plugin(CheckstylePlugin):
+  @classmethod
+  def name(cls):
+    return 'test-plugin'
+
+
+class PluginSubsystem(PluginSubsystemBase):
+  options_scope = 'pycheck-test-plugin'
+
+  @classmethod
+  def plugin_type(cls):
+    return Plugin
+
+  @classmethod
+  def register_plugin_options(cls, register):
+    register('--bob')
+    register('-j', '--jake', dest='jane')
+
+
+class PluginSubsystemBaseTest(unittest.TestCase):
+  def assert_options(self, subsystem_type, **input_opts):
+    expected = {'skip': True}
+    expected.update(**input_opts)
+
+    unexpected = {('random', 'global', 'option', 'non', 'string', 'key'): 42,
+                  'another_unneeded': 137}
+
+    opts = expected.copy()
+    opts.update(unexpected)
+
+    subsystem_util.init_subsystem(subsystem_type,
+                                  options={subsystem_type.options_scope: opts})
+
+    subsystem_instance = subsystem_type.global_instance()
+    self.assertTrue(subsystem_instance.get_options().skip)
+    self.assertEqual(expected, json.loads(subsystem_instance.options_blob()))
+
+  def test_default_subsystem_for_plugin(self):
+    subsystem_type = default_subsystem_for_plugin(Plugin)
+    self.assertEqual('pycheck-test-plugin', subsystem_type.options_scope)
+
+    self.assert_options(subsystem_type)
+
+  def test_default_subsystem_for_plugin_bad_plugin(self):
+    with self.assertRaises(ValueError):
+      default_subsystem_for_plugin(object)
+
+  def test_options_blob(self):
+    self.assert_options(PluginSubsystem, bob=42, jane=True)