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

Properly render HRTBs #84814

Merged
merged 3 commits into from
Jun 26, 2021
Merged

Properly render HRTBs #84814

merged 3 commits into from
Jun 26, 2021

Conversation

Stupremee
Copy link
Member

@Stupremee Stupremee commented May 2, 2021

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

This will now render properly including the for<'a>
image

I do not know if this covers all cases, it only covers everything that I could think of that includes for and lifetimes in where bounds.
Also someone need to mentor me on how to add a proper rustdoc test for this.

Resolves #78482

@rust-highfive
Copy link
Collaborator

r? @jyn514

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2021
@Stupremee Stupremee added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label May 2, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Stupremee Stupremee force-pushed the properly-render-hrtbs branch 2 times, most recently from 8cacbea to 31f9770 Compare May 2, 2021 19:29
@camelid
Copy link
Member

camelid commented May 5, 2021

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.

@camelid camelid added A-trait-system Area: Trait system A-lifetimes Area: Lifetimes / regions and removed A-trait-system Area: Trait system labels May 5, 2021
@jyn514
Copy link
Member

jyn514 commented May 5, 2021

@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
Copy link
Member Author

@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
Copy link
Member

camelid commented May 8, 2021

@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

@rust-highfive rust-highfive assigned camelid and unassigned jyn514 May 8, 2021
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2021
@Stupremee
Copy link
Member Author

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 camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 11, 2021
@camelid
Copy link
Member

camelid commented May 17, 2021

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 :)

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2021
@Stupremee
Copy link
Member Author

Stupremee commented May 18, 2021

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 Stupremee added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2021
@Stupremee
Copy link
Member Author

r? @camelid
This should be ready now!

@camelid camelid self-assigned this Jun 12, 2021
@Stupremee Stupremee force-pushed the properly-render-hrtbs branch 2 times, most recently from c31cb05 to d722c21 Compare June 18, 2021 19:57
@Stupremee
Copy link
Member Author

@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 😅 ?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Jun 19, 2021

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
Copy link
Member Author

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

r? @camelid

@Stupremee Stupremee added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2021
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 26, 2021

📌 Commit 4ea2748 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@bors
Copy link
Contributor

bors commented Jun 26, 2021

⌛ Testing commit 4ea2748 with merge 831ae3c...

@bors
Copy link
Contributor

bors commented Jun 26, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 26, 2021
@bors bors merged commit 831ae3c into rust-lang:master Jun 26, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 26, 2021
@Stupremee Stupremee deleted the properly-render-hrtbs branch June 26, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc doesn't render HRTBs
8 participants