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: Fix inconsistency of local blanket impls #93169

Merged
merged 3 commits into from
Jan 25, 2022

Conversation

CraftSpider
Copy link
Contributor

@CraftSpider CraftSpider commented Jan 21, 2022

When a blanket impl is local, go through HIR instead of middle. This fixes inconsistencies with data detected during JSON generation.

Expected this change to take longer. I also tried doing the whole item through existing clean architecture, but it didn't work out trivially, and felt like it would have added more complexity than it removed.

Properly fixes #83718

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 21, 2022
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(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 Jan 21, 2022
@GuillaumeGomez
Copy link
Member

Changes look good to me. Let's run a perf-check just in case:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2022
@bors
Copy link
Contributor

bors commented Jan 22, 2022

⌛ Trying commit 66d056a with merge 618360ac8cfc7538d9c2b2dc32ae7773ec3dd58b...

@bors
Copy link
Contributor

bors commented Jan 22, 2022

☀️ Try build successful - checks-actions
Build commit: 618360ac8cfc7538d9c2b2dc32ae7773ec3dd58b (618360ac8cfc7538d9c2b2dc32ae7773ec3dd58b)

@rust-timer
Copy link
Collaborator

Queued 618360ac8cfc7538d9c2b2dc32ae7773ec3dd58b with parent ecf7299, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (618360ac8cfc7538d9c2b2dc32ae7773ec3dd58b): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2022
@@ -4,10 +4,12 @@
// @has blanket_with_local.json "$.index[*][?(@.name=='Load')]"
pub trait Load {
fn load() {}
fn write(self) {}
Copy link
Member

Choose a reason for hiding this comment

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

Since you added this method, shouldn't there be a matching check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably add a check for both the methods. The checks are more to ensure the methods exist in the output, so we know we're testing the ICE correctly.

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 23, 2022

📌 Commit 9220631 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 Jan 23, 2022
@jackh726
Copy link
Member

There were no perf effects:

@bors rollup=maybe

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2022
…ency, r=GuillaumeGomez

Fix inconsistency of local blanket impls

When a blanket impl is local, go through HIR instead of middle. This fixes inconsistencies with data detected during JSON generation.

Expected this change to take longer. I also tried doing the whole item through existing clean architecture, but it didn't work out trivially, and felt like it would have added more complexity than it removed.

Properly fixes rust-lang#83718
This was referenced Jan 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88794 (Add a `try_clone()` function to `OwnedFd`.)
 - rust-lang#93064 (Properly track `DepNode`s in trait evaluation provisional cache)
 - rust-lang#93118 (Move param count error emission to end of `check_argument_types`)
 - rust-lang#93144 (Work around missing code coverage data causing llvm-cov failures)
 - rust-lang#93169 (Fix inconsistency of local blanket impls)
 - rust-lang#93175 (Implement stable overlap check considering negative traits)
 - rust-lang#93251 (rustdoc settings: use radio buttons for theme)
 - rust-lang#93269 (Use error-on-mismatch policy for PAuth module flags.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 677126c into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
@pnkfelix pnkfelix changed the title Fix inconsistency of local blanket impls rustdoc: Fix inconsistency of local blanket impls Jan 26, 2022
@CraftSpider CraftSpider deleted the rustdoc-clean-inconsistency branch February 23, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-json] Assertion error on blanket impls
7 participants