Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

derive: properly handle generic structs and enums #125

Merged
merged 5 commits into from
Aug 18, 2020
Merged

derive: properly handle generic structs and enums #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
85dc9ef
derive: properly handle generic structs and enums
japaric Aug 18, 2020
b069413
add more test cases
japaric Aug 18, 2020
6686b1f
document diff between derive(Format) and derive(Debug)
japaric Aug 18, 2020
317e766
fix book tests
japaric Aug 18, 2020
a52c5e7
Fix formatting
Aug 18, 2020
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
53 changes: 53 additions & 0 deletions book/src/format.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,56 @@ enum Request {
SetAddress { address: u8 },
}
```

NOTE: for generic structs and enums the `derive` macro adds `Format` bounds to the *types of the generic fields* rather than to all the generic (input) parameters of the struct / enum.
Built-in `derive` attributes like `#[derive(Debug)]` use the latter approach.
To our knowledge `derive(Format)` approach is more accurate in that it doesn't over-constrain the generic type parameters.
The different between the two approaches is depicted below:

``` rust
# extern crate defmt;
# use defmt::Format;

#[derive(Format)]
struct S<'a, T> {
x: Option<&'a T>,
y: u8,
}
```

``` rust
# extern crate defmt;
# use defmt::Format;

// `Format` produces this implementation
impl<'a, T> Format for S<'a, T>
where
Option<&'a T>: Format // <- main difference
{
// ..
# fn format(&self, f: &mut defmt::Formatter) {}
}

#[derive(Debug)]
struct S<'a, T> {
x: Option<&'a T>,
y: u8,
}
```

