Skip to content

Commit

Permalink
[nextest-filtering] switch default for package/deps/rdeps/binary from…
Browse files Browse the repository at this point in the history 
… equal to glob

This should not result in any changes to any of those because glob characters
(*, ?, [, ]) aren't legal in those positions.

Retain `Equal` for the kind() operator since there's no real use case for
anything but equality, I think.
  • Loading branch information
@sunshowers
sunshowers committed Nov 27, 2023
1 parent c91f5db commit 97bd625
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 48 deletions.
2 changes: 1 addition & 1 deletion nextest-filtering/src/expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ impl fmt::Display for NameMatcher {
f,
"{}{}",
if *implicit { "" } else { "#" },
DisplayParsedString(glob.glob_str())
DisplayParsedString(glob.as_str())
),
Self::Regex(r) => write!(f, "/{}/", DisplayParsedRegex(r)),
}
Expand Down
74 changes: 40 additions & 34 deletions nextest-filtering/src/parsing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,6 @@ fn parse_equal_matcher(input: Span) -> IResult<Option<NameMatcher>> {
))(input)
}

// This parse will never fail
fn default_matcher(
make: fn(String) -> NameMatcher,
) -> impl FnMut(Span) -> IResult<Option<NameMatcher>> {
move |input: Span| map(parse_matcher_text, |res: Option<String>| res.map(make))(input)
}

