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: various cross-crate reexport fixes #103885

Merged

Conversation

fmease
Copy link
Member

@fmease fmease commented Nov 2, 2022

Fixes for various smaller cross-crate reexport issues.
The PR is split into several commits for easier review. Will be squashed after approval.

Most notable changes:

  • We finally render late-bound lifetimes in the generic parameter list of cross-crate functions & methods.
    Previously, we would display the re-export of pub fn f<'s>(x: &'s str) {} as pub fn f(x: &'s str)
  • We now render unnamed parameters of cross-crate functions and function pointers as underscores
    since that's exactly what we do for local definitions, too. Mentioned as a bug in Re-exported types have confusingly different documentation #44306.
  • From now on, the rendering of cross-crate trait-object types is more correct:
    • for<> parameter lists (for higher-ranked lifetimes) are now shown
    • the return type of Fn{,Mut,Once} trait bounds is now displayed

Regarding the last list item, here is a diff for visualization (before vs. after):

- dyn FnOnce(&'any str) + 'static
+ dyn for<'any> FnOnce(&'any str) -> bool + 'static

The redundant + 'static will be removed in a follow-up PR that will hide trait-object lifetime-bounds if they coincide with their default (see Zulip discussion). FIXME(fmease)s were added.

@rustbot label A-cross-crate-reexports
r? @GuillaumeGomez

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 2, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2022

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@rustbot rustbot added the A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate label Nov 2, 2022
@fmease fmease force-pushed the rustdoc-various-cross-crate-reexport-fixes branch from 5040540 to 3e4c6ad Compare November 2, 2022 15:34
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks for working on this. Let's wait for @cjgillot's approval too. Also I'd like to have @notriddle's confirmation too.

@fmease fmease force-pushed the rustdoc-various-cross-crate-reexport-fixes branch from 3e4c6ad to eda9cd7 Compare November 3, 2022 11:48
@fmease fmease force-pushed the rustdoc-various-cross-crate-reexport-fixes branch from eda9cd7 to 3b9ed4a Compare November 3, 2022 14:41
@@ -1677,6 +1701,9 @@ pub(crate) fn clean_middle_ty<'tcx>(

inline::record_extern_fqn(cx, did, ItemType::Trait);

// FIXME(fmease): Hide the trait-object lifetime bound if it coincides with its default
// to partially address #44306. Follow the rules outlined at
// https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an object lifetime default query, so you can just test equality.

Copy link
Member Author

@fmease fmease Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I am aware of the existence of tcx.object_lifetime_default but it's not the whole solution to the problem, it's only part of it.

The query doesn't take a Region, it takes the DefId of a type parameter of an item, e.g. the T of Box. Anyways, that's all fine and good and explained in the linked Zulip topic. I'd really prefer to implement that in a follow-up PR to avoid bitrotting, to make reviewing easier and since that logic will be a little bit more complicated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so regarding trait-object-type lifetime-bound defaults, my local work-in-progress patch is currently +168/-43 in size and it's not as straightforward as this PR meaning I won't add it to it.

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
@fmease fmease force-pushed the rustdoc-various-cross-crate-reexport-fixes branch from 3b9ed4a to 52f6ba6 Compare November 3, 2022 21:28
@bors
Copy link
Contributor

bors commented Nov 4, 2022

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

@fmease fmease force-pushed the rustdoc-various-cross-crate-reexport-fixes branch from 52f6ba6 to 5ccaed2 Compare November 4, 2022 19:12
@cjgillot
Copy link
Contributor

cjgillot commented Nov 6, 2022

@GuillaumeGomez LGTM on the use of rustc data structures.

@GuillaumeGomez
Copy link
Member

Thanks to both of you!

@bors r=cjgillot,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Nov 6, 2022

📌 Commit 5ccaed2 has been approved by cjgillot,GuillaumeGomez

It is now in the queue for this repository.

@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 Nov 6, 2022
JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Nov 6, 2022
…-reexport-fixes, r=cjgillot,GuillaumeGomez

rustdoc: various cross-crate reexport fixes

Fixes for various smaller cross-crate reexport issues.
The PR is split into several commits for easier review. Will be squashed after approval.

Most notable changes:

* We finally render late-bound lifetimes in the generic parameter list of cross-crate functions & methods.
  Previously, we would display the re-export of `pub fn f<'s>(x: &'s str) {}` as `pub fn f(x: &'s str)`
* We now render unnamed parameters of cross-crate functions and function pointers as underscores
  since that's exactly what we do for local definitions, too. Mentioned as a bug in rust-lang#44306.
* From now on, the rendering of cross-crate trait-object types is more correct:
  * `for<>` parameter lists (for higher-ranked lifetimes) are now shown
  * the return type of `Fn{,Mut,Once}` trait bounds is now displayed

Regarding the last list item, here is a diff for visualization (before vs. after):

```patch
- dyn FnOnce(&'any str) + 'static
+ dyn for<'any> FnOnce(&'any str) -> bool + 'static
```

The redundant `+ 'static` will be removed in a follow-up PR that will hide trait-object lifetime-bounds if they coincide with [their default](https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes) (see [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/clean_middle_ty.3A.20I.20need.20to.20add.20a.20parameter/near/307143097)). `FIXME(fmease)`s were added.

`@rustbot` label A-cross-crate-reexports
r? `@GuillaumeGomez`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 7, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#103885 (rustdoc: various cross-crate reexport fixes)
 - rust-lang#103914 (Make underscore_literal_suffix a hard error.)
 - rust-lang#104045 (Add type_array to BaseTypeMethods)
 - rust-lang#104056 (Vec: IntoIterator signature consistency)
 - rust-lang#104059 (Fix typo in `rustc_middle/lint.rs`)
 - rust-lang#104062 (rustdoc: remove unused CSS `#sidebar-filler`)
 - rust-lang#104065 (Migrate rust logo filter to CSS variables)
 - rust-lang#104066 (LLVM 16: Update RISCV data layout)
 - rust-lang#104074 (rustdoc: Add an example for round that is different from truncate)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 63f78d1 into rust-lang:master Nov 7, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 7, 2022
@fmease fmease deleted the rustdoc-various-cross-crate-reexport-fixes branch November 7, 2022 15:33
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2023
…r-obj-lt-bnds, r=notriddle,cgillot,GuillaumeGomez

rustdoc: re-elide cross-crate default trait-object lifetime bounds

Hide trait-object lifetime bounds (re-exported from an external crate) if they coincide with [their default](https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes).
Partially addresses rust-lang#44306. Follow-up to rust-lang#103885. [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/clean_middle_ty.3A.20I.20need.20to.20add.20a.20parameter/near/307143097).

Most notably, if `std` exported something from `core` containing a type like `Box<dyn Fn()>`, then it would now be rendered as `Box<dyn Fn(), Global>` instead of `Box<dyn Fn() + 'static, Global>` (hiding `+ 'static` as it is the default in this case). Showing `Global` here is a separate issue, rust-lang#80379, which is on my agenda.

Note that I am not really fond of the fact that I had to add a parameter to such a widely used function (30+ call sites) to address such a niche bug.

CC `@GuillaumeGomez`
Requesting a review from a compiler contributor or team member as recommended on Zulip.
r? compiler

---

`@rustbot` label T-compiler T-rustdoc A-cross-crate-reexports
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2023
…-args, r=<try>

rustdoc: elide cross-crate default generic arguments

Early draft. Requesting perf run. Thx :) CC `@GuillaumeGomez`

