Skip to content

Commit

Permalink
Move most of the remaining ArgSplitter logic to Rust. (#21841)
Browse files Browse the repository at this point in the history
Only some deprecation logic remains, which will
be ported in a followup change, to keep this one
small.
  • Loading branch information
benjyw authored Jan 16, 2025
1 parent ef428fb commit a378fe1
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 31 deletions.
1 change: 1 addition & 0 deletions src/python/pants/engine/internals/native_engine.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,7 @@ class PyOptionId:
) -> None: ...

class PySplitArgs:
def builtin_or_auxiliary_goal(self) -> Optional[str]: ...
def goals(self) -> list[str]: ...
def unknown_goals(self) -> list[str]: ...
def specs(self) -> list[str]: ...
Expand Down
21 changes: 2 additions & 19 deletions src/python/pants/option/arg_splitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ def _get_known_goal_scopes(
def split_args(self, args: Sequence[str]) -> SplitArgs:
native_split_args = self._native_arg_splitter.split_args(tuple(args))

builtin_or_auxiliary_goal: str | None = None
canonical_goals = []
for goal in native_split_args.goals():
si = self._known_goal_scopes.get(goal)
if not si or not si.scope:
Expand All @@ -119,24 +117,9 @@ def split_args(self, args: Sequence[str]) -> SplitArgs:
f"The {si.deprecated_scope} goal was renamed to {si.subsystem_cls.options_scope}",
)

if (si.is_builtin or si.is_auxiliary) and (
builtin_or_auxiliary_goal is None or goal.startswith("-")
):
if builtin_or_auxiliary_goal:
canonical_goals.append(builtin_or_auxiliary_goal)
builtin_or_auxiliary_goal = si.scope
else:
canonical_goals.append(si.scope)

if not builtin_or_auxiliary_goal:
if native_split_args.unknown_goals() and UNKNOWN_GOAL_NAME in self._known_goal_scopes:
builtin_or_auxiliary_goal = UNKNOWN_GOAL_NAME
elif not canonical_goals and NO_GOAL_NAME in self._known_goal_scopes:
builtin_or_auxiliary_goal = NO_GOAL_NAME

return SplitArgs(
builtin_or_auxiliary_goal=builtin_or_auxiliary_goal,
goals=canonical_goals,
builtin_or_auxiliary_goal=native_split_args.builtin_or_auxiliary_goal(),
goals=native_split_args.goals(),
unknown_goals=native_split_args.unknown_goals(),
specs=native_split_args.specs(),
passthru=native_split_args.passthru(),
Expand Down
40 changes: 34 additions & 6 deletions src/rust/engine/options/src/arg_splitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ use regex::Regex;
use std::collections::{HashMap, HashSet};
use std::path::{Path, PathBuf};

// These are the names for the built in goals to print help message when there is no goal, or any
// unknown goals respectively. They begin with underlines to exclude them from the list of goals in
// the goal help output.
pub const NO_GOAL_NAME: &str = "__no_goal";
pub const UNKNOWN_GOAL_NAME: &str = "__unknown_goal";

lazy_static! {
static ref SPEC_RE: Regex = Regex::new(r"[/\\.:*#]").unwrap();
static ref SINGLE_DASH_FLAGS: HashSet<&'static str> =
Expand All @@ -15,10 +21,11 @@ lazy_static! {

#[derive(Debug, Eq, PartialEq)]
pub struct SplitArgs {
pub goals: Vec<String>, // The requested known goals.
pub unknown_goals: Vec<String>, // Any requested but unknown goals.
pub specs: Vec<String>, // What to run against, e.g. targets or files/dirs.
pub passthru: Vec<String>, // Any remaining args specified after a -- separator.
pub builtin_or_auxiliary_goal: Option<String>, // Requested builtin/auxiliary goal.
pub goals: Vec<String>, // Requested known goals.
pub unknown_goals: Vec<String>, // Any requested but unknown goals.
pub specs: Vec<String>, // What to run against, e.g. targets or files/dirs.
pub passthru: Vec<String>, // Any remaining args specified after a -- separator.
}

pub struct ArgSplitter {
Expand All @@ -43,6 +50,7 @@ impl ArgSplitter {
}

pub fn split_args(&self, args: Vec<String>) -> SplitArgs {
let mut builtin_or_auxiliary_goal: Option<String> = None;
let mut goals = vec![];
let mut unknown_goals: Vec<String> = vec![];
let mut specs = vec![];
Expand All @@ -64,10 +72,21 @@ impl ArgSplitter {
// We might want to deprecate this behavior and consistently assume that these are goals,
// since the user can always add a `./` prefix to override.
while let Some(arg) = unconsumed_args.pop() {
let goal_info = self.known_goals.get(&arg);
// Some special flags, such as `-v` and `--help`, are implemented as
// goal aliases, so we must check this before checking for any dash prefixes.
if self.known_goals.contains_key(arg.as_str()) {
goals.push(arg);
if let Some(goal_info) = goal_info {
let canonical_scope_name = goal_info.scope_name.clone();
if (goal_info.is_auxiliary || goal_info.is_builtin)
&& (builtin_or_auxiliary_goal.is_none() || arg.starts_with("-"))
{
if let Some(boag) = builtin_or_auxiliary_goal {
goals.push(boag);
}
builtin_or_auxiliary_goal = Some(canonical_scope_name);
} else {
goals.push(canonical_scope_name);
}
} else if arg == "--" {
// Arg is the passthru delimiter.
for item in unconsumed_args.drain(..) {
Expand All @@ -90,7 +109,16 @@ impl ArgSplitter {
}
}

if builtin_or_auxiliary_goal.is_none() {
if !unknown_goals.is_empty() {
builtin_or_auxiliary_goal = Some(UNKNOWN_GOAL_NAME.to_string());
} else if goals.is_empty() {
builtin_or_auxiliary_goal = Some(NO_GOAL_NAME.to_string())
}
}

SplitArgs {
builtin_or_auxiliary_goal,
goals,
unknown_goals,
specs,
Expand Down
122 changes: 116 additions & 6 deletions src/rust/engine/options/src/arg_splitter_tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2025 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use crate::arg_splitter::{ArgSplitter, SplitArgs};
use crate::arg_splitter::{ArgSplitter, SplitArgs, NO_GOAL_NAME, UNKNOWN_GOAL_NAME};
use crate::scope::GoalInfo;
use shlex;
use std::fs::File;
Expand All @@ -20,7 +20,9 @@ fn shlex_and_split_args(build_root: Option<&Path>, args_str: &str) -> SplitArgs
GoalInfo::new("check", false, false, vec![]),
GoalInfo::new("fmt", false, false, vec![]),
GoalInfo::new("test", false, false, vec![]),
GoalInfo::new("help", true, false, vec!["-h", "--help", "--help-advanced"]),
GoalInfo::new("help", true, false, vec!["-h", "--help"]),
GoalInfo::new("help-advanced", true, false, vec!["--help-advanced"]),
GoalInfo::new("help-all", true, false, vec![]),
GoalInfo::new("bsp", false, true, vec![]),
GoalInfo::new("version", true, false, vec!["-v", "-V"]),
],
Expand All @@ -30,9 +32,11 @@ fn shlex_and_split_args(build_root: Option<&Path>, args_str: &str) -> SplitArgs

#[test]
fn test_spec_detection() {
#[track_caller]
fn assert_spec(build_root: Option<&Path>, maybe_spec: &str) {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: Some(NO_GOAL_NAME.to_string()),
goals: vec![],
unknown_goals: vec![],
specs: _sv(&[maybe_spec]),
Expand All @@ -42,9 +46,11 @@ fn test_spec_detection() {
);
}

#[track_caller]
fn assert_goal(build_root: Option<&Path>, spec: &str) {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: Some(UNKNOWN_GOAL_NAME.to_string()),
goals: vec![],
unknown_goals: _sv(&[spec]),
specs: vec![],
Expand Down Expand Up @@ -93,9 +99,11 @@ fn test_spec_detection() {

#[test]
fn test_valid_arg_splits() {
#[track_caller]
fn assert(goals: &[&str], specs: &[&str], args_str: &str) {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(goals),
unknown_goals: vec![],
specs: _sv(specs),
Expand Down Expand Up @@ -151,6 +159,7 @@ fn test_valid_arg_splits() {
fn test_passthru_args() {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["test"]),
unknown_goals: vec![],
specs: _sv(&["foo/bar"]),
Expand All @@ -161,6 +170,7 @@ fn test_passthru_args() {

assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["check", "test"]),
unknown_goals: vec![],
specs: _sv(&[
Expand All @@ -182,6 +192,7 @@ fn test_passthru_args() {
fn test_split_args_simple() {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: Some(NO_GOAL_NAME.to_string()),
goals: vec![],
unknown_goals: vec![],
specs: vec![],
Expand All @@ -192,7 +203,8 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
goals: _sv(&["help"]),
builtin_or_auxiliary_goal: Some("help".to_string()),
goals: vec![],
unknown_goals: vec![],
specs: vec![],
passthru: vec![]
Expand All @@ -202,6 +214,7 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["fmt", "check"]),
unknown_goals: vec![],
specs: _sv(&["::"]),
Expand All @@ -212,6 +225,7 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["fmt", "check"]),
unknown_goals: vec![],
specs: _sv(&["path/to/dir", "file.py", ":target",]),
Expand All @@ -226,6 +240,7 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["run"]),
unknown_goals: vec![],
specs: _sv(&["path/to:bin"]),
Expand All @@ -236,7 +251,8 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
goals: _sv(&["-h"]),
builtin_or_auxiliary_goal: Some("help".to_string()),
goals: vec![],
unknown_goals: vec![],
specs: vec![],
passthru: vec![]
Expand All @@ -246,7 +262,8 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
goals: _sv(&["test", "--help"]),
builtin_or_auxiliary_goal: Some("help".to_string()),
goals: _sv(&["test"]),
unknown_goals: vec![],
specs: vec![],
passthru: vec![]
Expand All @@ -256,7 +273,8 @@ fn test_split_args_simple() {

assert_eq!(
SplitArgs {
goals: _sv(&["test", "--help"]),
builtin_or_auxiliary_goal: Some("help".to_string()),
goals: _sv(&["test"]),
unknown_goals: vec![],
specs: vec![],
passthru: vec![]
Expand All @@ -269,6 +287,7 @@ fn test_split_args_simple() {
fn test_split_args_short_flags() {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["run"]),
unknown_goals: vec![],
specs: _sv(&["path/to:bin"]),
Expand All @@ -279,6 +298,7 @@ fn test_split_args_short_flags() {

assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: None,
goals: _sv(&["run"]),
unknown_goals: vec![],
// An unknown short flag reads as a negative spec.
Expand All @@ -288,3 +308,93 @@ fn test_split_args_short_flags() {
shlex_and_split_args(None, "pants -x run path/to:bin")
);
}

#[test]
fn test_help() {
#[track_caller]
fn assert_help(args_str: &str, expected_goals: Vec<&str>, expected_specs: Vec<&str>) {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: Some("help".to_string()),
goals: _sv(&expected_goals),
unknown_goals: vec![],
specs: _sv(&expected_specs),
passthru: vec![]
},
shlex_and_split_args(None, args_str)
);
}

assert_help("pants help", vec![], vec![]);
assert_help("pants -h", vec![], vec![]);
assert_help("pants --help", vec![], vec![]);
assert_help("pants help test", vec!["test"], vec![]);
assert_help("pants test help", vec!["test"], vec![]);
assert_help("pants test --help", vec!["test"], vec![]);
assert_help("pants --help test", vec!["test"], vec![]);
assert_help("pants test --help check", vec!["test", "check"], vec![]);
assert_help(
"pants test src/foo/bar:baz -h",
vec!["test"],
vec!["src/foo/bar:baz"],
);
assert_help(
"pants help src/foo/bar:baz",
vec![],
vec!["src/foo/bar:baz"],
);
assert_help(
"pants --help src/foo/bar:baz",
vec![],
vec!["src/foo/bar:baz"],
);

#[track_caller]
fn assert_help_advanced(args_str: &str, expected_goals: Vec<&str>, expected_specs: Vec<&str>) {
assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: Some("help-advanced".to_string()),
goals: _sv(&expected_goals),
unknown_goals: vec![],
specs: _sv(&expected_specs),
passthru: vec![]
},
shlex_and_split_args(None, args_str)
);
}

assert_help_advanced("pants help-advanced", vec![], vec![]);
assert_help_advanced("pants --help-advanced", vec![], vec![]);
assert_help_advanced(
"pants test help-advanced check",
vec!["test", "check"],
vec![],
);
assert_help_advanced(
"pants --help-advanced test check",
vec!["test", "check"],
vec![],
);

assert_help_advanced(
"pants test help-advanced src/foo/bar:baz",
vec!["test"],
vec!["src/foo/bar:baz"],
);

assert_help("pants help help-advanced", vec!["help-advanced"], vec![]);
assert_help_advanced("pants help-advanced help", vec!["help"], vec![]);
assert_help("pants --help help-advanced", vec!["help-advanced"], vec![]);
assert_help_advanced("pants --help-advanced help", vec!["help"], vec![]);

assert_eq!(
SplitArgs {
builtin_or_auxiliary_goal: Some("help-all".to_string()),
goals: vec![],
unknown_goals: vec![],
specs: vec![],
passthru: vec![]
},
shlex_and_split_args(None, "pants help-all")
);
}
4 changes: 4 additions & 0 deletions src/rust/engine/src/externs/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ struct PySplitArgs(SplitArgs);

#[pymethods]
impl PySplitArgs {
fn builtin_or_auxiliary_goal(&self) -> &Option<String> {
&self.0.builtin_or_auxiliary_goal
}

fn goals(&self) -> &Vec<String> {
&self.0.goals
}
Expand Down

0 comments on commit a378fe1

Please sign in to comment.