Introduce "wrapper" helpers to rustdoc

[yotamofek]

Add a few traits for streamlining places where we need to wrap certain fmt::Displays in stuff like parentheses or brackets.
Hopefully this makes the actual display logic slightly easier to read.

First two commits are small, unrelated cleanups.

I'll probably add some doc comments to the stuff in display.rs, maybe also play around with the API, but wanted to get feedback on this idea first.

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[GuillaumeGomez]

I don't think these fields are actually useful: it always contains the same data after all. Can be handled directly into the Display implementation.

[yotamofek]

But then we would need to differentiate between the bracket used as a prefix and the one used as a suffix.
So an enum?
Or separating the generic arg for the prefix and suffix in Wrapper

[GuillaumeGomez]

Enum is a very good idea. 👍

[GuillaumeGomez]

Hum actually, two newtypes could be the solution here:

struct OpenBracket;
struct CloseBracket;

And that's it.

[yotamofek]

And then have two generic args for the Wrapper type?
Because right now it looks like this:

#[derive(Clone, Copy)]
pub(crate) struct Wrapper<T> {
    prefix: T,
    suffix: T,
}

so prefix and suffix have to be of the same type

[GuillaumeGomez]

Open and Close is better naming. 👍

Is there any need for the AngleBracket type alias though?

[yotamofek]

Pushed with AngleBracket being an enum with Open and Close variants.

(but FWIW, opaque types are just so cool)

[yotamofek]

Is there any need for the AngleBracket type alias though?

(not really important, just an aside)

Not sure why, but this doesn't work:

impl<T> Wrapper<T> {
    pub(crate) fn with_angle_brackets() -> Self {
        Self { prefix: angle_bracket(BracketPos::Open), suffix: angle_bracket(BracketPos::Close) }
    }
}

I'm getting this error:

error[E0308]: mismatched types
  --> src/librustdoc/display.rs:74:65
   |
61 | fn angle_bracket(pos: BracketPos) -> impl Display {
   |                                      ------------ the found opaque type
...
72 | impl<T> Wrapper<T> {
   |      - expected this type parameter
73 |     pub(crate) fn with_angle_brackets() -> Self {
74 |         Self { prefix: angle_bracket(BracketPos::Open), suffix: angle_bracket(BracketPos::Close) }
   |                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter `T`, found opaque type
   |
   = note: expected type parameter `T`
                 found opaque type `impl std::fmt::Display`
   = help: type parameters must be constrained to match other types
   = note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters

Maybe it's because every instance of an opaque type is considered different, so the prefix and suffix fields don't have the same type, as far as type checking is concerned?

[GuillaumeGomez]

Hum, let me try to hack that.

[GuillaumeGomez]

This works fine so I guess opaque type is mandatory:

#![feature(type_alias_impl_trait)]
#![feature(debug_closure_helpers)]

use std::fmt;

struct Wrapper<T> {
    prefix: T,
    suffix: T,
}

pub(crate) type AngleBracket = impl fmt::Display;

#[derive(Clone, Copy)]
enum BracketPos {
    Open,
    Close,
}

#[define_opaque(AngleBracket)]
fn angle_bracket(pos: BracketPos) -> AngleBracket {
    fmt::from_fn(move |f| {
        f.write_str(match (pos, f.alternate()) {
            (BracketPos::Open, true) => "&lt;",
            (BracketPos::Open, false) => "<",
            (BracketPos::Close, true) => "&gt;",
            (BracketPos::Close, false) => ">",
        })
    })
}

impl Wrapper<AngleBracket> {
    pub(crate) fn with_angle_brackets() -> Self {
        Self { prefix: angle_bracket(BracketPos::Open), suffix: angle_bracket(BracketPos::Close) }
    }
}

[lolbinarycat]

I think the basic idea is sound, but I think the naming could be a lot better.

I think Wrapped::with_parens().when(!sub_cfg.is_all()).around(Display(sub_cfg, self.1)) is way more immediatly readable than Wrapper::parentheses().optional(!sub_cfg.is_all()).wrap(Display(sub_cfg, self.1)). Also Wrapped::with('', '') seems more readable.

I would also rename WithOpts::new to WithOpts::from.

[yotamofek]

I think the basic idea is sound, but I think the naming could be a lot better.

Thanks, I completely agree 😅

I think Wrapped::with_parens().when(!sub_cfg.is_all()).around(Display(sub_cfg, self.1)) is way more immediatly readable than Wrapper::parentheses().optional(!sub_cfg.is_all()).wrap(Display(sub_cfg, self.1)).

when is much better than optional, thanks!
I do kinda prefer wrap over around, and am ambivalent about with_parens (because with_angled_brackets is kinda long?).

Also Wrapped::with('', '') seems more readable.

This part I didn't really understand.

I would also rename WithOpts::new to WithOpts::from.

👍

[lolbinarycat]

This part I didn't really understand.

I am suggesting to rename Wrapper::new to Wrapped::with.

[lolbinarycat]

I do kinda prefer wrap over around, and am ambivalent about with_parens (because with_angled_brackets is kinda long?).

Honestly around is the name I'm least sure of, but I think the with_* prefix does improve readability enough to be worth it (also nit: I feel like with_angle_brackets (no d) would be the name I would choose)

[yotamofek]

I do kinda prefer wrap over around, and am ambivalent about with_parens (because with_angled_brackets is kinda long?).

Honestly around is the name I'm least sure of, but I think the with_* prefix does improve readability enough to be worth it (also nit: I feel like with_angle_brackets (no d) would be the name I would choose)

Applied all your suggestions, apart from around 😁
(the angled was a typo)
Thanks for the suggestions, feels like an improvement to me.

[GuillaumeGomez]

Looks good to me, thanks! I'll let @lolbinarycat do the r+ once they're satisfied as well.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[lolbinarycat]

Applied all your suggestions, apart from around 😁

Sorry, I don't think I was very explicit, but I would like if Wrapper was renamed to Wrapped. This makes "wrapped with" read more like a sentence, and also "wrapper" just makes me think of the newtype pattern, while "wrapped" does not.

Additionally, it looks like you forgot to rename new to with.

Apologies for the inconvenience.

[yotamofek]

Apologies for the inconvenience.

No need to apologize 😁

Sorry, I don't think I was very explicit, but I would like if Wrapper was renamed to Wrapped. This makes "wrapped with" read more like a sentence, and also "wrapper" just makes me think of the newtype pattern, while "wrapped" does not.

Done.

Additionally, it looks like you forgot to rename new to with.

Woops, I think I did that and then at some point undid it by mistake.

[lolbinarycat]

LGTM, just waiting on CI

View changes since this review

[lolbinarycat]

@bors r+

[bors]

📌 Commit 2067160 has been approved by lolbinarycat

It is now in the queue for this repository.