Elide cross-crate generic arguments if they coincide with their default.
TL;DR: Most notably, no more `Box<…, Global>` in `std`'s docs, just `Box<…>` from now on.
Fixes rust-lang#80379.

Also helps with rust-lang#44306. Follow-up to rust-lang#103885,rust-lang#107637.

`@rustbot` label T-rustdoc A-cross-crate-reexports S-experimental
r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2023
…-args, r=<try>

rustdoc: elide cross-crate default generic arguments

Elide cross-crate generic arguments if they coincide with their default.
TL;DR: Most notably, no more `Box<…, Global>` in `std`'s docs, just `Box<…>` from now on.
Fixes rust-lang#80379.

Also helps with rust-lang#44306. Follow-up to rust-lang#103885, rust-lang#107637.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2023
…-args, r=<try>

rustdoc: elide cross-crate default generic arguments

Elide cross-crate generic arguments if they coincide with their default.
TL;DR: Most notably, no more `Box<…, Global>` in `std`'s docs, just `Box<…>` from now on.
Fixes rust-lang#80379.

Also helps with rust-lang#44306. Follow-up to rust-lang#103885, rust-lang#107637.

r? `@ghost`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 30, 2023
…en-args, r=GuillaumeGomez

rustdoc: elide cross-crate default generic arguments

Elide cross-crate generic arguments if they coincide with their default.
TL;DR: Most notably, no more `Box<…, Global>` in `std`'s docs, just `Box<…>` from now on.
Fixes rust-lang#80379.

Also helps with rust-lang#44306. Follow-up to rust-lang#103885, rust-lang#107637.

r? `@ghost`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 30, 2023
…en-args, r=GuillaumeGomez

rustdoc: elide cross-crate default generic arguments

Elide cross-crate generic arguments if they coincide with their default.
TL;DR: Most notably, no more `Box<…, Global>` in `std`'s docs, just `Box<…>` from now on.
Fixes rust-lang#80379.

Also helps with rust-lang#44306. Follow-up to rust-lang#103885, rust-lang#107637.

r? ``@ghost``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2023
Rollup merge of rust-lang#112463 - fmease:rustdoc-elide-x-crate-def-gen-args, r=GuillaumeGomez

rustdoc: elide cross-crate default generic arguments

Elide cross-crate generic arguments if they coincide with their default.
TL;DR: Most notably, no more `Box<…, Global>` in `std`'s docs, just `Box<…>` from now on.
Fixes rust-lang#80379.

Also helps with rust-lang#44306. Follow-up to rust-lang#103885, rust-lang#107637.

r? ``@ghost``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate 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.

5 participants