From 35f89975bcd7664eca32851c8cabb8d8e17e2c3a Mon Sep 17 00:00:00 2001 From: John-John Tedro Date: Sat, 2 Nov 2024 14:01:41 +0100 Subject: [PATCH] rune: Clean up macros and add more instrumentation --- crates/rune-macros/src/any.rs | 203 +++++------------- crates/rune-macros/src/context.rs | 171 +++++++-------- crates/rune-macros/src/internals.rs | 12 -- crates/rune/src/cli/tests.rs | 32 ++- crates/rune/src/compile/context.rs | 13 ++ crates/rune/src/doc/markdown.rs | 4 +- crates/rune/src/runtime/args.rs | 2 +- crates/rune/src/runtime/function.rs | 24 +-- crates/rune/src/runtime/guarded_args.rs | 48 +++-- crates/rune/src/runtime/to_value.rs | 8 - crates/rune/src/runtime/vm.rs | 6 +- crates/rune/src/runtime/vm_error.rs | 12 +- crates/rune/src/tests/function_guardedargs.rs | 40 ++-- 13 files changed, 248 insertions(+), 327 deletions(-) diff --git a/crates/rune-macros/src/any.rs b/crates/rune-macros/src/any.rs index d96d718c7..a9b020a99 100644 --- a/crates/rune-macros/src/any.rs +++ b/crates/rune-macros/src/any.rs @@ -521,24 +521,17 @@ where any_t, context_error, fmt, - from_value, hash, install_with, item, maybe_type_of, meta, module, - mut_, named, non_null, - raw_any_guard, raw_value_guard, - ref_, result, - runtime_error, static_type_info, - static_type_mod, - to_value, type_hash_t, type_of, unsafe_to_mut, @@ -617,180 +610,92 @@ where } }; - let impl_type_of = if let Some(ty) = attr.static_type { - let ty_hash = syn::Ident::new(&format!("{ty}_HASH"), ty.span()); + let type_hash = type_hash.into_inner(); - Some(quote! { - #[automatically_derived] - impl #impl_generics #type_hash_t for #ident #type_generics #where_clause { - const HASH: #hash = #static_type_mod::#ty_hash; - } - - #[automatically_derived] - impl #impl_generics #type_of for #ident #type_generics #where_clause { - const STATIC_TYPE_INFO: #static_type_info = #static_type_info::static_type(#static_type_mod::#ty); - } - - #[automatically_derived] - impl #impl_generics #maybe_type_of for #ident #type_generics #where_clause { - #[inline] - fn maybe_type_of() -> #alloc::Result<#meta::DocType> { - #meta::DocType::with_generics( - ::HASH, - [#(<#generic_names as #maybe_type_of>::maybe_type_of()),*] - ) - } - } - }) - } else if attr.builtin.is_none() { - let type_hash = type_hash.into_inner(); - - let make_hash = if !generic_names.is_empty() { - quote!(#hash::new_with_type_parameters(#type_hash, #hash::parameters([#(<#generic_names as #type_hash_t>::HASH),*]))) - } else { - quote!(#hash::new(#type_hash)) - }; - - let type_parameters = - quote!(#hash::parameters([#(<#generic_names as #type_hash_t>::HASH),*])); - - Some(quote! { - #[automatically_derived] - impl #impl_generics #type_hash_t for #ident #type_generics #where_clause { - const HASH: #hash = #make_hash; - } - - #[automatically_derived] - impl #impl_generics #type_of for #ident #type_generics #where_clause { - const PARAMETERS: #hash = #type_parameters; - const STATIC_TYPE_INFO: #static_type_info = #static_type_info::any_type_info(::ANY_TYPE_INFO); - } - - #[automatically_derived] - impl #impl_generics #maybe_type_of for #ident #type_generics #where_clause { - #[inline] - fn maybe_type_of() -> #alloc::Result<#meta::DocType> { - #meta::DocType::with_generics( - ::HASH, - [#(<#generic_names as #maybe_type_of>::maybe_type_of()?),*] - ) - } - } - }) + let make_hash = if !generic_names.is_empty() { + quote!(#hash::new_with_type_parameters(#type_hash, #hash::parameters([#(<#generic_names as #type_hash_t>::HASH),*]))) } else { - None + quote!(#hash::new(#type_hash)) }; - let non_builtin = attr.builtin.is_none().then(|| { - quote! { - #[automatically_derived] - impl #impl_generics #any_t for #ident #type_generics #where_clause { - } + let type_parameters = + quote!(#hash::parameters([#(<#generic_names as #type_hash_t>::HASH),*])); - #[automatically_derived] - impl #impl_generics #unsafe_to_ref for #ident #type_generics #where_clause { - type Guard = #raw_value_guard; - - unsafe fn unsafe_to_ref<'a>(value: #value) -> #vm_result<(&'a Self, Self::Guard)> { - let (value, guard) = #vm_try!(#value::into_any_ref_ptr(value)); - #vm_result::Ok((#non_null::as_ref(&value), guard)) - } - } - - #[automatically_derived] - impl #impl_generics #unsafe_to_mut for #ident #type_generics #where_clause { - type Guard = #raw_value_guard; - - unsafe fn unsafe_to_mut<'a>(value: #value) -> #vm_result<(&'a mut Self, Self::Guard)> { - let (mut value, guard) = #vm_try!(#value::into_any_mut_ptr(value)); - #vm_result::Ok((#non_null::as_mut(&mut value), guard)) - } - } - - #[automatically_derived] - impl #impl_generics #unsafe_to_value for &#ident #type_generics #where_clause { - type Guard = #value_ref_guard; - - unsafe fn unsafe_to_value(self) -> #vm_result<(#value, Self::Guard)> { - let (shared, guard) = #vm_try!(#value::from_ref(self)); - #vm_result::Ok((shared, guard)) - } - - fn try_into_to_value(self) -> Option { - Option::<&str>::None - } - } - - #[automatically_derived] - impl #impl_generics #unsafe_to_value for &mut #ident #type_generics #where_clause { - type Guard = #value_mut_guard; + let impl_type_of = Some(quote! { + #[automatically_derived] + impl #impl_generics #type_hash_t for #ident #type_generics #where_clause { + const HASH: #hash = #make_hash; + } - unsafe fn unsafe_to_value(self) -> #vm_result<(#value, Self::Guard)> { - let (shared, guard) = #vm_try!(#value::from_mut(self)); - #vm_result::Ok((shared, guard)) - } + #[automatically_derived] + impl #impl_generics #type_of for #ident #type_generics #where_clause { + const PARAMETERS: #hash = #type_parameters; + const STATIC_TYPE_INFO: #static_type_info = #static_type_info::any_type_info(::ANY_TYPE_INFO); + } - fn try_into_to_value(self) -> Option { - Option::<&str>::None - } + #[automatically_derived] + impl #impl_generics #maybe_type_of for #ident #type_generics #where_clause { + #[inline] + fn maybe_type_of() -> #alloc::Result<#meta::DocType> { + #meta::DocType::with_generics( + ::HASH, + [#(<#generic_names as #maybe_type_of>::maybe_type_of()?),*] + ) } } }); - let impl_from_value = attr.from_value.as_ref().map(|path| { - quote! { - impl #impl_generics #from_value for #ident #type_generics { - fn from_value(value: #value) -> #result { - #path(value) - } - } + let impl_any = quote! { + #[automatically_derived] + impl #impl_generics #any_t for #ident #type_generics #where_clause { } - }); - let impl_from_value_ref = attr.from_value_ref.as_ref().map(|path| quote! { - impl #impl_generics #unsafe_to_ref for #ident #type_generics { - type Guard = #raw_any_guard; + #[automatically_derived] + impl #impl_generics #unsafe_to_ref for #ident #type_generics #where_clause { + type Guard = #raw_value_guard; unsafe fn unsafe_to_ref<'a>(value: #value) -> #vm_result<(&'a Self, Self::Guard)> { - let value = #vm_try!(#path(value)); - let (value, guard) = #ref_::into_raw(value); - #vm_result::Ok((value.as_ref(), guard)) + let (value, guard) = #vm_try!(#value::into_any_ref_ptr(value)); + #vm_result::Ok((#non_null::as_ref(&value), guard)) } } - impl #impl_generics #from_value for #ref_<#ident #type_generics> { - fn from_value(value: #value) -> #result { - #path(value) + #[automatically_derived] + impl #impl_generics #unsafe_to_mut for #ident #type_generics #where_clause { + type Guard = #raw_value_guard; + + unsafe fn unsafe_to_mut<'a>(value: #value) -> #vm_result<(&'a mut Self, Self::Guard)> { + let (mut value, guard) = #vm_try!(#value::into_any_mut_ptr(value)); + #vm_result::Ok((#non_null::as_mut(&mut value), guard)) } } - }); - let impl_from_value_mut = attr.from_value_mut.as_ref().map(|path| quote! { - impl #impl_generics #unsafe_to_mut for #ident #type_generics { - type Guard = #raw_any_guard; + #[automatically_derived] + impl #impl_generics #unsafe_to_value for &#ident #type_generics #where_clause { + type Guard = #value_ref_guard; - unsafe fn unsafe_to_mut<'a>(value: #value) -> #vm_result<(&'a mut Self, Self::Guard)> { - let value = #vm_try!(#path(value)); - let (mut value, guard) = #mut_::into_raw(value); - #vm_result::Ok((value.as_mut(), guard)) + unsafe fn unsafe_to_value(self) -> #vm_result<(#value, Self::Guard)> { + let (shared, guard) = #vm_try!(#value::from_ref(self)); + #vm_result::Ok((shared, guard)) } } - impl #impl_generics #from_value for #mut_<#ident #type_generics> { - fn from_value(value: #value) -> #result { - #path(value) + #[automatically_derived] + impl #impl_generics #unsafe_to_value for &mut #ident #type_generics #where_clause { + type Guard = #value_mut_guard; + + unsafe fn unsafe_to_value(self) -> #vm_result<(#value, Self::Guard)> { + let (shared, guard) = #vm_try!(#value::from_mut(self)); + #vm_result::Ok((shared, guard)) } } - }); + }; quote! { #install_with #impl_named #impl_type_of - #impl_from_value - #impl_from_value_ref - #impl_from_value_mut - #non_builtin + #impl_any } } } diff --git a/crates/rune-macros/src/context.rs b/crates/rune-macros/src/context.rs index c8571c2f0..9f18f851a 100644 --- a/crates/rune-macros/src/context.rs +++ b/crates/rune-macros/src/context.rs @@ -86,17 +86,6 @@ pub(crate) struct TypeAttr { pub(crate) constructor: Option, /// Parsed documentation. pub(crate) docs: Vec, - /// Indicates that this is a builtin type, so don't generate an `Any` - /// implementation for it. - pub(crate) builtin: Option, - /// Indicate a static type to use. - pub(crate) static_type: Option, - /// Method to use to convert from value. - pub(crate) from_value: Option, - /// Method to use to convert from value ref. - pub(crate) from_value_ref: Option, - /// Method to use to convert from value mut. - pub(crate) from_value_mut: Option, /// Method to use to convert from value. pub(crate) impl_params: Option>, } @@ -509,7 +498,7 @@ impl Context { } let result = a.parse_nested_meta(|meta| { - if meta.path == MODULE || meta.path == CRATE { + if meta.path.is_ident("module") || meta.path.is_ident("crate") { // Parse `#[rune(crate [= ])]` if meta.input.parse::>()?.is_some() { attr.module = Some(parse_path_compat(meta.input)?); @@ -547,89 +536,93 @@ impl Context { continue; } - if a.path() == RUNE { - let result = a.parse_nested_meta(|meta| { - if meta.path == PARSE { - // Parse `#[rune(parse = "..")]` - meta.input.parse::()?; - let s: syn::LitStr = meta.input.parse()?; - - match s.value().as_str() { - "meta_only" => { - attr.parse = ParseKind::MetaOnly; - } - other => { - return Err(syn::Error::new( - meta.input.span(), - format!( - "Unsupported `#[rune(parse = ..)]` argument `{}`", - other - ), - )); - } - }; - } else if meta.path == ITEM { - // Parse `#[rune(item = "..")]` - meta.input.parse::()?; - attr.item = Some(meta.input.parse()?); - } else if meta.path == NAME { - // Parse `#[rune(name = "..")]` - meta.input.parse::()?; - attr.name = Some(meta.input.parse()?); - } else if meta.path == MODULE || meta.path == CRATE { - // Parse `#[rune(crate [= ])]` - if meta.input.parse::>()?.is_some() { - attr.module = Some(parse_path_compat(meta.input)?); - } else { - attr.module = Some(syn::parse_quote!(crate)); + if a.path() != RUNE { + continue; + } + + let result = a.parse_nested_meta(|meta| { + if meta.path.is_ident("parse") { + // Parse `#[rune(parse = "..")]` + meta.input.parse::()?; + let s: syn::LitStr = meta.input.parse()?; + + match s.value().as_str() { + "meta_only" => { + attr.parse = ParseKind::MetaOnly; } - } else if meta.path == INSTALL_WITH { - // Parse `#[rune(install_with = )]` - meta.input.parse::()?; - attr.install_with = Some(parse_path_compat(meta.input)?); - } else if meta.path == CONSTRUCTOR { - if attr.constructor.is_some() { + other => { return Err(syn::Error::new( - meta.path.span(), - "#[rune(constructor)] must only be used once", + meta.input.span(), + format!("Unsupported `#[rune(parse = ..)]` argument `{}`", other), )); } + }; - attr.constructor = Some(meta.path.span()); - } else if meta.path == BUILTIN { - attr.builtin = Some(meta.path.span()); - } else if meta.path == STATIC_TYPE { - meta.input.parse::()?; - attr.static_type = Some(meta.input.parse()?); - } else if meta.path == FROM_VALUE { - meta.input.parse::()?; - attr.from_value = Some(meta.input.parse()?); - } else if meta.path == FROM_VALUE_REF { - meta.input.parse::()?; - attr.from_value_ref = Some(meta.input.parse()?); - } else if meta.path == FROM_VALUE_MUT { - meta.input.parse::()?; - attr.from_value_mut = Some(meta.input.parse()?); - } else if meta.path.is_ident("impl_params") { - meta.input.parse::()?; - let content; - syn::bracketed!(content in meta.input); - attr.impl_params = - Some(syn::punctuated::Punctuated::parse_terminated(&content)?); + return Ok(()); + } + + if meta.path.is_ident("item") { + // Parse `#[rune(item = "..")]` + meta.input.parse::()?; + attr.item = Some(meta.input.parse()?); + return Ok(()); + } + + if meta.path.is_ident("name") { + // Parse `#[rune(name = "..")]` + meta.input.parse::()?; + attr.name = Some(meta.input.parse()?); + return Ok(()); + } + + if meta.path.is_ident("module") || meta.path.is_ident("crate") { + // Parse `#[rune(crate [= ])]` + if meta.input.parse::>()?.is_some() { + attr.module = Some(parse_path_compat(meta.input)?); } else { - return Err(syn::Error::new_spanned( - &meta.path, - "Unsupported type attribute", + attr.module = Some(syn::parse_quote!(crate)); + } + + return Ok(()); + } + + if meta.path.is_ident("install_with") { + // Parse `#[rune(install_with = )]` + meta.input.parse::()?; + attr.install_with = Some(parse_path_compat(meta.input)?); + return Ok(()); + } + + if meta.path.is_ident("constructor") { + if attr.constructor.is_some() { + return Err(syn::Error::new( + meta.path.span(), + "#[rune(constructor)] must only be used once", )); } - Ok(()) - }); + attr.constructor = Some(meta.path.span()); + return Ok(()); + } - if let Err(e) = result { - self.error(e); - }; - } + if meta.path.is_ident("impl_params") { + meta.input.parse::()?; + let content; + syn::bracketed!(content in meta.input); + attr.impl_params = + Some(syn::punctuated::Punctuated::parse_terminated(&content)?); + return Ok(()); + } + + Err(syn::Error::new_spanned( + &meta.path, + "Unsupported type attribute", + )) + }); + + if let Err(e) = result { + self.error(e); + }; } attr @@ -747,7 +740,6 @@ impl Context { maybe_type_of: path(m, ["runtime", "MaybeTypeOf"]), meta: path(m, ["compile", "meta"]), module: path(m, ["__private", "Module"]), - mut_: path(m, ["runtime", "Mut"]), named: path(m, ["compile", "Named"]), non_null: path(core, ["ptr", "NonNull"]), object: path(m, ["runtime", "Object"]), @@ -758,15 +750,12 @@ impl Context { parse: path(m, ["parse", "Parse"]), parser: path(m, ["parse", "Parser"]), protocol: path(m, ["runtime", "Protocol"]), - raw_any_guard: path(m, ["runtime", "RawAnyGuard"]), raw_value_guard: path(m, ["runtime", "RawValueGuard"]), - ref_: path(m, ["runtime", "Ref"]), result: path(core, ["result", "Result"]), runtime_error: path(m, ["runtime", "RuntimeError"]), span: path(m, ["ast", "Span"]), spanned: path(m, ["ast", "Spanned"]), static_type_info: path(m, ["runtime", "StaticTypeInfo"]), - static_type_mod: path(m, ["runtime", "static_type"]), string: path(m, ["alloc", "String"]), to_const_value_t: path(m, ["runtime", "ToConstValue"]), to_tokens: path(m, ["macros", "ToTokens"]), @@ -845,7 +834,6 @@ pub(crate) struct Tokens { pub(crate) maybe_type_of: syn::Path, pub(crate) meta: syn::Path, pub(crate) module: syn::Path, - pub(crate) mut_: syn::Path, pub(crate) named: syn::Path, pub(crate) non_null: syn::Path, pub(crate) object: syn::Path, @@ -856,15 +844,12 @@ pub(crate) struct Tokens { pub(crate) parse: syn::Path, pub(crate) parser: syn::Path, pub(crate) protocol: syn::Path, - pub(crate) raw_any_guard: syn::Path, pub(crate) raw_value_guard: syn::Path, - pub(crate) ref_: syn::Path, pub(crate) result: syn::Path, pub(crate) runtime_error: syn::Path, pub(crate) span: syn::Path, pub(crate) spanned: syn::Path, pub(crate) static_type_info: syn::Path, - pub(crate) static_type_mod: syn::Path, pub(crate) string: syn::Path, pub(crate) to_const_value_t: syn::Path, pub(crate) to_tokens: syn::Path, diff --git a/crates/rune-macros/src/internals.rs b/crates/rune-macros/src/internals.rs index 6ccbfeac3..0ad55d450 100644 --- a/crates/rune-macros/src/internals.rs +++ b/crates/rune-macros/src/internals.rs @@ -14,20 +14,8 @@ pub const OPTION: Symbol = Symbol("option"); pub const META: Symbol = Symbol("meta"); pub const SPAN: Symbol = Symbol("span"); pub const PARSE_WITH: Symbol = Symbol("parse_with"); -pub const PARSE: Symbol = Symbol("parse"); - -pub const NAME: Symbol = Symbol("name"); -pub const ITEM: Symbol = Symbol("item"); -pub const MODULE: Symbol = Symbol("module"); -pub const CRATE: Symbol = Symbol("crate"); -pub const INSTALL_WITH: Symbol = Symbol("install_with"); pub const CONSTRUCTOR: Symbol = Symbol("constructor"); -pub const BUILTIN: Symbol = Symbol("builtin"); -pub const STATIC_TYPE: Symbol = Symbol("static_type"); -pub const FROM_VALUE: Symbol = Symbol("from_value"); -pub const FROM_VALUE_REF: Symbol = Symbol("from_value_ref"); -pub const FROM_VALUE_MUT: Symbol = Symbol("from_value_mut"); pub const GET: Symbol = Symbol("get"); pub const SET: Symbol = Symbol("set"); pub const COPY: Symbol = Symbol("copy"); diff --git a/crates/rune/src/cli/tests.rs b/crates/rune/src/cli/tests.rs index 300f1c356..03418e7e1 100644 --- a/crates/rune/src/cli/tests.rs +++ b/crates/rune/src/cli/tests.rs @@ -39,6 +39,10 @@ mod cli { /// Break on the first test failed. #[arg(long)] pub fail_fast: bool, + /// Skip building dynamic lib tests from entrypoints. This means only + /// tests found in runtime contexts will be run. + #[arg(long)] + pub skip_lib_tests: bool, /// Filter tests by name. pub filters: Vec, } @@ -108,6 +112,7 @@ where let mut executed = 0usize; let mut skipped = 0usize; let mut build_errors = 0usize; + let mut skipped_entries = 0usize; let mut collected = Vec::new(); let capture = crate::modules::capture_io::CaptureIo::new(); @@ -134,6 +139,10 @@ where }; for e in entries { + if flags.skip_lib_tests { + continue; + } + let mut options = options.clone(); if e.is_argument() { @@ -215,6 +224,7 @@ where &options, &context, &mut build_errors, + &mut skipped_entries, &mut collected, &mut filter, )?; @@ -239,6 +249,7 @@ where options, &context, &mut build_errors, + &mut skipped_entries, &mut collected, &mut filter, )?; @@ -343,7 +354,7 @@ where section.append(format_args!(" {executed} tests"))?; - let any = failures > 0 || build_errors > 0 || skipped > 0; + let any = failures > 0 || build_errors > 0 || skipped > 0 || skipped_entries > 0; if any { section.append(" with")?; @@ -370,7 +381,13 @@ where emit(Color::Error, failures, "failure", "failures")?; emit(Color::Error, build_errors, "build error", "build errors")?; - emit(Color::Ignore, skipped, "ignored", "ignored")?; + emit(Color::Ignore, skipped, "filtered", "filtered")?; + emit( + Color::Ignore, + skipped_entries, + "filtered entries", + "filtered entries", + )?; } writeln!(io.stdout, " in {:.3} seconds", elapsed.as_secs_f64())?; @@ -390,6 +407,7 @@ fn populate_doc_tests( options: &Options, context: &crate::Context, build_errors: &mut usize, + skipped_entries: &mut usize, collected: &mut Vec<(Diagnostics, Sources)>, filter: &mut dyn FnMut(&Item) -> Result, ) -> Result> { @@ -400,6 +418,13 @@ fn populate_doc_tests( continue; } + let is_filtered = filter(&test.item)?; + + if is_filtered { + *skipped_entries = skipped_entries.wrapping_add(1); + continue; + } + let mut sources = Sources::new(); let source = Source::new(test.item.try_to_string()?, &test.content)?; @@ -442,10 +467,11 @@ fn populate_doc_tests( unit.clone(), sources.clone(), test.params, - filter(&test.item)?, + is_filtered, ))?; } } + Ok(cases) } diff --git a/crates/rune/src/compile/context.rs b/crates/rune/src/compile/context.rs index dc794daca..4af44234d 100644 --- a/crates/rune/src/compile/context.rs +++ b/crates/rune/src/compile/context.rs @@ -415,11 +415,13 @@ impl Context { /// This installs everything that has been declared in the given [Module] /// and ensures that they are compatible with the overall context, like /// ensuring that a given type is only declared once. + #[tracing::instrument(skip_all, fields(item = ?module.as_ref().item))] pub fn install(&mut self, module: M) -> Result<(), ContextError> where M: AsRef, { let module = module.as_ref(); + tracing::trace!("installing"); if let Some(id) = module.unique { if !self.unique.try_insert(id)? { @@ -431,32 +433,40 @@ impl Context { self.crates.try_insert(name.try_into()?)?; } + tracing::trace!("module"); self.install_module(module)?; + tracing::trace!(types = module.types.len(), "types"); for ty in &module.types { self.install_type(ty)?; } + tracing::trace!(traits = module.traits.len(), "traits"); for t in &module.traits { self.install_trait(t)?; } + tracing::trace!(items = module.items.len(), "items"); for item in &module.items { self.install_item(item)?; } + tracing::trace!(associated = module.associated.len(), "associated"); for assoc in &module.associated { self.install_associated(assoc)?; } + tracing::trace!(trait_impls = module.trait_impls.len(), "trait impls"); for t in &module.trait_impls { self.install_trait_impl(t)?; } + tracing::trace!(reexports = module.reexports.len(), "reexports"); for r in &module.reexports { self.install_reexport(r)?; } + tracing::trace!(construct = module.construct.len(), "construct"); for (hash, type_info, construct) in &module.construct { self.install_construct(*hash, type_info, construct)?; } @@ -623,8 +633,11 @@ impl Context { } /// Install the given meta. + #[tracing::instrument(skip_all)] fn install_meta(&mut self, meta: ContextMeta) -> Result<(), ContextError> { if let Some(item) = &meta.item { + tracing::trace!(?item); + self.names.insert(item)?; self.item_to_hash diff --git a/crates/rune/src/doc/markdown.rs b/crates/rune/src/doc/markdown.rs index f139583ce..d6813eb37 100644 --- a/crates/rune/src/doc/markdown.rs +++ b/crates/rune/src/doc/markdown.rs @@ -351,9 +351,7 @@ where } } None => { - if is_rune { - syntax = Some((token, None, is_rune)); - } + syntax = Some((token, None, is_rune)); } } } diff --git a/crates/rune/src/runtime/args.rs b/crates/rune/src/runtime/args.rs index 0b7f35028..4c387b9f8 100644 --- a/crates/rune/src/runtime/args.rs +++ b/crates/rune/src/runtime/args.rs @@ -80,7 +80,7 @@ where }; // SAFETY: We've setup the type so that the caller cannot ignore the guard. - self.guard = unsafe { Some(vm_try!(GuardedArgs::unsafe_into_stack(value, stack))) }; + self.guard = unsafe { Some(vm_try!(GuardedArgs::guarded_into_stack(value, stack))) }; VmResult::Ok(()) } diff --git a/crates/rune/src/runtime/function.rs b/crates/rune/src/runtime/function.rs index 2057aea50..bf7bd3ea1 100644 --- a/crates/rune/src/runtime/function.rs +++ b/crates/rune/src/runtime/function.rs @@ -553,7 +553,7 @@ where let size = count.max(1); // Ensure we have space for the return value. let mut stack = vm_try!(Stack::with_capacity(size)); - let _guard = vm_try!(unsafe { args.unsafe_into_stack(&mut stack) }); + let _guard = vm_try!(unsafe { args.guarded_into_stack(&mut stack) }); vm_try!(stack.resize(size)); vm_try!((handler.handler)( &mut stack, @@ -577,13 +577,9 @@ where } Inner::FnTupleStruct(tuple) => { vm_try!(check_args(args.count(), tuple.args)); - let Some(args) = args.try_into_args() else { - return VmResult::err(VmErrorKind::InvalidTupleCall); - }; - vm_try!(Value::tuple_struct( - tuple.rtti.clone(), - vm_try!(args.try_into_vec()) - )) + // SAFETY: We don't let the guard outlive the value. + let (args, _guard) = vm_try!(unsafe { args.guarded_into_vec() }); + vm_try!(Value::tuple_struct(tuple.rtti.clone(), args)) } Inner::FnUnitVariant(unit) => { vm_try!(check_args(args.count(), 0)); @@ -591,13 +587,9 @@ where } Inner::FnTupleVariant(tuple) => { vm_try!(check_args(args.count(), tuple.args)); - let Some(args) = args.try_into_args() else { - return VmResult::err(VmErrorKind::InvalidTupleCall); - }; - vm_try!(Value::tuple_variant( - tuple.rtti.clone(), - vm_try!(args.try_into_vec()) - )) + // SAFETY: We don't let the guard outlive the value. + let (args, _guard) = vm_try!(unsafe { args.guarded_into_vec() }); + vm_try!(Value::tuple_variant(tuple.rtti.clone(), args)) } }; @@ -947,7 +939,7 @@ impl FnOffset { let mut vm = Vm::new(self.context.clone(), self.unit.clone()); vm.set_ip(self.offset); - let _guard = vm_try!(unsafe { args.unsafe_into_stack(vm.stack_mut()) }); + let _guard = vm_try!(unsafe { args.guarded_into_stack(vm.stack_mut()) }); vm_try!(extra.into_stack(vm.stack_mut())); self.call.call_with_vm(vm) diff --git a/crates/rune/src/runtime/guarded_args.rs b/crates/rune/src/runtime/guarded_args.rs index bdb472b48..d6864e506 100644 --- a/crates/rune/src/runtime/guarded_args.rs +++ b/crates/rune/src/runtime/guarded_args.rs @@ -15,14 +15,19 @@ pub trait GuardedArgs { /// /// # Safety /// - /// This is implemented for and allows encoding references on the stack. - /// The returned guard must be dropped before any used references are - /// invalidated. - unsafe fn unsafe_into_stack(self, stack: &mut Stack) -> VmResult; + /// This can encode references onto the stack. The caller must ensure that + /// the guard is dropped before any references which have been encoded are + /// no longer alive. + unsafe fn guarded_into_stack(self, stack: &mut Stack) -> VmResult; - /// Attempts to convert this type into Args, which will only succeed as long - /// as it doesn't contain any references to Any types. - fn try_into_args(self) -> Option; + /// Encode arguments into a vector. + /// + /// # Safety + /// + /// This can encode references into the vector. The caller must ensure that + /// the guard is dropped before any references which have been encoded are + /// no longer alive. + unsafe fn guarded_into_vec(self) -> VmResult<(Vec, Self::Guard)>; /// The number of arguments. fn count(&self) -> usize; @@ -37,18 +42,25 @@ macro_rules! impl_into_args { type Guard = ($($ty::Guard,)*); #[allow(unused)] - unsafe fn unsafe_into_stack(self, stack: &mut Stack) -> VmResult { + #[inline] + unsafe fn guarded_into_stack(self, stack: &mut Stack) -> VmResult { let ($($value,)*) = self; $(let $value = vm_try!($value.unsafe_to_value());)* $(vm_try!(stack.push($value.0));)* VmResult::Ok(($($value.1,)*)) } - fn try_into_args(self) -> Option { + #[allow(unused)] + #[inline] + unsafe fn guarded_into_vec(self) -> VmResult<(Vec, Self::Guard)> { let ($($value,)*) = self; - Some(($($value.try_into_to_value()?,)*)) + $(let $value = vm_try!($value.unsafe_to_value());)* + let mut out = vm_try!(Vec::try_with_capacity($count)); + $(vm_try!(out.try_push($value.0));)* + VmResult::Ok((out, ($($value.1,)*))) } + #[inline] fn count(&self) -> usize { $count } @@ -62,12 +74,13 @@ impl GuardedArgs for Vec { type Guard = (); #[inline] - unsafe fn unsafe_into_stack(self, stack: &mut Stack) -> VmResult { + unsafe fn guarded_into_stack(self, stack: &mut Stack) -> VmResult { self.into_stack(stack) } - fn try_into_args(self) -> Option { - Some(self) + #[inline] + unsafe fn guarded_into_vec(self) -> VmResult<(Vec, Self::Guard)> { + VmResult::Ok((self, ())) } #[inline] @@ -81,16 +94,17 @@ impl GuardedArgs for ::rust_alloc::vec::Vec { type Guard = (); #[inline] - unsafe fn unsafe_into_stack(self, stack: &mut Stack) -> VmResult { + unsafe fn guarded_into_stack(self, stack: &mut Stack) -> VmResult { self.into_stack(stack) } - fn try_into_args(self) -> Option { - Some(self) + #[inline] + unsafe fn guarded_into_vec(self) -> VmResult<(Vec, Self::Guard)> { + VmResult::Ok((vm_try!(Vec::try_from(self)), ())) } #[inline] fn count(&self) -> usize { - (self as &dyn Args).count() + self.len() } } diff --git a/crates/rune/src/runtime/to_value.rs b/crates/rune/src/runtime/to_value.rs index dde413454..6362f5612 100644 --- a/crates/rune/src/runtime/to_value.rs +++ b/crates/rune/src/runtime/to_value.rs @@ -154,10 +154,6 @@ pub trait UnsafeToValue: Sized { /// The value returned must not be used after the guard associated with it /// has been dropped. unsafe fn unsafe_to_value(self) -> VmResult<(Value, Self::Guard)>; - - /// Attempts to convert this UnsafeToValue into a ToValue, which is only - /// possible if it is not a reference to an Any type. - fn try_into_to_value(self) -> Option; } impl ToValue for T @@ -179,10 +175,6 @@ where unsafe fn unsafe_to_value(self) -> VmResult<(Value, Self::Guard)> { VmResult::Ok((vm_try!(self.to_value()), ())) } - - fn try_into_to_value(self) -> Option { - Some(self) - } } impl ToValue for &Value { diff --git a/crates/rune/src/runtime/vm.rs b/crates/rune/src/runtime/vm.rs index fdf67691b..dd1d63486 100644 --- a/crates/rune/src/runtime/vm.rs +++ b/crates/rune/src/runtime/vm.rs @@ -494,7 +494,7 @@ impl Vm { // Safety: We hold onto the guard until the vm has completed and // `VmExecution` will clear the stack before this function returns. // Erronously or not. - let guard = unsafe { args.unsafe_into_stack(&mut self.stack).into_result()? }; + let guard = unsafe { args.guarded_into_stack(&mut self.stack).into_result()? }; let value = { // Clearing the stack here on panics has safety implications - see @@ -535,7 +535,7 @@ impl Vm { // Safety: We hold onto the guard until the vm has completed and // `VmExecution` will clear the stack before this function returns. // Erronously or not. - let guard = unsafe { args.unsafe_into_stack(&mut self.stack).into_result()? }; + let guard = unsafe { args.guarded_into_stack(&mut self.stack).into_result()? }; let value = { // Clearing the stack here on panics has safety implications - see @@ -578,7 +578,7 @@ impl Vm { // Safety: We hold onto the guard until the vm has completed and // `VmExecution` will clear the stack before this function returns. // Erronously or not. - let guard = unsafe { args.unsafe_into_stack(&mut self.stack).into_result()? }; + let guard = unsafe { args.guarded_into_stack(&mut self.stack).into_result()? }; let value = { // Clearing the stack here on panics has safety implications - see diff --git a/crates/rune/src/runtime/vm_error.rs b/crates/rune/src/runtime/vm_error.rs index bd3fb47f5..b7b3120b3 100644 --- a/crates/rune/src/runtime/vm_error.rs +++ b/crates/rune/src/runtime/vm_error.rs @@ -79,6 +79,11 @@ impl VmError { pub(crate) fn into_kind(self) -> VmErrorKind { self.inner.error.kind } + + #[cfg(test)] + pub(crate) fn as_kind(&self) -> &VmErrorKind { + &self.inner.error.kind + } } impl fmt::Display for VmError { @@ -827,7 +832,6 @@ pub(crate) enum VmErrorKind { }, MissingCallFrame, IllegalFormat, - InvalidTupleCall, } impl fmt::Display for VmErrorKind { @@ -1042,12 +1046,6 @@ impl fmt::Display for VmErrorKind { VmErrorKind::IllegalFormat => { write!(f, "Value cannot be formatted") } - VmErrorKind::InvalidTupleCall => { - write!( - f, - "Tuple struct/variant constructors cannot be called with references" - ) - } } } } diff --git a/crates/rune/src/tests/function_guardedargs.rs b/crates/rune/src/tests/function_guardedargs.rs index c9bece878..2b2a6506c 100644 --- a/crates/rune/src/tests/function_guardedargs.rs +++ b/crates/rune/src/tests/function_guardedargs.rs @@ -1,9 +1,7 @@ prelude!(); -#[derive(Default)] -struct MyAny {} - -crate::__internal_impl_any!(self, MyAny); +#[derive(Any)] +struct MyAny; fn get_vm() -> crate::support::Result { use std::sync::Arc; @@ -33,7 +31,7 @@ fn references_allowed_for_function_calls() { let value_result = function.call::((crate::Value::unit(),)); assert!(value_result.is_ok()); - let mut mine = MyAny::default(); + let mut mine = MyAny; let ref_result = function.call::((&mine,)); assert!(ref_result.is_ok()); @@ -55,17 +53,23 @@ fn references_disallowed_for_tuple_variant() { let value_result = constructor.call::((crate::Value::unit(),)); assert!(value_result.is_ok()); - let mut mine = MyAny::default(); + let mut mine = MyAny; - let VmResult::Err(ref_error) = constructor.call::((&mine,)) else { + let VmResult::Err(err) = constructor.call::((&mine,)) else { panic!("expected ref call to return an error") }; - assert_eq!(ref_error.into_kind(), VmErrorKind::InvalidTupleCall); + assert!( + matches!(err.as_kind(), VmErrorKind::AccessError { .. }), + "{err:?}" + ); - let VmResult::Err(mut_error) = constructor.call::((&mut mine,)) else { + let VmResult::Err(err) = constructor.call::((&mut mine,)) else { panic!("expected mut call to return an error") }; - assert_eq!(mut_error.into_kind(), VmErrorKind::InvalidTupleCall); + assert!( + matches!(err.as_kind(), VmErrorKind::AccessError { .. }), + "{err:?}" + ); let any_result = constructor.call::((mine,)); assert!(any_result.is_ok()); @@ -81,17 +85,23 @@ fn references_disallowed_for_tuple_struct() { let value_result = constructor.call::((crate::Value::unit(),)); assert!(value_result.is_ok()); - let mut mine = MyAny::default(); + let mut mine = MyAny; - let VmResult::Err(ref_error) = constructor.call::((&mine,)) else { + let VmResult::Err(err) = constructor.call::((&mine,)) else { panic!("expected ref call to return an error") }; - assert_eq!(ref_error.into_kind(), VmErrorKind::InvalidTupleCall); + assert!( + matches!(err.as_kind(), VmErrorKind::AccessError { .. }), + "{err:?}" + ); - let VmResult::Err(mut_error) = constructor.call::((&mut mine,)) else { + let VmResult::Err(err) = constructor.call::((&mut mine,)) else { panic!("expected mut call to return an error") }; - assert_eq!(mut_error.into_kind(), VmErrorKind::InvalidTupleCall); + assert!( + matches!(err.as_kind(), VmErrorKind::AccessError { .. }), + "{err:?}" + ); let any_result = constructor.call::((mine,)); assert!(any_result.is_ok());