Skip to content

Commit

Permalink
Remove unnecessary logic from ArgSplitter (#21811)
Browse files Browse the repository at this point in the history
Now that options parsing is done in Rust, the ArgSplitter
no longer has to track CLI flags or assign them to scopes.

The ArgSplitter's remaining responsibilities are just
tracking goals and specs. A future change will port that
to Rust as well.
  • Loading branch information
benjyw authored Jan 6, 2025
1 parent 441a17f commit 10a1699
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 106 deletions.
7 changes: 0 additions & 7 deletions src/python/pants/bin/local_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,18 +102,11 @@ def create(
run_tracker = RunTracker(options_bootstrapper.args, options)
native_engine.maybe_set_panic_handler()

# Option values are usually computed lazily on demand, but command line options are
# eagerly computed for validation.
with options_initializer.handle_unknown_flags(options_bootstrapper, env, raise_=True):
# Verify CLI flags.
if not build_config.allow_unknown_options:
options.verify_args()

for scope, values in options.scope_to_flags.items():
if values:
# Only compute values if there were any command line options presented.
options.for_scope(scope)

# Verify configs.
if global_bootstrap_options.verify_config:
options.verify_configs()
Expand Down
34 changes: 5 additions & 29 deletions src/python/pants/option/arg_splitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@

import os.path
from abc import ABC
from collections import defaultdict
from dataclasses import dataclass
from typing import DefaultDict, Iterable, Iterator, Sequence
from typing import Iterable, Iterator, Sequence

from pants.base.deprecated import warn_or_error
from pants.option.scope import GLOBAL_SCOPE, ScopeInfo
from pants.option.scope import ScopeInfo
from pants.util.ordered_set import OrderedSet


Expand All @@ -25,7 +24,6 @@ class SplitArgs:
builtin_or_auxiliary_goal: str | None # Requested builtin goal (explicitly or implicitly).
goals: list[str] # Explicitly requested goals.
unknown_goals: list[str] # Any unknown goals.
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/dirs.
passthru: list[str] # Any remaining args specified after a -- separator.

Expand Down Expand Up @@ -131,23 +129,16 @@ def split_args(self, args: Sequence[str]) -> SplitArgs:
Returns a SplitArgs tuple.
"""
goals: OrderedSet[str] = OrderedSet()
scope_to_flags: DefaultDict[str, list[str]] = defaultdict(list)
specs: list[str] = []
passthru: list[str] = []
unknown_scopes: list[str] = []
builtin_or_auxiliary_goal: str | None = None

def add_scope(s: str) -> None:
# Force the scope to appear, even if empty.
if s not in scope_to_flags:
scope_to_flags[s] = []

def add_goal(scope: str) -> str:
"""Returns the scope name to assign flags to."""
scope_info = self._known_goal_scopes.get(scope)
if not scope_info:
unknown_scopes.append(scope)
add_scope(scope)
return scope

nonlocal builtin_or_auxiliary_goal
Expand All @@ -161,7 +152,6 @@ def add_goal(scope: str) -> str:
builtin_or_auxiliary_goal = scope_info.scope
else:
goals.add(scope_info.scope)
add_scope(scope_info.scope)

# Use builtin/auxiliary goal as default scope for args.
return builtin_or_auxiliary_goal or scope_info.scope
Expand All @@ -170,28 +160,15 @@ def add_goal(scope: str) -> str:
# The first token is the binary name, so skip it.
self._unconsumed_args.pop()

def assign_flag_to_scope(flg: str, default_scope: str) -> None:
flag_scope, descoped_flag = self._descope_flag(flg, default_scope=default_scope)
scope_to_flags[flag_scope].append(descoped_flag)

global_flags = self._consume_flags()
add_scope(GLOBAL_SCOPE)
for flag in global_flags:
assign_flag_to_scope(flag, GLOBAL_SCOPE)

self._consume_flags()
scope, flags = self._consume_scope()
while scope:
# `add_goal` returns the currently active scope to assign flags to.
scope = add_goal(scope)
for flag in flags:
assign_flag_to_scope(flag, GLOBAL_SCOPE if self.is_level_short_arg(flag) else scope)
add_goal(scope)
scope, flags = self._consume_scope()

while self._unconsumed_args and not self._at_standalone_double_dash():
if self._at_flag():
arg = self._unconsumed_args.pop()
# We assume any args here are in global scope.
assign_flag_to_scope(arg, GLOBAL_SCOPE)
self._unconsumed_args.pop()
continue

arg = self._unconsumed_args.pop()
Expand Down Expand Up @@ -228,7 +205,6 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None:
builtin_or_auxiliary_goal=builtin_or_auxiliary_goal,
goals=list(goals),
unknown_goals=unknown_scopes,
scope_to_flags=dict(scope_to_flags),
specs=specs,
passthru=passthru,
)
Expand Down
70 changes: 7 additions & 63 deletions src/python/pants/option/arg_splitter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ def assert_valid_split(
args_str: str,
*,
expected_goals: list[str],
expected_scope_to_flags: dict[str, list[str]],
expected_specs: list[str],
expected_passthru: list[str] | None = None,
expected_is_help: bool = False,
Expand All @@ -49,7 +48,6 @@ def assert_valid_split(
args = shlex.split(args_str)
split_args = splitter.split_args(args)
assert expected_goals == split_args.goals
assert expected_scope_to_flags == split_args.scope_to_flags
assert expected_specs == split_args.specs
assert expected_passthru == split_args.passthru

Expand Down Expand Up @@ -115,7 +113,6 @@ def goal_split_test(command_line: str, **expected):
command_line,
{
"expected_goals": ["test"],
"expected_scope_to_flags": {"": [], "test": []},
**expected,
},
)
Expand All @@ -130,11 +127,6 @@ def goal_split_test(command_line: str, **expected):
+ " src/java/org/pantsbuild/foo src/java/org/pantsbuild/bar:baz",
dict(
expected_goals=["check", "test"],
expected_scope_to_flags={
"": ["--gg", "-ltrace"],
"check": ["--long-flag", "--cc"],
"test": ["--ii"],
},
expected_specs=["src/java/org/pantsbuild/foo", "src/java/org/pantsbuild/bar:baz"],
),
),
Expand All @@ -144,11 +136,6 @@ def goal_split_test(command_line: str, **expected):
+ " --another-global",
dict(
expected_goals=["check", "test"],
expected_scope_to_flags={
"": ["--fff=arg", "-ltrace", "--another-global"],
"check": ["--gg-gg=arg-arg", "--long-flag"],
"test": ["--iii"],
},
expected_specs=["src/java/org/pantsbuild/foo", "src/java/org/pantsbuild/bar:baz"],
),
),
Expand All @@ -157,23 +144,20 @@ def goal_split_test(command_line: str, **expected):
"./pants check test foo::",
dict(
expected_goals=["check", "test"],
expected_scope_to_flags={"": [], "check": [], "test": []},
expected_specs=["foo::"],
),
),
(
"./pants check test foo::",
dict(
expected_goals=["check", "test"],
expected_scope_to_flags={"": [], "check": [], "test": []},
expected_specs=["foo::"],
),
),
(
"./pants check test:test",
dict(
expected_goals=["check"],
expected_scope_to_flags={"": [], "check": []},
expected_specs=["test:test"],
),
),
Expand All @@ -194,7 +178,6 @@ def goal_split_test(command_line: str, **expected):
"./pants test check.java",
dict(
expected_goals=["test"],
expected_scope_to_flags={"": [], "test": []},
expected_specs=["check.java"],
),
),
Expand All @@ -215,15 +198,13 @@ def test_descoping_qualified_flags(splitter: ArgSplitter) -> None:
splitter,
"./pants check test --check-bar --no-test-baz foo/bar",
expected_goals=["check", "test"],
expected_scope_to_flags={"": [], "check": ["--bar"], "test": ["--no-baz"]},
expected_specs=["foo/bar"],
)
# Qualified flags don't count as explicit goals.
assert_valid_split(
splitter,
"./pants check --test-bar foo/bar",
expected_goals=["check"],
expected_scope_to_flags={"": [], "check": [], "test": ["--bar"]},
expected_specs=["foo/bar"],
)

Expand All @@ -233,7 +214,6 @@ def test_passthru_args(splitter: ArgSplitter) -> None:
splitter,
"./pants test foo/bar -- -t 'this is the arg'",
expected_goals=["test"],
expected_scope_to_flags={"": [], "test": []},
expected_specs=["foo/bar"],
expected_passthru=["-t", "this is the arg"],
)
Expand All @@ -243,11 +223,6 @@ def test_passthru_args(splitter: ArgSplitter) -> None:
+ " --check-long-flag src/java/org/pantsbuild/foo src/java/org/pantsbuild/bar:baz --"
+ " passthru1 passthru2 -linfo",
expected_goals=["check", "test"],
expected_scope_to_flags={
"": ["-lerror", "--fff=arg"],
"check": ["--gg-gg=arg-arg", "--long-flag"],
"test": ["--iii"],
},
expected_specs=["src/java/org/pantsbuild/foo", "src/java/org/pantsbuild/bar:baz"],
expected_passthru=["passthru1", "passthru2", "-linfo"],
)
Expand All @@ -259,18 +234,12 @@ def test_subsystem_flags(splitter: ArgSplitter) -> None:
splitter,
"./pants --jvm-options=-Dbar=baz test foo:bar",
expected_goals=["test"],
expected_scope_to_flags={"": [], "jvm": ["--options=-Dbar=baz"], "test": []},
expected_specs=["foo:bar"],
)
assert_valid_split(
splitter,
"./pants test --reporting-template-dir=path foo:bar",
expected_goals=["test"],
expected_scope_to_flags={
"": [],
"reporting": ["--template-dir=path"],
"test": [],
},
expected_specs=["foo:bar"],
)

Expand All @@ -286,11 +255,10 @@ def help_test(command_line: str, **expected):
)


def help_no_arguments_test(command_line: str, *scopes: str, **expected):
def help_no_arguments_test(command_line: str, **expected):
return help_test(
command_line,
expected_goals=[],
expected_scope_to_flags={scope: [] for scope in ("", *scopes)},
expected_specs=[],
**expected,
)
Expand All @@ -300,119 +268,95 @@ def help_no_arguments_test(command_line: str, *scopes: str, **expected):
"command_line, expected",
[
help_no_arguments_test("./pants"),
help_no_arguments_test("./pants help", "help"),
help_no_arguments_test("./pants -h", "help"),
help_no_arguments_test("./pants --help", "help"),
help_no_arguments_test(
"./pants help-advanced", "help-advanced", expected_help_advanced=True
),
help_no_arguments_test(
"./pants --help-advanced", "help-advanced", expected_help_advanced=True
),
help_no_arguments_test("./pants help-all", "help-all", expected_help_all=True),
help_no_arguments_test("./pants help"),
help_no_arguments_test("./pants -h"),
help_no_arguments_test("./pants --help"),
help_no_arguments_test("./pants help-advanced", expected_help_advanced=True),
help_no_arguments_test("./pants --help-advanced", expected_help_advanced=True),
help_no_arguments_test("./pants help-all", expected_help_all=True),
help_test(
"./pants --help-advanced --help",
expected_goals=["help-advanced"],
expected_scope_to_flags={"": [], "help": [], "help-advanced": []},
expected_specs=[],
expected_help_advanced=False,
),
help_test(
"./pants --help --help-advanced --builtin-option --help-advanced-option",
expected_goals=["help"],
expected_scope_to_flags={
"": [],
"help": [],
"help-advanced": ["--builtin-option", "--option"],
},
expected_specs=[],
expected_help_advanced=True,
),
help_test(
"./pants -f",
expected_goals=[],
expected_scope_to_flags={"": []},
expected_specs=["-f"],
),
help_test(
"./pants help check -x",
expected_goals=["check"],
expected_scope_to_flags={"": [], "help": [], "check": []},
expected_specs=["-x"],
),
help_test(
"./pants check -h",
expected_goals=["check"],
expected_scope_to_flags={"": [], "check": [], "help": []},
expected_specs=[],
),
help_test(
"./pants -linfo check -h",
expected_goals=["check"],
expected_scope_to_flags={"": ["-linfo"], "check": [], "help": []},
expected_specs=[],
),
help_test(
"./pants check -h -linfo",
expected_goals=["check"],
expected_scope_to_flags={"": ["-linfo"], "check": [], "help": []},
expected_specs=[],
),
help_test(
"./pants check --help test",
expected_goals=["check", "test"],
expected_scope_to_flags={"": [], "check": [], "help": [], "test": []},
expected_specs=[],
),
help_test(
"./pants test src/foo/bar:baz -h",
expected_goals=["test"],
expected_scope_to_flags={"": [], "test": [], "help": []},
expected_specs=["src/foo/bar:baz"],
),
help_test(
"./pants test src/foo/bar:baz --help",
expected_goals=["test"],
expected_scope_to_flags={"": [], "test": [], "help": []},
expected_specs=["src/foo/bar:baz"],
),
help_test(
"./pants --help test src/foo/bar:baz",
expected_goals=["test"],
expected_scope_to_flags={"": [], "test": [], "help": []},
expected_specs=["src/foo/bar:baz"],
),
help_test(
"./pants test --help src/foo/bar:baz",
expected_goals=["test"],
expected_scope_to_flags={"": [], "test": [], "help": []},
expected_specs=["src/foo/bar:baz"],
),
help_test(
"./pants check --help-advanced test",
expected_goals=["check", "test"],
expected_scope_to_flags={"": [], "check": [], "help-advanced": [], "test": []},
expected_specs=[],
expected_help_advanced=True,
),
help_test(
"./pants help-advanced check",
expected_goals=["check"],
expected_scope_to_flags={"": [], "check": [], "help-advanced": []},
expected_specs=[],
expected_help_advanced=True,
),
help_test(
"./pants check help-all test --help",
expected_goals=["check", "test", "help-all"],
expected_scope_to_flags={"": [], "check": [], "help": [], "help-all": [], "test": []},
expected_specs=[],
expected_help_all=False,
),
help_test(
"./pants bsp --help",
expected_goals=["bsp"],
expected_scope_to_flags={"": [], "help": [], "bsp": []},
expected_specs=[],
),
],
Expand Down
Loading

0 comments on commit 10a1699

Please sign in to comment.