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

rustdoc: Render HRTB correctly for bare functions #79991

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 13, 2020

The angle brackets were not rendered, so code like this:

some_func: for<'a> fn(val: &'a i32) -> i32

would be rendered as:

some_func: fn'a(val: &'a i32) -> i32

However, rendering with angle brackets is still invalid syntax:

some_func: fn<'a>(val: &'a i32) -> i32

so now it renders correctly as:

some_func: for<'a> fn(val: &'a i32) -> i32

However, note that this code:

some_trait: dyn for<'a> Trait<'a>

will still render as:

some_trait: dyn Trait<'a>

which is not invalid syntax, but is still unclear. Unfortunately I think
it's hard to fix that case because there isn't enough information in the
rustdoc::clean::Type that this code operates on. Perhaps that case can
be fixed in a later PR.

r? @jyn514

@camelid camelid added C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 13, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2020
@camelid camelid added the A-trait-system Area: Trait system label Dec 13, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 13, 2020

some_func: fn<'a>(val: &'a i32) -> i32

This is invalid syntax:

error: expected `(`, found `<`
 --> src/main.rs:1:30
  |
1 | fn higher_order(some_func: fn<'a>(val: &'a i32) -> &i32) {
  |                              ^ expected `(`

Is it simple to display for<'a> fn instead? If so, we should do that.

@camelid
Copy link
Member Author

camelid commented Dec 13, 2020

Yes, I was planning on opening a follow-up PR; I figured we should do it in two stages so the rendered syntax is at least plausible. I can do it in this PR instead if you think that's better though.

@jyn514
Copy link
Member

jyn514 commented Dec 13, 2020

Yes, I think it would be better to fix it here if possible.

@camelid
Copy link
Member Author

camelid commented Dec 13, 2020

I fixed it for bare functions, but I think it will be harder to fix for dyn Traits. See the commit for more: cf5a620.

@jyn514 jyn514 changed the title rustdoc: Fix bug when rendering generic bare function rustdoc: Fix bug when rendering function pointers with higher-ranked lifetimes Dec 13, 2020
src/librustdoc/html/format.rs Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
src/test/rustdoc/for-lifetime.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Dec 13, 2020

I fixed it for bare functions, but I think it will be harder to fix for dyn Traits. See the commit for more: cf5a620.

Ok, that seems reasonable. The new output for traits is at least vaguely right so this is definitely a step in the right direction 😆

@jyn514 jyn514 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 Dec 13, 2020
@jyn514

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Dec 23, 2020

@camelid would this help with #78482 ?

@camelid
Copy link
Member Author

camelid commented Dec 23, 2020

@camelid would this help with #78482 ?

@jyn514 No, I don't think so. This just fixes the rendering on bare functions: the HRT bounds aren't rendered at all on trait bounds or trait objects.

@camelid
Copy link
Member Author

camelid commented Jan 11, 2021

This PR is blocked on me figuring out why the @has checks are failing. Maybe @GuillaumeGomez knows? :P

These are the htmldocck errors: #79991 (comment)

@GuillaumeGomez
Copy link
Member

Can you provide the full diff provided by the tidy tool when running the tests too please? (You might need to install it, to update your nightly rustdoc and to rebase this PR though haha).

@camelid
Copy link
Member Author

camelid commented Jan 17, 2021

Rebased.

@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member Author

camelid commented Jan 19, 2021

I fixed the test! It was a silly mistake on my part:

There were two issues:

  1. I was using the wrong filename: foo/index.html instead of
    foo/struct.Foo.html
  2. I was using the wrong syntax (I had changed it after creating the
    test): fn<'a> instead of for<'a> fn

The first one was the primary issue, though, because I couldn't
understand why the text I was matching against was the same as the text
in the @has check but the test was failing.

Glad I was able to figure it out!

@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 Jan 19, 2021
@camelid
Copy link
Member Author

camelid commented Jan 19, 2021

Should be ready for review! (Let me know before you r+ and I can squash.)

@GuillaumeGomez
Copy link
Member

Looks good to me. Just waiting for @jyn514 now. ;)

@bors
Copy link
Contributor

