Skip to content

Commit b99f38d

Browse files
youknowdotyouknowone
youknowdot
and
youknowone
authored
Bind interpreter arguments with borrowed Clinic structs (#9763)
* Bind interpreter arguments with borrowed Clinic structs Extend the existing FromArgs derive with borrowed binding. Retain the native call input owners, validate and bind slots once before conversion, and generate signatures from the same fields. Remove the interpreter ArgSpec parsers and preserve conversion and finalization order without keepalive clones or allocation. Cover binding precedence, raw and inspected signatures, temporary lifetimes, reference counts, generated code, and borrowed payload validation. Assisted-by: OpenAI Codex:model version not exposed * Return infallible interpreter query results directly Assisted-by: OpenAI Codex:model version not exposed * Match tuple reference-counted destruction order Release references extracted from tuples in reverse order, including nested and subclassed tuples. Select the order through a payload constant so other payloads keep their existing deallocation path and no new allocation is needed. Assisted-by: OpenAI Codex:model version not exposed * Retain expanded call argument owners through dispatch Keep the original keyword dictionary and positional tuple alive through non-Python and instrumented CALL_FUNCTION_EX invocations. Preserve the early container release for ordinary exact Python functions. Release keyword owners before positional owners, including exceptions and monitoring callbacks. Normalize tuple subclasses through iteration. A general iterable's normalized values retain their own storage for calls that need it, without allocating an otherwise unobservable tuple object. Cover exact functions, methods, callable objects, native calls, conversion failures, and monitoring transitions. Assisted-by: OpenAI Codex:model version not exposed * Release borrowed native arguments in stack order Release keyword and then positional owners in reverse evaluation order after binding, conversion, execution, and return conversion, including errors. The borrowed argument abstraction needs no additional allocation or reference clone. Expanded calls retain their original container ownership at the call boundary. Match CPython temporary lifetimes for reordered, mixed, expanded, successful, and failing interpreter calls instead of preserving the former keyword-order difference. Assisted-by: OpenAI Codex:model version not exposed * Avoid call ownership helper overhead Combine expanded-argument collection with dispatch so original owners stay local instead of crossing two helper boundaries in a large aggregate. Select instrumentation at compile time and reserve keyword space when copying the positional values, avoiding a subsequent vector reallocation. Inline borrowed-owner cleanup into native wrappers so it does not introduce an additional call and owned FuncArgs transfer on every native invocation. Assisted-by: OpenAI Codex:model version not exposed * Parse from_args modes in one place Parse `borrowed`, `legacy`, and `fname` through a single FromArgsMode so `#[from_args(borrowed, legacy)]` is rejected and the unknown-key error lists `borrowed`. Bind `_interpreters.list_all` with an owned struct instead of a borrowed struct carrying only a PhantomData lifetime. Reword two comments. Assisted-by: Claude:claude-opus-5-5 Assisted-by: Grok:grok-4.7 --------- Co-authored-by: Jeong, YunWon <jeong@youknowone.org>
1 parent 0181d04 commit b99f38d

15 files changed

Lines changed: 1458 additions & 369 deletions

File tree

‎crates/derive-impl/src/from_args.rs‎

Lines changed: 210 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,13 @@ struct LegacyFromArgs {
623623
fname: String,
624624
}
625625

626-
fn parse_from_args_attr(attrs: &[Attribute]) -> Result<Option<LegacyFromArgs>> {
626+
enum FromArgsMode {
627+
Owned,
628+
Legacy(LegacyFromArgs),
629+
Borrowed,
630+
}
631+
632+
fn parse_from_args_attr(attrs: &[Attribute]) -> Result<FromArgsMode> {
627633
let mut found = None;
628634
for attr in attrs {
629635
if !attr.path().is_ident("from_args") {
@@ -635,13 +641,20 @@ fn parse_from_args_attr(attrs: &[Attribute]) -> Result<Option<LegacyFromArgs>> {
635641
found = Some(attr);
636642
}
637643
let Some(attr) = found else {
638-
return Ok(None);
644+
return Ok(FromArgsMode::Owned);
639645
};
640646

647+
let mut borrowed = false;
641648
let mut legacy = false;
642649
let mut fname = None;
643650
attr.parse_nested_meta(|meta| {
644-
if meta.path.is_ident("legacy") {
651+
if meta.path.is_ident("borrowed") {
652+
if borrowed {
653+
return Err(meta.error("duplicate `borrowed`"));
654+
}
655+
borrowed = true;
656+
Ok(())
657+
} else if meta.path.is_ident("legacy") {
645658
if legacy {
646659
return Err(meta.error("duplicate `legacy`"));
647660
}
@@ -655,21 +668,28 @@ fn parse_from_args_attr(attrs: &[Attribute]) -> Result<Option<LegacyFromArgs>> {
655668
fname = Some(val.value());
656669
Ok(())
657670
} else {
658-
Err(meta.error("unknown #[from_args] key; expected `legacy` or `fname = \"...\"`"))
671+
Err(meta.error(
672+
"unknown #[from_args] key; expected `borrowed`, `legacy`, or `fname = \"...\"`",
673+
))
659674
}
660675
})?;
661676

662-
match (legacy, fname) {
663-
(true, Some(fname)) => Ok(Some(LegacyFromArgs { fname })),
664-
(true, None) => Err(err_span!(
677+
match (borrowed, legacy, fname) {
678+
(true, false, None) => Ok(FromArgsMode::Borrowed),
679+
(false, true, Some(fname)) => Ok(FromArgsMode::Legacy(LegacyFromArgs { fname })),
680+
(true, _, _) => Err(err_span!(
681+
attr,
682+
"borrowed FromArgs uses Clinic binding; legacy parsing requires owned arguments"
683+
)),
684+
(false, true, None) => Err(err_span!(
665685
attr,
666686
"#[from_args(legacy)] requires fname = \"...\""
667687
)),
668-
(false, Some(_)) => Err(err_span!(
688+
(false, false, Some(_)) => Err(err_span!(
669689
attr,
670690
"#[from_args(fname = \"...\")] requires `legacy`"
671691
)),
672-
(false, None) => Err(err_span!(
692+
(false, false, None) => Err(err_span!(
673693
attr,
674694
"#[from_args] requires `legacy` and fname = \"...\""
675695
)),
@@ -768,16 +788,139 @@ fn compute_arity_bounds(field_attrs: &[ArgAttribute]) -> (usize, usize) {
768788
(min_arity, max_arity)
769789
}
770790

791+
fn impl_borrowed_from_args(input: &DeriveInput, fields: &syn::Fields) -> Result<TokenStream> {
792+
let lifetimes = input.generics.lifetimes().collect::<Vec<_>>();
793+
if lifetimes.len() != 1 || input.generics.params.len() != 1 {
794+
bail_span!(
795+
input.generics,
796+
"borrowed FromArgs requires exactly one lifetime and no type or const parameters"
797+
);
798+
}
799+
let lifetime = &lifetimes[0].lifetime;
800+
let name = &input.ident;
801+
let mut params = Vec::new();
802+
let mut values = Vec::new();
803+
let attrs = fields
804+
.iter()
805+
.filter(|field| !is_phantom(field))
806+
.map(ArgAttribute::try_from)
807+
.collect::<Result<Vec<_>>>()?;
808+
let (min, max) = compute_arity_bounds(&attrs);
809+
let mut slot = 0usize;
810+
for (i, field) in fields.iter().enumerate() {
811+
let attr = ArgAttribute::try_from(field)?;
812+
if is_phantom(field) {
813+
let member = &field.ident;
814+
values.push(quote!(#member: ::core::marker::PhantomData,));
815+
continue;
816+
}
817+
if attr.kind == ParameterKind::Flatten {
818+
bail_span!(field, "borrowed FromArgs does not support flattened fields");
819+
}
820+
if attr.py_default.is_some() && attr.default.is_none() {
821+
bail_span!(
822+
field,
823+
"a borrowed py_default requires optional or default omission handling"
824+
);
825+
}
826+
params.push(param_token(field, &attr)?);
827+
let member = field.ident.as_ref().map_or_else(
828+
|| syn::Index::from(i).into_token_stream(),
829+
ToTokens::to_token_stream,
830+
);
831+
let pyname = attr
832+
.name
833+
.clone()
834+
.or_else(|| field.ident.as_ref().map(|name| name.unraw().to_string()))
835+
.ok_or_else(|| err_span!(field, "field in tuple struct must have name attribute"))?;
836+
let mut convert = quote! {
837+
::rustpython_vm::convert::TryFromBorrowedObject::try_from_borrowed_argument(vm, x)
838+
};
839+
if let Some(message) = &attr.error_msg {
840+
convert = quote! {
841+
#convert.map_err(|_| ::rustpython_vm::function::ArgumentError::from(vm.new_type_error(#message)))
842+
};
843+
} else if attr.kind != ParameterKind::PositionalOnly {
844+
convert = quote! { #convert.map_err(|err| err.with_argument_name(#pyname)) };
845+
}
846+
let ending = if let Some(default) = attr.default {
847+
let ty = &field.ty;
848+
if let Some(expr) = &default
849+
&& !keeps_direct_literal(ty)
850+
&& !is_wrapped_arg_ty(ty)
851+
&& literal_object_expr(expr).is_some()
852+
{
853+
bail_span!(
854+
field,
855+
"a borrowed default cannot allocate an owned Python object; use OptionalArg with py_default or a Rust converter constant"
856+
);
857+
}
858+
let default = match default {
859+
Some(expr) => match absolute_const_ident(&expr)? {
860+
Some(name) => quote!(::core::convert::Into::into(#name)),
861+
None => quote!(#expr),
862+
},
863+
None => quote!(::core::default::Default::default()),
864+
};
865+
quote! {
866+
.map(<#ty as ::rustpython_vm::function::FromBorrowedArgOptional<#lifetime>>::from_inner)
867+
.unwrap_or_else(|| #default)
868+
}
869+
} else {
870+
quote! { .expect("Clinic checked the required argument") }
871+
};
872+
let i = slot;
873+
slot += 1;
874+
let pos = i + 1;
875+
values.push(quote! {
876+
#member: slots[#i].map(|x| #convert)
877+
.transpose()
878+
.map_err(|error| error.into_exception(&(#min..=#max), args.args.len(), #pos, callee, vm))?
879+
#ending,
880+
});
881+
}
882+
let n = slot;
883+
let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();
884+
Ok(quote! {
885+
impl #impl_generics ::rustpython_vm::function::ArgsMetadata for #name #ty_generics #where_clause {
886+
const DISPLAY_PARAMS: ::core::option::Option<&'static [::rustpython_vm::function::Param]> =
887+
Some(&[#(#params),*]);
888+
}
889+
890+
impl ::rustpython_vm::function::BorrowedArgs for #name<'static> {
891+
type Bound<#lifetime> = #name<#lifetime>;
892+
893+
#[inline]
894+
fn bind<#lifetime>(
895+
args: &#lifetime ::rustpython_vm::function::FuncArgs,
896+
vm: &::rustpython_vm::VirtualMachine,
897+
callee: ::rustpython_vm::function::Callee,
898+
) -> ::rustpython_vm::PyResult<Self::Bound<#lifetime>> {
899+
let slots = args.bind_borrowed::<#n, Self>(vm, callee)?;
900+
Ok(#name { #(#values)* })
901+
}
902+
}
903+
})
904+
}
905+
771906
pub(crate) fn impl_from_args(input: DeriveInput) -> Result<TokenStream> {
907+
let legacy = match parse_from_args_attr(&input.attrs)? {
908+
FromArgsMode::Borrowed => {
909+
let Data::Struct(data) = &input.data else {
910+
bail_span!(input, "FromArgs input must be a struct");
911+
};
912+
return impl_borrowed_from_args(&input, &data.fields);
913+
}
914+
FromArgsMode::Legacy(legacy) => Some(legacy),
915+
FromArgsMode::Owned => None,
916+
};
772917
let Data::Struct(syn::DataStruct {
773918
fields: struct_fields,
774919
..
775920
}) = input.data
776921
else {
777922
bail_span!(input, "FromArgs input must be a struct")
778923
};
779-
780-
let legacy = parse_from_args_attr(&input.attrs)?;
781924
if legacy.is_some() {
782925
validate_legacy_fields(&struct_fields)?;
783926
}
@@ -921,6 +1064,62 @@ pub(crate) fn impl_from_args(input: DeriveInput) -> Result<TokenStream> {
9211064
mod tests {
9221065
use super::*;
9231066

1067+
#[test]
1068+
fn borrowed_codegen_uses_one_binding_and_no_owners() {
1069+
let input = parse_quote! {
1070+
#[from_args(borrowed)]
1071+
struct Args<'a> {
1072+
#[pyarg(any)]
1073+
value: &'a PyObject,
1074+
#[pyarg(named, optional, py_default = "None")]
1075+
optional: OptionalArg<&'a Py<PyDict>>,
1076+
}
1077+
};
1078+
let tokens = impl_from_args(input).unwrap().to_string();
1079+
assert_eq!(tokens.matches("bind_borrowed").count(), 1);
1080+
assert_eq!(tokens.matches("try_from_borrowed_argument").count(), 2);
1081+
for forbidden in [
1082+
"clone",
1083+
"Vec",
1084+
"ArgSpec",
1085+
"take_keyword",
1086+
"take_positional",
1087+
"check_before_convert",
1088+
] {
1089+
assert!(!tokens.contains(forbidden), "{forbidden}: {tokens}");
1090+
}
1091+
assert!(tokens.contains("BorrowedArgs"));
1092+
assert!(tokens.contains("ArgsMetadata"));
1093+
}
1094+
1095+
#[test]
1096+
fn a_lifetime_alone_does_not_change_owned_binding() {
1097+
let input = parse_quote! {
1098+
struct Args<'a> {
1099+
#[pyarg(flatten)]
1100+
value: ExistingOwnedArgs<'a>,
1101+
}
1102+
};
1103+
let tokens = impl_from_args(input).unwrap().to_string();
1104+
assert!(tokens.contains("FromArgs for Args"));
1105+
assert!(!tokens.contains("BorrowedArgs"));
1106+
}
1107+
1108+
#[test]
1109+
fn borrowed_binding_rejects_unsupported_layouts_and_defaults() {
1110+
for input in [
1111+
parse_quote! { #[from_args(borrowed)] struct Args { value: PyObjectRef } },
1112+
parse_quote! { #[from_args(borrowed)] struct Args<'a> { #[pyarg(any, py_default = "None")] value: &'a PyObject } },
1113+
parse_quote! { #[from_args(borrowed)] struct Args<'a, T> { value: &'a T } },
1114+
parse_quote! { #[from_args(borrowed)] struct Args<'a> { #[pyarg(flatten)] value: &'a PyObject } },
1115+
parse_quote! { #[from_args(borrowed)] struct Args<'a> { #[pyarg(any, default = "x")] value: &'a Py<PyStr> } },
1116+
parse_quote! { #[from_args(borrowed)] #[from_args(legacy, fname = "f")] struct Args<'a> { value: &'a PyObject } },
1117+
parse_quote! { #[from_args(borrowed, legacy)] struct Args<'a> { value: &'a PyObject } },
1118+
] {
1119+
assert!(impl_from_args(input).is_err());
1120+
}
1121+
}
1122+
9241123
#[test]
9251124
fn option_none_default_keeps_rust_value_and_metadata() {
9261125
let types: [Type; 2] = [

‎crates/derive/src/lib.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,35 @@ use syn::punctuated::Punctuated;
8585
/// | `Option<T>` | `None` or a value. A missing argument and an explicit `None` are the same | `= None` | `None` |
8686
/// | `OptionalOption<T>` (`OptionalArg<Option<T>>`) | missing, `None`, and a value are all distinct | `= NULL`, and `None` is accepted | `<unrepresentable>` |
8787
///
88+
/// # Borrowed arguments
89+
/// `#[from_args(borrowed)]` selects borrowing with the same strict Clinic rules:
90+
///
91+
/// ```rust,ignore
92+
/// #[derive(FromArgs)]
93+
/// #[from_args(borrowed)]
94+
/// struct CallArgs<'a> {
95+
/// #[pyarg(any)]
96+
/// object: &'a PyObject,
97+
/// #[pyarg(named, default = ArgIntoBool::FALSE, py_default = "False")]
98+
/// flag: ArgIntoBool,
99+
/// }
100+
///
101+
/// #[pyfunction]
102+
/// fn call(args: CallArgs<'_>, vm: &VirtualMachine) -> PyResult { /* ... */ }
103+
/// ```
104+
///
105+
/// Binding uses stack slots and `TryFromBorrowedObject`, leaving the native
106+
/// call's original owners alive through the implementation and return conversion.
107+
/// It adds no owning copies or heap allocation. All Clinic binding errors are
108+
/// checked before converting fields in declaration order. `OptionalArg<&Py<T>>`
109+
/// distinguishes omission from an explicitly supplied value without constructing
110+
/// a default Python object. Rust converter constants can supply scalar defaults.
111+
/// A borrowed `py_default` requires `optional` or `default` omission handling.
112+
///
113+
/// Borrowed structs currently support fixed fields, one lifetime, and no other
114+
/// generic parameters. Flattened and legacy bindings remain owned. A borrowed
115+
/// struct is passed as the sole arguments struct followed by `&VirtualMachine`.
116+
///
88117
/// # `#[from_args(legacy, fname = "...")]`
89118
/// Bind arguments the way `PyArg_ParseTupleAndKeywords` does: convert each
90119
/// supplied value in parameter order, then report a missing argument, a leftover

‎crates/vm/src/builtins/tuple.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ thread_local! {
132132
impl PyPayload for PyTuple {
133133
const MAX_FREELIST: usize = 2000;
134134
const HAS_FREELIST: bool = true;
135+
const REVERSE_DEALLOC: bool = true;
135136

136137
#[inline]
137138
fn class(ctx: &Context) -> &'static Py<PyType> {

‎crates/vm/src/convert/try_from.rs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,21 @@ where
7878
{
7979
/// Attempt to convert a Python object to a value of this type.
8080
fn try_from_borrowed_object(vm: &VirtualMachine, obj: &'a PyObject) -> PyResult<Self>;
81+
82+
#[inline]
83+
fn try_from_borrowed_argument(
84+
vm: &VirtualMachine,
85+
obj: &'a PyObject,
86+
) -> Result<Self, ArgumentError> {
87+
Self::try_from_borrowed_object(vm, obj).map_err(Into::into)
88+
}
89+
}
90+
91+
impl<'a> TryFromBorrowedObject<'a> for &'a PyObject {
92+
#[inline]
93+
fn try_from_borrowed_object(_vm: &VirtualMachine, obj: &'a PyObject) -> PyResult<Self> {
94+
Ok(obj)
95+
}
8196
}
8297

8398
impl<T> TryFromObject for PyRef<T>
@@ -155,6 +170,26 @@ impl<'a, T: PyPayload> TryFromBorrowedObject<'a> for &'a Py<T> {
155170
Err(vm.new_downcast_type_error(class, &obj))
156171
}
157172
}
173+
174+
#[inline]
175+
fn try_from_borrowed_argument(
176+
vm: &VirtualMachine,
177+
obj: &'a PyObject,
178+
) -> Result<Self, ArgumentError> {
179+
let class = T::class(&vm.ctx);
180+
if obj.fast_isinstance(class) {
181+
obj.try_downcast_ref::<T>(vm).map_err(Into::into)
182+
} else {
183+
Err(ArgumentError::BadArgument {
184+
expected: class,
185+
or_none: false,
186+
// Only an error needs an owning diagnostic, never the call's
187+
// successful borrowed conversion path.
188+
found: obj.to_owned(),
189+
name: None,
190+
})
191+
}
192+
}
158193
}
159194

160195
impl TryFromObject for core::time::Duration {

0 commit comments

Comments
 (0)