Skip to content

Properly render HRTBs#84814

Merged
bors merged 3 commits into
rust-lang:masterfrom
Stupremee:properly-render-hrtbs
Jun 26, 2021
Merged

Properly render HRTBs#84814
bors merged 3 commits into
rust-lang:masterfrom
Stupremee:properly-render-hrtbs

Conversation

@Stupremee

@Stupremee Stupremee commented May 2, 2021

Copy link
Copy Markdown
Contributor
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
Copy Markdown
Contributor

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

camelid commented May 5, 2021

Copy link
Copy Markdown
Member

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

jyn514 commented May 5, 2021

Copy link
Copy Markdown
Member

@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
Copy Markdown
Contributor 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

camelid commented May 8, 2021

Copy link
Copy Markdown
Member

@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
Comment thread src/test/rustdoc/higher-ranked-trait-bounds.rs Outdated
@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
Copy Markdown
Contributor 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

camelid commented May 17, 2021

Copy link
Copy Markdown
Member

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

Stupremee commented May 18, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor 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 Stupremee force-pushed the properly-render-hrtbs branch from d722c21 to 3f9f7db Compare June 18, 2021 19:58
@Stupremee

Copy link
Copy Markdown
Contributor 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.

@Stupremee Stupremee force-pushed the properly-render-hrtbs branch from 3f9f7db to f90d268 Compare June 18, 2021 20:05
@rust-log-analyzer

This comment has been minimized.

@camelid

camelid commented Jun 19, 2021

Copy link
Copy Markdown
Member

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 Stupremee force-pushed the properly-render-hrtbs branch from f90d268 to db8fd51 Compare June 19, 2021 07:30
@Stupremee Stupremee force-pushed the properly-render-hrtbs branch from db8fd51 to 4ea2748 Compare June 19, 2021 07:44
@Stupremee

Copy link
Copy Markdown
Contributor 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
Copy Markdown
Member

Thanks!

@bors: r+

@bors

bors commented Jun 26, 2021

Copy link
Copy Markdown
Collaborator

📌 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

bors commented Jun 26, 2021

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Jun 26, 2021

Copy link
Copy Markdown
Collaborator

☀️ 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