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: Correctly merge import's and its target's docs in one more case #109266

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Mar 17, 2023

Fixes #108061.
Fixes #108334.
Fixes #108378.
Fixes #108658.

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2023

r? @notriddle

(rustbot has picked a reviewer for you, use r? to override)

@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 Mar 17, 2023
@clubby789
Copy link
Contributor

Also #108658, #108501, and #108658 I believe

@petrochenkov
Copy link
Contributor Author

I'll check #108658 now.

#108501 is different and is not fixed by this PR, but I'm also investigating it now.

@@ -2373,21 +2374,22 @@ fn clean_maybe_renamed_item<'tcx>(
_ => unreachable!("not yet converted"),
};

let mut extra_attrs = Vec::new();
let mut import_attrs = Vec::new();
Copy link
Member

@GuillaumeGomez GuillaumeGomez Mar 17, 2023

Choose a reason for hiding this comment

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

I'd nitpick to rename it into imports_attrs but feel free to ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are multiple imports there, then there should be multiple import_ids and their parents, so there's still a lot to fix here besides the naming.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure we're talking about this same thing:

mod foo {
    struct A;

    pub use self::A as B;
    pub use self::B as C;
}
pub use foo::C as D;

Intermediate imports are B and C and they are the ones we're gathering attributes from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and their attributes should not be all merged together, but should instead be broken into groups with a separate group with a separate parent id for every use item in the reexport chain.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have cases where it would be problematic by any chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the test in this PR, but with a doc link on an intermediate import.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm gonna send a fix for that too in the next days. Thanks for fixing this bug already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how many places in rustdoc merge attributes from different items, but all of them should ideally apply the same fix - a separate parent per item.

@GuillaumeGomez
Copy link
Member

Apart from my nitpick, looks all good to me, thanks! r=me if you don't want to fix the nitpicking.

@petrochenkov
Copy link
Contributor Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2023

📌 Commit 5e0fc04 has been approved by petrochenkov

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 Mar 17, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 17, 2023
rustdoc: Correctly merge import's and its target's docs in one more case

Fixes rust-lang#108334.
Fixes rust-lang#108378.
Fixes rust-lang#108658.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#109170 (Set `CMAKE_SYSTEM_NAME` for Linux targets)
 - rust-lang#109266 (rustdoc: Correctly merge import's and its target's docs in one more case)
 - rust-lang#109267 (Add tests for configure.py)
 - rust-lang#109273 (Make `slice::is_sorted_by` implementation nicer)
 - rust-lang#109277 (Fix generics_of for impl's RPITIT synthesized associated type)
 - rust-lang#109307 (Ignore `Inlined` spans when computing caller location.)
 - rust-lang#109364 (Only expect a GAT const param for `type_of` of GAT const arg)
 - rust-lang#109365 (Update mdbook)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0e8085a into rust-lang:master Mar 20, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 20, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 27, 2023
…rt-intra-doc-ice, r=petrochenkov

rustdoc: Fix ICE for intra-doc link on intermediate re-export

Fixes rust-lang#109282.

This PR is based on rust-lang#109266 as it includes its commit to make this work.

`@petrochenkov:` It was exactly as you predicted, adding the `DefId` to the attributes fixed the error for intermediate re-exports as well. Thanks a lot!

r? `@petrochenkov`
@jyn514 jyn514 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 10, 2023
@compiler-errors
Copy link
Member

Adding context to the beta nomination -- @kpreid asked for this to be nominated because they hit the problem on beta (or so it seems, and linked to these logs: https://github.com/kpreid/exhaust/actions/runs/4653637162/jobs/8234695803?pr=15).

@petrochenkov, do you know when the regression fixed by this PR was originally introduced into the compiler? That's probably useful information in helping pinpoint whether this should be backported or if there's another fix that needs to be backported to fix @kpreid's problem instead.

@petrochenkov
Copy link
Contributor Author

@compiler-errors

do you know when the regression fixed by this PR was originally introduced into the compiler?

In #94857.
It's not exactly a regression, but rather a silent incorrect behavior turning into ICE.

@apiraino
Copy link
Contributor

I am approving the beta backport by looking at the number of issues it fixes. The beta cut is tomorrow IIRC, though (so not a lot of time).

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 13, 2023
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2023
…troalbini

[stable] Prepare Rust 1.69.0

Last minute backports:

*  rust-lang#109643
* rust-lang#110135
* rust-lang#109938
* rust-lang#109937
* rust-lang#109266

This PR also bumps the channel to stable, and backports the release notes from master.

r? `@ghost`
cc `@rust-lang/release`
tarcieri added a commit to rustsec/rustsec that referenced this pull request Apr 24, 2023
Workaround for rust-lang/rust#109266

This pins the docs build to MSRV, which should prevent regressions where
new releases break the docs build.
tarcieri added a commit to rustsec/rustsec that referenced this pull request Apr 24, 2023
Workaround for rust-lang/rust#109266

This pins the docs build to MSRV, which should prevent regressions where
new releases break the docs build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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
10 participants