From 9515fc52161093cd9fcf59b22f8d5f4f49012d10 Mon Sep 17 00:00:00 2001 From: Debanjan Basu Date: Mon, 7 Apr 2025 23:39:22 +1000 Subject: [PATCH 1/4] made code safe, and fixed clippy lints --- Cargo.toml | 6 +- capnp_conv/src/lib.rs | 13 +- capnp_conv_macros/src/generators.rs | 124 +++++------ capnp_conv_macros/src/lib.rs | 10 + capnp_conv_macros/src/models.rs | 31 +-- capnp_conv_macros/src/parsers.rs | 322 +++++++++++++++++----------- capnp_conv_macros/src/utils.rs | 19 +- example/src/main.rs | 32 ++- 8 files changed, 330 insertions(+), 227 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e092ff2..985deb4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,9 +3,9 @@ resolver = "2" members = ["capnp_conv", "capnp_conv_macros", "capnp_conv_tests", "example"] [workspace.package] -authors = ["Aik Kalantarian "] -version = "0.3.1" -edition = "2021" +authors = ["Aik Kalantarian ", "Debanjan Basu "] +version = "0.3.2" +edition = "2024" license = "MIT" repository = "https://github.com/aikalant/capnp_conv" readme = "README.md" diff --git a/capnp_conv/src/lib.rs b/capnp_conv/src/lib.rs index 286da12..cad7606 100644 --- a/capnp_conv/src/lib.rs +++ b/capnp_conv/src/lib.rs @@ -1,4 +1,14 @@ -use capnp::{traits::Owned, Result}; +#![deny( + clippy::nursery, + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::string_slice, + clippy::pedantic +)] +#![forbid(unsafe_code)] + +use capnp::{Result, traits::Owned}; pub use capnp_conv_macros::capnp_conv; pub trait Writable { @@ -13,6 +23,7 @@ where { type OwnedType: Owned; + #[allow(clippy::missing_errors_doc)] fn read(reader: ::Reader<'_>) -> Result; } diff --git a/capnp_conv_macros/src/generators.rs b/capnp_conv_macros/src/generators.rs index 2c4b0b9..d9547b9 100644 --- a/capnp_conv_macros/src/generators.rs +++ b/capnp_conv_macros/src/generators.rs @@ -1,6 +1,6 @@ use heck::{ToSnakeCase, ToUpperCamelCase}; use proc_macro2::TokenStream as TokenStream2; -use quote::{format_ident, quote, ToTokens}; +use quote::{ToTokens, format_ident, quote}; use syn::{Ident, Path}; use crate::{ @@ -11,17 +11,17 @@ use crate::{ impl ItemInfo { pub fn generate_impls(&self, capnp_path: &Path) -> TokenStream2 { let impls = match self { - ItemInfo::Struct(struct_info) => vec![ + Self::Struct(struct_info) => vec![ struct_info.generate_writer_impl(capnp_path), struct_info.generate_reader_impl(capnp_path), struct_info.generate_try_from_impl(capnp_path), ], - ItemInfo::Enum(enum_info) if enum_info.is_union() => vec![ + Self::Enum(enum_info) if enum_info.is_union() => vec![ enum_info.generate_writer_impl(capnp_path), enum_info.generate_reader_impl(capnp_path), enum_info.generate_try_from_impl(capnp_path), ], - ItemInfo::Enum(enum_info) => vec![ + Self::Enum(enum_info) => vec![ enum_info.generate_into_impl(capnp_path), enum_info.generate_from_impl(capnp_path), enum_info.generate_to_impl(capnp_path), @@ -238,10 +238,10 @@ impl FieldInfo { if matches!(self.field_type, FieldType::Phantom) { quote!(::std::marker::PhantomData) } else if self.skip_read { - let field_reader = match &self.default_override { - Some(default_override) => quote!(#default_override()), - None => self.generate_default_reader(), - }; + let field_reader = self.default_override.as_ref().map_or_else( + || self.generate_default_reader(), + |default_override| quote!(#default_override()), + ); if self.is_optional { quote!(Some(#field_reader)) } else { @@ -279,16 +279,16 @@ impl FieldInfo { fn generate_default_reader(&self) -> TokenStream2 { let path = match &self.field_type { FieldType::Void() => return quote!(()), - FieldType::Primitive(path) => path, - FieldType::Data(path) => path, - FieldType::Text(path) => path, - FieldType::Struct(path) => path, - FieldType::EnumRemote(path) => path, - FieldType::Enum(path) => path, - FieldType::GroupOrUnion(path) => path, - FieldType::UnnamedUnion(path) => path, + FieldType::Primitive(path) + | FieldType::Data(path) + | FieldType::Text(path) + | FieldType::Struct(path) + | FieldType::EnumRemote(path) + | FieldType::Enum(path) + | FieldType::GroupOrUnion(path) + | FieldType::UnnamedUnion(path) + | FieldType::GenericStruct(path) => path, FieldType::List(_) => return quote!(Vec::default()), - FieldType::GenericStruct(path) => path, _ => unimplemented!(), }; let path = as_turbofish(path); @@ -366,27 +366,27 @@ impl FieldType { quote!(#reader_name.#getter()) }; match self { - FieldType::Phantom => unimplemented!(), - FieldType::EnumVariant => unimplemented!(), - FieldType::Void() => quote!(()), - FieldType::Primitive(_) => quote!(#getter), - FieldType::Data(_) => quote!(#getter?.to_owned()), - FieldType::Text(_) => quote!(#getter?.to_string()?), - FieldType::Struct(struct_path) => { + Self::Phantom => unimplemented!(), + Self::EnumVariant => unimplemented!(), + Self::Void() => quote!(()), + Self::Primitive(_) => quote!(#getter), + Self::Data(_) => quote!(#getter?.to_owned()), + Self::Text(_) => quote!(#getter?.to_string()?), + Self::Struct(struct_path) | Self::GenericStruct(struct_path) => { let struct_path = as_turbofish(struct_path); quote!(#struct_path::read(#getter?)?) } - FieldType::EnumRemote(_) => quote!(#getter?.into()), - FieldType::Enum(_) => quote!(#getter?), - FieldType::GroupOrUnion(path) => { + Self::EnumRemote(_) => quote!(#getter?.into()), + Self::Enum(_) => quote!(#getter?), + Self::GroupOrUnion(path) => { let path = as_turbofish(path); quote!(#path::read(#getter)?) } - FieldType::UnnamedUnion(union_path) => { + Self::UnnamedUnion(union_path) => { let union_path = as_turbofish(union_path); quote!(#union_path::read(#reader_name)?) } - FieldType::List(item_type) => { + Self::List(item_type) => { let item_getter = item_type.generate_struct_field_reader_list_item(); quote! { { @@ -400,26 +400,22 @@ impl FieldType { } } } - FieldType::GenericStruct(struct_path) => { - let struct_path = as_turbofish(struct_path); - quote!(#struct_path::read(#getter?)?) - } } } fn generate_struct_field_reader_list_item(&self) -> TokenStream2 { match self { - FieldType::Void() => quote!(()), - FieldType::Primitive(_) => quote!(reader.get(idx)), - FieldType::Data(_) => quote!(reader.get(idx)?.to_owned()), - FieldType::Text(_) => quote!(reader.get(idx)?.to_string()?), - FieldType::Struct(struct_path) => { - let struct_path = as_turbofish(struct_path); + Self::Void() => quote!(()), + Self::Primitive(_) => quote!(reader.get(idx)), + Self::Data(_) => quote!(reader.get(idx)?.to_owned()), + Self::Text(_) => quote!(reader.get(idx)?.to_string()?), + Self::Struct(struct_path) => { + let struct_path: Path = as_turbofish(struct_path); quote!(#struct_path::read(reader.get(idx))?) } - FieldType::EnumRemote(_) => quote!(reader.get(idx)?.into()), - FieldType::Enum(_) => quote!(reader.get(idx)?), - FieldType::List(item_type) => { + Self::EnumRemote(_) => quote!(reader.get(idx)?.into()), + Self::Enum(_) => quote!(reader.get(idx)?), + Self::List(item_type) => { let item_getter = item_type.generate_struct_field_reader_list_item(); quote! { { @@ -433,7 +429,7 @@ impl FieldType { } } } - FieldType::GenericStruct(struct_path) => { + Self::GenericStruct(struct_path) => { let struct_path = as_turbofish(struct_path); quote!(#struct_path::read(reader.get(idx))?) } @@ -454,22 +450,21 @@ impl FieldType { (quote!(*#field), quote!(#field)) }; match self { - FieldType::Phantom => unimplemented!(), - FieldType::EnumVariant => unimplemented!(), - FieldType::Void() => quote!(builder.#setter(())), - FieldType::Primitive(_) => quote!(builder.#setter(#deref_field)), - FieldType::Data(_) => quote!(builder.#setter(#ref_field)), - FieldType::Text(_) => quote!(builder.#setter(#field.as_str())), - FieldType::Struct(_) => quote!(#field.write(builder.reborrow().#initializer())), - FieldType::EnumRemote(_) => { + Self::Phantom => unimplemented!(), + Self::EnumVariant => unimplemented!(), + Self::Void() => quote!(builder.#setter(())), + Self::Primitive(_) | Self::Enum(_) => quote!(builder.#setter(#deref_field)), + Self::Data(_) => quote!(builder.#setter(#ref_field)), + Self::Text(_) => quote!(builder.#setter(#field.as_str())), + Self::Struct(_) => quote!(#field.write(builder.reborrow().#initializer())), + Self::EnumRemote(_) => { quote!(builder.#setter(::capnp_conv::RemoteEnum::to_capnp_enum(#ref_field))) } - FieldType::Enum(_) => quote!(builder.#setter(#deref_field)), - FieldType::GroupOrUnion(_) => { + Self::GroupOrUnion(_) | Self::GenericStruct(_) => { quote!(#field.write(builder.reborrow().#initializer())) } - FieldType::UnnamedUnion(_) => quote!(#field.write(builder.reborrow())), - FieldType::List(item_type) => { + Self::UnnamedUnion(_) => quote!(#field.write(builder.reborrow())), + Self::List(item_type) => { let field_setter = item_type.generate_struct_field_writer_list_item(); quote! { { @@ -482,23 +477,18 @@ impl FieldType { } } } - FieldType::GenericStruct(_) => { - quote!(#field.write(builder.reborrow().#initializer())) - } } } fn generate_struct_field_writer_list_item(&self) -> TokenStream2 { match self { - FieldType::Void() => quote!(builder.set(idx as u32, ())), - FieldType::Primitive(_) => quote!(builder.set(idx as u32, *item)), - FieldType::Data(_) => quote!(builder.set(idx as u32, item)), - FieldType::Text(_) => quote!(builder.set(idx as u32, item)), - FieldType::Struct(_) => quote!(item.write(builder.reborrow().get(idx as u32))), - FieldType::EnumRemote(_) => { + Self::Void() => quote!(builder.set(idx as u32, ())), + Self::Primitive(_) | Self::Enum(_) => quote!(builder.set(idx as u32, *item)), + Self::Data(_) | Self::Text(_) => quote!(builder.set(idx as u32, item)), + Self::Struct(_) => quote!(item.write(builder.reborrow().get(idx as u32))), + Self::EnumRemote(_) => { quote!(builder.set(idx as u32, ::capnp_conv::RemoteEnum::to_capnp_enum(item))) } - FieldType::Enum(_) => quote!(builder.set(idx as u32, *item)), - FieldType::List(item_type) => { + Self::List(item_type) => { let field_setter = item_type.generate_struct_field_writer_list_item(); quote! { let list = item; @@ -509,7 +499,7 @@ impl FieldType { } } } - FieldType::GenericStruct(_) => { + Self::GenericStruct(_) => { quote!(item.write(builder.reborrow().get(idx as u32))) } _ => unimplemented!(), diff --git a/capnp_conv_macros/src/lib.rs b/capnp_conv_macros/src/lib.rs index 64db097..eeabfcc 100644 --- a/capnp_conv_macros/src/lib.rs +++ b/capnp_conv_macros/src/lib.rs @@ -1,3 +1,13 @@ +#![deny( + clippy::nursery, + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::string_slice, + clippy::pedantic +)] +#![forbid(unsafe_code)] + mod generators; mod models; mod parsers; diff --git a/capnp_conv_macros/src/models.rs b/capnp_conv_macros/src/models.rs index 62b591e..00fce6c 100644 --- a/capnp_conv_macros/src/models.rs +++ b/capnp_conv_macros/src/models.rs @@ -22,6 +22,7 @@ pub struct EnumInfo { } #[derive(Debug)] +#[allow(clippy::struct_excessive_bools)] pub struct FieldInfo { pub rust_name: Ident, pub field_type: FieldType, @@ -37,36 +38,36 @@ pub struct FieldInfo { #[derive(Debug)] pub enum FieldType { - Phantom, - /// Only for capnp enums - EnumVariant, - /// () - Void(), - /// bool, i8/16/32/64, u8/16/32/64, f32/64 - Primitive(Path), /// Vec Data(Path), - /// String - Text(Path), - /// Non-generic capnp structs - Struct(Path), /// Requires field attribute `#[capnp_conv(type = "enum")]` /// Indicates to use the pre-existing capnp code generated enum Enum(Path), /// Requires field attribute `#[capnp_conv(type = "enum_remote")]` /// Indicates to use the a manually defined enum EnumRemote(Path), + /// Only for capnp enums + EnumVariant, + /// CapnpStruct(T1, T2, ...) + GenericStruct(Path), /// Requires field attribute `#[capnp_conv(type = "group")]` or `#[capnp_conv(type = "union")]` /// Applys to named unions only /// These don't need to be unwrapped by readers GroupOrUnion(Path), + /// Vec + List(Box), + Phantom, + /// bool, i8/16/32/64, u8/16/32/64, f32/64 + Primitive(Path), + /// Non-generic capnp structs + Struct(Path), + /// String + Text(Path), /// Requires field attribute `#[capnp_conv(type = "unnamed_union")]` /// Reader/writer acts as a "passthrough", not needing to get/init anything UnnamedUnion(Path), - /// Vec - List(Box), - /// CapnpStruct(T1, T2, ...) - GenericStruct(Path), + /// () + Void(), } #[derive(Debug)] diff --git a/capnp_conv_macros/src/parsers.rs b/capnp_conv_macros/src/parsers.rs index 29da07d..4b96388 100644 --- a/capnp_conv_macros/src/parsers.rs +++ b/capnp_conv_macros/src/parsers.rs @@ -1,13 +1,13 @@ use std::{ - collections::{hash_map, HashMap}, + collections::{HashMap, hash_map}, mem::discriminant, }; use proc_macro2::{Ident, TokenStream}; use quote::ToTokens; use syn::{ - spanned::Spanned, Attribute, Data, DataEnum, DataStruct, DeriveInput, Field, Fields, - GenericArgument, GenericParam, Generics, LitStr, Path, PathArguments, Result, Type, Variant, + Attribute, Data, DataEnum, DataStruct, DeriveInput, Field, Fields, GenericArgument, + GenericParam, Generics, LitStr, Path, PathArguments, Result, Type, Variant, spanned::Spanned, }; use crate::{ @@ -18,12 +18,12 @@ use crate::{ impl ItemInfo { pub fn parse_input(input: &DeriveInput) -> Result { match &input.data { - Data::Struct(struct_data) => Ok(ItemInfo::Struct(StructInfo::parse_struct( + Data::Struct(struct_data) => Ok(Self::Struct(StructInfo::parse_struct( &input.ident, &input.generics, struct_data, )?)), - Data::Enum(enum_data) => Ok(ItemInfo::Enum(EnumInfo::parse_enum( + Data::Enum(enum_data) => Ok(Self::Enum(EnumInfo::parse_enum( &input.ident, &input.generics, enum_data, @@ -54,7 +54,7 @@ impl StructInfo { }) .collect::>>()?; - Ok(StructInfo { + Ok(Self { ident, fields, generics, @@ -80,7 +80,7 @@ impl EnumInfo { }) .collect::>>()?; - Ok(EnumInfo { + Ok(Self { ident, fields, generics, @@ -93,8 +93,22 @@ impl FieldInfo { let attr_info = FieldAttributesInfo::new(&field.attrs)?; let (field_type, field_wrapper) = FieldType::parse(&field.ty, attr_info.type_specifier)?; - if let FieldType::Phantom = field_type { - if attr_info.skip + // Check for tuple struct field (ident is None) and error out if necessary + let rust_name = match field.ident.as_ref() { + Some(ident) => ident.clone(), + None => { + // This occurs for tuple struct fields like `struct Foo(i32);` + // The current implementation doesn't handle tuple structs correctly. + // Error out until support is added. + return error( + field.span(), // Span of the whole field + "Tuple structs are not currently supported. Use a named struct instead.", + ); + } + }; + + if matches!(field_type, FieldType::Phantom) + && (attr_info.skip || attr_info.skip_read || attr_info.skip_write || attr_info.union_field @@ -103,21 +117,20 @@ impl FieldInfo { || !matches!( attr_info.type_specifier, FieldAttributeTypeSpecifier::Default - ) - { - return error( - field.ty.span(), - "PhantomData fields cannot have field attributes", - ); - } + )) + { + return error( + field.ty.span(), + "PhantomData fields cannot have field attributes", + ); } let (is_union_field, is_optional, is_boxed) = match field_wrapper { FieldWrapper::Box(box_ident) if attr_info.union_field => { - return error(box_ident.span(), "`Box` types cannot be `union_field`s") + return error(box_ident.span(), "`Box` types cannot be `union_field`s"); } FieldWrapper::None if attr_info.union_field => { - return error(field.ty.span(), "`union_field`s must be `Option`") + return error(field.ty.span(), "`union_field`s must be `Option`"); } FieldWrapper::Option(_) if attr_info.union_field => (true, false, false), FieldWrapper::Option(_) => (false, true, false), @@ -133,13 +146,13 @@ impl FieldInfo { match field_type { FieldType::UnnamedUnion(union_path) if is_union_field => { - return error(union_path.span(), "unions cannot contain unnamed unions") + return error(union_path.span(), "unions cannot contain unnamed unions"); } FieldType::GroupOrUnion(path) if is_optional => { - return error(path.span(), "Groups and unions cannot be optional") + return error(path.span(), "Groups and unions cannot be optional"); } FieldType::UnnamedUnion(path) if is_optional => { - return error(path.span(), "Groups and unions cannot be optional") + return error(path.span(), "Groups and unions cannot be optional"); } _ => {} } @@ -148,8 +161,8 @@ impl FieldInfo { todo!("`Box`") } - Ok(FieldInfo { - rust_name: field.ident.as_ref().unwrap().clone(), + Ok(Self { + rust_name, // Use the validated rust_name field_type, capnp_name_override: attr_info.name_override, has_phantom_in_variant: false, @@ -171,20 +184,24 @@ impl FieldInfo { match field_type { FieldType::Phantom => { + // If field_type is Phantom, it means the first type in the variant was PhantomData. + // We use variant.fields.span() as variant_type must have been Some in this branch. return error( - variant_type.unwrap().span(), + variant.fields.span(), "Enums may not have `PhantomData` in the first spot in their variants. \ - Place them in the second slot.", - ) + Place them in the second slot.", + ); } FieldType::UnnamedUnion(_) => { + // If field_type is UnnamedUnion, variant_type must have been Some. + // Use variant.fields.span() for error location. return error( - variant_type.unwrap().span(), + variant.fields.span(), "unions cannot contain unnamed unions.", - ) + ); } _ => {} - }; + } if let FieldWrapper::Option(ident) = field_wrapper { return error(ident.span(), "Enums may not have `Option`"); @@ -219,7 +236,7 @@ impl FieldInfo { todo!("`Box`") } - Ok(FieldInfo { + Ok(Self { rust_name: variant.ident.clone(), field_type, capnp_name_override: attr_info.name_override, @@ -238,96 +255,122 @@ impl FieldType { fn parse(ty: &Type, specifier: FieldAttributeTypeSpecifier) -> Result<(Self, FieldWrapper)> { match try_peel_type(ty) { Some((ident, sub_type)) => match ident.to_string().as_str() { - "PhantomData" => Ok((FieldType::Phantom, FieldWrapper::None)), + "PhantomData" => Ok((Self::Phantom, FieldWrapper::None)), "Option" => Ok(( - FieldType::parse_type(sub_type, specifier)?, + Self::parse_type(sub_type, specifier)?, FieldWrapper::Option(ident.clone()), )), "Box" => Ok(( - FieldType::parse_type(sub_type, specifier)?, + Self::parse_type(sub_type, specifier)?, FieldWrapper::Box(ident.clone()), )), - _ => Ok((FieldType::parse_type(ty, specifier)?, FieldWrapper::None)), + _ => Ok((Self::parse_type(ty, specifier)?, FieldWrapper::None)), }, - None => Ok((FieldType::parse_type(ty, specifier)?, FieldWrapper::None)), + None => Ok((Self::parse_type(ty, specifier)?, FieldWrapper::None)), } } + fn parse_type(ty: &Type, specifier: FieldAttributeTypeSpecifier) -> Result { match ty { - Type::Tuple(tuple) if tuple.elems.is_empty() => Ok(FieldType::Void()), + Type::Tuple(tuple) if tuple.elems.is_empty() => Ok(Self::Void()), Type::Path(path) => { let path = &path.path; - let last_segment = path.segments.last().unwrap(); + // Use ok_or_else to handle potential empty path segments + let last_segment = path.segments.last().ok_or_else::(|| { + syn::Error::new(path.span(), "Type path must have at least one segment") + })?; let ident = &last_segment.ident; if matches!(ident.to_string().as_str(), "Option" | "Box" | "PhantomData") { - // These are taken care of in before this - error(ident.span(), "invalid generic argument type") + // These are taken care of before this function is called + error( + ident.span(), + "invalid nested type (Option/Box/PhantomData should be handled externally)", + ) } else if is_capnp_primative(path) { - Ok(FieldType::Primitive(path.clone())) + Ok(Self::Primitive(path.clone())) } else if *ident == "String" { - Ok(FieldType::Text(path.clone())) + Ok(Self::Text(path.clone())) } else if matches!(specifier, FieldAttributeTypeSpecifier::Data) && is_capnp_data_type(path) { - Ok(FieldType::Data(path.clone())) + // Specific check for `Vec` when `data` specifier is used + Ok(Self::Data(path.clone())) } else { + // Handle different path argument types and specifiers match &last_segment.arguments { PathArguments::None => match specifier { - FieldAttributeTypeSpecifier::Default => { - Ok(FieldType::Struct(path.clone())) - } + // No type arguments, decision based on specifier + FieldAttributeTypeSpecifier::Default => Ok(Self::Struct(path.clone())), FieldAttributeTypeSpecifier::EnumRemote => { - Ok(FieldType::EnumRemote(path.clone())) + Ok(Self::EnumRemote(path.clone())) } - FieldAttributeTypeSpecifier::Enum => Ok(FieldType::Enum(path.clone())), + FieldAttributeTypeSpecifier::Enum => Ok(Self::Enum(path.clone())), FieldAttributeTypeSpecifier::GroupOrUnion => { - Ok(FieldType::GroupOrUnion(path.clone())) + Ok(Self::GroupOrUnion(path.clone())) } FieldAttributeTypeSpecifier::UnnamedUnion => { - Ok(FieldType::UnnamedUnion(path.clone())) + Ok(Self::UnnamedUnion(path.clone())) } FieldAttributeTypeSpecifier::Data => error( - ident.span(), + path.span(), // Use path span for better error location "fields with `data` attribute must be of type `Vec`", ), }, PathArguments::AngleBracketed(args) if ident == "Vec" => { - match args.args.len() { - 1 => { - let arg = args.args.first().unwrap(); - match arg { - GenericArgument::Type(ty) => Ok(FieldType::List(Box::new( - FieldType::parse_type(ty, specifier)?, - ))), - _ => error(arg.span(), "invalid generic argument type"), - } + // Handle `Vec` specifically + // Use pattern matching on slice to safely handle arguments + match args.args.iter().collect::>().as_slice() { + [GenericArgument::Type(inner_ty)] => { + Ok(Self::List(Box::new(Self::parse_type(inner_ty, specifier)?))) } - _ => error(args.span(), "`Vec` fields must have only one argument"), + [arg] => error( + arg.span(), + "invalid generic argument type for Vec: expected a type", + ), + [] => error( + args.span(), + "`Vec` fields must have exactly one type argument, found zero", + ), + _ => error( + args.span(), + "`Vec` fields must have exactly one type argument, found multiple", + ), } } - PathArguments::AngleBracketed(args) => match specifier { - FieldAttributeTypeSpecifier::Default => { - Ok(FieldType::GenericStruct(path.clone())) - } - FieldAttributeTypeSpecifier::GroupOrUnion => { - Ok(FieldType::GroupOrUnion(path.clone())) - } - FieldAttributeTypeSpecifier::UnnamedUnion => { - Ok(FieldType::UnnamedUnion(path.clone())) + PathArguments::AngleBracketed(args) => { + // Handle generic types like `MyStruct` + match specifier { + FieldAttributeTypeSpecifier::Default => { + Ok(Self::GenericStruct(path.clone())) + } + FieldAttributeTypeSpecifier::GroupOrUnion => { + Ok(Self::GroupOrUnion(path.clone())) + } + FieldAttributeTypeSpecifier::UnnamedUnion => { + Ok(Self::UnnamedUnion(path.clone())) + } + // Other specifiers generally don't support generic arguments + _ => error( + args.span(), + "generic arguments are not supported with this type specifier", + ), } - _ => error( - args.span(), - "generic arguments can not be specified in unions", - ), - }, + } PathArguments::Parenthesized(args) => { - error(args.span(), "invalid generic argument types") + // Parenthesized arguments like `Fn(T) -> U` are not supported + error( + args.span(), + "parenthesized generic arguments (Fn traits) are not supported", + ) } } } } - _ => error(ty.span(), "incompatible field type"), + _ => error( + ty.span(), + "incompatible field type (expected path or unit tuple)", + ), } } } @@ -342,6 +385,7 @@ enum FieldAttributeTypeSpecifier { Data, } +#[allow(clippy::struct_excessive_bools)] struct FieldAttributesInfo { pub name_override: Option, pub type_specifier: FieldAttributeTypeSpecifier, @@ -375,7 +419,7 @@ impl FieldAttributesInfo { let attr = if meta.path.is_ident("name") { let name = meta.value()?.parse::()?.parse::()?; - attr_info.name_override = Some(name.clone()); + attr_info.name_override = Some(name); FieldAttribute::Name(meta.path.clone()) } else if meta.path.is_ident("type") { let lit_str = meta.value()?.parse::()?.value(); @@ -389,11 +433,7 @@ impl FieldAttributesInfo { attr_info.type_specifier = FieldAttributeTypeSpecifier::EnumRemote; FieldAttribute::Type(meta.path.clone()) } - "group" => { - attr_info.type_specifier = FieldAttributeTypeSpecifier::GroupOrUnion; - FieldAttribute::Type(meta.path.clone()) - } - "union" => { + "group" | "union" => { attr_info.type_specifier = FieldAttributeTypeSpecifier::GroupOrUnion; FieldAttribute::Type(meta.path.clone()) } @@ -415,7 +455,7 @@ impl FieldAttributesInfo { let path = meta.value()?.parse::()?.parse::()?; if path == as_turbofish(&path) { - attr_info.default = Some(path.clone()); + attr_info.default = Some(path); FieldAttribute::Default(meta.path.clone()) } else { return Err(meta.error("not in turbofish format")); @@ -462,7 +502,7 @@ impl FieldAttributesInfo { return error( ident.span(), "`default` attribute with no `skip` or `skip_read` will never be used", - ) + ); } FieldAttribute::Skip(ident) if processed_attrs.values().any(|a| { @@ -475,7 +515,7 @@ impl FieldAttributesInfo { return error( ident.span(), "`skip` specified in additon to `skip_read` and/or `skip_write`", - ) + ); } _ => {} } @@ -499,31 +539,52 @@ enum FieldAttribute { impl ToTokens for FieldAttribute { fn to_tokens(&self, tokens: &mut TokenStream) { match self { - FieldAttribute::Name(a) => tokens.extend(a.into_token_stream()), - FieldAttribute::Type(a) => tokens.extend(a.into_token_stream()), - FieldAttribute::Default(a) => tokens.extend(a.into_token_stream()), - FieldAttribute::Skip(a) => tokens.extend(a.into_token_stream()), - FieldAttribute::SkipRead(a) => tokens.extend(a.into_token_stream()), - FieldAttribute::SkipWrite(a) => tokens.extend(a.into_token_stream()), - FieldAttribute::UnionField(a) => tokens.extend(a.into_token_stream()), + Self::Type(a) + | Self::Default(a) + | Self::Skip(a) + | Self::SkipRead(a) + | Self::SkipWrite(a) + | Self::UnionField(a) + | Self::Name(a) => tokens.extend(a.into_token_stream()), } } } fn is_capnp_primative(path: &Path) -> bool { - matches!( - path.segments.last().unwrap().ident.to_string().as_str(), - "bool" | "i8" | "i16" | "i32" | "i64" | "u8" | "u16" | "u32" | "u64" | "f32" | "f64" + path.segments.last().map_or_else( + || false, + |last_segment| { + matches!( + last_segment.ident.to_string().as_str(), + "bool" + | "i8" + | "i16" + | "i32" + | "i64" + | "u8" + | "u16" + | "u32" + | "u64" + | "f32" + | "f64" + ) + }, ) } /// Returns if the type signature is `Vec`, which corresponds to capnp's `Data` fn is_capnp_data_type(path: &Path) -> bool { - if path.segments.last().unwrap().ident == "Vec" { - if let PathArguments::AngleBracketed(args) = &path.segments.last().unwrap().arguments { - if args.args.len() == 1 { - if let GenericArgument::Type(Type::Path(path)) = args.args.first().unwrap() { - return path.path.segments.last().unwrap().ident == "u8"; + if let Some(last_segment) = path.segments.last() { + if last_segment.ident == "Vec" { + if let PathArguments::AngleBracketed(args) = &last_segment.arguments { + if args.args.len() == 1 { + if let Some(GenericArgument::Type(Type::Path(inner_type_path))) = + args.args.first() + { + if let Some(inner_last_segment) = inner_type_path.path.segments.last() { + return inner_last_segment.ident == "u8"; + } + } } } } @@ -535,35 +596,42 @@ fn is_capnp_data_type(path: &Path) -> bool { fn get_variant_type(fields: &Fields) -> Result<(Option<&Type>, bool)> { match fields { Fields::Unit => Ok((None, false)), - Fields::Unnamed(fields) => match fields.unnamed.len() { - 1 => Ok((Some(&fields.unnamed.first().unwrap().ty), false)), - 2 => { - let second_field_type = &fields.unnamed[1].ty; - match second_field_type { - Type::Path(path) - if path - .path - .segments - .last() - .unwrap() - .ident - .to_string() - .as_str() - == "PhantomData" => {} - _ => { - return error( + Fields::Unnamed(fields) => { + match fields.unnamed.iter().collect::>().as_slice() { + // Enum variant with one field: E::Variant(T) + [field] => Ok((Some(&field.ty), false)), + // Enum variant with two fields: E::Variant(T, PhantomData<...>) + [first_field, second_field] => { + let second_field_type = &second_field.ty; + match second_field_type { + Type::Path(path) => { + path.path.segments.last().map_or_else(|| error(path.span(), "internal error: Type::Path has no segments"), |last_segment| if last_segment.ident == "PhantomData" { + Ok((Some(&first_field.ty), true)) + } else { + error( + second_field_type.span(), + "second type of an enum variant tuple can only be `PhantomData`", + ) + }) + } + _ => error( second_field_type.span(), - "second type of an enum can only be `PhantomData`", - ) + "second type of an enum variant tuple can only be `PhantomData`", + ), } - }; - Ok((Some(&fields.unnamed.first().unwrap().ty), true)) + } + // Enum variant with zero fields (e.g. `E::Variant()`) - treat like Unit + [] => Ok((None, false)), + // Enum variant with more than two fields + _ => error( + fields.span(), + "enum variants may only contain 1 field (plus an optional `PhantomData`)", + ), } - _ => error( - fields.span(), - "enum variants may only contain 1 field (plus an optional `PhantomData`", - ), - }, - Fields::Named(_) => unimplemented!(), + } + Fields::Named(_) => error( + fields.span(), + "named fields in enum variants are not supported yet. Use tuple variants like `Variant(T)` instead.", + ), // Or `unimplemented!()` if preferred } } diff --git a/capnp_conv_macros/src/utils.rs b/capnp_conv_macros/src/utils.rs index e368c3b..7b4e0d2 100644 --- a/capnp_conv_macros/src/utils.rs +++ b/capnp_conv_macros/src/utils.rs @@ -1,9 +1,9 @@ use std::fmt::Display; use proc_macro2::{Ident, Span}; -use quote::{format_ident, IdentFragment}; +use quote::{IdentFragment, format_ident}; use syn::{ - token::PathSep, AttrStyle, Attribute, Error, GenericArgument, Path, PathArguments, Result, Type, + AttrStyle, Attribute, Error, GenericArgument, Path, PathArguments, Result, Type, token::PathSep, }; use crate::models::FieldType; @@ -14,7 +14,7 @@ pub fn error(span: Span, message: impl Display) -> Result { pub fn is_capnp_attr(attribute: &Attribute) -> bool { attribute.style == AttrStyle::Outer - && attribute.path().segments.last().unwrap().ident == "capnp_conv" + && matches!(attribute.path().segments.last(), Some(segment) if segment.ident == "capnp_conv") } pub fn to_ident(fragment: impl IdentFragment) -> Ident { @@ -28,11 +28,12 @@ pub fn to_capnp_generic(generic: &Ident) -> Ident { /// for a type of `Option::`, will return `"Option"`, the `bool` subtype pub fn try_peel_type(ty: &Type) -> Option<(&Ident, &Type)> { if let Type::Path(type_path) = ty { - let last_segment = type_path.path.segments.last().unwrap(); - if let PathArguments::AngleBracketed(arguments) = &last_segment.arguments { - if arguments.args.len() == 1 { - if let GenericArgument::Type(sub_type) = arguments.args.first().unwrap() { - return Some((&last_segment.ident, sub_type)); + if let Some(last_segment) = type_path.path.segments.last() { + if let PathArguments::AngleBracketed(arguments) = &last_segment.arguments { + if arguments.args.len() == 1 { + if let Some(GenericArgument::Type(sub_type)) = arguments.args.first() { + return Some((&last_segment.ident, sub_type)); + } } } } @@ -51,7 +52,7 @@ pub fn as_turbofish(path: &Path) -> Path { path } -pub fn is_ptr_type(field_type: &FieldType) -> bool { +pub const fn is_ptr_type(field_type: &FieldType) -> bool { matches!( field_type, FieldType::Data(_) diff --git a/example/src/main.rs b/example/src/main.rs index 1d12d2c..9c4d318 100644 --- a/example/src/main.rs +++ b/example/src/main.rs @@ -1,3 +1,13 @@ +#![deny( + clippy::nursery, + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::string_slice, + clippy::pedantic +)] +#![forbid(unsafe_code)] + mod rust_types; pub mod example_capnp { include!(concat!(env!("OUT_DIR"), "/", "example_capnp.rs")); @@ -9,7 +19,7 @@ use capnp::message::TypedBuilder; use capnp_conv::{Readable, Writable}; use example_capnp as capnp_types; -#[allow(clippy::print_stdout)] +#[allow(clippy::print_stdout, clippy::print_stderr)] fn main() { let basic_struct = rust_types::BasicStruct { val: 10 }; let generic_struct = rust_types::GenericStruct:: { @@ -27,14 +37,14 @@ fn main() { generic_generic_struct: generic_struct.clone(), list_val: vec![ vec![generic_struct.clone()], - vec![generic_struct.clone(), generic_struct.clone()], + vec![generic_struct.clone(), generic_struct], ], group_val: rust_types::ExampleGroup { val1: basic_struct.clone(), val2: basic_struct.clone(), }, union_val: rust_types::ExampleUnion::Val2(basic_struct.clone()), - unnamed_union: rust_types::ExampleUnnamedUnion::Val2(basic_struct.clone()), + unnamed_union: rust_types::ExampleUnnamedUnion::Val2(basic_struct), }; let mut builder = TypedBuilder::< @@ -43,9 +53,21 @@ fn main() { input.write(builder.init_root()); - let reader = builder.get_root_as_reader().unwrap(); + let reader = match builder.get_root_as_reader() { + Ok(reader) => reader, + Err(e) => { + eprintln!("Error getting reader: {e}"); + return; + } + }; - let output = rust_types::ExampleStruct::::read(reader).unwrap(); + let output = match rust_types::ExampleStruct::::read(reader) { + Ok(output) => output, + Err(e) => { + eprintln!("Error reading: {e}"); + return; + } + }; println!("Input == Output: {}", input == output); } From 5518981ca7f0daf83d846a298a21ce44a0b8c8e8 Mon Sep 17 00:00:00 2001 From: Debanjan Basu Date: Tue, 8 Apr 2025 09:20:27 +1000 Subject: [PATCH 2/4] migrated properly to edition 2024 --- Cargo.toml | 5 ++++- capnp_conv_macros/src/lib.rs | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 985deb4..19d743b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,10 @@ resolver = "2" members = ["capnp_conv", "capnp_conv_macros", "capnp_conv_tests", "example"] [workspace.package] -authors = ["Aik Kalantarian ", "Debanjan Basu "] +authors = [ + "Aik Kalantarian ", + "Debanjan Basu ", +] version = "0.3.2" edition = "2024" license = "MIT" diff --git a/capnp_conv_macros/src/lib.rs b/capnp_conv_macros/src/lib.rs index eeabfcc..248ae74 100644 --- a/capnp_conv_macros/src/lib.rs +++ b/capnp_conv_macros/src/lib.rs @@ -24,7 +24,7 @@ pub fn capnp_conv(attr_stream: TokenStream, input_stream: TokenStream) -> TokenS let capnp_struct = parse_macro_input!(attr_stream as Path); let mut input = parse_macro_input!(input_stream as DeriveInput); - match ItemInfo::parse_input(&input) { + let result = match ItemInfo::parse_input(&input) { Ok(item_info) => { let output = item_info.generate_impls(&capnp_struct); remove_capnp_field_attrs(&mut input); @@ -34,8 +34,8 @@ pub fn capnp_conv(attr_stream: TokenStream, input_stream: TokenStream) -> TokenS } } Err(error) => error.to_compile_error(), - } - .into() + }; + result.into() } fn remove_capnp_field_attrs(input: &mut DeriveInput) { From 6517b8af8a1552e59ecf3d27d2d38bf323c63c5a Mon Sep 17 00:00:00 2001 From: Debanjan Basu Date: Tue, 8 Apr 2025 09:48:49 +1000 Subject: [PATCH 3/4] minor optimizations --- capnp_conv_macros/src/lib.rs | 27 +++++--------- capnp_conv_macros/src/utils.rs | 65 ++++++++++++++++++++++------------ rustfmt.toml | 2 +- 3 files changed, 52 insertions(+), 42 deletions(-) diff --git a/capnp_conv_macros/src/lib.rs b/capnp_conv_macros/src/lib.rs index 248ae74..7afd083 100644 --- a/capnp_conv_macros/src/lib.rs +++ b/capnp_conv_macros/src/lib.rs @@ -24,44 +24,33 @@ pub fn capnp_conv(attr_stream: TokenStream, input_stream: TokenStream) -> TokenS let capnp_struct = parse_macro_input!(attr_stream as Path); let mut input = parse_macro_input!(input_stream as DeriveInput); - let result = match ItemInfo::parse_input(&input) { + let output = match ItemInfo::parse_input(&input) { Ok(item_info) => { - let output = item_info.generate_impls(&capnp_struct); + let impls = item_info.generate_impls(&capnp_struct); remove_capnp_field_attrs(&mut input); quote! { - #input - #output + #input + #impls } } Err(error) => error.to_compile_error(), }; - result.into() + + output.into() } fn remove_capnp_field_attrs(input: &mut DeriveInput) { match &mut input.data { syn::Data::Struct(data) => { for field in &mut data.fields { - drain_filter(&mut field.attrs, is_capnp_attr); + field.attrs.retain(|attr| !is_capnp_attr(attr)); } } syn::Data::Enum(data) => { for variant in &mut data.variants { - drain_filter(&mut variant.attrs, is_capnp_attr); + variant.attrs.retain(|attr| !is_capnp_attr(attr)); } } syn::Data::Union(_) => unimplemented!(), } } - -//not using nightly so we need to do this manually -fn drain_filter(vec: &mut Vec, predicate: fn(&T) -> bool) { - let mut i = 0; - while i != vec.len() { - if predicate(&vec[i]) { - vec.remove(i); - } else { - i += 1; - } - } -} diff --git a/capnp_conv_macros/src/utils.rs b/capnp_conv_macros/src/utils.rs index 7b4e0d2..34ec723 100644 --- a/capnp_conv_macros/src/utils.rs +++ b/capnp_conv_macros/src/utils.rs @@ -1,9 +1,10 @@ use std::fmt::Display; use proc_macro2::{Ident, Span}; -use quote::{IdentFragment, format_ident}; +use quote::{format_ident, IdentFragment}; use syn::{ - AttrStyle, Attribute, Error, GenericArgument, Path, PathArguments, Result, Type, token::PathSep, + AttrStyle, Attribute, Error, GenericArgument, Path, PathArguments, Result, Type, + token::PathSep, }; use crate::models::FieldType; @@ -14,7 +15,7 @@ pub fn error(span: Span, message: impl Display) -> Result { pub fn is_capnp_attr(attribute: &Attribute) -> bool { attribute.style == AttrStyle::Outer - && matches!(attribute.path().segments.last(), Some(segment) if segment.ident == "capnp_conv") + && attribute.path().is_ident("capnp_conv") } pub fn to_ident(fragment: impl IdentFragment) -> Ident { @@ -25,23 +26,36 @@ pub fn to_capnp_generic(generic: &Ident) -> Ident { format_ident!("__CaPnP__{}", generic) } -/// for a type of `Option::`, will return `"Option"`, the `bool` subtype +/// For a type like `Option`, returns `Some(("Option", &bool))`. +/// Returns `None` if the type is not a path, has no segments, +/// the last segment has no angle-bracketed arguments, or has zero or more than one generic type argument. pub fn try_peel_type(ty: &Type) -> Option<(&Ident, &Type)> { - if let Type::Path(type_path) = ty { - if let Some(last_segment) = type_path.path.segments.last() { - if let PathArguments::AngleBracketed(arguments) = &last_segment.arguments { - if arguments.args.len() == 1 { - if let Some(GenericArgument::Type(sub_type)) = arguments.args.first() { - return Some((&last_segment.ident, sub_type)); - } - } - } - } + let type_path = if let Type::Path(type_path) = ty { + type_path + } else { + return None; + }; + + let last_segment = type_path.path.segments.last()?; + + let arguments = if let PathArguments::AngleBracketed(arguments) = &last_segment.arguments { + arguments + } else { + return None; + }; + + // Ensure exactly one generic argument which is a type + if arguments.args.len() == 1 { + if let Some(GenericArgument::Type(sub_type)) = arguments.args.first() { + return Some((&last_segment.ident, sub_type)); + } } + None } -/// Turns `Foo>` into `Foo::>` + +/// Turns `Foo>` into `Foo::>` by adding `::` before generic arguments. pub fn as_turbofish(path: &Path) -> Path { let mut path = path.clone(); for segment in &mut path.segments { @@ -52,6 +66,7 @@ pub fn as_turbofish(path: &Path) -> Path { path } +/// Checks if the field type corresponds to a Cap'n Proto pointer type. pub const fn is_ptr_type(field_type: &FieldType) -> bool { matches!( field_type, @@ -63,13 +78,19 @@ pub const fn is_ptr_type(field_type: &FieldType) -> bool { ) } -// copied from how https://github.com/capnproto/capnproto-rust/blob/master/capnpc -// generates enum names +/// Capitalizes the first ASCII character of a string. +/// Returns an empty string if the input string is empty. pub fn capitalize_first_letter(s: &str) -> String { - let mut result_chars: Vec = Vec::new(); - for c in s.chars() { - result_chars.push(c); + let mut chars = s.chars(); + match chars.next() { + None => String::new(), + Some(first) => { + // Pre-allocate string capacity for efficiency. + let mut result = String::with_capacity(s.len()); + result.push(first.to_ascii_uppercase()); + // Append the rest of the string slice efficiently. + result.push_str(chars.as_str()); + result + } } - result_chars[0] = result_chars[0].to_ascii_uppercase(); - result_chars.into_iter().collect() } diff --git a/rustfmt.toml b/rustfmt.toml index 082d6ef..0a13382 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -1,7 +1,7 @@ imports_granularity = "Crate" group_imports = "StdExternalCrate" newline_style = "Unix" -edition = "2021" +edition = "2024" tab_spaces = 4 use_try_shorthand = true use_field_init_shorthand = true From 3dfc141da61f1b5999b55d95d933e889b64819da Mon Sep 17 00:00:00 2001 From: Debanjan Basu Date: Tue, 8 Apr 2025 10:57:24 +1000 Subject: [PATCH 4/4] updated the capnp version to match the latest --- Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 19d743b..97c2dad 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,8 +17,8 @@ readme = "README.md" capnp_conv = { path = "capnp_conv", version = "0.3.1" } capnp_conv_macros = { path = "capnp_conv_macros", version = "0.3.1" } -capnp = "0.20" -capnpc = "0.20" +capnp = "0.21" +capnpc = "0.21" heck = "0.5" proc-macro2 = "1.0" quote = "1.0"