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

Wrong result using &[&dyn Formatter] #458

Closed
Dirbaio opened this issue Apr 22, 2021 · 7 comments
Closed

Wrong result using &[&dyn Formatter] #458

Dirbaio opened this issue Apr 22, 2021 · 7 comments
Labels
breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version difficulty: hard Fairly difficult to solve priority: high High priority for the Knurling team status: needs design This feature needs design work to move forward type: bug Something isn't working
Milestone

Comments

@Dirbaio
Copy link
Contributor

Dirbaio commented Apr 22, 2021

This is a trickier version of #455. The needs_tag optimization can be broken even without using manual Format impls.

    #[derive(defmt::Format)]
    struct Foo;

    #[derive(defmt::Format)]
    struct Bar;

    let what: &[&dyn defmt::Format] = &[&Foo, &Bar];
    info!("what: {:?}", what);

prints

0 INFO  what: [Foo, Foo]
@Urhengulas Urhengulas added priority: high High priority for the Knurling team type: bug Something isn't working labels Apr 26, 2021
@Urhengulas
Copy link
Member

@Dirbaio: Could you please specify what is wrong about the resulting logline? It seems legit to me 🤔 😄

@jonas-schievink
Copy link
Contributor

It should be

0 INFO  what: [Foo, Bar]

but it's actually

0 INFO  what: [Foo, Foo]

@jonas-schievink
Copy link
Contributor

I think the solution here is to only perform the optimization if the element type explicitly supports it (presumably via a default-implemented but hidden method of the Format trait?). #[derive(Format)] would then implement that function for structs in terms of the contained fields.

@jonas-schievink
Copy link
Contributor

Ah, but we'd also have to make sure we don't use that method through the trait object, I'm not sure if that's possible

@jonas-schievink
Copy link
Contributor

It seems like it could be made to work by replacing the impl<T: Format + ?Sized> Format for &'_ T impls with these:

impl<T> Format for &'_ T
where
    T: Format,
{
    fn format(&self, fmt: Formatter) {
        T::format(self, fmt)
    }
}

impl Format for &'_ str {
    fn format(&self, fmt: Formatter) {
        str::format(self, fmt)
    }
}

impl<T> Format for &'_ [T]
where
    T: Format,
{
    fn format(&self, fmt: Formatter) {
        <[T]>::format(self, fmt)
    }
}

impl Format for &'_ dyn Format {
    fn format(&self, fmt: Formatter) {
        <dyn Format>::format(self, fmt)
    }
}

...then all but the last impl could opt into "static tags" if T also supports it. It'll be a breaking change though, because &CustomType<str> could lose its Format impl and would need a hand-written one.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Apr 26, 2021

I wonder if the needs_tag optimization is too complex? What if the Format trait were something like this:

trait Format {
    fn format_tag(fmt: Formatter);
    fn format_data(&self, fmt: Formatter);
}

Formats {=?} are encoded with tag+data. Format slices {=[?]} are encoded with len+tag+data+data+data+data... Tag must be guaranteed to be the same for all instances of the same type.

The impl for dyn Format would use {=?} as tag, then the inner tag+data as data

This is less efficient because it'll still repeat the inner tags, but I think printing slices of deep structs is less common. This maybe gives 80% of the wire-size savings with 20% of the complexity which might be a worthy tradeoff? More complexity means higher code size too...

@Dirbaio Dirbaio mentioned this issue Apr 26, 2021
@jonas-schievink jonas-schievink added difficulty: hard Fairly difficult to solve status: needs design This feature needs design work to move forward labels May 5, 2021
@Urhengulas Urhengulas added the breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version label May 7, 2021
@Urhengulas Urhengulas added this to the v0.3.0 milestone Jun 18, 2021
@japaric
Copy link
Member

japaric commented Jul 7, 2021

the new format trait (PR #508) is not object safe so trait objects are no longer supported. Closing this issue because it no longer applies.

@japaric japaric closed this as completed Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version difficulty: hard Fairly difficult to solve priority: high High priority for the Knurling team status: needs design This feature needs design work to move forward type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants