Must_use messages

[joshka]

Not something entirely specific to this PR as it’s in other places too…

The ownership of the original object is not moved so the message is confusing with ownership in mind.

It actually does not change the value of self and returns a new, modified value. With a lifetime reference shared with the original.

Not sure how often these must_use messages are misleading in other places too… maybe we should create an issue about it to check that?

Originally posted by @EdJoPaTo in #987 (comment)

[joshka]

I suspect the right answer is either to just ditch the message altogether (and only use it for actual novel meaningful situations), or alternatively link to a page that describes how we use the builder-lite pattern in Ratatui and why

[EdJoPaTo]

Another point which is kinda annoying here: There is both a doc comment and a must_use comment:

/// This is a fluent setter method which must be chained or used as it consumes self
#[must_use = "method moves the value of self and returns the modified value"]

It should have either of those, and I would prefer a must_use comment as it helps the compiler to point out the mistake exactly when it's relevant. The doc comment is only human-readable and not machine-readable.

[joshka]

Once ratatui-org/ratatui-website#558 finishes deploying, perhaps we can change these all to just:

#[must_use("https://ratatui.rs/concepts/builder-lite-pattern")]

[EdJoPaTo]

It should keep a simple message in there. Just pointing to a URL is not very friendly. "The return value contains your object. Read more about it on x" seems more friendly.

[joshka]

It should keep a simple message in there. Just pointing to a URL is not very friendly. "The return value contains your object. Read more about it on x" seems more friendly.

I think predominantly (at least from what I've seen) the std lib doesn't add any message for must_use at all.

Here's the compiler warning without any added message:

warning: unused return value of `style::Style::fg` that must be used
   --> src/style.rs:892:9
    |
892 |         style.fg(Color::Red);
    |         ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
892 |         let _ = style.fg(Color::Red);
    |         +++++++

Here's the same with just the bare url:

warning: unused return value of `style::Style::fg` that must be used
   --> src/style.rs:892:9
    |
892 |         style.fg(Color::Red);
    |         ^^^^^^^^^^^^^^^^^^^^
    |
    = note: https://ratatui.rs/concepts/builder-lite-pattern
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
892 |         let _ = style.fg(Color::Red);
    |         +++++++

Perhaps this is enough:

warning: unused return value of `style::Style::fg` that must be used
   --> src/style.rs:892:9
    |
892 |         style.fg(Color::Red);
    |         ^^^^^^^^^^^^^^^^^^^^
    |
    = note: See https://ratatui.rs/concepts/builder-lite-pattern for more info
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
892 |         let _ = style.fg(Color::Red);
    |         +++++++

I think the message we're trying to convey is that this isn't a mutable setter method, it's a method which returns the value with the updated field set. What about: "This is a builder lite method. See https://ratatui.rs/concepts/builder-lite-pattern"?

I suspect that we're over-indexing on a problem that even the newest rustacean should be able to get past (the curse of knowledge unfortunately means that it's difficult to know if this suspicion is correct, but a useful proxy for working that out is the click stats on that page)

In times past, I've had good success and feedback from users just putting a url to every possible error (and it makes it easy to update the explanation without re-releasing the software). So it might be even better to just link e.g. https://ratatui.rs/warnings/must-use instead.

[EdJoPaTo]

With the example output: yeah. Nothing or an URL seems like a good enough thing. It’s clear enough that the result is relevant and needs to be used.

[joshka]

With the example output: yeah. Nothing or an URL seems like a good enough thing. It’s clear enough that the result is relevant and needs to be used.

Cool. My preferences in order would be:

• bare /warning url
• small amount of text with /concepts url
• bare /concepts url
• <a big gap>
• nothing
• custom message we have now

I don't have a strong preference for any of the first three over the other, so a bare /concepts url works if that's what you prefer.

[EdJoPaTo]

A warning having a note: output including warning… not sure if I like that. I understand the reasoning. It explains the warning further. But I think a concept is more relevant there. That's why its this way.

But all together… A person will either just use the compiler output to improve that and does not care about the pattern ratatui was built with, or that person reads into the ratatui then a concept is more helpful.

I think nothing would also be perfectly fine and the easiest to maintain / not to mess up on additions. So I'm for nothing or a concept URL.

[joshka]

Let's go with the concepts url. :)