Properly render HRTBs by Stupremee · Pull Request #84814 · rust-lang/rust · GitH...

Copy link

Member

pub fn test<T>() 
where
    for<'a> &'a T: Iterator,
{}

rust-highfive commented on May 2

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been hidden.

This comment has been hidden.

camelid commented on May 5 •

Also someone need to mentor me on how to add a proper rustdoc test for this.

It looks like you already added a test? But could you also add a test case for HRTB in struct and enum definitions? For example:

struct Foo<'b> {
    pub my_fn: for<'a> fn(&'a u32) -> &'b u32,
}

// and something equivalent for enums

EDIT: I forgot that a test already exists for this because I fixed this case in an earlier PR.

jyn514 commented on May 5

@camelid are you interested in reviewing this? I have a lot of PRs assigned and I'm about to go on break for a few weeks.

Stupremee commented on May 5

@camelid are you interested in reviewing this? I have a lot of PRs assigned and I'm about to go on break for a few weeks.

I will stop assigning you! Have fun!

camelid commented on May 8

@camelid are you interested in reviewing this? I have a lot of PRs assigned and I'm about to go on break for a few weeks.

Sure! Enjoy your break :)

r? @camelid

Stupremee commented on May 9

I need your opionion on this @camelid. The "best" solution I found, that is not just a dirty fix, would be to introduce a new Type called TraitObject which would contain all important data. However, that seems like a big change which might need more discussion? Since this could also influence rustdoc-json and probably more.

camelid commented on May 17

The "best" solution I found, that is not just a dirty fix, would be to introduce a new Type called TraitObject which would contain all important data.

Hmm, but how would that handle the case of bare functions like for<'a> fn(&'a i32) -> i32? And also how would it work with where bounds?

I tried looking in the Rust Reference to see what other places HRTBs can be used, but unfortunately it only seems to include information about uses in where clauses, and not bare functions.

I also found another place where HRTBs can show up, though it probably is handled the same way as with where in impls:

struct Foo<T>
where
    for<'a> T: PartialEq<&'a T>,
{
    x: T,
}

struct Bar<T>
where
    T: for<'a> PartialEq<&'a T>,
{
    x: T,
}

I would recommend asking on Zulip for help with figuring out where HRTBs can appear and how they should be handled. Hopefully some people more knowledgeable about the intricacies of HRTB can help you there :)

Stupremee commented on May 18 •

Hmm, but how would that handle the case of bare functions like for<'a> fn(&'a i32) -> i32? And also how would it work with where bounds?

These are both already handled. My current fix includes a new Type because the information which lifetimes are specified was previously discarded while cleaning, so I needed to introduce a new Type called PolyTraitTy (maybe TraitObject is a better name, not sure).
I also removed the old for-lifetime.rs test and moved it into higher-kind-trait-bounds.rs together with some other tests.

EDIT: I did not add a new Type, and instead added a new field to ResolvedPath. This makes more sense because there's already a field that is only present for trait objects on ResolvedPath.

Stupremee commented on May 19

r? @camelid
This should be ready now!

Stupremee commented 12 days ago

@camelid Sorry for the long delay here.

My new commit introduces the new DynTrait type, and in general, everything works really well.
However, there are still two tests failing, due to cross crate re-exports, but I wasn't able to find out why they are failing, yet.

Might you be able to look at them and see if you can spot the mistake faster than me spending hours debbuing ?

This comment has been hidden.

This comment has been hidden.

camelid commented 12 days ago

However, there are still two tests failing, due to cross crate re-exports, but I wasn't able to find out why they are failing, yet.

Might you be able to look at them and see if you can spot the mistake faster than me spending hours debbuing ?

Could you post screenshots of what the generated pages look like for the parts of the tests that are failing? I probably won't be able to look at them for the next week or so, but I'll make sure to take a look after.

Stupremee commented 12 days ago

Ahh, I finally found it. This should be ready to merge now!

r? @camelid

GuillaumeGomez commented 5 days ago

Thanks!

@bors: r+

bors commented 5 days ago

Commit 4ea2748 has been approved by GuillaumeGomez

bors commented 5 days ago

Testing commit 4ea2748 with merge 831ae3c...

bors commented 5 days ago

Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 831ae3c to master...