Skip to content

Commit

Permalink
Fix the behavior of list removals in the Rust option code. (#20793)
Browse files Browse the repository at this point in the history
In the Python code, removals trump adds at any precedence. 

Previously, the Rust code applied removals and adds in
source precedence order. This PR makes the Rust implementation
align with the existing Python behavior. 

It adds tests to verify this behavior, copied directly from the Python
tests.
  • Loading branch information
benjyw authored Apr 15, 2024
1 parent 5d0e09e commit b7b0e9c
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 4 deletions.
15 changes: 13 additions & 2 deletions src/rust/engine/options/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,19 +489,30 @@ impl OptionParser {
}
derivation = Some(derivations);
}

// Removals from any source apply after adds from any source (but are themselves
// overridden by later replacements), so we collect them here and apply them later.
let mut removal_lists: Vec<Vec<T>> = vec![];

let mut highest_priority_source = Source::Default;
for (source_type, source) in self.sources.iter() {
if let Some(list_edits) = getter(source, id)? {
highest_priority_source = source_type.clone();
for list_edit in list_edits {
match list_edit.action {
ListEditAction::Replace => list = list_edit.items,
ListEditAction::Replace => {
list = list_edit.items;
removal_lists.clear();
}
ListEditAction::Add => list.extend(list_edit.items),
ListEditAction::Remove => remover(&mut list, &list_edit.items),
ListEditAction::Remove => removal_lists.push(list_edit.items),
}
}
}
}
for removals in removal_lists {
remover(&mut list, &removals);
}
Ok(ListOptionValue {
derivation,
source: highest_priority_source,
Expand Down
61 changes: 59 additions & 2 deletions src/rust/engine/options/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ fn test_parse_list_options() {
);

check(
vec![1, 3, 4],
vec![1, 3],
vec![
(Source::Default, vec![replace(vec![0])]),
(config_source(), vec![replace(vec![1, 2])]),
Expand All @@ -290,7 +290,7 @@ fn test_parse_list_options() {
vec![],
vec![("PANTS_SCOPE_FOO", "+[3, 4]")],
"[scope]\nfoo = [1, 2]",
"[scope]\nfoo = '-[2, 4]'", // 2 should be removed, but not 4, since env has precedence.
"[scope]\nfoo = '-[2, 4]'",
);

check(
Expand Down Expand Up @@ -330,6 +330,63 @@ fn test_parse_list_options() {
"",
"",
);

// Filtering all instances of repeated values.
check(
vec![1, 2, 2, 4],
vec![
(Source::Default, vec![replace(vec![0])]),
(config_source(), vec![add(vec![1, 2, 3, 2, 0, 3, 3, 4])]),
(Source::Env, vec![remove(vec![0, 3])]),
],
vec![],
vec![("PANTS_SCOPE_FOO", "-[0, 3]")],
"[scope]\nfoo.add = [1, 2, 3, 2, 0, 3, 3, 4]",
"",
);

// Filtering a value even though it was appended again at a higher rank.
check(
vec![0, 2],
vec![
(Source::Default, vec![replace(vec![0])]),
(config_source(), vec![add(vec![1, 2])]),
(Source::Env, vec![remove(vec![1])]),
(Source::Flag, vec![add(vec![1])]),
],
vec!["--scope-foo=+[1]"],
vec![("PANTS_SCOPE_FOO", "-[1]")],
"[scope]\nfoo.add = [1, 2]",
"",
);

// Filtering a value even though it was appended again at the same rank.
check(
vec![0, 2],
vec![
(Source::Default, vec![replace(vec![0])]),
(config_source(), vec![add(vec![1, 2])]),
(Source::Env, vec![remove(vec![1]), add(vec![1])]),
],
vec![],
vec![("PANTS_SCOPE_FOO", "-[1],+[1]")],
"[scope]\nfoo.add = [1, 2]",
"",
);

// Overwriting cancels filters.
check(
vec![0],
vec![
(Source::Default, vec![replace(vec![0])]),
(config_source(), vec![remove(vec![0])]),
(Source::Env, vec![replace(vec![0])]),
],
vec![],
vec![("PANTS_SCOPE_FOO", "[0]")],
"[scope]\nfoo.remove = [0]",
"",
);
}

#[test]
Expand Down

0 comments on commit b7b0e9c

Please sign in to comment.