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

Fix ICE in rustdoc when merging generic and where bounds of an Fn with an output #63937

Merged
merged 3 commits into from
Sep 27, 2019

Conversation

Nashenas88
Copy link
Contributor

Fixes #57180

@Nashenas88
Copy link
Contributor Author

r+ @QuietMisdreavus

@Nashenas88
Copy link
Contributor Author

Nashenas88 commented Aug 27, 2019

I still need to add the test, just not sure where to put it. I have a minimized case that shows this is resolved. Done

@Nashenas88
Copy link
Contributor Author

r? @QuietMisdreavus

}
_ => panic!("expected only type parameters"),
}
}
params
}

fn ty_bounds(bounds: Vec<clean::GenericBound>) -> Vec<clean::GenericBound> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed like pointless noise, so I removed it. If this was setup for future work, let me know so I can add a comment explaining that for future readers 😃

@@ -45,11 +45,6 @@ pub fn where_clauses(cx: &DocContext<'_>, clauses: Vec<WP>) -> Vec<WP> {
}
}

// Simplify the type parameter bounds on all the generics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since ty_bounds was a nop, just a pass-through, this transform seemed to just be added noise and should be safe to remove.

@Nashenas88
Copy link
Contributor Author

I was also able to determine that this bug was introduced somewhere between 1.30 and 1.31. The compiler processes the code just fine in 1.30, but ICE's in 1.31.

impl<F: Fn() -> u32>
Trait for Struct<F>
where
F: Fn() -> u32,
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 I run format on this? Also, does this test need a comment saying it's a regression test, or is its existence enough?

Copy link
Member

Choose a reason for hiding this comment

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

Existence alone is fine.

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2019
@JohnCSimon
Copy link
Member

Ping from triage
@QuietMisdreavus can you please review this?
Thank you.

@JohnCSimon
Copy link
Member

Ping from triage
@QuietMisdreavus @Nashenas88 This PR hasn't been updated in the last six days. Can you post any status?
Thank you.

@Nashenas88
Copy link
Contributor Author

I was doing some analysis on the originating cause (documented in #57180). I mostly believe this is still the proper fix, but I was hoping someone familiar with this codebase could review and help bump up the confidence here. I requested @QuietMisdreavus for review since she was helping me track down the bug during impl days.

@Alexendoo
Copy link
Member

Ping from triage: requesting a review from @rust-lang/rustdoc

@GuillaumeGomez
Copy link
Member

Since the real change is only one line, pretty quick to review. The regression test is also present so all good. Don't hesitate to ping me or @rust-lang/rustdoc directly next time.

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 26, 2019

📌 Commit 143b83a 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 Sep 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 27, 2019
…meGomez

 Fix ICE in rustdoc when merging generic and where bounds of an Fn with an output

Fixes rust-lang#57180
@bors
Copy link
Contributor

bors commented Sep 27, 2019

⌛ Testing commit 143b83a with merge a37fe2d...

bors added a commit that referenced this pull request Sep 27, 2019
 Fix ICE in rustdoc when merging generic and where bounds of an Fn with an output

Fixes #57180
@bors
Copy link
Contributor

bors commented Sep 27, 2019

☀️ Test successful - checks-azure
Approved by: GuillaumeGomez
Pushing a37fe2d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2019
@bors bors merged commit 143b83a into rust-lang:master Sep 27, 2019
@Nashenas88 Nashenas88 deleted the rustdoc_57180 branch September 30, 2019 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustdoc ICEs when a dependency has duplicate generic bounds to functions returning tuples
8 participants