From 540331d191183bf8fa472707e6b3d2c86c10377e Mon Sep 17 00:00:00 2001 From: Ashy Date: Sun, 23 Oct 2022 19:15:40 +0100 Subject: [PATCH] Refactored feature gating Applied the requested changes to be more sane and heavily simplify the feature gating for `dynamic_fields`. --- crates/rune-cli/src/main.rs | 4 +- crates/rune-macros/Cargo.toml | 3 -- crates/rune-macros/src/any.rs | 47 +++++++------------ crates/rune/Cargo.toml | 2 +- crates/rune/src/any.rs | 26 ---------- crates/rune/src/compile/dynamic_field_mode.rs | 8 ++-- crates/rune/src/compile/mod.rs | 4 +- crates/rune/src/compile/options.rs | 40 ++++++++++++---- crates/rune/src/runtime/any_obj.rs | 21 +++------ crates/rune/src/runtime/inst.rs | 4 -- crates/rune/src/runtime/protocol.rs | 2 - crates/rune/src/runtime/value.rs | 2 - crates/rune/src/runtime/vm.rs | 12 ++--- tests/tests/bug_344.rs | 3 +- .../{meta_fields.rs => dynamic_fields.rs} | 0 15 files changed, 72 insertions(+), 106 deletions(-) rename tests/tests/{meta_fields.rs => dynamic_fields.rs} (100%) diff --git a/crates/rune-cli/src/main.rs b/crates/rune-cli/src/main.rs index 4cdc3f981..5cc6f634e 100644 --- a/crates/rune-cli/src/main.rs +++ b/crates/rune-cli/src/main.rs @@ -50,7 +50,7 @@ //! [rune]: https://github.com/rune-rs/rune use anyhow::{anyhow, Result}; -use rune::compile::ParseOptionError; +use rune::compile::ParseOptionsErrors; use rune::termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; use rune::workspace::WorkspaceFilter; use rune::{Context, ContextError, Options}; @@ -329,7 +329,7 @@ struct Args { impl Args { /// Construct compiler options from cli arguments. - fn options(&self) -> Result { + fn options(&self) -> Result { let mut options = Options::default(); // Command-specific override defaults. diff --git a/crates/rune-macros/Cargo.toml b/crates/rune-macros/Cargo.toml index 1f2bc19f7..1a340ad0b 100644 --- a/crates/rune-macros/Cargo.toml +++ b/crates/rune-macros/Cargo.toml @@ -14,9 +14,6 @@ description = """ Helper macros for Rune. """ -[features] -dynamic_fields = [] - [dependencies] syn = { version = "1.0.82", features = ["full"] } quote = "1.0.10" diff --git a/crates/rune-macros/src/any.rs b/crates/rune-macros/src/any.rs index 586099a15..e971b558b 100644 --- a/crates/rune-macros/src/any.rs +++ b/crates/rune-macros/src/any.rs @@ -48,10 +48,7 @@ impl InternalCall { Ok(()) }; - #[cfg(feature = "dynamic_fields")] - let meta_fields = Some(quote! {Never}); - #[cfg(not(feature = "dynamic_fields"))] - let meta_fields = None; + let meta_fields = quote! {Never}; let generics = syn::Generics::default(); expand_any( @@ -60,7 +57,7 @@ impl InternalCall { &expand_into, &tokens, &generics, - meta_fields, + &meta_fields, ) } } @@ -104,24 +101,24 @@ impl Derive { let meta_fields = match attrs.meta_fields { Some(meta_fields) => { if cfg!(feature = "dynamic_fields") { - Some(quote! {#meta_fields}) + quote! {#meta_fields} } else { - None - } - } - None => { - if cfg!(feature = "dynamic_fields") { - Some(quote! {Never}) - } else { - None + return Err(vec![syn::Error::new( + meta_fields.span(), + format!( + "attempted to set dynamic_fields to `{}` while the feature `dynamic_fields` is disabled.", + meta_fields + ), + )]); } } + None => quote! {Never}, }; let name = "e!(#name); let ident = &self.input.ident; - expand_any(ident, name, &install_with, &tokens, generics, meta_fields) + expand_any(ident, name, &install_with, &tokens, generics, &meta_fields) } } @@ -400,7 +397,7 @@ pub(super) fn expand_any( installers: &TokenStream, tokens: &Tokens, generics: &syn::Generics, - meta_fields: Option, + meta_fields: &TokenStream, ) -> Result> where T: Copy + ToTokens, @@ -408,8 +405,8 @@ where let Tokens { any, context_error, - field_search: field_mode, - field_mode: field_search, + field_mode, + field_search, hash, module, named, @@ -432,16 +429,6 @@ where let generic_names = generics.type_params().map(|v| &v.ident).collect::>(); - let impl_field_mode = if meta_fields.is_some() { - quote! { - impl #impl_generics #field_mode for #ident #ty_generics #where_clause { - const FIELD_MODE: #field_search = #field_search::#meta_fields; - } - } - } else { - quote! {} - }; - let impl_named = if !generic_names.is_empty() { quote! { impl #impl_generics #named for #ident #ty_generics #where_clause { @@ -477,7 +464,9 @@ where #impl_named - #impl_field_mode + impl #impl_generics #field_search for #ident #ty_generics #where_clause { + const DYNAMIC_FIELD_MODE: #field_mode = #field_mode::#meta_fields; + } impl #impl_generics #type_of for #ident #ty_generics #where_clause { fn type_hash() -> #hash { diff --git a/crates/rune/Cargo.toml b/crates/rune/Cargo.toml index 9b202fed3..e66551f93 100644 --- a/crates/rune/Cargo.toml +++ b/crates/rune/Cargo.toml @@ -19,7 +19,7 @@ default = ["emit"] emit = ["codespan-reporting"] bench = [] workspace = ["toml", "toml-spanned-value", "semver", "relative-path", "serde-hashkey"] -dynamic_fields = ["rune-macros/dynamic_fields"] +dynamic_fields = [] [dependencies] thiserror = "1.0.30" diff --git a/crates/rune/src/any.rs b/crates/rune/src/any.rs index 070bc8816..388f0a5d3 100644 --- a/crates/rune/src/any.rs +++ b/crates/rune/src/any.rs @@ -1,4 +1,3 @@ -#[cfg(feature = "dynamic_fields")] use crate::compile::DynamicFieldSearch; use crate::compile::Named; @@ -22,31 +21,6 @@ pub use rune_macros::Any; /// name: String, /// } /// ``` -#[cfg(not(feature = "dynamic_fields"))] -pub trait Any: Named + std::any::Any { - /// The type hash of the type. - /// - /// TODO: make const field when `TypeId::of` is const. - fn type_hash() -> Hash; -} - -/// A trait which can be stored inside of an [AnyObj](crate::runtime::AnyObj). -/// -/// We use our own marker trait that must be explicitly derived to prevent other -/// VM native types (like strings) which also implement `std::any::Any` from -/// being stored as an `AnyObj`. -/// -/// This means, that only types which derive `Any` can be used inside of the VM: -/// -/// ``` -/// use rune::Any; -/// -/// #[derive(Any)] -/// struct Npc { -/// name: String, -/// } -/// ``` -#[cfg(feature = "dynamic_fields")] pub trait Any: Named + DynamicFieldSearch + std::any::Any { /// The type hash of the type. /// diff --git a/crates/rune/src/compile/dynamic_field_mode.rs b/crates/rune/src/compile/dynamic_field_mode.rs index 855ce1774..d196df245 100644 --- a/crates/rune/src/compile/dynamic_field_mode.rs +++ b/crates/rune/src/compile/dynamic_field_mode.rs @@ -1,24 +1,26 @@ -#[cfg(feature = "dynamic_fields")] /// The trait used to identify when to search for fields /// within a `AnyObj` via `Protocol::DYNAMIC_FIELD_GET` and /// `Protocol::DYNAMIC_FIELD_SET` pub trait DynamicFieldSearch { /// When to use Protocol::DYNAMIC_FIELD_SET/Protocol::DYNAMIC_FIELD_GET over Protocol::SET/Protocol::GET. - const FIELD_MODE: DynamicFieldMode; + const DYNAMIC_FIELD_MODE: DynamicFieldMode; } -#[cfg(feature = "dynamic_fields")] /// The possible values for the `MetaFieldMode` trait. #[derive(Debug, Clone, Copy)] +#[non_exhaustive] pub enum DynamicFieldMode { /// Never use `Protocol::DYNAMIC_FIELD_GET` or `Protocol::DYNAMIC_FIELD_SET` Never, /// Use `Protocol::DYNAMIC_FIELD_GET` or `Protocol::DYNAMIC_FIELD_SET` before /// `Protocol::GET` and `Protocol::SET` respectively. + #[cfg(feature = "dynamic_fields")] First, /// Use `Protocol::GET` or `Protocol::SET` before /// `Protocol::DYNAMIC_FIELD_GET` and `Protocol::DYNAMIC_FIELD_SET` respectively. + #[cfg(feature = "dynamic_fields")] Last, /// Use `Protocol::DYNAMIC_FIELD_GET` or `Protocol::DYNAMIC_FIELD_SET` exclusively. + #[cfg(feature = "dynamic_fields")] Only, } diff --git a/crates/rune/src/compile/mod.rs b/crates/rune/src/compile/mod.rs index 6cf070810..6baa43d14 100644 --- a/crates/rune/src/compile/mod.rs +++ b/crates/rune/src/compile/mod.rs @@ -48,7 +48,7 @@ pub(crate) use self::unit_builder::UnitBuilder; mod v1; mod options; -pub use self::options::{Options, ParseOptionError}; +pub use self::options::{Options, ParseOptionsErrors}; mod location; pub use self::location::Location; @@ -77,9 +77,7 @@ pub(crate) use self::names::Names; mod visibility; pub(crate) use self::visibility::Visibility; -#[cfg(feature = "dynamic_fields")] mod dynamic_field_mode; -#[cfg(feature = "dynamic_fields")] pub use self::dynamic_field_mode::{DynamicFieldMode, DynamicFieldSearch}; /// A compile result alias. diff --git a/crates/rune/src/compile/options.rs b/crates/rune/src/compile/options.rs index 9bdb1f921..b0b7cc0b4 100644 --- a/crates/rune/src/compile/options.rs +++ b/crates/rune/src/compile/options.rs @@ -1,10 +1,22 @@ use thiserror::Error; -/// Error raised when trying to parse an invalid option. +/// Errors possibly returned while parsing options. #[derive(Debug, Clone, Error)] -#[error("unsupported compile option `{option}`")] -pub struct ParseOptionError { - option: Box, +pub enum ParseOptionsErrors { + /// Error raised when trying to parse an invalid option. + #[error("unsupported compile option `{option}`")] + UnsupportedOption { + /// The unsupported option. + option: Box, + }, + /// Error raised when trying to enable a option which is not enabled by a feature gate. + #[error("Featured gated compile option `{option}` requires feature `{required_feature}`")] + DisabledFeatureOptionError { + /// The disabled option. + option: Box, + /// The required feature to enable this option. + required_feature: &'static str, + }, } /// Options that can be provided to the compiler. @@ -40,7 +52,7 @@ impl Options { /// /// It can be used to consistenly parse a collection of options by other /// programs as well. - pub fn parse_option(&mut self, option: &str) -> Result<(), ParseOptionError> { + pub fn parse_option(&mut self, option: &str) -> Result<(), ParseOptionsErrors> { let mut it = option.split('='); match it.next() { @@ -65,12 +77,24 @@ impl Options { Some("v2") => { self.v2 = it.next() != Some("false"); } - #[cfg(feature = "dynamic_fields")] Some("dynamic_fields") => { - self.dynamic_fields = it.next() != Some("false"); + let result = it.next() != Some("false"); + if result { + #[cfg(feature = "dynamic_fields")] + { + self.dynamic_fields = result; + } + #[cfg(not(feature = "dynamic_fields"))] + { + return Err(ParseOptionsErrors::DisabledFeatureOptionError { + option: option.into(), + required_feature: "dynamic_fields", + }); + } + } } _ => { - return Err(ParseOptionError { + return Err(ParseOptionsErrors::UnsupportedOption { option: option.into(), }); } diff --git a/crates/rune/src/runtime/any_obj.rs b/crates/rune/src/runtime/any_obj.rs index ee6c5c928..ff499a33d 100644 --- a/crates/rune/src/runtime/any_obj.rs +++ b/crates/rune/src/runtime/any_obj.rs @@ -1,6 +1,5 @@ //! Helper types for a holder of data. -#[cfg(feature = "dynamic_fields")] use crate::compile::{DynamicFieldMode, DynamicFieldSearch}; use crate::runtime::RawStr; @@ -59,8 +58,7 @@ impl AnyObj { debug: debug_impl::, type_name: type_name_impl::, type_hash: type_hash_impl::, - #[cfg(feature = "dynamic_fields")] - field_mode: T::FIELD_MODE, + field_mode: T::DYNAMIC_FIELD_MODE, }, data: data as *mut (), } @@ -117,8 +115,7 @@ impl AnyObj { debug: debug_ref_impl::, type_name: type_name_impl::, type_hash: type_hash_impl::, - #[cfg(feature = "dynamic_fields")] - field_mode: T::FIELD_MODE, + field_mode: T::DYNAMIC_FIELD_MODE, }, data: data as *const _ as *const (), } @@ -165,8 +162,7 @@ impl AnyObj { debug: debug_ref_impl::, type_name: type_name_impl::, type_hash: type_hash_impl::, - #[cfg(feature = "dynamic_fields")] - field_mode: T::Target::FIELD_MODE, + field_mode: T::Target::DYNAMIC_FIELD_MODE, }, data: boxed_guard as *const _ as *const (), } @@ -229,8 +225,7 @@ impl AnyObj { debug: debug_mut_impl::, type_name: type_name_impl::, type_hash: type_hash_impl::, - #[cfg(feature = "dynamic_fields")] - field_mode: T::FIELD_MODE, + field_mode: T::DYNAMIC_FIELD_MODE, }, data: data as *const _ as *const (), } @@ -277,8 +272,7 @@ impl AnyObj { debug: debug_mut_impl::, type_name: type_name_impl::, type_hash: type_hash_impl::, - #[cfg(feature = "dynamic_fields")] - field_mode: T::Target::FIELD_MODE, + field_mode: T::Target::DYNAMIC_FIELD_MODE, }, data: boxed_guard as *const _ as *const (), } @@ -462,7 +456,6 @@ impl AnyObj { (self.vtable.type_hash)() } - #[cfg(feature = "dynamic_fields")] pub(crate) fn field_mode(&self) -> DynamicFieldMode { self.vtable.field_mode } @@ -522,9 +515,7 @@ pub struct AnyObjVtable { type_name: TypeNameFn, /// Get the type hash of the stored type. type_hash: TypeHashFn, - - #[cfg(feature = "dynamic_fields")] - /// Get the method of searching fields. + /// When to search for dynamic fields. field_mode: DynamicFieldMode, } diff --git a/crates/rune/src/runtime/inst.rs b/crates/rune/src/runtime/inst.rs index b25355d7e..316564cea 100644 --- a/crates/rune/src/runtime/inst.rs +++ b/crates/rune/src/runtime/inst.rs @@ -997,7 +997,6 @@ pub enum Inst { /// /// => /// ``` - #[cfg(feature = "dynamic_fields")] ObjectIndexDynamicGet { /// The static string slot corresponding to the index to fetch. slot: usize, @@ -1015,7 +1014,6 @@ pub enum Inst { /// /// => /// ``` - #[cfg(feature = "dynamic_fields")] ObjectIndexDynamicSet { /// The static string slot corresponding to the index to set. slot: usize, @@ -1322,11 +1320,9 @@ impl fmt::Display for Inst { Self::Panic { reason } => { write!(fmt, "panic reason={}", reason.ident())?; } - #[cfg(feature = "dynamic_fields")] Self::ObjectIndexDynamicGet { slot } => { write!(fmt, "object-index-dynamic-get slot={}", slot)?; } - #[cfg(feature = "dynamic_fields")] Self::ObjectIndexDynamicSet { slot } => { write!(fmt, "object-index-dynamic-set slot={}", slot)?; } diff --git a/crates/rune/src/runtime/protocol.rs b/crates/rune/src/runtime/protocol.rs index 2fe45f73c..46597b428 100644 --- a/crates/rune/src/runtime/protocol.rs +++ b/crates/rune/src/runtime/protocol.rs @@ -81,14 +81,12 @@ impl Protocol { hash: Hash::new(0x418f5becbf885806), }; - #[cfg(feature = "dynamic_fields")] /// Search for a field by `String` pub const DYNAMIC_FIELD_GET: Protocol = Protocol { name: "dynamic_field_get", hash: Hash::new(0x6dda58b140dfeaf9), }; - #[cfg(feature = "dynamic_fields")] /// Search for a field by `String` and set it. pub const DYNAMIC_FIELD_SET: Protocol = Protocol { name: "dynamic_field_set", diff --git a/crates/rune/src/runtime/value.rs b/crates/rune/src/runtime/value.rs index 6fe6a12a0..1f93926ae 100644 --- a/crates/rune/src/runtime/value.rs +++ b/crates/rune/src/runtime/value.rs @@ -7,7 +7,6 @@ use crate::runtime::{ VmErrorKind, }; -#[cfg(feature = "dynamic_fields")] use crate::compile::DynamicFieldMode; use crate::{Any, Hash}; @@ -1061,7 +1060,6 @@ impl Value { })) } - #[cfg(feature = "dynamic_fields")] /// Function returns the method in which to search for fields within /// the value. pub(crate) fn field_mode(&self) -> Result { diff --git a/crates/rune/src/runtime/vm.rs b/crates/rune/src/runtime/vm.rs index df66a3eeb..7c92e64ae 100644 --- a/crates/rune/src/runtime/vm.rs +++ b/crates/rune/src/runtime/vm.rs @@ -993,7 +993,6 @@ impl Vm { } } - #[cfg(feature = "dynamic_fields")] /// Implementation of getting a string index on an object-like type. fn try_object_slot_index_dynamic_get( &mut self, @@ -1035,6 +1034,7 @@ impl Vm { CallResult::Unsupported(target) => CallResult::Unsupported(target), } } + #[cfg(feature = "dynamic_fields")] DynamicFieldMode::First => { let hash = index.hash(); match self.call_instance_fn( @@ -1070,6 +1070,7 @@ impl Vm { } } } + #[cfg(feature = "dynamic_fields")] DynamicFieldMode::Last => { let hash = index.hash(); let index = index.clone(); @@ -1101,6 +1102,7 @@ impl Vm { } } } + #[cfg(feature = "dynamic_fields")] DynamicFieldMode::Only => { let index = index.clone(); match self.call_instance_fn( @@ -1133,7 +1135,6 @@ impl Vm { })) } - #[cfg(feature = "dynamic_fields")] fn try_object_slot_index_dynamic_set( &mut self, target: Value, @@ -1188,6 +1189,7 @@ impl Vm { CallResult::Unsupported(target) => CallResult::Unsupported(target), } } + #[cfg(feature = "dynamic_fields")] DynamicFieldMode::First => { let hash = field.hash(); match self.call_instance_fn( @@ -1230,6 +1232,7 @@ impl Vm { } } } + #[cfg(feature = "dynamic_fields")] DynamicFieldMode::Last => { let hash = field.hash(); let field = field.clone(); @@ -1265,6 +1268,7 @@ impl Vm { } } } + #[cfg(feature = "dynamic_fields")] DynamicFieldMode::Only => { let field = field.clone(); match self.call_instance_fn( @@ -2424,7 +2428,6 @@ impl Vm { } /// Perform a specialized potentially dynamic index get operation on an object. - #[cfg(feature = "dynamic_fields")] #[cfg_attr(feature = "bench", inline(never))] fn op_object_index_dynamic_get(&mut self, string_slot: usize) -> Result<(), VmError> { let target = self.stack.pop()?; @@ -2443,7 +2446,6 @@ impl Vm { } /// Perform a specialized potentially dynamic index set operation on an object. - #[cfg(feature = "dynamic_fields")] #[cfg_attr(feature = "bench", inline(never))] fn op_object_index_dynamic_set(&mut self, string_slot: usize) -> Result<(), VmError> { let target = self.stack.pop()?; @@ -3481,11 +3483,9 @@ impl Vm { reason: Panic::from(reason), })); } - #[cfg(feature = "dynamic_fields")] Inst::ObjectIndexDynamicGet { slot } => { self.op_object_index_dynamic_get(slot)?; } - #[cfg(feature = "dynamic_fields")] Inst::ObjectIndexDynamicSet { slot } => { self.op_object_index_dynamic_set(slot)?; } diff --git a/tests/tests/bug_344.rs b/tests/tests/bug_344.rs index aaaca51ec..f4ac98e56 100644 --- a/tests/tests/bug_344.rs +++ b/tests/tests/bug_344.rs @@ -171,9 +171,8 @@ impl Named for GuardCheck { const BASE_NAME: RawStr = RawStr::from_str("GuardCheck"); } -#[cfg(feature = "dynamic_fields")] impl DynamicFieldSearch for GuardCheck { - const FIELD_MODE: DynamicFieldMode = DynamicFieldMode::Never; + const DYNAMIC_FIELD_MODE: DynamicFieldMode = DynamicFieldMode::Never; } impl TypeOf for GuardCheck { diff --git a/tests/tests/meta_fields.rs b/tests/tests/dynamic_fields.rs similarity index 100% rename from tests/tests/meta_fields.rs rename to tests/tests/dynamic_fields.rs