Use fmt precision if specified in graphical report handler

[diondokter]

I'm displaying diagnostics on a web page.
Currently the only way I know of to change the width with which the diagnostics get printed is in setting the hook.
But the hook can only be set once, so that won't work sadly.

So this PR adds the option to print the diagnostics with the fmt width that overrides the existing configured width.

Strictly speaking this not exactly correct. fmt width is supposed to be a minimum while we're using it as a maximum here.

If there's another better way to do dynamic width support, I'm all ears. But I think this is a decent solution :)

[cgettys-microsoft]

Wouldn't precision support exactly this, while being technically correct?
"For non-numeric types, this can be considered a “maximum width”. If the resulting string is longer than this width, then it is truncated down to this many characters and that truncated value is emitted with proper fill, alignment and width if those parameters are set.

For integral types, this is ignored.

For floating-point types, this indicates how many digits after the decimal point should be printed.
"
emphasis mine
https://doc.rust-lang.org/std/fmt/#precision

[cgettys-microsoft]

I believe .precision() is treated as maximum width for stringy / non-numeric types, would that make more sense?
https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.precision

[diondokter]

Ah didn't know that! Will change 😁

[cgettys-microsoft]

Neither did I, fwiw 😄. I just went "there has to be some way that that's supported, surely" and had better luck stumbling onto the right method documentation apparently

[diondokter]

Alright, did the change! Working as expected.
There's a bunch of new clippy warnings, but I see #451 tackles that already.

[cgettys-microsoft]

To be clear - this is just a musing from another fellow user of the library, not something you necessarily need to address.
But if we were being hyper-pedantic, this is controlling line wrapping, right? not truncating like .precision() presumably is supposed to?

It seems reasonable enough to me. But that .clone() might get expensive.

I feel like there's another issue we're kinda circling around, that I've ran into in tests too - which is that given a ReportHandler, and a Diagnostic, there's no obvious way (such as a macro) documented on how to put them together.

Most of the time, setting a hook early in startup is what a user wants to do.
But as you point out, the hook is settable only once. Hmmm.... I dunno, just thinking

[diondokter]

But that .clone() might get expensive.

I considered that too, but the current struct doesn't look too bad.
It's also already being cloned in multiple places (partly to change the width being used).
The struct is 360 bytes according to RA and has 3 or so allocations in it.

there's no obvious way (such as a macro) documented on how to put them together.

There is a way to print a diagnostic without using the hook I guess: https://docs.rs/miette/latest/miette/struct.GraphicalReportHandler.html#method.render_report
But is it the intended way to make things work? I guess not?

I'm only now thinking I could use this instead of this PR.
But idk what's better. I guess Kat will have some opinions :P

[cgettys-microsoft]

Perhaps another way to solve for this scenario would be to have:

