Skip to content

Commit

Permalink
be more careful when parsing adjacently restructed blocks
Browse files Browse the repository at this point in the history
- Pick only errors that are improving situation over previous parse
- Look for the front parser according to its width
  • Loading branch information
pacak committed Mar 24, 2024
1 parent ff4f2b5 commit 5a4ca0a
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 15 deletions.
33 changes: 26 additions & 7 deletions src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ pub use inner::State;
mod inner {
use std::{ops::Range, rc::Rc};

use crate::{error::Message, Args};
use crate::{error::Message, item::Item, Args};

use super::{split_os_argument, Arg, ArgType, ItemState};
#[derive(Clone, Debug)]
Expand Down Expand Up @@ -531,8 +531,19 @@ mod inner {
start..start + span_size
}

pub(crate) fn ranges(&self) -> ArgRangesIter {
ArgRangesIter { args: self, cur: 0 }
pub(crate) fn ranges(&'a self, item: &'a Item) -> ArgRangesIter<'a> {
let width = match item {
Item::Any { .. }
| Item::Positional { .. }
| Item::Command { .. }
| Item::Flag { .. } => 1,
Item::Argument { .. } => 2,
};
ArgRangesIter {
args: self,
cur: 0,
width,
}
}

pub(crate) fn scope(&self) -> Range<usize> {
Expand Down Expand Up @@ -573,10 +584,15 @@ mod inner {

pub(crate) struct ArgRangesIter<'a> {
args: &'a State,
width: usize,
cur: usize,
}
impl<'a> Iterator for ArgRangesIter<'a> {
type Item = (usize, State);
type Item = (
/* start offset */ usize,
/* width of the first item */ usize,
State,
);

fn next(&mut self) -> Option<Self::Item> {
loop {
Expand All @@ -585,14 +601,17 @@ mod inner {
return None;
}
self.cur += 1;

if !self.args.present(cur)? {
continue;
}

if cur + self.width > self.args.items.len() {
return None;
}
// It should be possible to optimize this code a bit
// by checking if first item can possibly
let mut args = self.args.clone();
args.set_scope(cur..self.args.items.len());
return Some((cur, args));
return Some((cur, self.width, args));
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,10 @@ impl Meta {
}

/// Used by adjacent parsers since it inherits behavior of the front item
pub(crate) fn first_item(meta: &Meta) -> Option<Item> {
pub(crate) fn first_item(meta: &Meta) -> Option<&Item> {
match meta {
Meta::And(xs) => xs.first().and_then(Self::first_item),
Meta::Item(item) => Some(*item.clone()),
Meta::Item(item) => Some(item),
Meta::Skip | Meta::Or(_) => None,
Meta::Optional(x)
| Meta::Strict(x)
Expand Down
14 changes: 8 additions & 6 deletions src/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1056,9 +1056,12 @@ where
fn eval(&self, args: &mut State) -> Result<T, Error> {
let original_scope = args.scope();

let mut best_error = if let Some(item) = Meta::first_item(&self.inner.meta()) {
let first_item;
let inner_meta = self.inner.meta();
let mut best_error = if let Some(item) = Meta::first_item(&inner_meta) {
first_item = item;
let missing_item = MissingItem {
item,
item: item.clone(),
position: original_scope.start,
scope: original_scope.clone(),
};
Expand All @@ -1069,7 +1072,7 @@ where
let mut best_args = args.clone();
let mut best_consumed = 0;

for (start, mut this_arg) in args.ranges() {
for (start, width, mut this_arg) in args.ranges(first_item) {
// since we only want to parse things to the right of the first item we perform
// parsing in two passes:
// - try to run the parser showing only single argument available at all the indices
Expand All @@ -1079,8 +1082,7 @@ where
// consider examples "42 -n" and "-n 42"
// without multi step approach first command line also parses into 42
let mut scratch = this_arg.clone();
#[allow(clippy::range_plus_one)] // clippy suggests wrong type
scratch.set_scope(start..start + 1);
scratch.set_scope(start..start + width);
let before = scratch.len();

// nothing to consume, might as well skip this segment right now
Expand Down Expand Up @@ -1124,8 +1126,8 @@ where
if consumed > best_consumed {
best_consumed = consumed;
std::mem::swap(&mut best_args, &mut this_arg);
best_error = err;
}
best_error = err;
break;
}
}
Expand Down
25 changes: 25 additions & 0 deletions tests/adjacent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,28 @@ Available commands:

assert_eq!(r, expected);
}

#[test]
fn two_adjacent_args() {
let x = short('x').argument::<usize>("X");
let y = short('y').argument::<usize>("Y");
let c = short('c').switch();
let point = construct!(x, y).adjacent();
let parser = construct!(point, c).to_options();

let r = parser.run_inner(&["-x", "3", "-y", "4", "-c"]).unwrap();
assert_eq!(r, ((3, 4), true));

// they are adjacent to each other, but the way it is coded currently - they must be adjacent
// to the first element.
// Proper fix is to split "adjacent" into "adjacent to" and "adjacent block"

// let r = parser.run_inner(&["-y", "3", "-x", "4", "-c"]).unwrap();
// assert_eq!(r, ((4, 3), true));

let r = parser
.run_inner(&["-y", "3", "-c", "-x", "4"])
.unwrap_err()
.unwrap_stderr();
assert_eq!(r, "expected `-y=Y`, pass `--help` for usage information");
}

0 comments on commit 5a4ca0a

Please sign in to comment.