diff --git a/docs/notes/2.22.x.md b/docs/notes/2.22.x.md index 616cf26f603..0fa4cba64fa 100644 --- a/docs/notes/2.22.x.md +++ b/docs/notes/2.22.x.md @@ -24,6 +24,10 @@ If you encounter such discrepancies, and you can't resolve them easily, please [ ### Backends +#### BUILD + +Non-parametrized values in `overrides` will now be removed from the target's address parameters. This fixes [a bug](https://github.com/pantsbuild/pants/issues/20933) where a target with parametrized default values would have inconsistent address parameters with its field values. + #### NEW: SQL A new experimental `SQL` backend was added along with the [sqlfluff diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index 080d01fd219..017af625e0a 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -224,8 +224,9 @@ class Address: ... @property def path_safe_spec(self) -> str: ... - def parametrize(self, parameters: Mapping[str, str]) -> Address: - """Creates a new Address with the given `parameters` merged over self.parameters.""" + def parametrize(self, parameters: Mapping[str, str], replace: bool = False) -> Address: + """Creates a new Address with the given `parameters` merged or replaced over + self.parameters.""" ... def maybe_convert_to_target_generator(self) -> Address: """If this address is generated or parametrized, convert it to its generator target. diff --git a/src/python/pants/engine/internals/parametrize.py b/src/python/pants/engine/internals/parametrize.py index 0fc2bfdefe0..ee1e5f145b8 100644 --- a/src/python/pants/engine/internals/parametrize.py +++ b/src/python/pants/engine/internals/parametrize.py @@ -166,24 +166,45 @@ def expand( except Exception as e: raise Exception(f"Failed to parametrize `{address}`:\n {e}") from e + parameters = address.parameters + non_parametrized = tuple( + (field_name, field_value) + for field_name, field_value in fields.items() + if not isinstance(field_value, Parametrize) + ) if parametrized_groups: # Add the groups as one vector for the cross-product. parametrized.append(parametrized_groups) + unparametrize_keys = {k for k, _ in non_parametrized if k in parameters} + + # Remove non-parametrized fields from the address parameters. + for k in unparametrize_keys: + parameters.pop(k, None) + if not parametrized: + if unparametrize_keys: + address = address.parametrize(parameters, replace=True) yield (address, fields) return - non_parametrized = tuple( - (field_name, field_value) - for field_name, field_value in fields.items() - if not isinstance(field_value, Parametrize) - ) - for parametrized_args in itertools.product(*parametrized): - expanded_address = address.parametrize( - {field_name: alias for field_name, alias, _ in parametrized_args} + expanded_parameters = parameters | { + field_name: alias for field_name, alias, _ in parametrized_args + } + # There will be at most one group per cross product. + group_kwargs: Mapping[str, Any] = next( + ( + field_value.kwargs + for _, _, field_value in parametrized_args + if isinstance(field_value, Parametrize) and field_value.is_group + ), + {}, ) + # Exclude fields from parametrize group from address parameters. + for k in group_kwargs.keys() & parameters.keys(): + expanded_parameters.pop(k, None) + parametrized_args_fields = tuple( (field_name, field_value) for field_name, _, field_value in parametrized_args @@ -191,17 +212,9 @@ def expand( if not (isinstance(field_value, Parametrize) and field_value.is_group) ) expanded_fields: dict[str, Any] = dict(non_parametrized + parametrized_args_fields) - expanded_fields.update( - # There will be at most one group per cross product. - next( - ( - field_value.kwargs - for _, _, field_value in parametrized_args - if isinstance(field_value, Parametrize) and field_value.is_group - ), - {}, - ) - ) + expanded_fields.update(group_kwargs) + + expanded_address = address.parametrize(expanded_parameters, replace=True) yield expanded_address, expanded_fields @staticmethod diff --git a/src/python/pants/engine/internals/parametrize_test.py b/src/python/pants/engine/internals/parametrize_test.py index 37448b73a77..32ebf7f983f 100644 --- a/src/python/pants/engine/internals/parametrize_test.py +++ b/src/python/pants/engine/internals/parametrize_test.py @@ -127,6 +127,59 @@ def test_expand( ) +@pytest.mark.parametrize( + "expected,parameters,fields", + [ + # Single parameters remove any existing parameters from the address. + ([("a:a", {"f": "1"})], {"f": "0"}, {"f": "1"}), + # But keeps other parameters. + ([("a@g=0", {"f": "1"})], {"g": "0"}, {"f": "1"}), + # Test both case at the same time. + ([("a@g=0", {"f": "1"})], {"f": "0", "g": "0"}, {"f": "1"}), + # Group parameters remove existing covered parameters. + ( + [ + ("a@h=0,parametrize=A", {"f": "1", "g": "2", "i": "1"}), + ("a@f=0,h=0,parametrize=B", {"g": "1", "i": "1"}), + ], + {"f": "0", "g": "0", "h": "0", "i": "0"}, + dict( + g="1", + i="1", + **Parametrize("A", f="1", g="2"), # type: ignore[arg-type] + **Parametrize("B"), + ), + ), + # Re-Parametrize existing parameters + ( + [ + ("a@f=1,g=1,h=0", {"f": "1", "g": "1"}), + ("a@f=1,g=2,h=0", {"f": "1", "g": "2"}), + ], + {"f": "0", "g": "0", "h": "0"}, + { + "f": Parametrize("1"), + "g": Parametrize("1", "2"), + }, + ), + ], +) +def test_expand_existing_parameters( + expected: list[tuple[str, dict[str, Any]]], + parameters: dict[str, Any], + fields: dict[str, Any | Parametrize], +) -> None: + assert sorted(expected) == sorted( + ( + (address.spec, result_fields) + for address, result_fields in Parametrize.expand( + Address("a", parameters=parameters), fields + ) + ), + key=lambda value: value[0], + ) + + @pytest.mark.parametrize( "fields, expected_error", [ diff --git a/src/rust/engine/src/externs/address.rs b/src/rust/engine/src/externs/address.rs index 57cbf1dd3c7..f909fc7ce3f 100644 --- a/src/rust/engine/src/externs/address.rs +++ b/src/rust/engine/src/externs/address.rs @@ -680,9 +680,15 @@ impl Address { Ok(format!("{prefix}{path}{target}{generated}{params}")) } - fn parametrize(&self, parameters: BTreeMap) -> Self { - let mut merged_parameters = self.parameters.clone(); - merged_parameters.extend(parameters); + #[pyo3(signature = (parameters, replace=false))] + fn parametrize(&self, parameters: BTreeMap, replace: bool) -> Self { + let merged_parameters = if replace { + parameters + } else { + let mut merged_parameters = parameters.clone(); + merged_parameters.extend(parameters); + merged_parameters + }; Self { spec_path: self.spec_path.clone(),