Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve handling of the generic baggage fields #5656

Merged
merged 6 commits into from
Jun 14, 2022
Merged

Improve handling of the generic baggage fields #5656

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe73a31
Parse generic baggage types more carefully to preserve inner structure
vstakhov Jun 9, 2022
2f54932
Add example
vstakhov Jun 9, 2022
0f577ae
Way too many clones
vstakhov Jun 9, 2022
f86d647
Allow multiple generic arguments for baggage fields
vstakhov Jun 10, 2022
be25e51
Try to detect errors earlier
vstakhov Jun 10, 2022
87dcd9f
Support more types for the baggage fields, get rid of the path constr…
vstakhov Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions node/orchestra/examples/duo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! A dummy to be used with cargo expand

use orchestra::{self as orchestra, Spawner, *};
use std::collections::HashMap;
use std::{collections::HashMap, sync::Arc};
mod misc;

pub use self::misc::*;
Expand Down Expand Up @@ -61,27 +61,27 @@ impl<Context> Fortified {
}

#[orchestra(signal=SigSigSig, event=EvX, error=Yikes, gen=AllMessages)]
struct Duo<T> {
struct Duo<T, U, V> {
#[subsystem(consumes: MsgStrukt, sends: [Plinko])]
sub0: Awesome,

#[subsystem(blocking, consumes: Plinko, sends: [MsgStrukt])]
plinkos: GoblinTower,

i_like_pi: f64,
i_like_generic: T,
i_like_hash: HashMap<f64, f64>,
i_like_generic: Arc<T>,
i_like_hash: HashMap<U, Arc<V>>,
}

fn main() {
use futures::{executor, pin_mut};

executor::block_on(async move {
let (orchestra, _handle): (Duo<_, f64>, _) = Duo::builder()
let (orchestra, _handle): (Duo<_, f64, u32, f32>, _) = Duo::builder()
.sub0(AwesomeSubSys::default())
.plinkos(Fortified::default())
.i_like_pi(::std::f64::consts::PI)
.i_like_generic(42.0)
.i_like_generic(Arc::new(42.0))
.i_like_hash(HashMap::new())
.spawner(DummySpawner)
.build()
Expand Down
11 changes: 3 additions & 8 deletions node/orchestra/proc-macro/src/impl_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,14 +249,9 @@ pub(crate) fn impl_builder(info: &OrchestraInfo) -> proc_macro2::TokenStream {
post_setter_generics[idx] = parse_quote! { Init<#field_type> };

// Baggage can also be generic, so we need to include that to a signature
let preserved_baggage_generic = if bag_field.generic {
quote! {#field_type,}
} else {
TokenStream::new()
};

let preserved_baggage_generics = &bag_field.generic_types;
quote! {
impl <InitStateSpawner, #preserved_baggage_generic #( #subsystem_passthrough_state_generics, )* #( #impl_baggage_state_generics, )* >
impl <InitStateSpawner, #( #preserved_baggage_generics, )* #( #subsystem_passthrough_state_generics, )* #( #impl_baggage_state_generics, )* >
#builder <InitStateSpawner, #( #subsystem_passthrough_state_generics, )* #( #pre_setter_generics, )* >
{
/// Specify the baggage in the builder when it was not initialized before
Expand All @@ -278,7 +273,7 @@ pub(crate) fn impl_builder(info: &OrchestraInfo) -> proc_macro2::TokenStream {
}
}
}
impl <InitStateSpawner, #preserved_baggage_generic #( #subsystem_passthrough_state_generics, )* #( #impl_baggage_state_generics, )* >
impl <InitStateSpawner, #( #preserved_baggage_generics, )* #( #subsystem_passthrough_state_generics, )* #( #impl_baggage_state_generics, )* >
#builder <InitStateSpawner, #( #subsystem_passthrough_state_generics, )* #( #post_setter_generics, )* > {
/// Specify the baggage in the builder when it has been previously initialized
pub fn #fname (self, var: #field_type ) ->
Expand Down
65 changes: 52 additions & 13 deletions node/orchestra/proc-macro/src/parse/parse_orchestra_struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ use syn::{
punctuated::Punctuated,
spanned::Spanned,
token::Bracket,
AttrStyle, Error, Field, FieldsNamed, GenericParam, Ident, ItemStruct, Path, Result, Token,
Type, Visibility,
AttrStyle, Error, Field, FieldsNamed, GenericParam, Ident, ItemStruct, Path, PathSegment,
Result, Token, Type, Visibility,
};

use quote::{quote, ToTokens};
Expand Down Expand Up @@ -108,11 +108,40 @@ pub(crate) struct SubSysField {

fn try_type_to_path(ty: Type, span: Span) -> Result<Path> {
match ty {
Type::Path(path) => Ok(path.path),
Type::Path(path) => Ok(path.path.clone()),
_ => Err(Error::new(span, "Type must be a path expression.")),
}
}

fn flatten_path_segments(path_segment: &PathSegment, span: Span) -> Vec<Ident> {
let mut result = vec![path_segment.ident.clone()];

match &path_segment.arguments {
syn::PathArguments::AngleBracketed(args) => {
let mut recursive_idents = args
.args
.iter()
.filter_map(|gen| match gen {
syn::GenericArgument::Type(ty) =>
try_type_to_path(ty.clone(), span.clone()).ok(),
Copy link
Contributor

@drahnr drahnr Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This swallows somerrors, which we shouldn't swallow but expose as part of the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GenericArgument enum looks the following:

pub enum GenericArgument {
        /// A lifetime argument.
        Lifetime(Lifetime),
        /// A type argument.
        Type(Type),
        /// A binding (equality constraint) on an associated type: the `Item =
        /// u8` in `Iterator<Item = u8>`.
        Binding(Binding),
        /// An associated type bound: `Iterator<Item: Display>`.
        Constraint(Constraint),
        /// A const expression. Must be inside of a block.
        ///
        /// NOTE: Identity expressions are represented as Type arguments, as
        /// they are indistinguishable syntactically.
        Const(Expr),
    }

Lifetimes are irrelevant - we can emit error on that, but it will be also emitted during compilation, as lifetimes are not efficiently supported. Binding is more interesting here, as there could be something like Item = T where T is indeed a generic. Constraint can also have generic inside. Probably, it is not a bad option just to throw an error there as it will be a compile error afterwards.

_ => None,
})
.flat_map(|gen_ty| {
gen_ty
.segments
.iter()
.flat_map(|seg| flatten_path_segments(seg, span.clone()))
.collect::<Vec<_>>()
})
.collect::<Vec<_>>();
result.append(&mut recursive_idents);
},
_ => {},
}

result
}

macro_rules! extract_variant {
($unique:expr, $variant:ident ; default = $fallback:expr) => {
extract_variant!($unique, $variant).unwrap_or_else(|| $fallback)
Expand Down Expand Up @@ -255,7 +284,7 @@ impl Parse for SubSystemAttrItems {
pub(crate) struct BaggageField {
pub(crate) field_name: Ident,
pub(crate) field_ty: Path,
pub(crate) generic: bool,
pub(crate) generic_types: Vec<Ident>,
pub(crate) vis: Visibility,
}

Expand Down Expand Up @@ -359,8 +388,7 @@ impl OrchestraInfo {
pub(crate) fn baggage_generic_types(&self) -> Vec<Ident> {
self.baggage
.iter()
.filter(|bag| bag.generic)
.filter_map(|bag| bag.field_ty.get_ident().cloned())
.flat_map(|bag| bag.generic_types.clone())
.collect::<Vec<_>>()
}

Expand Down Expand Up @@ -423,7 +451,7 @@ impl OrchestraGuts {
let ident = ident.ok_or_else(|| {
Error::new(
ty.span(),
"Missing identifier for field, only named fields are expceted.",
"Missing identifier for field, only named fields are expected.",
)
})?;

Expand Down Expand Up @@ -485,11 +513,22 @@ impl OrchestraGuts {
});
} else {
let field_ty = try_type_to_path(ty, ident.span())?;
let generic = field_ty
.get_ident()
.map(|ident| baggage_generics.contains(ident))
.unwrap_or(false);
baggage.push(BaggageField { field_name: ident, generic, field_ty, vis });
let generic_types = if let Some(ident) = field_ty.get_ident() {
match baggage_generics.get(ident) {
Some(ident) => vec![ident.clone()],
None => vec![],
}
} else {
field_ty
.segments
.iter()
.flat_map(|path_segment| {
flatten_path_segments(path_segment, field_ty.span().clone())
})
.filter(|seg_ident| baggage_generics.contains(seg_ident))
.collect::<Vec<_>>()
};
baggage.push(BaggageField { field_name: ident, generic_types, field_ty, vis });
}
}
Ok(Self { name, subsystems, baggage })
Expand All @@ -503,7 +542,7 @@ impl Parse for OrchestraGuts {
syn::Fields::Named(named) => {
let name = ds.ident.clone();

// collect the indepedentent subsystem generics
// collect the independent subsystem generics
// which need to be carried along, there are the non-generated ones
let mut orig_generics = ds.generics;

Expand Down