#[tracable_parser]
fn parse_regex_inner(input: Span) -> IResult<String> {
enum Frag<'a> {
Expand Down Expand Up @@ -469,20 +462,20 @@ fn parse_regex_matcher(input: Span) -> IResult<Option<NameMatcher>> {

#[tracable_parser]
fn parse_glob_matcher(input: Span) -> IResult<Option<NameMatcher>> {
ws(preceded(char('#'), glob::parse_glob))(input)
ws(preceded(char('#'), |input| glob::parse_glob(input, false)))(input)
}

// This parse will never fail (because default_matcher won't)
fn set_matcher(
make: fn(String) -> NameMatcher,
default_matcher: DefaultMatcher,
) -> impl FnMut(Span) -> IResult<Option<NameMatcher>> {
move |input: Span| {
ws(alt((
parse_regex_matcher,
parse_glob_matcher,
parse_equal_matcher,
parse_contains_matcher,
default_matcher(make),
default_matcher.into_parser(),
)))(input)
}
}
Expand Down Expand Up @@ -525,16 +518,38 @@ fn nullary_set_def(
}
}

#[derive(Copy, Clone, Debug)]
enum DefaultMatcher {
// Equal is no longer used and glob is always favored.
Equal,
Contains,
Glob,
}

impl DefaultMatcher {
fn into_parser(self) -> impl FnMut(Span) -> IResult<Option<NameMatcher>> {
move |input| match self {
Self::Equal => map(parse_matcher_text, |res: Option<String>| {
res.map(NameMatcher::implicit_equal)
})(input),
Self::Contains => map(parse_matcher_text, |res: Option<String>| {
res.map(NameMatcher::implicit_contains)
})(input),
Self::Glob => glob::parse_glob(input, true),
}
}
}

fn unary_set_def(
name: &'static str,
make_default_matcher: fn(String) -> NameMatcher,
default_matcher: DefaultMatcher,
make_set: fn(NameMatcher, SourceSpan) -> SetDef,
) -> impl FnMut(Span) -> IResult<Option<SetDef>> {
move |i| {
let (i, _) = tag(name)(i)?;
let (i, _) = expect_char('(', ParseSingleError::ExpectedOpenParenthesis)(i)?;
let start = i.location_offset();
let (i, res) = set_matcher(make_default_matcher)(i)?;
let (i, res) = set_matcher(default_matcher)(i)?;
let end = i.location_offset();
let (i, _) = recover_unexpected_comma(i)?;
let (i, _) = expect_char(')', ParseSingleError::ExpectedCloseParenthesis)(i)?;
Expand Down Expand Up @@ -580,12 +595,12 @@ fn platform_def(i: Span) -> IResult<Option<SetDef>> {
#[tracable_parser]
fn parse_set_def(input: Span) -> IResult<Option<SetDef>> {
ws(alt((
unary_set_def("package", NameMatcher::implicit_equal, SetDef::Package),
unary_set_def("deps", NameMatcher::implicit_equal, SetDef::Deps),
unary_set_def("rdeps", NameMatcher::implicit_equal, SetDef::Rdeps),
unary_set_def("kind", NameMatcher::implicit_equal, SetDef::Kind),
unary_set_def("binary", NameMatcher::implicit_equal, SetDef::Binary),
unary_set_def("test", NameMatcher::implicit_contains, SetDef::Test),
unary_set_def("package", DefaultMatcher::Glob, SetDef::Package),
unary_set_def("deps", DefaultMatcher::Glob, SetDef::Deps),
unary_set_def("rdeps", DefaultMatcher::Glob, SetDef::Rdeps),
unary_set_def("kind", DefaultMatcher::Equal, SetDef::Kind),
unary_set_def("binary", DefaultMatcher::Glob, SetDef::Binary),
unary_set_def("test", DefaultMatcher::Contains, SetDef::Test),
platform_def,
nullary_set_def("all", || SetDef::All),
nullary_set_def("none", || SetDef::None),
Expand Down Expand Up @@ -976,10 +991,7 @@ mod tests {
assert_set_def!(
parse_set("package(something)"),
Package,
NameMatcher::Equal {
value: "something".to_string(),
implicit: true,
}
make_glob_matcher("something", true)
);

// Explicit contains matching
Expand Down Expand Up @@ -1125,28 +1137,22 @@ mod tests {
assert_eq!(SetDef::None, parse_set("none()"));

assert_set_def!(
parse_set("package(something)"),
parse_set("package(=something)"),
Package,
NameMatcher::Equal {
value: "something".to_string(),
implicit: true,
implicit: false,
}
);
assert_set_def!(
parse_set("deps(something)"),
Deps,
NameMatcher::Equal {
value: "something".to_string(),
implicit: true,
}
make_glob_matcher("something", true)
);
assert_set_def!(
parse_set("rdeps(something)"),
Rdeps,
NameMatcher::Equal {
value: "something".to_string(),
implicit: true,
}
make_glob_matcher("something", true)
);
assert_set_def!(
parse_set("test(something)"),
Expand Down Expand Up @@ -1341,9 +1347,9 @@ mod tests {
fn parse_future_syntax(input: Span) -> IResult<(Option<NameMatcher>, Option<NameMatcher>)> {
let (i, _) = tag("something")(input)?;
let (i, _) = char('(')(i)?;
let (i, n1) = set_matcher(NameMatcher::implicit_contains)(i)?;
let (i, n1) = set_matcher(DefaultMatcher::Contains)(i)?;
let (i, _) = ws(char(','))(i)?;
let (i, n2) = set_matcher(NameMatcher::implicit_contains)(i)?;
let (i, n2) = set_matcher(DefaultMatcher::Contains)(i)?;
let (i, _) = char(')')(i)?;
Ok((i, (n1, n2)))
}
Expand Down
12 changes: 3 additions & 9 deletions nextest-filtering/src/parsing/glob.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ impl GenericGlob {
}

/// Returns the glob string.
pub fn glob_str(&self) -> &str {
pub fn as_str(&self) -> &str {
&self.glob_str
}

Expand All @@ -59,7 +59,7 @@ impl GenericGlob {

// This never returns Err(()) -- instead, it reports an error to the parsing state.
#[tracable_parser]
pub(super) fn parse_glob(input: Span) -> IResult<Option<NameMatcher>> {
pub(super) fn parse_glob(input: Span, implicit: bool) -> IResult<Option<NameMatcher>> {
let (i, res) = match parse_matcher_text(input.clone()) {
Ok((i, res)) => (i, res),
Err(_) => {
Expand All @@ -72,13 +72,7 @@ pub(super) fn parse_glob(input: Span) -> IResult<Option<NameMatcher>> {
};

match GenericGlob::new(parsed_value) {
Ok(glob) => Ok((
i,
Some(NameMatcher::Glob {
glob,
implicit: false,
}),
)),
Ok(glob) => Ok((i, Some(NameMatcher::Glob { glob, implicit }))),
Err(error) => {
let start = input.location_offset();
let end = i.location_offset();
Expand Down
31 changes: 27 additions & 4 deletions nextest-filtering/src/proptest_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,11 @@ impl ParsedExpr<()> {
impl SetDef<()> {
pub(crate) fn strategy() -> impl Strategy<Value = Self> {
prop_oneof![
1 => NameMatcher::default_equal_strategy().prop_map(|s| Self::Package(s, ())),
1 => NameMatcher::default_equal_strategy().prop_map(|s| Self::Deps(s, ())),
1 => NameMatcher::default_equal_strategy().prop_map(|s| Self::Rdeps(s, ())),
1 => NameMatcher::default_glob_strategy().prop_map(|s| Self::Package(s, ())),
1 => NameMatcher::default_glob_strategy().prop_map(|s| Self::Deps(s, ())),
1 => NameMatcher::default_glob_strategy().prop_map(|s| Self::Rdeps(s, ())),
1 => NameMatcher::default_equal_strategy().prop_map(|s| Self::Kind(s, ())),
1 => NameMatcher::default_equal_strategy().prop_map(|s| Self::Binary(s, ())),
1 => NameMatcher::default_glob_strategy().prop_map(|s| Self::Binary(s, ())),
1 => build_platform_strategy().prop_map(|p| Self::Platform(p, ())),
1 => NameMatcher::default_contains_strategy().prop_map(|s| Self::Test(s, ())),
1 => Just(Self::All),
Expand Down Expand Up @@ -164,6 +164,29 @@ impl NameMatcher {
1 => glob_strategy().prop_map(|glob| { Self::Glob { glob, implicit: false }}),
]
}

pub(crate) fn default_glob_strategy() -> impl Strategy<Value = Self> {
prop_oneof![
1 => name_strategy().prop_map(|value| {
Self::Equal { value, implicit: false }
}),
1 => name_strategy().prop_map(|value| {
Self::Contains { value, implicit: false }
}),
1 => regex_strategy().prop_map(Self::Regex),
1 => (glob_strategy(), any::<bool>()).prop_filter_map(
"implicit = true can't begin with operators",
|(glob, implicit)| {
let accept = match (implicit, begins_with_operator(glob.as_str())) {
(false, _) => true,
(true, false) => true,
(true, true) => false,
};
accept.then_some(Self::Glob { glob, implicit })
},
),
]
}
}

fn begins_with_operator(value: &str) -> bool {
Expand Down

0 comments on commit 97bd625

Please sign in to comment.