``` rust
# use std::fmt::Debug;
# struct S<'a, T> {
# x: Option<&'a T>,
# y: u8,
# }

// `Debug` produces this implementation
impl<'a, T> Debug for S<'a, T>
where
T: Debug // <- main difference
{
// ..
# fn fmt(&self, f: &mut core::fmt::Formatter) -> std::fmt::Result { Ok(()) }
}
```
123 changes: 123 additions & 0 deletions firmware/qemu/src/bin/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,129 @@ fn main() -> ! {
// issue #111
defmt::info!("{:[?]}", [true, true, false]);

/* issue #124 (start) */
// plain generic struct
{
#[derive(Format)]
struct S<T> {
x: u8,
y: T,
}

defmt::info!("{:?}", S { x: 42, y: 43u8 });
}

// generic struct with bounds
{
#[derive(Format)]
struct S<T>
where
T: Copy,
{
x: u8,
y: T,
}

defmt::info!("{:?}", S { x: 44, y: 45u8 });
}

// generic struct with `Option` field
{
#[derive(Format)]
struct S<T>
where
T: Copy,
{
x: u8,
y: Option<T>,
}

defmt::info!(
"{:?}",
S {
x: 46,
y: Some(47u8)
}
);
}

// generic struct with lifetimes and lifetime bounds
{
#[derive(Format)]
struct S<'a, T>
where
T: 'a,
{
x: Option<&'a u8>,
y: T,
}

defmt::info!("{:?}", S { x: Some(&48), y: 49u8 });
}

// plain generic enum
{
#[derive(Format)]
enum E<X, Y> {
A,
B(X),
C { y: Y },
}

defmt::info!("{:?}", E::<u8, u8>::A);
defmt::info!("{:?}", E::<u8, u8>::B(42));
defmt::info!("{:?}", E::<u8, u8>::C { y: 43 });
}

// generic enum with bounds
{
#[derive(Format)]
enum E<X, Y>
where
X: Copy,
{
A,
B(X),
C { y: Y },
}

defmt::info!("{:?}", E::<u8, u8>::A);
defmt::info!("{:?}", E::<u8, u8>::B(44));
defmt::info!("{:?}", E::<u8, u8>::C { y: 45 });
}

/* issue #124 (end) */
// generic enum with `Option`/`Result` fields
{
#[derive(Format)]
enum E<X, Y> {
A,
B(Option<X>),
C { y: Result<Y, u8> },
}

defmt::info!("{:?}", E::<u8, u8>::A);
defmt::info!("{:?}", E::<u8, u8>::B(Some(46)));
defmt::info!("{:?}", E::<u8, u8>::C { y: Ok(47) });
}

// generic enum with lifetimes and lifetime bounds
{
#[derive(Format)]
enum E<'a, T>
where
T: 'a,
{
A,
B(Option<&'a u8>),
C { y: T },
}

defmt::info!("{:?}", E::<u8>::A);
defmt::info!("{:?}", E::<u8>::B(Some(&48)));
defmt::info!("{:?}", E::C { y: 49u8 });
}

loop {
debug::exit(debug::EXIT_SUCCESS)
}
Expand Down
50 changes: 39 additions & 11 deletions macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,7 @@ impl MLevel {
MLevel::Info => {
// defmt-default is enabled for dev & release profile so debug_assertions
// does not matter
&[
"defmt-info",
"defmt-debug",
"defmt-trace",
"defmt-default",
]
&["defmt-info", "defmt-debug", "defmt-trace", "defmt-default"]
}
MLevel::Warn => {
// defmt-default is enabled for dev & release profile so debug_assertions
Expand Down Expand Up @@ -185,6 +180,7 @@ pub fn format(ts: TokenStream) -> TokenStream {

let ident = input.ident;
let mut fs = String::new();
let mut field_types = vec![];
let mut exprs = vec![];
match input.data {
Data::Enum(de) => {
Expand Down Expand Up @@ -216,6 +212,7 @@ pub fn format(ts: TokenStream) -> TokenStream {
let exprs = fields(
&var.fields,
&mut fs,
&mut field_types,
Kind::Enum {
patterns: &mut pats,
},
Expand Down Expand Up @@ -243,7 +240,7 @@ pub fn format(ts: TokenStream) -> TokenStream {

Data::Struct(ds) => {
fs = ident.to_string();
let args = fields(&ds.fields, &mut fs, Kind::Struct);
let args = fields(&ds.fields, &mut fs, &mut field_types, Kind::Struct);
// FIXME expand this `write!` and conditionally omit the tag (string index)
exprs.push(quote!(defmt::export::write!(f, #fs #(,#args)*);))
}
Expand All @@ -255,8 +252,30 @@ pub fn format(ts: TokenStream) -> TokenStream {
}
}

let params = input.generics.params;
let predicates = if params.is_empty() {
vec![]
} else {
// `Format` bounds for non-native field types
let mut preds = field_types
.into_iter()
.map(|ty| quote!(#ty: defmt::Format))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so this creates bounds for each field's type, which is different from what the built-in custom derives are doing (which is adding bounds to each type parameter rust-lang/rust#26925). This is more correct, but I wonder if we should document it?

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't hurt to document it!

.collect::<Vec<_>>();
// extend with the where clause from the struct/enum declaration
if let Some(where_clause) = input.generics.where_clause {
preds.extend(
where_clause
.predicates
.into_iter()
.map(|pred| quote!(#pred)),
)
}
preds
};
quote!(
impl defmt::Format for #ident {
impl<#params> defmt::Format for #ident<#params>
where #(#predicates),*
{
fn format(&self, f: &mut defmt::Formatter) {
#(#exprs)*
}
Expand All @@ -270,7 +289,13 @@ enum Kind<'p> {
Enum { patterns: &'p mut TokenStream2 },
}

fn fields(fields: &Fields, format: &mut String, mut kind: Kind) -> Vec<TokenStream2> {
fn fields(
fields: &Fields,
format: &mut String,
// collect all *non-native* types that appear as fields
field_types: &mut Vec<Type>,
mut kind: Kind,
) -> Vec<TokenStream2> {
let mut list = vec![];
match fields {
Fields::Named(FieldsNamed { named: fs, .. })
Expand All @@ -295,7 +320,10 @@ fn fields(fields: &Fields, format: &mut String, mut kind: Kind) -> Vec<TokenStre
} else {
format.push_str(", ");
}
let ty = as_native_type(&f.ty).unwrap_or_else(|| "?".to_string());
let ty = as_native_type(&f.ty).unwrap_or_else(|| {
field_types.push(f.ty.clone());
"?".to_string()
});
if let Some(ident) = f.ident.as_ref() {
core::write!(format, "{}: {{:{}}}", ident, ty).ok();

Expand Down Expand Up @@ -797,4 +825,4 @@ impl Codegen {

Ok(Codegen { pats, exprs })
}
}
}