bors commented Jan 27, 2021

☔ The latest upstream changes (presumably #80987) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid
Copy link
Member Author

camelid commented Jan 28, 2021

I'm going to squash before I rebase since the conflicts are non-trivial (and it
makes sense to squash anyway).

@camelid camelid changed the title rustdoc: Fix bug when rendering function pointers with higher-ranked lifetimes rustdoc: Render HRTB correctly for bare functions Jan 28, 2021
@camelid
Copy link
Member Author

camelid commented Jan 28, 2021

Rebased!

@GuillaumeGomez Given that Joshua is taking a break for a bit, do you want to be
the reviewer for this? If not, I can wait until Joshua gets back, but I wanted
to check with you first :)

The angle brackets were not rendered, so code like this:

    some_func: for<'a> fn(val: &'a i32) -> i32

would be rendered as:

    some_func: fn'a(val: &'a i32) -> i32

However, rendering with angle brackets is still invalid syntax:

    some_func: fn<'a>(val: &'a i32) -> i32

so now it renders correctly as:

    some_func: for<'a> fn(val: &'a i32) -> i32

-----

However, note that this code:

    some_trait: dyn for<'a> Trait<'a>

will still render as:

    some_trait: dyn Trait<'a>

which is not invalid syntax, but is still unclear. Unfortunately I think
it's hard to fix that case because there isn't enough information in the
`rustdoc::clean::Type` that this code operates on. Perhaps that case can
be fixed in a later PR.
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks!

@bors: r=GuillaumeGomez,jyn514

@bors
Copy link
Contributor

bors commented Jan 28, 2021

📌 Commit cd8dcee has been approved by GuillaumeGomez,jyn514

@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 Jan 28, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 28, 2021
…llaumeGomez,jyn514

rustdoc: Render HRTB correctly for bare functions

The angle brackets were not rendered, so code like this:

    some_func: for<'a> fn(val: &'a i32) -> i32

would be rendered as:

    some_func: fn'a(val: &'a i32) -> i32

However, rendering with angle brackets is still invalid syntax:

    some_func: fn<'a>(val: &'a i32) -> i32

so now it renders correctly as:

    some_func: for<'a> fn(val: &'a i32) -> i32

-----

However, note that this code:

    some_trait: dyn for<'a> Trait<'a>

will still render as:

    some_trait: dyn Trait<'a>

which is not invalid syntax, but is still unclear. Unfortunately I think
it's hard to fix that case because there isn't enough information in the
`rustdoc::clean::Type` that this code operates on. Perhaps that case can
be fixed in a later PR.

r? `@jyn514`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#79570 (rustc: Stabilize `-Zrun-dsymutil` as `-Csplit-debuginfo`)
 - rust-lang#79819 (Add `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` lint)
 - rust-lang#79991 (rustdoc: Render HRTB correctly for bare functions)
 - rust-lang#80215 (Use -target when linking binaries for Mac Catalyst)
 - rust-lang#81158 (Point to span of upvar making closure FnMut)
 - rust-lang#81176 (Improve safety of `LateContext::qpath_res`)
 - rust-lang#81287 (Split rustdoc JSON types into separately versioned crate)
 - rust-lang#81306 (Fuse inner iterator in FlattenCompat and improve related tests)
 - rust-lang#81333 (clean up some const error reporting around promoteds)
 - rust-lang#81459 (Fix rustdoc text selection for page titles)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3eac643 into rust-lang:master Jan 29, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 29, 2021
@camelid camelid deleted the rustdoc-for-lifetime branch January 29, 2021 04:26
@Rustin170506
Copy link
Member

@camelid would this help with #78482 ?

@jyn514 No, I don't think so. This just fixes the rendering on bare functions: the HRT bounds aren't rendered at all on trait bounds or trait objects.

@camelid Do you have any suggestions for rendering it?

@jyn514
Copy link
Member

jyn514 commented Apr 5, 2021

@hi-rustin there are some instructions in #78482. Please try to keep discussion in one place, this PR is not super relevant to 78482.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system C-bug Category: This is a bug. 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.

8 participants