From caf0201340ab7449fe5a11deda1225611cc0fc4f Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Sun, 27 Sep 2020 21:26:11 -0700 Subject: [PATCH] A new mechanism for showing "did you mean" help. Makes suggestions for unknown goals and unknown flags. The old system for suggesting similar flag names was complex, and didn't handle or display scopes in a user-helpful way. So for example if the user used a global flag in goal scope, we would show a suggestion like: "Unknown flag --foobar, did you mean --foobar". This new implementation handles that case, and moves the error message computation and display out of the options system and into the local_pants_runner.py, which has enough context to do so. [ci skip-rust] [ci skip-build-wheels] --- 3rdparty/python/BUILD | 1 - 3rdparty/python/constraints.txt | 11 +- 3rdparty/python/requirements.txt | 1 - src/python/pants/bin/local_pants_runner.py | 58 +++++-- src/python/pants/help/flag_help_printer.py | 55 +++++++ src/python/pants/help/help_formatter.py | 42 ++--- src/python/pants/help/help_info_extracter.py | 16 +- src/python/pants/help/help_printer.py | 41 ++--- src/python/pants/help/maybe_color.py | 18 +++ src/python/pants/init/options_initializer.py | 6 +- src/python/pants/option/arg_splitter.py | 17 +- src/python/pants/option/arg_splitter_test.py | 6 +- src/python/pants/option/errors.py | 16 +- src/python/pants/option/global_options.py | 2 +- src/python/pants/option/options.py | 31 +--- src/python/pants/option/options_test.py | 3 - src/python/pants/option/parser.py | 160 +------------------ 17 files changed, 202 insertions(+), 282 deletions(-) create mode 100644 src/python/pants/help/flag_help_printer.py create mode 100644 src/python/pants/help/maybe_color.py diff --git a/3rdparty/python/BUILD b/3rdparty/python/BUILD index ae0c7280cd1f..6296da02e61c 100644 --- a/3rdparty/python/BUILD +++ b/3rdparty/python/BUILD @@ -6,7 +6,6 @@ python_requirements( module_mapping={ "ansicolors": ["colors"], "beautifulsoup4": ["bs4"], - "python-Levenshtein": ["Levenshtein"], "PyYAML": ["yaml"], "setuptools": ["pkg_resources"], } diff --git a/3rdparty/python/constraints.txt b/3rdparty/python/constraints.txt index 32dd5b796ca7..497a23e6a6c0 100644 --- a/3rdparty/python/constraints.txt +++ b/3rdparty/python/constraints.txt @@ -1,15 +1,15 @@ -# Generated by build-support/bin/generate_lockfile.sh on Mon Sep 21 16:22:12 MST 2020 +# Generated by build-support/bin/generate_lockfile.sh on Mon Sep 28 09:49:00 PDT 2020 ansicolors==1.1.8 attrs==20.2.0 beautifulsoup4==4.6.3 certifi==2020.6.20 cffi==1.14.3 chardet==3.0.4 -cryptography==3.1 +cryptography==3.1.1 dataclasses==0.6 fasteners==0.15 idna==2.10 -importlib-metadata==1.7.0 +importlib-metadata==2.0.0 iniconfig==1.0.1 monotonic==1.5 more-itertools==8.5.0 @@ -18,7 +18,7 @@ mypy-extensions==0.4.3 packaging==20.4 pathspec==0.8.0 pex==2.1.16 -pip==18.1 +pip==19.0.3 pluggy==0.13.1 psutil==5.7.0 py==1.9.0 @@ -27,7 +27,6 @@ pyOpenSSL==19.1.0 pyparsing==2.4.7 pystache==0.5.4 pytest==6.0.2 -python-Levenshtein==0.12.0 PyYAML==5.3.1 requests==2.24.0 setproctitle==1.1.10 @@ -38,4 +37,4 @@ typed-ast==1.4.1 typing-extensions==3.7.4.2 urllib3==1.25.10 www-authenticate==0.9.2 -zipp==3.1.0 +zipp==3.2.0 diff --git a/3rdparty/python/requirements.txt b/3rdparty/python/requirements.txt index 9d5651680011..2cd69d3df485 100644 --- a/3rdparty/python/requirements.txt +++ b/3rdparty/python/requirements.txt @@ -22,7 +22,6 @@ psutil==5.7.0 pystache==0.5.4 # This should be kept in sync with `pytest.py`. pytest>=6.0.1,<6.1 -python-Levenshtein==0.12.0 PyYAML>=5.3.1,<5.4 requests[security]>=2.20.1 setproctitle==1.1.10 diff --git a/src/python/pants/bin/local_pants_runner.py b/src/python/pants/bin/local_pants_runner.py index dae8650c3650..cf04d409497b 100644 --- a/src/python/pants/bin/local_pants_runner.py +++ b/src/python/pants/bin/local_pants_runner.py @@ -3,7 +3,7 @@ import logging import os -from dataclasses import dataclass +from dataclasses import dataclass, replace from typing import Mapping, Optional, Tuple from pants.base.build_environment import get_buildroot @@ -19,11 +19,13 @@ from pants.engine.internals.session import SessionValues from pants.engine.unions import UnionMembership from pants.goal.run_tracker import RunTracker +from pants.help.flag_help_printer import FlagErrorHelpPrinter from pants.help.help_info_extracter import HelpInfoExtracter from pants.help.help_printer import HelpPrinter from pants.init.engine_initializer import EngineInitializer, GraphScheduler, GraphSession from pants.init.options_initializer import BuildConfigInitializer, OptionsInitializer from pants.init.specs_calculator import calculate_specs +from pants.option.errors import UnknownFlagsError from pants.option.options import Options from pants.option.options_bootstrapper import OptionsBootstrapper from pants.option.subsystem import Subsystem @@ -54,16 +56,22 @@ class LocalPantsRunner: profile_path: Optional[str] _run_tracker: RunTracker - @staticmethod + @classmethod def parse_options( + cls, options_bootstrapper: OptionsBootstrapper, ) -> Tuple[Options, BuildConfiguration]: build_config = BuildConfigInitializer.get(options_bootstrapper) - options = OptionsInitializer.create(options_bootstrapper, build_config) + try: + options = OptionsInitializer.create(options_bootstrapper, build_config) + except UnknownFlagsError as err: + cls._handle_unknown_flags(err, options_bootstrapper) + raise return options, build_config - @staticmethod + @classmethod def _init_graph_session( + cls, options_bootstrapper: OptionsBootstrapper, build_config: BuildConfiguration, options: Options, @@ -75,7 +83,11 @@ def _init_graph_session( options_bootstrapper, build_config ) - global_scope = options.for_global_scope() + try: + global_scope = options.for_global_scope() + except UnknownFlagsError as err: + cls._handle_unknown_flags(err, options_bootstrapper) + raise dynamic_ui = global_scope.dynamic_ui if global_scope.v2 else False use_colors = global_scope.get("colors", True) @@ -93,6 +105,16 @@ def _init_graph_session( ), ) + @staticmethod + def _handle_unknown_flags(err: UnknownFlagsError, options_bootstrapper: OptionsBootstrapper): + build_config = BuildConfigInitializer.get(options_bootstrapper) + # We need an options instance in order to get "did you mean" suggestions, but we know + # there are bad flags in the args, so we generate options with no flags. + no_arg_bootstrapper = replace(options_bootstrapper, args=("dummy_first_arg",)) + # Note: doesn't validate, so won't fail on the unknown flags. + options = OptionsInitializer.construct_options(no_arg_bootstrapper, build_config) + FlagErrorHelpPrinter(options).handle_unknown_flags(err) + @classmethod def create( cls, @@ -111,16 +133,7 @@ def create( """ build_root = get_buildroot() global_bootstrap_options = options_bootstrapper.bootstrap_options.for_global_scope() - options, build_config = LocalPantsRunner.parse_options(options_bootstrapper) - - # Option values are usually computed lazily on demand, - # but command line options are eagerly computed for validation. - for scope in options.scope_to_flags.keys(): - options.for_scope(scope) - - # Verify configs. - if global_bootstrap_options.verify_config: - options.verify_configs(options_bootstrapper.config) + options, build_config = cls.parse_options(options_bootstrapper) union_membership = UnionMembership.from_rules(build_config.union_rules) @@ -130,6 +143,19 @@ def create( options_bootstrapper, build_config, options, scheduler ) + # Option values are usually computed lazily on demand, + # but command line options are eagerly computed for validation. + for scope in options.scope_to_flags.keys(): + try: + options.for_scope(scope) + except UnknownFlagsError as err: + cls._handle_unknown_flags(err, options_bootstrapper) + raise + + # Verify configs. + if global_bootstrap_options.verify_config: + options.verify_configs(options_bootstrapper.config) + specs = calculate_specs( options_bootstrapper=options_bootstrapper, options=options, @@ -246,7 +272,7 @@ def run(self, start_time: float) -> ExitCode: bin_name=global_options.pants_bin_name, help_request=self.options.help_request, all_help_info=all_help_info, - use_color=global_options.colors, + color=global_options.colors, ) return help_printer.print_help() diff --git a/src/python/pants/help/flag_help_printer.py b/src/python/pants/help/flag_help_printer.py new file mode 100644 index 000000000000..6f69819d20d3 --- /dev/null +++ b/src/python/pants/help/flag_help_printer.py @@ -0,0 +1,55 @@ +# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +import difflib + +from pants.engine.unions import UnionMembership +from pants.help.help_info_extracter import HelpInfoExtracter +from pants.help.maybe_color import MaybeColor +from pants.option.errors import UnknownFlagsError +from pants.option.options import Options +from pants.option.scope import GLOBAL_SCOPE + + +class FlagErrorHelpPrinter(MaybeColor): + """Prints help related to erroneous command-line flag specification to the console.""" + + def __init__(self, options: Options): + super().__init__(options.for_global_scope().colors) + self._bin_name = options.for_global_scope().pants_bin_name + self._all_help_info = HelpInfoExtracter.get_all_help_info( + options, + # We only care about the options-related help info, so we pass in + # dummy values for union_membership and consumed_scopes_mapper. + UnionMembership({}), + lambda x: tuple(), + ) + + def handle_unknown_flags(self, err: UnknownFlagsError): + global_flags = self._all_help_info.scope_to_help_info[GLOBAL_SCOPE].collect_unscoped_flags() + oshi_for_scope = self._all_help_info.scope_to_help_info.get(err.arg_scope) + possibilities = oshi_for_scope.collect_unscoped_flags() if oshi_for_scope else [] + + if err.arg_scope == GLOBAL_SCOPE: + # We allow all scoped flags for any scope in the global scope position on + # the cmd line (that is, to the left of any goals). + for oshi in self._all_help_info.scope_to_help_info.values(): + possibilities.extend(oshi.collect_scoped_flags()) + + for flag in err.flags: + print(f"Unknown flag {self.maybe_red(flag)} on {err.arg_scope or 'global'} scope") + did_you_mean = difflib.get_close_matches(flag, possibilities) + if err.arg_scope != GLOBAL_SCOPE and flag in global_flags: + # It's a common error to use a global flag in a goal scope, so we special-case it. + print( + f"Did you mean to use the global {self.maybe_cyan(flag)}? " + f"Global options must come before any goals." + ) + elif did_you_mean: + print(f"Did you mean {', '.join(self.maybe_cyan(g) for g in did_you_mean)}?") + + help_cmd = ( + f"{self._bin_name} help" + f"{'' if err.arg_scope == GLOBAL_SCOPE else (' ' + err.arg_scope)}" + ) + print(f"Use `{self.maybe_green(help_cmd)}` to get help.") diff --git a/src/python/pants/help/help_formatter.py b/src/python/pants/help/help_formatter.py index 15368c22f8fe..d81457c4e436 100644 --- a/src/python/pants/help/help_formatter.py +++ b/src/python/pants/help/help_formatter.py @@ -5,32 +5,16 @@ from textwrap import wrap from typing import List, Optional -from colors import cyan, green, magenta, red - from pants.help.help_info_extracter import OptionHelpInfo, OptionScopeHelpInfo, to_help_str +from pants.help.maybe_color import MaybeColor from pants.option.ranked_value import Rank, RankedValue -class HelpFormatter: +class HelpFormatter(MaybeColor): def __init__(self, *, show_advanced: bool, show_deprecated: bool, color: bool) -> None: + super().__init__(color=color) self._show_advanced = show_advanced self._show_deprecated = show_deprecated - self._color = color - - def _maybe_cyan(self, s): - return self._maybe_color(cyan, s) - - def _maybe_green(self, s): - return self._maybe_color(green, s) - - def _maybe_red(self, s): - return self._maybe_color(red, s) - - def _maybe_magenta(self, s): - return self._maybe_color(magenta, s) - - def _maybe_color(self, color, s): - return color(s) if self._color else s def format_options(self, oshi: OptionScopeHelpInfo): """Return a help message for the specified options.""" @@ -42,17 +26,17 @@ def add_option(ohis, *, category=None): display_scope = f"`{oshi.scope}` {goal_or_subsystem}" if oshi.scope else "Global" if category: title = f"{display_scope} {category} options" - lines.append(self._maybe_green(f"{title}\n{'-' * len(title)}")) + lines.append(self.maybe_green(f"{title}\n{'-' * len(title)}")) else: # The basic options section gets the description and options scope info. # No need to repeat those in the advanced section. title = f"{display_scope} options" - lines.append(self._maybe_green(f"{title}\n{'-' * len(title)}")) + lines.append(self.maybe_green(f"{title}\n{'-' * len(title)}")) if oshi.description: lines.append(f"\n{oshi.description}") lines.append(" ") config_section = f"[{oshi.scope or 'GLOBAL'}]" - lines.append(f"Config section: {self._maybe_magenta(config_section)}") + lines.append(f"Config section: {self.maybe_magenta(config_section)}") lines.append(" ") if not ohis: lines.append("None available.") @@ -84,16 +68,16 @@ def format_value(val: RankedValue, prefix: str, left_padding: str) -> List[str]: val_lines = [to_help_str(val.value)] val_lines[0] = f"{prefix}{val_lines[0]}" val_lines[-1] = f"{val_lines[-1]}{maybe_parens(val.details)}" - val_lines = [self._maybe_cyan(f"{left_padding}{line}") for line in val_lines] + val_lines = [self.maybe_cyan(f"{left_padding}{line}") for line in val_lines] return val_lines indent = " " - arg_lines = [f" {self._maybe_magenta(args)}" for args in ohi.display_args] - arg_lines.append(self._maybe_magenta(f" {ohi.env_var}")) - arg_lines.append(self._maybe_magenta(f" {ohi.config_key}")) + arg_lines = [f" {self.maybe_magenta(args)}" for args in ohi.display_args] + arg_lines.append(self.maybe_magenta(f" {ohi.env_var}")) + arg_lines.append(self.maybe_magenta(f" {ohi.config_key}")) choices = "" if ohi.choices is None else f"one of: [{', '.join(ohi.choices)}]" choices_lines = [ - f"{indent}{' ' if i != 0 else ''}{self._maybe_cyan(s)}" + f"{indent}{' ' if i != 0 else ''}{self.maybe_cyan(s)}" for i, s in enumerate(wrap(f"{choices}", 96)) ] default_lines = format_value(RankedValue(Rank.HARDCODED, ohi.default), "default: ", indent) @@ -127,7 +111,7 @@ def format_value(val: RankedValue, prefix: str, left_padding: str) -> List[str]: *description_lines, ] if ohi.deprecated_message: - lines.append(self._maybe_red(f"{indent}{ohi.deprecated_message}.")) + lines.append(self.maybe_red(f"{indent}{ohi.deprecated_message}.")) if ohi.removal_hint: - lines.append(self._maybe_red(f"{indent}{ohi.removal_hint}")) + lines.append(self.maybe_red(f"{indent}{ohi.removal_hint}")) return lines diff --git a/src/python/pants/help/help_info_extracter.py b/src/python/pants/help/help_info_extracter.py index 5faa5cfaef39..595a2f8601ea 100644 --- a/src/python/pants/help/help_info_extracter.py +++ b/src/python/pants/help/help_info_extracter.py @@ -6,7 +6,7 @@ import json from dataclasses import dataclass from enum import Enum -from typing import Any, Callable, Dict, Optional, Tuple, Type, cast +from typing import Any, Callable, Dict, List, Optional, Tuple, Type, cast from pants.base import deprecated from pants.engine.goal import GoalSubsystem @@ -99,6 +99,20 @@ class OptionScopeHelpInfo: advanced: Tuple[OptionHelpInfo, ...] deprecated: Tuple[OptionHelpInfo, ...] + def collect_unscoped_flags(self) -> List[str]: + flags: List[str] = [] + for options in (self.basic, self.advanced, self.deprecated): + for ohi in options: + flags.extend(ohi.unscoped_cmd_line_args) + return flags + + def collect_scoped_flags(self) -> List[str]: + flags: List[str] = [] + for options in (self.basic, self.advanced, self.deprecated): + for ohi in options: + flags.extend(ohi.scoped_cmd_line_args) + return flags + @dataclass(frozen=True) class GoalHelpInfo: diff --git a/src/python/pants/help/help_printer.py b/src/python/pants/help/help_printer.py index f6b10eaf4dfa..02ff30bf4f3c 100644 --- a/src/python/pants/help/help_printer.py +++ b/src/python/pants/help/help_printer.py @@ -2,16 +2,17 @@ # Licensed under the Apache License, Version 2.0 (see LICENSE). import dataclasses +import difflib import json import textwrap from typing import Dict, cast -from colors import cyan, green from typing_extensions import Literal from pants.base.build_environment import pants_version from pants.help.help_formatter import HelpFormatter from pants.help.help_info_extracter import AllHelpInfo, HelpJSONEncoder +from pants.help.maybe_color import MaybeColor from pants.option.arg_splitter import ( AllHelp, GoalsHelp, @@ -24,8 +25,8 @@ from pants.option.scope import GLOBAL_SCOPE -class HelpPrinter: - """Prints help to the console.""" +class HelpPrinter(MaybeColor): + """Prints general and goal-related help to the console.""" def __init__( self, @@ -33,19 +34,19 @@ def __init__( bin_name: str, help_request: HelpRequest, all_help_info: AllHelpInfo, - use_color: bool, + color: bool, ) -> None: + super().__init__(color) self._bin_name = bin_name self._help_request = help_request self._all_help_info = all_help_info - self._use_color = use_color def print_help(self) -> Literal[0, 1]: """Print help to the console.""" def print_hint() -> None: - print(f"Use `{self._bin_name} goals` to list goals.") - print(f"Use `{self._bin_name} help` to get help.") + print(f"Use `{self.maybe_green(self._bin_name + ' goals')}` to list goals.") + print(f"Use `{self.maybe_green(self._bin_name + ' help')}` to get help.") if isinstance(self._help_request, VersionHelp): print(pants_version()) @@ -56,7 +57,17 @@ def print_hint() -> None: elif isinstance(self._help_request, OptionsHelp): self._print_options_help() elif isinstance(self._help_request, UnknownGoalHelp): - print("Unknown goals: {}".format(", ".join(self._help_request.unknown_goals))) + # Only print help and suggestions for the first unknown goal. + # It gets confusing to try and show suggestions for multiple cases. + unknown_goal = self._help_request.unknown_goals[0] + print(f"Unknown goal: {self.maybe_red(unknown_goal)}") + did_you_mean = list( + difflib.get_close_matches( + unknown_goal, self._all_help_info.name_to_goal_info.keys() + ) + ) + if did_you_mean: + print(f"Did you mean: {', '.join(self.maybe_cyan(g) for g in did_you_mean)}?") print_hint() return 1 elif isinstance(self._help_request, NoGoalHelp): @@ -73,17 +84,13 @@ def _print_goals_help(self) -> None: goal_descriptions[goal_info.name] = goal_info.description title_text = "Goals" - title = f"{title_text}\n{'-' * len(title_text)}" - if self._use_color: - title = green(title) + title = self.maybe_green(f"{title_text}\n{'-' * len(title_text)}") max_width = max((len(name) for name in goal_descriptions.keys()), default=0) chars_before_description = max_width + 2 def format_goal(name: str, descr: str) -> str: - name = name.ljust(chars_before_description) - if self._use_color: - name = cyan(name) + name = self.maybe_cyan(name.ljust(chars_before_description)) description_lines = textwrap.wrap(descr, 80 - chars_before_description) if len(description_lines) > 1: description_lines = [ @@ -167,7 +174,7 @@ def _format_help(self, scope: str, show_advanced_and_deprecated: bool) -> str: help_formatter = HelpFormatter( show_advanced=show_advanced_and_deprecated, show_deprecated=show_advanced_and_deprecated, - color=self._use_color, + color=self.color, ) oshi = self._all_help_info.scope_to_help_info.get(scope) if not oshi: @@ -177,9 +184,7 @@ def _format_help(self, scope: str, show_advanced_and_deprecated: bool) -> str: if goal_info: related_scopes = sorted(set(goal_info.consumed_scopes) - {GLOBAL_SCOPE, goal_info.name}) if related_scopes: - related_subsystems_label = "Related subsystems:" - if self._use_color: - related_subsystems_label = green(related_subsystems_label) + related_subsystems_label = self.maybe_green("Related subsystems:") formatted_lines.append(f"{related_subsystems_label} {', '.join(related_scopes)}") formatted_lines.append("") return "\n".join(formatted_lines).rstrip() diff --git a/src/python/pants/help/maybe_color.py b/src/python/pants/help/maybe_color.py new file mode 100644 index 000000000000..8df9f4518577 --- /dev/null +++ b/src/python/pants/help/maybe_color.py @@ -0,0 +1,18 @@ +# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +from colors import cyan, green, magenta, red + + +class MaybeColor: + def __init__(self, color: bool) -> None: + self._color = color + noop = lambda x: x + self.maybe_cyan = cyan if color else noop + self.maybe_green = green if color else noop + self.maybe_red = red if color else noop + self.maybe_magenta = magenta if color else noop + + @property + def color(self) -> bool: + return self._color diff --git a/src/python/pants/init/options_initializer.py b/src/python/pants/init/options_initializer.py index c94c477093fa..dc68db099d9a 100644 --- a/src/python/pants/init/options_initializer.py +++ b/src/python/pants/init/options_initializer.py @@ -74,9 +74,11 @@ class OptionsInitializer: """Initializes options.""" @staticmethod - def _construct_options(options_bootstrapper, build_configuration): + def construct_options(options_bootstrapper, build_configuration): """Parse and register options. + Does not validate, so most clients should call create() below instead. + :returns: An Options object representing the full set of runtime options. """ optionables = {GlobalOptions, *GlobalSubsystems.get(), *build_configuration.optionables} @@ -162,7 +164,7 @@ def create(cls, options_bootstrapper, build_configuration, init_subsystems=True) ) # Parse and register options. - options = cls._construct_options(options_bootstrapper, build_configuration) + options = cls.construct_options(options_bootstrapper, build_configuration) GlobalOptions.validate_instance(options.for_global_scope()) diff --git a/src/python/pants/option/arg_splitter.py b/src/python/pants/option/arg_splitter.py index 2babfaf214fc..11e688ec7e8d 100644 --- a/src/python/pants/option/arg_splitter.py +++ b/src/python/pants/option/arg_splitter.py @@ -24,7 +24,6 @@ class SplitArgs: scope_to_flags: Dict[str, List[str]] # Scope name -> list of flags in that scope. specs: List[str] # The specifications for what to run against, e.g. the targets or files passthru: List[str] # Any remaining args specified after a -- separator. - unknown_scopes: List[str] class HelpRequest(ABC): @@ -91,9 +90,6 @@ class ArgSplitter: def __init__(self, known_scope_infos: Iterable[ScopeInfo], buildroot: str) -> None: self._buildroot = Path(buildroot) self._known_scope_infos = known_scope_infos - # TODO: Get rid of our reliance on known scopes here. We don't really need it now - # that we heuristically identify target specs based on it containing /, : or being - # a top-level directory. self._known_scopes = {si.scope for si in known_scope_infos} | { "version", "goals", @@ -101,7 +97,6 @@ def __init__(self, known_scope_infos: Iterable[ScopeInfo], buildroot: str) -> No "help-advanced", "help-all", } - self._unknown_scopes: List[str] = [] self._unconsumed_args: List[ str ] = [] # In reverse order, for efficient popping off the end. @@ -162,8 +157,9 @@ def add_scope(s: str) -> None: if s not in scope_to_flags: scope_to_flags[s] = [] - specs = [] - passthru = [] + specs: List[str] = [] + passthru: List[str] = [] + unknown_scopes: List[str] = [] self._unconsumed_args = list(reversed(args)) # The first token is the binary name, so skip it. @@ -198,14 +194,14 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None: elif self.likely_a_spec(arg): specs.append(arg) elif arg not in self._known_scopes: - self._unknown_scopes.append(arg) + unknown_scopes.append(arg) if self._at_double_dash(): self._unconsumed_args.pop() passthru = list(reversed(self._unconsumed_args)) - if self._unknown_scopes: - self._help_request = UnknownGoalHelp(tuple(self._unknown_scopes)) + if unknown_scopes: + self._help_request = UnknownGoalHelp(tuple(unknown_scopes)) if not goals and not self._help_request: self._help_request = NoGoalHelp() @@ -217,7 +213,6 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None: scope_to_flags=scope_to_flags, specs=specs, passthru=passthru, - unknown_scopes=self._unknown_scopes, ) def likely_a_spec(self, arg: str) -> bool: diff --git a/src/python/pants/option/arg_splitter_test.py b/src/python/pants/option/arg_splitter_test.py index 552f2b8ba6ff..acd93bdf3010 100644 --- a/src/python/pants/option/arg_splitter_test.py +++ b/src/python/pants/option/arg_splitter_test.py @@ -43,10 +43,8 @@ def assert_valid_split( expected_is_help: bool = False, expected_help_advanced: bool = False, expected_help_all: bool = False, - expected_unknown_scopes: Optional[List[str]] = None ) -> None: expected_passthru = expected_passthru or [] - expected_unknown_scopes = expected_unknown_scopes or [] splitter = ArgSplitter(ArgSplitterTest._known_scope_infos, buildroot=os.getcwd()) args = shlex.split(args_str) split_args = splitter.split_args(args) @@ -59,15 +57,13 @@ def assert_valid_split( isinstance(splitter.help_request, OptionsHelp) and splitter.help_request.advanced ) assert expected_help_all == isinstance(splitter.help_request, AllHelp) - assert expected_unknown_scopes == split_args.unknown_scopes @staticmethod def assert_unknown_goal(args_str: str, unknown_goals: List[str]) -> None: splitter = ArgSplitter(ArgSplitterTest._known_scope_infos, buildroot=os.getcwd()) - result = splitter.split_args(shlex.split(args_str)) + splitter.split_args(shlex.split(args_str)) assert isinstance(splitter.help_request, UnknownGoalHelp) assert set(unknown_goals) == set(splitter.help_request.unknown_goals) - assert result.unknown_scopes == unknown_goals def test_is_spec(self) -> None: unambiguous_specs = [ diff --git a/src/python/pants/option/errors.py b/src/python/pants/option/errors.py index 4f30f719aaf6..84521dca3a57 100644 --- a/src/python/pants/option/errors.py +++ b/src/python/pants/option/errors.py @@ -1,6 +1,8 @@ # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). +from typing import Tuple + from pants.option.scope import GLOBAL_SCOPE @@ -102,5 +104,15 @@ class FromfileError(ParseError): class MutuallyExclusiveOptionError(ParseError): - """Raised when more than one option belonging to the same mutually exclusive group is - specified.""" + """Indicates that two options in the same mutually exclusive group werre specified.""" + + +class UnknownFlagsError(ParseError): + """Indicates that unknown command-line flags were encountered in some scope.""" + + def __init__(self, flags: Tuple[str, ...], arg_scope: str): + self.flags = flags + self.arg_scope = arg_scope + scope = f"scope {self.arg_scope}" if self.arg_scope else "global scope" + msg = f"Unknown flags {', '.join(self.flags)} on {scope}" + super().__init__(msg) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 2d99dfbb6d96..fceb9eff3990 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -257,7 +257,7 @@ def register_bootstrap_options(cls, register): removal_version="2.1.0.dev0", removal_hint=( "The option `--option-name-check-distance` no longer does anything, as Pants now " - "always uses the default of 2." + "uses a different method to compute suggestions." ), ) diff --git a/src/python/pants/option/options.py b/src/python/pants/option/options.py index 0deadd51c01a..894fea6d075a 100644 --- a/src/python/pants/option/options.py +++ b/src/python/pants/option/options.py @@ -11,10 +11,10 @@ from pants.option.config import Config from pants.option.option_util import is_list_option from pants.option.option_value_container import OptionValueContainer, OptionValueContainerBuilder -from pants.option.parser import Parser, ScopedFlagNameForFuzzyMatching +from pants.option.parser import Parser from pants.option.parser_hierarchy import ParserHierarchy, all_enclosing_scopes, enclosing_scope from pants.option.scope import GLOBAL_SCOPE, GLOBAL_SCOPE_CONFIG_SECTION, ScopeInfo -from pants.util.memo import memoized_method, memoized_property +from pants.util.memo import memoized_method from pants.util.ordered_set import FrozenOrderedSet, OrderedSet logger = logging.getLogger(__name__) @@ -170,7 +170,6 @@ def create( parser_hierarchy=parser_hierarchy, bootstrap_option_values=bootstrap_option_values, known_scope_to_info=known_scope_to_info, - unknown_scopes=split_args.unknown_scopes, ) def __init__( @@ -183,7 +182,6 @@ def __init__( parser_hierarchy: ParserHierarchy, bootstrap_option_values: Optional[OptionValueContainer], known_scope_to_info: Dict[str, ScopeInfo], - unknown_scopes: List[str], ) -> None: """The low-level constructor for an Options instance. @@ -198,7 +196,6 @@ def __init__( self._bootstrap_option_values = bootstrap_option_values self._known_scope_to_info = known_scope_to_info self._frozen = False - self._unknown_scopes = unknown_scopes # TODO: Eliminate this in favor of a builder/factory. @property @@ -290,7 +287,6 @@ def drop_flag_values(self) -> "Options": parser_hierarchy=self._parser_hierarchy, bootstrap_option_values=self._bootstrap_option_values, known_scope_to_info=self._known_scope_to_info, - unknown_scopes=self._unknown_scopes, ) def is_known_scope(self, scope: str) -> bool: @@ -388,35 +384,12 @@ def _check_and_apply_deprecations(self, scope, values): hint=f"Use scope {scope} instead (options: {', '.join(explicit_keys)})", ) - @memoized_property - def _all_scoped_flag_names_for_fuzzy_matching(self) -> Iterable[ScopedFlagNameForFuzzyMatching]: - """A list of all registered flags in all their registered scopes. - - This list is used for fuzzy matching against unrecognized option names across registered - scopes on the command line. - """ - all_scoped_flag_names = [] - - def register_all_scoped_names(parser): - scope = parser.scope - known_args = parser.known_args - for arg in known_args: - scoped_flag = ScopedFlagNameForFuzzyMatching( - scope=scope, - arg=arg, - ) - all_scoped_flag_names.append(scoped_flag) - - self.walk_parsers(register_all_scoped_names) - return sorted(all_scoped_flag_names, key=lambda flag_info: flag_info.scoped_arg) - def _make_parse_args_request( self, flags_in_scope, namespace: OptionValueContainerBuilder ) -> Parser.ParseArgsRequest: return Parser.ParseArgsRequest( flags_in_scope=flags_in_scope, namespace=namespace, - get_all_scoped_flag_names=lambda: self._all_scoped_flag_names_for_fuzzy_matching, passthrough_args=self._passthru, ) diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index 629b1b3e8427..9cab5d73fc4d 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -4,7 +4,6 @@ import json import os import shlex -import unittest import unittest.mock import warnings from collections import defaultdict @@ -1608,8 +1607,6 @@ def parse_joined_command_line(*args): bootstrap_options = create_options( { GLOBAL_SCOPE: { - # Set the Levenshtein edit distance to search for misspelled options. - "option_name_check_distance": 2, # If bootstrap option values are provided, this option is accessed and must be provided. "spec_files": [], "spec_file": RankedValue(Rank.HARDCODED, []), diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py index 1b4519911de9..b35a2e98e987 100644 --- a/src/python/pants/option/parser.py +++ b/src/python/pants/option/parser.py @@ -3,11 +3,9 @@ import copy import inspect -import itertools import json import os import re -import textwrap import traceback from collections import defaultdict from dataclasses import dataclass @@ -21,18 +19,14 @@ Iterable, List, Mapping, - NoReturn, Optional, - Sequence, Set, Tuple, Type, Union, ) -import Levenshtein import yaml -from typing_extensions import Protocol from pants.base.build_environment import get_buildroot from pants.base.deprecated import validate_deprecation_semver, warn_or_error @@ -65,6 +59,7 @@ RecursiveSubsystemOption, RegistrationError, Shadowing, + UnknownFlagsError, ) from pants.option.option_util import flatten_shlexed_list, is_dict_option, is_list_option from pants.option.option_value_container import OptionValueContainer, OptionValueContainerBuilder @@ -82,40 +77,6 @@ def final_value(self) -> RankedValue: return self.ranked_values[-1] -@frozen_after_init -@dataclass(unsafe_hash=True) -class ScopedFlagNameForFuzzyMatching: - """Specify how a registered option would look like on the command line. - - This information enables fuzzy matching to suggest correct option names when a user specifies an - unregistered option on the command line. - - scope: the 'scope' component of a command-line flag. - arg: the unscoped flag name as it would appear on the command line. - normalized_arg: the fully-scoped option name, without any leading dashes. - scoped_arg: the fully-scoped option as it would appear on the command line. - """ - - scope: str - arg: str - normalized_arg: str - scoped_arg: str - - def __init__(self, scope: str, arg: str) -> None: - self.scope = scope - self.arg = arg - self.normalized_arg = re.sub("^-+", "", arg) - if scope == GLOBAL_SCOPE: - self.scoped_arg = arg - else: - dashed_scope = scope.replace(".", "-") - self.scoped_arg = f"--{dashed_scope}-{self.normalized_arg}" - - @property - def normalized_scoped_arg(self): - return re.sub(r"^-+", "", self.scoped_arg) - - class Parser: """An argument parser in a hierarchy. @@ -216,35 +177,22 @@ def walk(self, callback: Callable) -> None: @frozen_after_init @dataclass(unsafe_hash=True) class ParseArgsRequest: - # N.B.: We use this callable protocol instead of Callable directly to work around the - # dataclass-specific issue described here: https://github.com/python/mypy/issues/6910 - class FlagNameProvider(Protocol): - def __call__(self) -> Iterable[ScopedFlagNameForFuzzyMatching]: - ... - flag_value_map: Dict[str, List[Any]] namespace: OptionValueContainerBuilder - get_all_scoped_flag_names: FlagNameProvider passthrough_args: List[str] def __init__( self, flags_in_scope: Iterable[str], namespace: OptionValueContainerBuilder, - get_all_scoped_flag_names: FlagNameProvider, passthrough_args: List[str], ) -> None: """ :param flags_in_scope: Iterable of arg strings to parse into flag values. :param namespace: The object to register the flag values on - :param get_all_scoped_flag_names: A 0-argument function which returns an iterable of - all registered option names in all their scopes. This - is used to create an error message with suggestions - when raising a `ParseError`. """ self.flag_value_map = self._create_flag_value_map(flags_in_scope) self.namespace = namespace - self.get_all_scoped_flag_names = get_all_scoped_flag_names self.passthrough_args = passthrough_args @staticmethod @@ -279,7 +227,6 @@ def parse_args(self, parse_args_request: ParseArgsRequest) -> OptionValueContain flag_value_map = parse_args_request.flag_value_map namespace = parse_args_request.namespace - get_all_scoped_flag_names = parse_args_request.get_all_scoped_flag_names mutex_map: DefaultDict[str, List[str]] = defaultdict(list) for args, kwargs in self._unnormalized_option_registrations_iter(): @@ -360,112 +307,11 @@ def add_flag_val(v: Optional[Union[int, float, bool, str]]) -> None: setattr(namespace, dest, val) - # See if there are any unconsumed flags remaining, and if so, raise a ParseError. if flag_value_map: - self._raise_error_for_invalid_flag_names( - tuple(flag_value_map.keys()), - all_scoped_flag_names=get_all_scoped_flag_names(), - max_edit_distance=2, - ) - + # There were unconsumed flags. + raise UnknownFlagsError(tuple(flag_value_map.keys()), self.scope) return namespace.build() - def _raise_error_for_invalid_flag_names( - self, - flags: Sequence[str], - all_scoped_flag_names: Iterable[ScopedFlagNameForFuzzyMatching], - max_edit_distance: int, - ) -> NoReturn: - """Identify similar option names to unconsumed flags and raise a ParseError with those - names.""" - matching_flags: Dict[str, List[str]] = {} - for flag in flags: - # We will be matching option names without their leading hyphens, in order to capture - # both short and long-form options. - flag_normalized_unscoped_name = re.sub(r"^-+", "", flag) - flag_normalized_scoped_name = ( - f"{self.scope.replace('.', '-')}-{flag_normalized_unscoped_name}" - if self.scope != GLOBAL_SCOPE - else flag_normalized_unscoped_name - ) - - substring_matching_flags: List[str] = [] - edit_distance_to_matching_flags: DefaultDict[int, List[str]] = defaultdict(list) - for other_scoped_flag in all_scoped_flag_names: - other_complete_flag_name = other_scoped_flag.scoped_arg - other_normalized_scoped_name = other_scoped_flag.normalized_scoped_arg - other_normalized_unscoped_name = other_scoped_flag.normalized_arg - if flag_normalized_unscoped_name == other_normalized_unscoped_name: - # If the unscoped option name itself matches, but the scope doesn't, display it. - substring_matching_flags.append(other_complete_flag_name) - elif other_normalized_scoped_name.startswith(flag_normalized_scoped_name): - # If the invalid scoped option name is the beginning of another scoped option - # name, display it. This will also suggest long-form options such as --verbose - # for an attempted -v (if -v isn't defined as an option). - substring_matching_flags.append(other_complete_flag_name) - else: - # If an unscoped option name is similar to the unscoped option from the command - # line according to --option-name-check-distance, display the matching scoped - # option name. This covers misspellings. - unscoped_option_edit_distance: int = Levenshtein.distance( - flag_normalized_unscoped_name, other_normalized_unscoped_name - ) - if unscoped_option_edit_distance <= max_edit_distance: - # NB: We order the matched flags by Levenshtein distance compared to the - # entire option string. - fully_scoped_edit_distance: int = Levenshtein.distance( - flag_normalized_scoped_name, other_normalized_scoped_name - ) - edit_distance_to_matching_flags[fully_scoped_edit_distance].append( - other_complete_flag_name - ) - - # If any option name matched or started with the invalid flag in any scope, put that - # first. Then, display the option names matching in order of overall edit distance. - # Flags with the same distance will be sorted alphabetically. - all_matching_scoped_flags = [ - *substring_matching_flags, - *itertools.chain.from_iterable( - matched_flags - for _, matched_flags in sorted(edit_distance_to_matching_flags.items()) - ), - ] - if all_matching_scoped_flags: - matching_flags[flag] = all_matching_scoped_flags - - scope = self._scope_str() - help_instructions_scope = f" {self.scope}" if self.scope != GLOBAL_SCOPE else "" - help_instructions = ( - f"(Run `./pants help-advanced{help_instructions_scope}` for all available options.)" - ) - - if len(flags) == 1: - matches = list(matching_flags.values())[0] if matching_flags else [] - message = textwrap.fill( - ( - f"Unrecognized command line flag {repr(flags[0])} on {scope}." - f"{' Suggestions:' if matching_flags else ''}\n" - ), - 80, - ) - if matching_flags: - message += f"\n{', '.join(matches)}" - raise ParseError(f"{message}\n\n{help_instructions}") - message = textwrap.fill( - ( - f"Unrecognized command line flags on {scope}: {', '.join(flags)}." - f"{' Suggestions:' if matching_flags else ''}\n" - ), - 80, - ) - if matching_flags: - suggestions = "\n".join( - f"{flag_name}: [{', '.join(matches)}]" - for flag_name, matches in matching_flags.items() - ) - message += f"\n{suggestions}" - raise ParseError(f"{message}\n\n{help_instructions}") - def option_registrations_iter(self): """Returns an iterator over the normalized registration arguments of each option in this parser.