Implement precedence of inner display hint #359

justahero · 2021-01-27T15:26:25Z

This PR implements precedence of display hints inner types, e.g. u8, u16 etc. It implements the display hint precedence rules given in #319.

When formating variables using the defmt::Format trait, either by implementing it or using the derive macro, the outer display hint is ignored depending on the inner type & its display hint.

For example, a struct that uses the derive marco

#[derive(Format)]
struct S { x: u8 }

// derive would expand to
impl defmt::Format for S {
    fn format(&self, f: defmt::Formatter) {
        write!(f, "S {{ x: {=u8:?} }}", self.x);
    }
}

This uses the Debug display hint (:?) for the inner type x of struct S. When displaying the above struct S with

info!("{:x}", S { x: 42 });
// -> INFO S { x: 0x2a }

the outer display hint :x is applied to the inner type x. In this case the outer display hint hexadecimal is used.

On the other hand if the inner type is one of :x (hex), :b (binary), the outer display hint will be ignored and the inner hint has precedence.

This time the implementation uses the :b (binary) display hint to format inner type x.

impl defmt::Format for S {
    fn format(&self, f: defmt::Formatter) {
        write!(f, "S {{ x: {=u8:b} }}", self.x);
    }
}

When logging the struct this time

info!("{:x}", S { x: 42 });
// -> INFO S { x: 0b101010 }

the outer display hint :x is ignored, the inner display hint :b is used, therefore the value 42 is formatted as a binary string 0b101010.

Another precedence rule is related to str fields of structs. As given in the RFC #319, the following struct with an inner str field is deriving the defmt::Format trait as follows.

#[derive(Format)]
struct S { x: &'static str }

// derive now expands to
impl defmt::Format for S {
    fn format(&self, f: defmt::Formatter) {
        write!(f, "S {{ x: {=str:?} }}", self.x);
    }
}

The difference is x: &'static str is expanded to {=str:?} instead of a plain {=str}. This way string in x is enclosed by double quotes.

defmt::info!("{} world", S { x: "hello" });
// -> "hello" world
defmt::info("hello {:?}", S { x: "world" });
// -> hello "world"

Both display hints {} and {:?} now produce the same desired output.

japaric · 2021-02-02T13:13:29Z

macros/src/lib.rs

@@ -429,7 +429,11 @@ fn fields(
                        "?".to_string()
                    });
                    if let Some(ident) = f.ident.as_ref() {
-                        core::write!(format, "{}: {{={}}}", ident, ty).ok();
+                        if ty == "str" {


the problem with this approach (attaching :? only to the =str type) is that it won't work with generic structures. For example:

#[derive(Format)] struct S1<T> { x: T, y: u8, } // outputs: "S { x: hi, y: 42 }" defmt::info!("{}", S1 { x: "hi", y: 42 }); // outputs: "S { x: hi, y: 0x2a }" defmt::info!("{:x}", S1 { x: "hi", y: 42 });

This is because the expansion of the macro will use the format string "S1 {{ x: {=?}, y: {=u8} }}". The proc-macro cannot know whether the generic type T will be instantiated to str or not. Also, you don't the :? format specifier if T is instantiated to a type that's not a string.

Thanks for the feedback. The code is changed now to apply the Debug hint :? to all inner types. As a side effect this changes the output in the log test program as well.

#[derive(Format)] struct Data<'a> { name: &'a [u8], value: bool, } let data = &[Data { name: b"Hi", value: true }]; defmt::info!("{=[?]:a}", *data); // before the change // -> INFO [Data { name: b"Hi", value: true }] // after the change // -> INFO [Data { name: [72, 105], value: true }]

jonas-schievink · 2021-02-12T13:12:46Z

firmware/qemu/src/bin/log.out

@@ -111,6 +111,6 @@
 0.000110 INFO b"Hi"
 0.000111 INFO b"Hi"
 0.000112 INFO [45054, 49406]
-0.000113 INFO [Data { name: b"Hi", value: true }]
+0.000113 INFO [Data { name: [72, 105], value: true }]


This slice is formatted with {=[?]:a}, why is the :a hint ignored?

jonas-schievink · 2021-02-12T13:20:48Z

firmware/qemu/src/bin/hints_inner.rs

+            y: u8,
+        }
+
+        // outputs: "S { x: hi, y: 42 }"


Suggested change

// outputs: "S { x: hi, y: 42 }"

// outputs: "S { x: "hi", y: 42 }"

jonas-schievink · 2021-02-12T13:20:55Z

firmware/qemu/src/bin/hints_inner.rs

+
+        // outputs: "S { x: hi, y: 42 }"
+        defmt::info!("{}", S1 { x: "hi", y: 42 });
+        // outputs: "S { x: hi, y: 0x2a }"


Suggested change

// outputs: "S { x: hi, y: 0x2a }"

// outputs: "S { x: "hi", y: 0x2a }"

Urhengulas

Looks generally good to me. I have a couple of questions (not all particular about this PR) and style suggestions:

Could and should we drop :? from our grammar, since "Both display hints {} and {:?} now produce the same desired output."?
Does this PR respect all of x, X, #x, b?

Urhengulas · 2021-06-29T14:18:19Z

decoder/src/lib.rs

+        let mut entries = BTreeMap::new();
+
+        entries.insert(
+            0,
+            TableEntry::new_without_symbol(Tag::Info, "x={:b}".to_owned()),
+        );
+        entries.insert(
+            1,
+            TableEntry::new_without_symbol(Tag::Derived, "S {{ x: {=u8:?} }}".to_owned()),
+        );
+
+        let table = Table {
+            entries,


What do you think about this pattern? It would also be applicable for some other tests.

Suggested change

let mut entries = BTreeMap::new();

entries.insert(

0,

TableEntry::new_without_symbol(Tag::Info, "x={:b}".to_owned()),

);

entries.insert(

1,

TableEntry::new_without_symbol(Tag::Derived, "S {{ x: {=u8:?} }}".to_owned()),

);

let table = Table {

entries,

let entries = vec![

TableEntry::new_without_symbol(Tag::Info, "x={:b}".to_owned()),

TableEntry::new_without_symbol(Tag::Derived, "S {{ x: {=u8:x} }}".to_owned()),

];

let table = Table {

entries: entries.into_iter().enumerate().collect::<BTreeMap<_, _>>(),

Urhengulas · 2021-06-29T14:25:27Z

decoder/src/frame.rs

+                            _ => {
+                                if let Some(DisplayHint::Debug) = hint {
+                                    format_u128(*x as u128, parent_hint, &mut buf)?;
+                                } else {
+                                    format_u128(*x as u128, hint, &mut buf)?;
+                                }
+                            }


Suggested change

_ => {

if let Some(DisplayHint::Debug) = hint {

format_u128(*x as u128, parent_hint, &mut buf)?;

} else {

format_u128(*x as u128, hint, &mut buf)?;

}

}

_ => match hint {

Some(DisplayHint::Debug) => {

format_u128(*x as u128, parent_hint, &mut buf)?

}

_ => format_u128(*x as u128, hint, &mut buf)?,

},

Urhengulas · 2021-06-29T14:26:50Z

macros/src/lib.rs

@@ -410,7 +410,7 @@ fn fields(
                        "?".to_string()
                    });
                    if let Some(ident) = f.ident.as_ref() {
-                        core::write!(format, "{}: {{={}}}", ident, ty).ok();
+                        core::write!(format, "{}: {{={}:?}}", ident, ty).ok();


Why is the :? needed here?

japaric

this looks good to me. let's see if bors accepts it

japaric · 2021-06-29T15:01:55Z

decoder/src/frame.rs

                        }
                    }
                    Arg::Ixx(x) => format_i128(*x as i128, hint, &mut buf)?,
                    Arg::Str(x) | Arg::Preformatted(x) => format_str(x, hint, &mut buf)?,
                    Arg::IStr(x) => format_str(x, hint, &mut buf)?,
-                    Arg::Format { format, args } => buf.push_str(&format_args(format, args, hint)),
+                    Arg::Format { format, args } => match parent_hint {
+                        Some(DisplayHint::Ascii) => {


I don't quite follow why :a is a special case here. (there some bugs / limitations (?) around :a and generics but this doesn't seem like it fixes them or creates more)

japaric · 2021-06-29T15:02:30Z

decoder/src/lib.rs

+            entries,
+            timestamp: Some(TableEntry::new_without_symbol(
+                Tag::Timestamp,
+                "{=u8:µs}".to_owned(),


(this may need to be changed to :us after a recent PR but bors will point it out I think)

japaric · 2021-06-29T15:06:28Z

bors r+

359: Implement precedence of inner display hint r=japaric a=justahero This PR implements precedence of display hints inner types, e.g. `u8`, `u16` etc. It implements the display hint precedence rules given in #319. When formating variables using the `defmt::Format` trait, either by implementing it or using the `derive` macro, the outer display hint is ignored depending on the inner type & its display hint. For example, a struct that uses the `derive` marco ```rust #[derive(Format)] struct S { x: u8 } // derive would expand to impl defmt::Format for S { fn format(&self, f: defmt::Formatter) { write!(f, "S {{ x: {=u8:?} }}", self.x); } } ``` This uses the `Debug` display hint (`:?`) for the inner type `x` of struct `S`. When displaying the above struct `S` with ```rust info!("{:x}", S { x: 42 }); // -> INFO S { x: 0x2a } ``` the outer display hint `:x` is applied to the inner type `x`. In this case the outer display hint hexadecimal is used. On the other hand if the inner type is one of `:x` (hex), `:b` (binary), the outer display hint will be ignored and the inner hint has precedence. This time the implementation uses the `:b` (binary) display hint to format inner type `x`. ```rust impl defmt::Format for S { fn format(&self, f: defmt::Formatter) { write!(f, "S {{ x: {=u8:b} }}", self.x); } } ``` When logging the struct this time ```rust info!("{:x}", S { x: 42 }); // -> INFO S { x: 0b101010 } ``` the outer display hint `:x` is ignored, the inner display hint `:b` is used, therefore the value `42` is formatted as a binary string `0b101010`. Another precedence rule is related to `str` fields of structs. As given in the RFC #319, the following struct with an inner `str` field is deriving the `defmt::Format` trait as follows. ```rust #[derive(Format)] struct S { x: &'static str } // derive now expands to impl defmt::Format for S { fn format(&self, f: defmt::Formatter) { write!(f, "S {{ x: {=str:?} }}", self.x); } } ``` The difference is `x: &'static str` is expanded to `{=str:?}` instead of a plain `{=str}`. This way string in `x` is enclosed by double quotes. ```rust defmt::info!("{} world", S { x: "hello" }); // -> "hello" world defmt::info("hello {:?}", S { x: "world" }); // -> hello "world" ``` Both display hints `{}` and `{:?}` now produce the same desired output. Co-authored-by: Sebastian Ziebell <sebastian.ziebell@asquera.de>

bors · 2021-06-29T15:10:07Z

Build failed:

ci

japaric · 2021-06-29T15:12:41Z

@Urhengulas

Could and should we drop :? from our grammar, since "Both display hints {} and {:?} now produce the same desired output."?

{} is Display-like format. {:?} is Debug-like format. This PR doesn't change that distinction. The following code prints the logs as desired (behavior matches std) even with this PR

defmt::info!("the answer is {}", "42"); // INFO the answer is 42
defmt::info!("the answer is {:?}", "42"); // INFO the answer is "42"

japaric · 2021-06-29T15:16:47Z

failures:
tests::display_inner_str_in_struct
tests::display_u8_vec
tests::display_use_inner_type_hint
tests::display_use_outer_type_hint

those are the new tests. @justahero could you please update the timestamp format to match PR #522 ? feel free to bors r+ this again after that

* add test to check generated bytes for inner hint * add log output files for hints_inner program

This change checks the inner and outer type hint of nested `Uxx` values. For example: ```rust struct S { x: u8 } // derive expands to impl defmt::Format for S { fn format(&self, f: defmt::Formatter) { write!(f, "S {{ x: {=u8:?} }}", self.x); } } ``` where inner value `x` is of type `u8` with display hint `Debug`. When this struct is formated using an outer display hint then the outer hint will be used. ```rust info!("{:x}", S { x: 42 }); ``` renders as: `INFO S { x: 0x2a }` due to the `:x` (hex) display hint. On the other hand if the inner type is one of `:x` (hex), `:b` (binary) or `:a` (ascii), the outer display hint will be ignored. ```rust impl defmt::Format for S { fn format(&self, f: defmt::Formatter) { write!(f, "S {{ x: {=u8:b} }}", self.x); } } ``` The outer display hint (`:x`) in ```rust info!("{:x}", S { x: 42 }); ``` will be ignored, the output is rendered with inner display hint `:b` as `INFO S { x: 0b101010 }` * remove `seen_hits` property from `Arg::Format` variant

* add test log files for inner hints

Instead of applying the Debug type hint `:?` to all inner fields. * add test to check struct with generic type * update test output

This change adds display type hint precedence of `Uxxx` nested attributes. For example given the following struct and implementation. ```rust struct S { y: u8 } impl defmt::Format for S { fn format(&self, f: defmt::Formatter) { write!(f, "S {{ y: {=u8:?} }}") } } ``` If the user prints the struct as: ```rust info!("{:x}", S { y: 42 }); ``` the formatter should output `S { y: 0x2a }`, the outermost display hint `:x` applies.

The given test checks the display output of ```rust struct Data<'a> { name: &'a [u8], } let data = &[Data { name: b"Hi" }]; defmt::info!("{=[?]:a}", *data); ``` The test is currently failing, instead of `"Hi"` it outputs `[72, 105]` instead.

The display hint `"{=[?]:a}"` now applies as outer most type hint instead of the inner type. For example the struct with inner type: ```rust struct Data<'a'> { name: &'a [u8]', } let data = &[Data{name: b"Hi"}] dfmt::info!("{=[?]:a}"); ``` Before the string was displayed as `INFO [Data { name: [72, 105] }]`, while now it renders correctly as `INFO [Data { name: b"Hi" }]`. * update snapshot test * comment each single entry in `bytes` array in test

* apply pretty-printing to all `:b` and `:x` * change `µs` to `us`

Urhengulas · 2021-06-30T11:15:59Z

bors r+

bors · 2021-06-30T11:22:26Z

Build succeeded:

ci

justahero changed the title ~~[WIP] Implement precedence of inner display hint with unsigned types~~ [WIP] Implement precedence of inner display hint Jan 28, 2021

justahero force-pushed the knurling-rs/gh319 branch from dc6e35f to 14b0600 Compare January 29, 2021 13:52

justahero changed the title ~~[WIP] Implement precedence of inner display hint~~ Implement precedence of inner display hint Jan 29, 2021

justahero force-pushed the knurling-rs/gh319 branch from 14b0600 to decfc08 Compare January 29, 2021 14:13

justahero marked this pull request as ready for review February 1, 2021 10:46

justahero requested review from japaric and jonas-schievink February 1, 2021 15:12

japaric self-assigned this Feb 2, 2021

japaric requested changes Feb 2, 2021

View reviewed changes

japaric removed their assignment Feb 3, 2021

justahero force-pushed the knurling-rs/gh319 branch from 6a56b7f to 2bbc44d Compare February 4, 2021 11:21

jonas-schievink added the pr waits on: review Pull Request is waiting on review label Feb 10, 2021

jonas-schievink reviewed Feb 12, 2021

View reviewed changes

jonas-schievink added pr waits on: author Pull Request requires changes from the author and removed pr waits on: review Pull Request is waiting on review labels Feb 17, 2021

justahero force-pushed the knurling-rs/gh319 branch from 2bbc44d to 8d97af4 Compare March 26, 2021 09:22

justahero force-pushed the knurling-rs/gh319 branch from c7b7fbd to 57e88fc Compare May 26, 2021 10:18

Urhengulas added pr waits on: review Pull Request is waiting on review and removed pr waits on: author Pull Request requires changes from the author labels Jun 1, 2021

Urhengulas self-requested a review June 15, 2021 13:55

Urhengulas self-assigned this Jun 15, 2021

justahero requested review from japaric and jonas-schievink June 29, 2021 13:17

Urhengulas requested changes Jun 29, 2021

View reviewed changes

japaric approved these changes Jun 29, 2021

View reviewed changes

justahero force-pushed the knurling-rs/gh319 branch from c40cca2 to 1528d6e Compare June 29, 2021 18:03

justahero and others added 18 commits June 30, 2021 12:10

Add program to test inner hints

0e4386d

* add test to check generated bytes for inner hint * add log output files for hints_inner program

Add test to check outer type hint of struct

7c0591a

Expand sample program to display hints

6f28f4d

Add tests to check inner structs

a46697f

* add test log files for inner hints

Set default type hint for nested "str" field

933a5a9

Adjust code comments

b28946f

Update rustfmt changes

d53a993

Update code with rustfmt

f91d9a7

Set :? type hint to inner fields

5196a16

Instead of applying the Debug type hint `:?` to all inner fields. * add test to check struct with generic type * update test output

Fix test

76f7b76

Add test to check display hints of vec

ce9182f

The given test checks the display output of ```rust struct Data<'a> { name: &'a [u8], } let data = &[Data { name: b"Hi" }]; defmt::info!("{=[?]:a}", *data); ``` The test is currently failing, instead of `"Hi"` it outputs `[72, 105]` instead.

Add hints_inner as a program

87d2390

Update timestamp format in tests

b7deb6f

qemu: Fix hints_inner.rs

a971def

* apply pretty-printing to all `:b` and `:x` * change `µs` to `us`

decoder: Fix unittests

b3b0af0

Urhengulas force-pushed the knurling-rs/gh319 branch from 1528d6e to b3b0af0 Compare June 30, 2021 11:15

Urhengulas approved these changes Jun 30, 2021

View reviewed changes

bors bot merged commit 0b88433 into main Jun 30, 2021

bors bot deleted the knurling-rs/gh319 branch June 30, 2021 11:22

Urhengulas mentioned this pull request Jul 6, 2021

String fields in structs should be formatted using ':?' hint #319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement precedence of inner display hint #359

Implement precedence of inner display hint #359

justahero commented Jan 27, 2021 •

edited

Loading

japaric Feb 2, 2021

justahero Feb 4, 2021

jonas-schievink Feb 12, 2021

jonas-schievink Feb 12, 2021

jonas-schievink Feb 12, 2021

Urhengulas left a comment

Urhengulas Jun 29, 2021

Urhengulas Jun 29, 2021

Urhengulas Jun 29, 2021 •

edited

Loading

japaric left a comment

japaric Jun 29, 2021

japaric Jun 29, 2021

japaric commented Jun 29, 2021

bors bot commented Jun 29, 2021

japaric commented Jun 29, 2021

japaric commented Jun 29, 2021

Urhengulas commented Jun 30, 2021

bors bot commented Jun 30, 2021

	// outputs: "S { x: hi, y: 42 }"
	// outputs: "S { x: "hi", y: 42 }"

	// outputs: "S { x: hi, y: 0x2a }"
	// outputs: "S { x: "hi", y: 0x2a }"

Implement precedence of inner display hint #359

Implement precedence of inner display hint #359

Conversation

justahero commented Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urhengulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urhengulas Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

japaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japaric commented Jun 29, 2021

bors bot commented Jun 29, 2021

japaric commented Jun 29, 2021

japaric commented Jun 29, 2021

Urhengulas commented Jun 30, 2021

bors bot commented Jun 30, 2021

justahero commented Jan 27, 2021 •

edited

Loading

Urhengulas Jun 29, 2021 •

edited

Loading