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: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType #84867

Merged
merged 2 commits into from
Jun 15, 2021

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented May 3, 2021

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

Issue #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 #80653.

Reverting #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 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.

@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(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 May 3, 2021
@pnkfelix pnkfelix added beta-nominated Nominated for backporting to the compiler in the beta channel. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 3, 2021
@jyn514 jyn514 self-assigned this May 3, 2021
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

@pnkfelix can you also revert this on nightly? I don't feel comfortable having different behavior on beta, the underlying issue won't be fixed by the next release.

cc @GuillaumeGomez @jryans

fn as_ref(&self) -> &Local {
todo!()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this test case :)

@jyn514
Copy link
Member

jyn514 commented May 3, 2021

Oh, this is for nightly - I guess it needs to be backported to beta.

@jyn514
Copy link
Member

jyn514 commented May 3, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2021

📌 Commit 47d606fc82cedb4602ddcd284c1e37e73525176e has been approved by jyn514

@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 May 3, 2021
@jyn514
Copy link
Member

jyn514 commented May 3, 2021

Err I guess CI is failing - do you mind fixing it real quick?

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 3, 2021
@rust-log-analyzer

This comment has been minimized.

let inner_impl = target
.def_id_full(c)
let inner_impl = target.def_id_full(c);
debug!("target.def_id_full: {:?}", inner_impl);
Copy link
Member Author

Choose a reason for hiding this comment

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

(sigh, I wonder if this extra debugging lines were part of why this didn't backport cleanly...)

@pnkfelix
Copy link
Member Author

pnkfelix commented May 3, 2021

I'm unilaterally beta-accepting this. Even though its somewhat hand-crafted, its almost entirely based on eyeball review of PR #80653 , so I think it is in the spirit of the revert that the T-compiler team agreed upon in last week's triage meeting.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 3, 2021
@pnkfelix
Copy link
Member Author

pnkfelix commented May 3, 2021

the beta-targeted version is PR #84868

@pnkfelix
Copy link
Member Author

pnkfelix commented May 3, 2021

(looking at CI failure now)

@pnkfelix
Copy link
Member Author

pnkfelix commented May 3, 2021

oh. good old tidy. (I usually automatically run tidy, but I think there's some reason that I cannot run tidy when I'm targetting beta channel, at least when I do it and actually choose channel=beta in config.toml)

@pnkfelix
Copy link
Member Author

pnkfelix commented May 3, 2021

(this will give me a chance to remove the debug! lines I left in by accident. 😄 )

@pnkfelix pnkfelix force-pushed the rustdoc-revert-deref-recur branch from 47d606f to 86e3f76 Compare May 3, 2021 15:35
@pnkfelix
Copy link
Member Author

pnkfelix commented May 3, 2021

@bors r=jyn514

@bors
Copy link
Contributor

bors commented May 3, 2021

📌 Commit 86e3f76 has been approved by jyn514

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[  6%] Building CXX object lib/Core/CMakeFiles/lldCore.dir/DefinedAtom.cpp.o
[  7%] Building CXX object lib/Core/CMakeFiles/lldCore.dir/Error.cpp.o
[  8%] Building CXX object Common/CMakeFiles/lldCommon.dir/Args.cpp.o
error: Connection to server timed out
make[2]: *** [lib/Core/CMakeFiles/lldCore.dir/DefinedAtom.cpp.o] Error 2
make[2]: *** Waiting for unfinished jobs....
error: Connection to server timed out
error: Connection to server timed out
make[2]: *** [Common/CMakeFiles/lldCommon.dir/Args.cpp.o] Error 2
make[2]: *** Waiting for unfinished jobs....
error: Connection to server timed out
make[2]: *** [lib/Core/CMakeFiles/lldCore.dir/Error.cpp.o] Error 2
make[1]: *** [lib/Core/CMakeFiles/lldCore.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
clang-12: warning: argument unused during compilation: '-static-libstdc++' [-Wunused-command-line-argument]
make[1]: *** [Common/CMakeFiles/lldCommon.dir/all] Error 2
make: *** [all] Error 2
command did not execute successfully, got: exit status: 2


build script failed, must exit now', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.44/src/lib.rs:885:5
 finished in 33.146 seconds
failed to run: /Users/runner/work/rust/rust/build/bootstrap/debug/bootstrap dist
Build completed unsuccessfully in 0:44:56

@bors
Copy link
Contributor

bors commented Jun 14, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 14, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 14, 2021

@bors retry

@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 Jun 14, 2021
@bors
Copy link
Contributor

bors commented Jun 15, 2021

⌛ Testing commit b894f75 with merge d74b36e...

@CraftSpider
Copy link
Contributor

Sorry, meant to review this when you fixed the merge conflict, but it slipped through. Thanks joshua for handling this

@bors
Copy link
Contributor

bors commented Jun 15, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing d74b36e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2021
@bors bors merged commit d74b36e into rust-lang:master Jun 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 15, 2021
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.55.0, 1.54.0 Jun 17, 2021
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2021
…ulacrum

[beta] Bootstrap from stable

This is the follow up to master/beta promotion, as well as the first round of backports:

* Revert "Allow specifying alignment for functions rust-lang#81234"
* Revert rust-lang#85176 addition of clone_from for ManuallyDrop rust-lang#85758
* rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType rust-lang#84867
*  [beta] Update cargo rust-lang#86563

r? `@Mark-Simulacrum`
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. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.