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 drops impl Foreign<Local> for Foreign #82465

Closed
guswynn opened this issue Feb 24, 2021 · 11 comments
Closed

rustdoc drops impl Foreign<Local> for Foreign #82465

guswynn opened this issue Feb 24, 2021 · 11 comments
Assignees
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Milestone

Comments

@guswynn
Copy link
Contributor

guswynn commented Feb 24, 2021

I tried this code:

use std::convert::AsRef;
pub struct Gus;

impl AsRef<str> for Gus {
    fn as_ref(&self) -> &str {
        todo!()
    }
}

impl AsRef<Gus> for str {
    fn as_ref(&self) -> &Gus {
        todo!()
    }
}

with cargo doc I get:
image

with cargo +nightly I get:
image

Meta

stable: rustc 1.50.0 (cb75ad5db 2021-02-10)
nightly: rustc 1.52.0-nightly (d1206f950 2021-02-15) but this also affects whatever nightly docs.rs is on (https://docs.rs/camino/1.0.0/camino/struct.Utf8Path.html#impl-AsRef%3Cstr%3E is missing the for str impl

(separately, I hit rust-lang/cargo#6783 when testing this, where cargo +stable doc --open still used the nightly generated code)

@guswynn guswynn added the C-bug Category: This is a bug. label Feb 24, 2021
@guswynn guswynn changed the title rustdoc drops impl Foreign<Local> for Foreign in nightly channel(which affects docs.rs) rustdoc drops impl Foreign<Local> for Foreign in nightly channel (which affects docs.rs) Feb 24, 2021
@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Feb 24, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 24, 2021
@jyn514 jyn514 added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Feb 24, 2021
@guswynn
Copy link
Contributor Author

guswynn commented Feb 24, 2021

this also appears to affect beta:
image

@jyn514 jyn514 added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Feb 24, 2021
@jyn514 jyn514 changed the title rustdoc drops impl Foreign<Local> for Foreign in nightly channel (which affects docs.rs) rustdoc drops impl Foreign<Local> for Foreign Feb 24, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 24, 2021

nightly: rustc 1.52.0-nightly (d1206f9 2021-02-15) but this also affects whatever nightly docs.rs is on (https://docs.rs/camino/1.0.0/camino/struct.Utf8Path.html#impl-AsRef%3Cstr%3E is missing the for str impl

That's rustc 1.52.0-nightly (a15f484b9 2021-02-22), so your nightly is actually earlier.

@apiraino
Copy link
Contributor

apiraino commented Feb 24, 2021

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 24, 2021
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Feb 24, 2021

The commit responsible is this one: 937f629 from #80653

EDIT: For future reference: I was able to find it using cargo-bisect on a "foo" project I created (cargo new foo --lib), containing the file provided in the first comment as lib.rs.

Then I created a test.sh file containing:

#!/bin/sh
rm -rf target-bisect*; cargo doc;grep '"impl-AsRef%3CGus%3E' ` find . -name struct.Gus.html`

Then you just need to run (in your "foo" folder):

$ cargo bisect-rustc --script=./test.sh --test-dir=`pwd`

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Feb 24, 2021

Actually, I'm wondering about something: why should we see impl AsRef<Gus> for str on the Gus page? It's not implemented on Gus after all... (original issue remains though!)

@guswynn
Copy link
Contributor Author

guswynn commented Feb 24, 2021

@GuillaumeGomez because it's both 1. An api that the crate creates (the way I found this was I saw a str.as_ref call in some code I was reading in the camino crate imma fn that returned a &Utf8Path, that public api was not exposed on doc.rs) and 2. Can't be on the docs for the foreign type (as this impl is in the local crate)

This is definitely a really specific case of coherence, but it would be a downgrade to lose this in docs

@pnkfelix
Copy link
Member

Discussed in T-compiler triage meeting. We're going to revert PR #80653 as a way to fix this in the short-term, while the kinks get worked out in the appropriate fix to get this all working (with #80653) again.

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 3, 2021
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue May 3, 2021
…ng#82465.

adapted from 513756bb55a0dbc6e74d0043afd1727bd3c73aae
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…, r=jyn514

rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType

As discussed here: rust-lang#82465 (comment), Revert PR rust-lang#80653 to resolve issue rust-lang#82465.

Issue rust-lang#82465 was we had stopped including certain trait implementations, namely implementations on an imported type of an imported trait *instantiated on a local type*. That bug was injected by PR rust-lang#80653.

Reverting rust-lang#80653 means we don't list all the methods that you have accessible via recursively applying `Deref`.

[Discussion in last week's rustc triage meeting](https://zulip-archive.rust-lang.org/238009tcompilermeetings/19557weekly2021042954818.html#236680594) led us to conclude that the bug was worse than the enhancement, and there was not an obvious fix for the bug itself. So for the short term we  remove the enhancement, while in the long term we will work on figuring out a way to have our imported trait implementation cake and eat it too.
pietroalbini pushed a commit to pietroalbini/rust that referenced this issue May 4, 2021
@pietroalbini pietroalbini added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 4, 2021
@pietroalbini pietroalbini modified the milestones: 1.52.0, 1.54.0 May 4, 2021
@pietroalbini
Copy link
Member

A revert for this was backported in both stable 1.52.0 and beta 1.53.0, so it only affects nightly 1.54.0 now. Updating the labels accordingly.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…, r=jyn514

rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType

As discussed here: rust-lang#82465 (comment), Revert PR rust-lang#80653 to resolve issue rust-lang#82465.

Issue rust-lang#82465 was we had stopped including certain trait implementations, namely implementations on an imported type of an imported trait *instantiated on a local type*. That bug was injected by PR rust-lang#80653.

Reverting rust-lang#80653 means we don't list all the methods that you have accessible via recursively applying `Deref`.

[Discussion in last week's rustc triage meeting](https://zulip-archive.rust-lang.org/238009tcompilermeetings/19557weekly2021042954818.html#236680594) led us to conclude that the bug was worse than the enhancement, and there was not an obvious fix for the bug itself. So for the short term we  remove the enhancement, while in the long term we will work on figuring out a way to have our imported trait implementation cake and eat it too.
@jyn514
Copy link
Member

jyn514 commented May 4, 2021

it only affects nightly 1.54.0 now

The revert should also have been applied to nightly - is #84867 only landing on 1.53? It should land on 1.54 too if so.

@pietroalbini
Copy link
Member

@jyn514 well that PR didn't land on 1.54 yet :)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…, r=jyn514

rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType

As discussed here: rust-lang#82465 (comment), Revert PR rust-lang#80653 to resolve issue rust-lang#82465.

Issue rust-lang#82465 was we had stopped including certain trait implementations, namely implementations on an imported type of an imported trait *instantiated on a local type*. That bug was injected by PR rust-lang#80653.

Reverting rust-lang#80653 means we don't list all the methods that you have accessible via recursively applying `Deref`.

[Discussion in last week's rustc triage meeting](https://zulip-archive.rust-lang.org/238009tcompilermeetings/19557weekly2021042954818.html#236680594) led us to conclude that the bug was worse than the enhancement, and there was not an obvious fix for the bug itself. So for the short term we  remove the enhancement, while in the long term we will work on figuring out a way to have our imported trait implementation cake and eat it too.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 4, 2021
…, r=jyn514

rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType

As discussed here: rust-lang#82465 (comment), Revert PR rust-lang#80653 to resolve issue rust-lang#82465.

Issue rust-lang#82465 was we had stopped including certain trait implementations, namely implementations on an imported type of an imported trait *instantiated on a local type*. That bug was injected by PR rust-lang#80653.

Reverting rust-lang#80653 means we don't list all the methods that you have accessible via recursively applying `Deref`.

[Discussion in last week's rustc triage meeting](https://zulip-archive.rust-lang.org/238009tcompilermeetings/19557weekly2021042954818.html#236680594) led us to conclude that the bug was worse than the enhancement, and there was not an obvious fix for the bug itself. So for the short term we  remove the enhancement, while in the long term we will work on figuring out a way to have our imported trait implementation cake and eat it too.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Jun 10, 2021
…lang#82465.

(update: placated tidy)
(update: rebased post PR rust-lang#84707 )

merge me
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 15, 2021
…r=jyn514

rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType

As discussed here: rust-lang#82465 (comment), Revert PR rust-lang#80653 to resolve issue rust-lang#82465.

Issue rust-lang#82465 was we had stopped including certain trait implementations, namely implementations on an imported type of an imported trait *instantiated on a local type*. That bug was injected by PR rust-lang#80653.

Reverting rust-lang#80653 means we don't list all the methods that you have accessible via recursively applying `Deref`.

[Discussion in last week's rustc triage meeting](https://zulip-archive.rust-lang.org/238009tcompilermeetings/19557weekly2021042954818.html#236680594) led us to conclude that the bug was worse than the enhancement, and there was not an obvious fix for the bug itself. So for the short term we  remove the enhancement, while in the long term we will work on figuring out a way to have our imported trait implementation cake and eat it too.
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Jun 19, 2021
@pietroalbini
Copy link
Member

This has been fixed by #84867 on 1.54 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants