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.

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

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

Closed
japaric opened this issue Dec 18, 2020 · 3 comments
Closed

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

japaric opened this issue Dec 18, 2020 · 3 comments
Assignees
Labels
difficulty: medium Somewhat difficult to solve priority: low Low priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: bug Something isn't working

Comments

@japaric
Copy link
Member

japaric commented Dec 18, 2020

Current output:

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

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

Desired output: S { x: "hi" }


I wasn't sure how to best resolve this one.
In the RFC, we discussed applying the ':?' hint to all struct fields and that does the job in the previous example.
But seems like it would have the adverse effect of display hints never propagating into structs. Example below:

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

// derive would expand into
impl defmt::Format for S {
    fn format(&self, f: defmt::Formatter) {
        write!(f, "S {{ x: {=str:?}, y: {=u8:?} }}")
    }
}
// matches the expected output
info!("{}", S { x: "hi", y: 42 });
// -> INFO S { x: "hi", y: 42 }

// `y` field ignores top-level `:x` display hint
info!("{:x}", S { x: "hi", y: 42 });
// -> INFO S { x: "hi", y: 42 }

The second statement ignores the ':x' hint.

We discussed in the RFC that the innermost display hint should not be overridden by an outer one.
Example from the RFC:

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

// binary hint should be ignored
info!("{:b}", S { x: 42 });
// -> INFO S { x: 0x2a }

Given that, using ':?' in fields would make non-string fields ignore display hints specified at the top call which is not what I would expect.

@japaric japaric added the type: bug Something isn't working label Dec 18, 2020
@japaric japaric added this to the v0.2.0 milestone Jan 5, 2021
@japaric
Copy link
Member Author

japaric commented Jan 5, 2021

Here's one way to make this work.

We discussed in the RFC that the innermost display hint should not be overridden by an outer one.

We revise this part of the specification to say "innermost display hint should not be overridden by an applicable outer one" and introduce the idea of certain hints not being applicable to certain types.

For example, :x, :b and :a do not apply to type =str, but :? does (it escapes the string).
Similarly, :? does not apply to integer types like =u8 but :b and :x do apply.

With that adjusted semantics we can tweak the derive macro to always add the :? hint to all struct / enum fields. Example:

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

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

Now if the user does:

info!("{:x}", S { x: "hi", y: 42 });

A printer (like probe-run) needs to consider all the nested hints and apply the innermost applicable one. Following the previous example:

  • field x has hints :? and :x (in the innermost to outermost order). x has type &str and :? is applicable to &str so x is formatted with the :? hint.

  • likewise, field y has hints :? and :x. y has type u8 so the outermost hint :? does not apply. Therefore, y is formatted with the :x hint.

The output then is S { x: "hi", y: 0x2a }

Looking at the other example from the issue description:

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

// binary hint should be ignored
info!("{:b}", S { x: 42 });

Here field x has hints :x and :b (in the innermost to outermost order). x has type =u8 and hint :x applies to that type so hint :b is ignored (as expected).

@japaric japaric added the status: needs PR Issue just needs a Pull Request implementing the changes label Jan 5, 2021
@justahero justahero self-assigned this Jan 5, 2021
@Urhengulas Urhengulas removed this from the v0.2.0 milestone Feb 28, 2021
@Urhengulas Urhengulas added difficulty: medium Somewhat difficult to solve priority: low Low priority for the Knurling team labels Mar 14, 2021
bors bot added a commit that referenced this issue Jun 29, 2021
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 bot added a commit that referenced this issue Jun 30, 2021
359: Implement precedence of inner display hint r=Urhengulas 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>
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
@Urhengulas
Copy link
Member

@japaric Do you consider this fixed by #359?

@japaric
Copy link
Member Author

japaric commented Jul 6, 2021

yes!

@japaric japaric closed this as completed Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium Somewhat difficult to solve priority: low Low priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants