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

Check predicates from blanket trait impls while testing if they apply #78683

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Nov 2, 2020

fixes #78673

@rust-highfive
Copy link
Collaborator

r? @ollie27

(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 Nov 2, 2020
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 2, 2020
@jyn514 jyn514 added the A-trait-system Area: Trait system label Nov 2, 2020
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 3, 2020

I've built the std docs before and after this change and diffed them, this results in these impls disappearing:

  • std::collections::BinaryHeap (and all other collections, Option, Result, mpsc::Receiver)
     impl<I> IntoIterator for I where I: Iterator
  • std::ffi::CStr (and OsStr, Path, slice, str):
    impl<T> ToOwned for T where T: Clone
  • char:
    impl<'a, F> Pattern<'a> for F where F: FnMut(char) -> bool

These all seem correct, so this isn't excluding any impls from std that shouldn't have been excluded.

@jyn514
Copy link
Member

jyn514 commented Nov 13, 2020

r? @lcnr

The rustdoc side of this looks fine, is the way Nemo is using evaluate_obligation going to cause ICEs?

@rust-highfive rust-highfive assigned lcnr and unassigned ollie27 Nov 13, 2020
@lcnr
Copy link
Contributor

lcnr commented Nov 13, 2020

I don't expect it to cause any ICE, though why do we drop the obligations from eq in line 59?

What happens if we try to evaluate them instead of the impl predicates? I guess cc @eddyb

@jyn514
Copy link
Member

jyn514 commented Nov 13, 2020

why do we drop the obligations from eq in line 59?

I'm not sure, but since that's pre-existing does it need to block this change?

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 13, 2020

why do we drop the obligations from eq in line 59?

That being wrong was my first suspicion too, but during my testing those obligations were always empty 🤷.

@lcnr
Copy link
Contributor

lcnr commented Nov 13, 2020

hmm, though a bit about this and it definitely makes sense to me.

@bors r+

about the obligations of the eq, afaict we try to equate ?0 = Foo<?T, ?U> which can fail if there are obligations on ?0 but probably just doesn't add any new obligations, so that might be fine. Would be interesting to assert that obligations is always empty in a followup PR to see where my thinking is incorrect 🤔 @Nemo157 do you have the time and are interested to try this out?

@bors
Copy link
Contributor

bors commented Nov 13, 2020

📌 Commit 0e2af5c has been approved by lcnr

@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 13, 2020
@bors
Copy link
Contributor

bors commented Nov 13, 2020

⌛ Testing commit 0e2af5c with merge b63d05a...

@bors
Copy link
Contributor

bors commented Nov 14, 2020

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing b63d05a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 14, 2020
@bors bors merged commit b63d05a into rust-lang:master Nov 14, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 14, 2020
@Nemo157 Nemo157 deleted the issue-78673 branch November 15, 2020 21:16
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 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. 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 shows inapplicable blanket implementations
7 participants