impl Report
{
    pub fn with_handler(self, new_handler: Box<dyn ReportHandler> -> Self
   {
        let inner = unsafe { self.inner.boxed() }.into_inner(); 
        unsafe { Report::construct(inner.vtable, Some(new_handler), inner.object) }
   }

Then you could do

report = Report::from(my_diagnostic).with_handler(my_overridden_handler)
println!("{report}");

But even that seems wasteful unless the ReportHandler being cloned for every error is expected / reasonable.

Or, alternatively, one could add a bunch of methods to Report that would take the handler up front.

It also looks likely to me that you could install a HOOK that say, looks at thread local instead of always returning the same handler. Which might make sense in some scenarios. Might be another way to solve your use case.

But yeah, I have no idea what's better either, and don't have a strong opinion. I just thought this PR was interesting and thought I'd share some ideas, which may or may not be any good.

Kat's opinion on this is obviously massively more informed than my own, and I look forward to seeing their much more informed thoughts.

[cgettys-microsoft]

But that .clone() might get expensive.

I considered that too, but the current struct doesn't look too bad. It's also already being cloned in multiple places (partly to change the width being used). The struct is 360 bytes according to RA and has 3 or so allocations in it.

there's no obvious way (such as a macro) documented on how to put them together.

There is a way to print a diagnostic without using the hook I guess: https://docs.rs/miette/latest/miette/struct.GraphicalReportHandler.html#method.render_report But is it the intended way to make things work? I guess not?

I'm only now thinking I could use this instead of this PR. But idk what's better. I guess Kat will have some opinions :P

Yeah, definitely doable. like the comment on GraphicalReportHandler::render_report (and the other render_report methods too) says:

    /// Render a [`Diagnostic`]. This function is mostly internal and meant to
    /// be called by the toplevel [`ReportHandler`] handler, but is made public
    /// to make it easier (possible) to test in isolation from global state.

Something like this does the trick and I think works in your scenario.

fn get_formatted_diagnostic(diagnostic: &dyn miette::Diagnostic) -> String
{
      let handler =  miette::GraphicalReportHandler::new_themed(GraphicalTheme::unicode_nocolor());
      let mut result = String::new();
      handler.render_report(&mut result, diagnostic).expect("Should render");
      result
}

This is very useful to my tests, glad I commented on this issue, because I didn't quite get what that comment meant before.
That being said, it's less convenient if you're looking to do a custom ReportHandler that actually is interested in the format args or the like and thus needs to pass in a Formatter (nightly API yes, but not on stable, it's expected that format! or the like will provide it).
Report wrapping the inner type seems to be how that is worked around (since it gets called with the formatter).

Which I guess would be another way to solve this - you could basically create your own version of Report (potentially less optimal) that does the same sort of trick of implementing Debug and Display to call into your ReportHandler, ignoring the one contained inside the Report.

[zkat]

As a general thought on performance: I don't consider extreme performance to be super important for the graphical renderer. It's kind of already assumed this is a "one and done" kinda situation, not something you're going to be doing in a tight loop or printing a literal ton of. Even if people printed a lot of diagnostics, most folks get cut off at 1-10k lines in their terminal. These extra clone allocations wouldn't even be a blip at that scale.

[cgettys-microsoft]

As a general thought on performance: I don't consider extreme performance to be super important for the graphical renderer. It's kind of already assumed this is a "one and done" kinda situation, not something you're going to be doing in a tight loop or printing a literal ton of. Even if people printed a lot of diagnostics, most folks get cut off at 1-10k lines in their terminal. These extra clone allocations wouldn't even be a blip at that scale.

Fair point. Most compilers and similar tools I've worked with have some error count limit where they stop, like msvc's C1003 for example (if they're kind enough to go beyond "the first error") - even if we're talking about a thousand errors, we're talking about under a megabyte of ReportHandlers themselves. And the actual span contents may dwarf the ReportHandlers in a lot of cases anyway.

Edit: I also think at some point I was thinking "what if ReportHandler is very large, it could be expensive". But the logic in question is specific to GraphicalReportHandler, not Box<dyn ReportHandler> or the like, so that's obviously not relevant.

[diondokter]

Ok, I think this is blocked on #451

[cgettys-microsoft]

Ok, I think this is blocked on #451

#451 perhaps isn't the optimal solution (I couldn't get any combination of --locked and package versions to play nice without raising the MSRV a lot - from its current 1.70 to 1.82 - and I don't understand why not), but it is the best I could come up with in a few hours of trying to fix it. If you can find a way to do so without raising the MSRV, that'd presumably be better than what I did - I don't personally have any need for more than a few versions back (e.g. even a MSRV of like, 1.86 or 1.88 wouldn't bother me), so I don't really have a strong personal opinion, but there may well be people out there to whom it does matter.

I'm not clear what the project's policy is on raising the MSRV, or whether @zkat has plans to address the downlevel issues themself in a better way - I just thought I'd raise #451 in case it was helpful, as I believe in contributing back to libraries I'm using wherever I can.

[cgettys-microsoft]

Just in case you missed it, #451 merged