From bc7fd206ec8d50955a30defe93afa0fb66492f53 Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 23 May 2024 13:22:06 +0200 Subject: [PATCH 1/4] Introduce support for per field instance default values. --- .../pants/engine/internals/native_engine.pyi | 24 ++++ src/python/pants/engine/target.py | 1 + src/python/pants/engine/target_test.py | 26 ++++ src/rust/engine/src/externs/target.rs | 132 +++++++++++++----- 4 files changed, 151 insertions(+), 32 deletions(-) diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index 3d29a6b9484..53d3ace63d6 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -269,6 +269,30 @@ class _NoValue: # Marker for unspecified field values that should use the default value if applicable. NO_VALUE: _NoValue + +class FieldDefaultValue: + """Field value wrapper to indicate this should be treated as the default value for this instance + of the field. + + Example: + class SomeField: + default = "some" + + field = SomeField(raw_value=FieldDefaultValue("other"), address=addr) + field.default == "other" + field.value == "other" + + The value is unwrapped and assigned to the `default` property on the field instance. During + `compute_value`, the wrapped value is passed as `raw_value`, while the unwrapped value is + returned from `super().compute_value(...)`. + + """ + value: ImmutableValue + + def __repr__(self) -> str: ... + def __str__(self) -> str: ... + + class Field: """A Field. diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index f25af7b1de6..5b3fa0efd37 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -60,6 +60,7 @@ ) from pants.engine.internals.native_engine import NO_VALUE as NO_VALUE # noqa: F401 from pants.engine.internals.native_engine import Field as Field # noqa: F401 +from pants.engine.internals.native_engine import FieldDefaultValue as FieldDefaultValue # noqa: F401 from pants.engine.unions import UnionMembership, UnionRule, distinct_union_type_per_subclass, union from pants.option.global_options import UnmatchedBuildFileGlobs from pants.source.filespec import Filespec, FilespecMatcher diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py index 74cb89d2edd..4ddddb8f68c 100644 --- a/src/python/pants/engine/target_test.py +++ b/src/python/pants/engine/target_test.py @@ -14,6 +14,7 @@ from pants.engine.target import ( NO_VALUE, AsyncFieldMixin, + FieldDefaultValue, BoolField, CoarsenedTarget, CoarsenedTargets, @@ -471,6 +472,31 @@ def test_target_residence_dir() -> None: ) +def test_field_dynamic_default() -> None: + class SomeField(Field): + alias: str = "some" + default: str = "some-default" + + @classmethod + def compute_value( + cls, raw_value: Optional[str], address: Address + ) -> str: + value_or_default = super().compute_value(raw_value, address) + print(f"compute {raw_value=}, {value_or_default=}") + assert not isinstance(value_or_default, FieldDefaultValue) + return value_or_default + + def check_field(fld: SomeField, value: str, default: str) -> None: + # The class var should never change. + assert SomeField.default == "some-default" + assert fld.value == value + assert fld.default == default + + addr = Address("a") + check_field(SomeField("regular", addr), "regular", SomeField.default) + check_field(SomeField(FieldDefaultValue("other-default"), addr), "other-default", "other-default") + + # ----------------------------------------------------------------------------------------------- # Test CoarsenedTarget # ----------------------------------------------------------------------------------------------- diff --git a/src/rust/engine/src/externs/target.rs b/src/rust/engine/src/externs/target.rs index a817037f02a..f6741a11945 100644 --- a/src/rust/engine/src/externs/target.rs +++ b/src/rust/engine/src/externs/target.rs @@ -3,18 +3,18 @@ use std::fmt::Write; +use crate::externs::address::Address; use pyo3::basic::CompareOp; use pyo3::exceptions::PyValueError; +use pyo3::ffi; use pyo3::intern; use pyo3::prelude::*; use pyo3::types::PyType; -use crate::externs::address::Address; - pub fn register(m: &PyModule) -> PyResult<()> { m.add_class::()?; m.add_class::()?; - + m.add_class::()?; m.add("NO_VALUE", NoFieldValue)?; Ok(()) @@ -35,9 +35,34 @@ impl NoFieldValue { } } +#[pyclass] +#[derive(Clone)] +struct FieldDefaultValue { + value: PyObject, +} + +#[pymethods] +impl FieldDefaultValue { + #[new] + fn __new__(value: PyObject) -> Self { + FieldDefaultValue { value } + } + + fn __str__(self_: &PyCell) -> PyResult { + Ok(format!("default({})", self_.borrow().value)) + } + + fn __repr__(self_: &PyCell) -> PyResult { + Ok(format!("", self_.borrow().value)) + } +} + #[pyclass(subclass)] +#[derive(Clone)] pub struct Field { value: PyObject, + // Overrides the class var default per instance when set. + default: Option, } #[pymethods] @@ -59,17 +84,28 @@ impl Field { let raw_value = match raw_value { Some(value) if value.extract::(py).is_ok() - && !Self::cls_none_is_valid_value(cls)? => + && !Self::cls_none_is_valid_value(cls, py)? => { None } rv => rv, }; + let maybe_default = match raw_value { + Some(ref value) => { + if let Ok(default) = value.extract::(py) { + Some(default.value) + } else { + None + } + } + _ => None, + }; Ok(Self { value: cls .call_method(intern!(py, "compute_value"), (raw_value, address), None)? .into(), + default: maybe_default, }) } @@ -112,26 +148,32 @@ impl Field { py: Python, ) -> PyResult { let default = || -> PyResult { - if Self::cls_required(cls)? { + if Self::cls_required(cls, py)? { // TODO: Should be `RequiredFieldMissingException`. Err(PyValueError::new_err(format!( "The `{}` field in target {} must be defined.", - Self::cls_alias(cls)?, + Self::cls_alias(cls, py)?, *address, ))) } else { - Self::cls_default(cls) + Self::cls_default(cls, py) } }; - let none_is_valid_value = Self::cls_none_is_valid_value(cls)?; + let none_is_valid_value = Self::cls_none_is_valid_value(cls, py)?; match raw_value { Some(value) if none_is_valid_value && value.extract::(py).is_ok() => { default() } None if none_is_valid_value => Ok(py.None()), None => default(), - Some(value) => Ok(value), + Some(value) => { + if let Ok(dyn_default) = value.extract::(py) { + Ok(dyn_default.value) + } else { + Ok(value) + } + } } } @@ -144,17 +186,17 @@ impl Field { Ok(self_.get_type().hash()? & self_.borrow().value.as_ref(py).hash()?) } - fn __repr__(self_: &PyCell) -> PyResult { + fn __repr__(self_: &PyCell, py: Python) -> PyResult { let mut result = String::new(); write!( result, "{}(alias={}, value={}", self_.get_type(), - Self::cls_alias(self_)?, + Self::cls_alias(self_, py)?, self_.borrow().value ) .unwrap(); - if let Ok(default) = self_.getattr("default") { + if let Ok(default) = self_.getattr(intern!(py, "default")) { write!(result, ", default={})", default).unwrap(); } else { write!(result, ")").unwrap(); @@ -162,10 +204,10 @@ impl Field { Ok(result) } - fn __str__(self_: &PyCell) -> PyResult { + fn __str__(self_: &PyCell, py: Python) -> PyResult { Ok(format!( "{}={}", - Self::cls_alias(self_)?, + Self::cls_alias(self_, py)?, self_.borrow().value )) } @@ -188,32 +230,58 @@ impl Field { _ => Ok(py.NotImplemented()), } } + + fn __getattribute__<'a>( + self_: &'a PyCell, + name: String, + py: Python<'a>, + ) -> PyResult<*mut ffi::PyObject> { + if name == "default" { + if let Some(default) = &self_.extract::()?.default { + // Return instance default, overriding the field class var default. + return Ok(default.as_ptr()); + } + } + + unsafe { + // The ffi::PyObject_GenericGetAttr() call is unsafe, so we need to be in an unsafe + // context to call it. + let slf = self_.borrow_mut().into_ptr(); + let attr = name.into_py(py).into_ptr(); + let res = ffi::PyObject_GenericGetAttr(slf, attr); + if res.is_null() { + Err(PyErr::fetch(py)) + } else { + Ok(res) + } + } + } } impl Field { - fn cls_none_is_valid_value(cls: &PyAny) -> PyResult { - cls.getattr("none_is_valid_value")?.extract::() + fn cls_none_is_valid_value(cls: &PyAny, py: Python) -> PyResult { + cls.getattr(intern!(py, "none_is_valid_value"))? + .extract::() } - fn cls_default(cls: &PyAny) -> PyResult { - cls.getattr("default")?.extract() + fn cls_default(cls: &PyAny, py: Python) -> PyResult { + cls.getattr(intern!(py, "default"))?.extract() } - fn cls_required(cls: &PyAny) -> PyResult { - cls.getattr("required")?.extract() + fn cls_required(cls: &PyAny, py: Python) -> PyResult { + cls.getattr(intern!(py, "required"))?.extract() } - fn cls_alias(cls: &PyAny) -> PyResult<&str> { - // TODO: All of these methods should use interned attr names. - cls.getattr("alias")?.extract() + fn cls_alias<'a>(cls: &'a PyAny, py: Python<'a>) -> PyResult<&'a str> { + cls.getattr(intern!(py, "alias"))?.extract() } - fn cls_removal_version(cls: &PyAny) -> PyResult> { - cls.getattr("removal_version")?.extract() + fn cls_removal_version<'a>(cls: &'a PyAny, py: Python<'a>) -> PyResult> { + cls.getattr(intern!(py, "removal_version"))?.extract() } - fn cls_removal_hint(cls: &PyAny) -> PyResult> { - cls.getattr("removal_hint")?.extract() + fn cls_removal_hint<'a>(cls: &'a PyAny, py: Python<'a>) -> PyResult> { + cls.getattr(intern!(py, "removal_hint"))?.extract() } fn check_deprecated( @@ -225,7 +293,7 @@ impl Field { if address.is_generated_target() { return Ok(()); } - let Some(removal_version) = Self::cls_removal_version(cls)? else { + let Some(removal_version) = Self::cls_removal_version(cls, py)? else { return Ok(()); }; match raw_value { @@ -233,16 +301,16 @@ impl Field { _ => (), } - let Some(removal_hint) = Self::cls_removal_hint(cls)? else { + let Some(removal_hint) = Self::cls_removal_hint(cls, py)? else { return Err(PyValueError::new_err( "You specified `removal_version` for {cls:?}, but not the class \ property `removal_hint`.", )); }; - let alias = Self::cls_alias(cls)?; - let deprecated = PyModule::import(py, "pants.base.deprecated")?; - deprecated.getattr("warn_or_error")?.call( + let alias = Self::cls_alias(cls, py)?; + let deprecated = PyModule::import(py, intern!(py, "pants.base.deprecated"))?; + deprecated.getattr(intern!(py, "warn_or_error"))?.call( ( removal_version, format!("the {alias} field"), From f5cbc3e534b4d0ab14dcdd497d6dd98cb4500ee9 Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 23 May 2024 13:30:46 +0200 Subject: [PATCH 2/4] add notes. --- docs/notes/2.22.x.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/notes/2.22.x.md b/docs/notes/2.22.x.md index 46fa44d43d6..418bcd7d2db 100644 --- a/docs/notes/2.22.x.md +++ b/docs/notes/2.22.x.md @@ -89,6 +89,8 @@ Setting [the `orphan_files_behaviour = "ignore"` option](https://www.pantsbuild. The `PythonToolRequirementsBase` and `PythonToolBase` classes now have a new `help_short` field. Subclasses should now use `help_short` instead of the `help` field. The `help` field will be automatically generated using `help_short`, and will include the tool's default package version and provide instructions on how to override this version using a custom lockfile. +Target `Field` types can now have per instance default values, overriding the class var `default` value declared on the `Field` subclass by wrapping the field value in a `FieldDefaultValue` when creating the field instance. + ## Full Changelog For the full changelog, see the individual GitHub Releases for this series: https://github.com/pantsbuild/pants/releases From 565748c61d2c46e820094f8b27928c5bb0d3b927 Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 23 May 2024 14:15:00 +0200 Subject: [PATCH 3/4] fmt --- src/python/pants/engine/internals/native_engine.pyi | 4 +--- src/python/pants/engine/target.py | 4 +++- src/python/pants/engine/target_test.py | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index 53d3ace63d6..a56100f7f3e 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -269,7 +269,6 @@ class _NoValue: # Marker for unspecified field values that should use the default value if applicable. NO_VALUE: _NoValue - class FieldDefaultValue: """Field value wrapper to indicate this should be treated as the default value for this instance of the field. @@ -285,14 +284,13 @@ class FieldDefaultValue: The value is unwrapped and assigned to the `default` property on the field instance. During `compute_value`, the wrapped value is passed as `raw_value`, while the unwrapped value is returned from `super().compute_value(...)`. - """ + value: ImmutableValue def __repr__(self) -> str: ... def __str__(self) -> str: ... - class Field: """A Field. diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index 4536ed3e9a5..e2a119decc6 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -61,7 +61,9 @@ ) from pants.engine.internals.native_engine import NO_VALUE as NO_VALUE # noqa: F401 from pants.engine.internals.native_engine import Field as Field # noqa: F401 -from pants.engine.internals.native_engine import FieldDefaultValue as FieldDefaultValue # noqa: F401 +from pants.engine.internals.native_engine import ( # noqa: F401 + FieldDefaultValue as FieldDefaultValue, +) from pants.engine.unions import UnionMembership, UnionRule, distinct_union_type_per_subclass, union from pants.option.global_options import UnmatchedBuildFileGlobs from pants.source.filespec import Filespec, FilespecMatcher diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py index 4ddddb8f68c..b1509073736 100644 --- a/src/python/pants/engine/target_test.py +++ b/src/python/pants/engine/target_test.py @@ -14,7 +14,6 @@ from pants.engine.target import ( NO_VALUE, AsyncFieldMixin, - FieldDefaultValue, BoolField, CoarsenedTarget, CoarsenedTargets, @@ -22,6 +21,7 @@ DictStringToStringSequenceField, ExplicitlyProvidedDependencies, Field, + FieldDefaultValue, FieldSet, FloatField, GeneratedTargets, @@ -478,9 +478,7 @@ class SomeField(Field): default: str = "some-default" @classmethod - def compute_value( - cls, raw_value: Optional[str], address: Address - ) -> str: + def compute_value(cls, raw_value: Optional[str], address: Address) -> str: value_or_default = super().compute_value(raw_value, address) print(f"compute {raw_value=}, {value_or_default=}") assert not isinstance(value_or_default, FieldDefaultValue) @@ -494,7 +492,9 @@ def check_field(fld: SomeField, value: str, default: str) -> None: addr = Address("a") check_field(SomeField("regular", addr), "regular", SomeField.default) - check_field(SomeField(FieldDefaultValue("other-default"), addr), "other-default", "other-default") + check_field( + SomeField(FieldDefaultValue("other-default"), addr), "other-default", "other-default" + ) # ----------------------------------------------------------------------------------------------- From d8af6eec7570fe5117f429f684642d100e662ae9 Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 23 May 2024 15:04:00 +0200 Subject: [PATCH 4/4] fix type issues. --- src/python/pants/engine/internals/native_engine.pyi | 1 + src/python/pants/engine/target_test.py | 7 +++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index a56100f7f3e..891c81e519c 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -288,6 +288,7 @@ class FieldDefaultValue: value: ImmutableValue + def __init__(self, value: ImmutableValue) -> None: ... def __repr__(self) -> str: ... def __str__(self) -> str: ... diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py index b1509073736..b568d1110b4 100644 --- a/src/python/pants/engine/target_test.py +++ b/src/python/pants/engine/target_test.py @@ -474,14 +474,13 @@ def test_target_residence_dir() -> None: def test_field_dynamic_default() -> None: class SomeField(Field): - alias: str = "some" - default: str = "some-default" + alias: ClassVar[str] = "some" + default: ClassVar[str] = "some-default" @classmethod def compute_value(cls, raw_value: Optional[str], address: Address) -> str: value_or_default = super().compute_value(raw_value, address) - print(f"compute {raw_value=}, {value_or_default=}") - assert not isinstance(value_or_default, FieldDefaultValue) + assert isinstance(value_or_default, str) return value_or_default def check_field(fld: SomeField, value: str, default: str) -> None: