-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Refactor auto trait handling in librustdoc to be accessible from librustc. #49711
Conversation
Hmm, r? @nikomatsakis perhaps? |
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits, but the big one is a comment explaining the purpose of this function -- it's hard to review without that.
src/librustc/traits/auto_trait.rs
Outdated
trait_did: DefId, | ||
generics: &ty::Generics, | ||
auto_trait_callback: F) | ||
-> AutoTraitResult<A> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we rustfmt this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do!
src/librustc/traits/auto_trait.rs
Outdated
} | ||
|
||
pub struct AutoTraitFinder<'a, 'tcx: 'a> { | ||
pub tcx: &'a TyCtxt<'a, 'tcx, 'tcx>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is a leftover from a previous iteration. Good catch.
src/librustc/traits/auto_trait.rs
Outdated
generics: &ty::Generics, | ||
auto_trait_callback: F) | ||
-> AutoTraitResult<A> | ||
where F: for<'b, 'cx, 'cx2> Fn(&InferCtxt<'b, 'cx, 'cx2>, AutoTraitInfo<'cx2>) -> A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: using impl trait and '_
, this can be:
auto_trait_callback: impl for<'infcx> Fn(&InferCtxt<'_, 'tcx, 'infcx>, AutoTraitInfo<'infcx>) -> A
src/librustc/traits/auto_trait.rs
Outdated
} | ||
|
||
impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> { | ||
pub fn find_auto_trait_generics<A, F>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hard to review without understanding what it does...can you write a comment explaining the purpose of this function? Thanks! <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. To be honest, I am also quite unhappy with the actual implementation in this spot -- it is quite awkward (and hence awkward to describe). The idea is basically to offload some of the computation to the auto_trait_callback
(because it, for example, requires some types outside rustc) after the main work the function's equivalent in librustdoc performed without this changeset is done.
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
a189e95
to
6d11fe7
Compare
I think the new comment in the file somewhat goes in the right direction, but to be honest, I find it hard making it more precise. |
@ibabushkin Looks like you may have used an older version of rustfmt. Would you mind reformatting the file with the latest version? FWIW |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@topecongiro That's what happens when I run |
@ibabushkin Thanks, that is interesting...would you mind checking the version of rustfmt? It should be |
@topecongiro Surprisingly, a rustfmt installed/updated via
However, I suggest we move this discussion somewhere else, as this somewhat derails the discussion in this issue :) |
@ibabushkin Ok, looks like you already have My appologies for derailing the discussion in this issue. |
6e8227d
to
0c735a6
Compare
#[derive(Eq, PartialEq, Hash, Copy, Clone, Debug)] | ||
pub enum RegionTarget<'tcx> { | ||
Region(Region<'tcx>), | ||
RegionVid(RegionVid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this exist, given that Region
has a case for inference variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following enum is defined in librustdoc, and used in the code I moved over:
#[derive(Eq, PartialEq, Hash, Copy, Clone, Debug)]
enum RegionTarget<'tcx> {
Region(Region<'tcx>),
RegionVid(RegionVid)
}
I did not find any explanation or reasoning there, so I duplicated the enumeration (as stated in the TODO comment). I suppose we could eliminate this type altogether given what you said about Region
, but that would require further testing. Maybe @Aaron1011 can tell us more about the bits of code here that use this type.
So I have sort of a meta question: Is the goal here to just kind of "rubber stamp" moving some code out of rustdoc into rustc, or to really design a nice API? If the latter, obviously, I'd want to think over more carefully what's going on here. If the former, we can add some comments like "stay away from this code" and land with lighter review, I imagine. Overall, I do want to add more stable APIs for working with traits that consumers like rustdoc etc can use. This particular API is going in precisely the opposite direction of what I had in mind, though, in terms of exposing internal concepts like vtable that I'd like to remove. (It's kind of interesting, though, since many of the concepts I'd like to remove are basically those that rustdoc wants -- i.e., those that are tightly tied to the source code structure.) |
Well, the point you bring up is an important one -- I initially was working on adding support for auto traits to To conclude: I agree that a properly designed refactor for this machinery would be much nicer, but that would come at a cost of a time penalty for what I was working on, and require some work (I'd love to help, but I sadly am somewhat short on time). |
OK, I skimmed in a bit more depth. I think I see now what this code is trying to do -- it seems like it's trying to find, if there is an auto trait impl, what the transitive conditions are on this impl and report those back to the user? That's interesting. Is there an example of the resulting output? (Also, I was wrong to say it exposes |
As far as I am aware, auto trait instances are being generated in rustdoc output. I've not seen examples of that however (and couldn't find any right now). The original PR shows the functionality in a quite detailed way though. There is also a rust-semverver branch which uses some of the functionality in a rudimentary way. Regarding the inner working of the code: Yes, your description pretty much coincides with my mental model of it. |
☔ The latest upstream changes (presumably #49939) made this pull request unmergeable. Please resolve the merge conflicts. |
0c735a6
to
c35f334
Compare
Do we have a definite conclusion on this? Any further changes needed? A proper, clean rewrite from scratch of the mechanics underneath? |
@ibabushkin: I'm sorry about that - I should have taken a closer look at what work would be required before telling you that. I'd be happy to help out with this in any way I can, especially if parts of my code are unclear or poorly written. |
On 2018-04-21, Aaron Hill wrote:
@ibabushkin: I'm sorry about that - I should have taken a closer look at what work would be required before telling you that.
I'd be happy to help out with this in any way I can, especially if parts of my code are unclear or poorly written.
Well, I'd be glad to take part in a "clean" rewrite of the functionality
in librustc if this current PR still causes too many headaches. No
feedback yet on the definite state of things.
|
OK. So, I'm of mixed minds here. First off, a question: What does this code do for a type like this: struct Foo<T: Iterator> {
data: <T as Iterator>::Item
} i.e., what where clauses does it give back? In general, the "always correct" answer for send impls is actually way simpler than what this code does. The correct answer is just to enumerate the field types. So for
This is possible only because I don't really feel like I have the bandwidth to think carefully about whether this API is the kind we would like to support longer term. I suspect something like it would be fine, but we'd want to make changes. I think I'm inclined therefore to r+ this, since it's basically just moving existing code around it, but with the warning that as the work on chalk etc integration proceeds we may want to make radical changes to the API. |
I specifically wanted to know how you felt about that =) |
I'm in the same boat there -- the API as-is, after the move, certainly isn't pretty. Given that only two consumers of it are to be expected in the foreseeable future, and that I'd like to get back to this at some point in the future, I am totally fine with it being a temporary state of things. |
Am I mistaken or are the build failures unrelated to the changes? |
@ibabushkin you'd think so... but it seems repeatable. =) |
Huh, re-reading the code I can't really see how those failures could come about. It feels like they have to be spurious. |
@bors retry |
@bors r- -- well, let me check |
Ping from triage, @nikomatsakis, any updates? |
Argh I forgot I had left this on that note. No, I've no idea how this could be anything but spurious. Shall we give it another try? |
@bors r+ |
📌 Commit 8901392 has been approved by |
Refactor auto trait handling in librustdoc to be accessible from librustc. These commits transfer some of the functionality introduced in #47833 to librustc with the intention of making the tools to work with auto traits accessible to third-party code, for example [rust-semverver](https://github.com/rust-lang-nursery/rust-semverver). Some rough edges remain, and I'm certain some of the FIXMEs introduced will need some discussion, most notably the fairly ugly overall approach to pull out the core logic into librustc, which was previously fairly tightly coupled with various bits and bobs from librustdoc. cc @Aaron1011
☀️ Test successful - status-appveyor, status-travis |
…mpl-synth, r=<try> rustdoc: heavily simplify the synthesis of auto trait impls `gd --numstat HEAD~2 HEAD src/librustdoc/clean/auto_trait.rs` **+315 -705** 🟩🟥🟥🟥⬛ --- As outlined in issue rust-lang#113015, there are currently 3[^1] large separate routines that “clean” `rustc_middle::ty` data types related to generics & predicates to rustdoc data types. Every single one has their own kinds of bugs. While I've patched a lot of bugs in each of the routines in the past, it's about time to unify them. This PR is only the first in a series. It completely *yanks* the custom “bounds cleaning” of mod `auto_trait` and reuses the routines found in mod `simplify`. As mentioned, `simplify` is also flawed but still it's more complete than `auto_trait`'s routines. See also my review comment over at `tests/rustdoc/synthetic_auto/bounds.rs`. This is preparatory work for rewriting “bounds cleaning” in follow-up PRs in order to finally [fix] rust-lang#113015. Apart from that, I've eliminated all potential sources of *unstable rendering output*. See also rust-lang#119597. I'm pretty sure this fixes rust-lang#119597. This PR does not attempt to fix [any other issues related to synthetic auto trait impls](https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AA-synthetic-impls%20label%3AA-auto-traits). However, it's definitely meant to be a *stepping stone* by making `auto_trait` more contributor friendly. --- * Replace `FxHash{Map,Set}` with `FxIndex{Map,Set}` to guarantee a stable / more predictable iteration order * Or as a perf opt, `UnordSet` (a thin wrapper around `FxHashSet`) in cases where we never iterate over the set. * Remove the types `RegionTarget` and `RegionDeps` from `librustdoc`. They were duplicates of the identical types found in `rustc`. Just import them from `rustc`. For some reason, they were duplicated when splitting `auto_trait` in two in rust-lang#49711. * Get rid of the useless “type namespace” `AutoTraitFinder` in `librustdoc` * The struct only held a `DocContext`, it was over-engineered * Turn the associated functions into free ones * Eliminates rightward drift; increases legibility * `rustc` also contains a useless `AutoTraitFinder` struct but I plan on removing that in a follow-up PR * Rename a bunch of methods to be way more descriptive * Eliminate `use super::*;` * Lead to `clean/mod.rs` accumulating a lot of unnecessary imports and therefore * made `auto_traits` less modular I plan on adding extensive documentation to `librustdoc`'s `auto_trait` in follow-up PRs. I don't want to do that in this PR because further refactoring & bug fix PRs may alter the overall structure of `librustdoc`'s & `rustc`'s `auto_trait` modules to a great degree. I'm slowly digging into the dark details of `rustc`'s `auto_trait` module again and once I have the full picture I will be able to provide proper docs. --- While this PR does indeed touch `rustc`'s `auto_trait` — mostly tiny refactorings — I argue this PR doesn't need any compiler reviewers next to rustdoc ones since that module falls under the purview of rustdoc — it used to be part of `librustdoc` after all (rust-lang#49711). Sorry for not having split this into more commits. If you'd like me to I can try to retroactively separate it into more atomic commits. However, I don't know if that would actually make reviewing this PR easier. r? `@GuillaumeGomez` [^1]: Or even 4 depending on the way you're counting.
…mpl-synth, r=GuillaumeGomez rustdoc: heavily simplify the synthesis of auto trait impls `gd --numstat HEAD~2 HEAD src/librustdoc/clean/auto_trait.rs` **+315 -705** 🟩🟥🟥🟥⬛ --- As outlined in issue rust-lang#113015, there are currently 3[^1] large separate routines that “clean” `rustc_middle::ty` data types related to generics & predicates to rustdoc data types. Every single one has their own kinds of bugs. While I've patched a lot of bugs in each of the routines in the past, it's about time to unify them. This PR is only the first in a series. It completely **yanks** the custom “bounds cleaning” of mod `auto_trait` and reuses the routines found in mod `simplify`. As alluded to, `simplify` is also flawed but it's still more complete than `auto_trait`'s routines. [See also my review comment over at `tests/rustdoc/synthetic_auto/bounds.rs`](rust-lang#123340 (comment)). This is preparatory work for rewriting “bounds cleaning” from scratch in follow-up PRs in order to finally [fix] rust-lang#113015. Apart from that, I've eliminated all potential sources of *instability* in the rendered output. See also rust-lang#119597. I'm pretty sure this fixes rust-lang#119597. This PR does not attempt to fix [any other issues related to synthetic auto trait impls](https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AA-synthetic-impls%20label%3AA-auto-traits). However, it's definitely meant to be a *stepping stone* by making `auto_trait` more contributor-friendly. --- * Replace `FxHash{Map,Set}` with `FxIndex{Map,Set}` to guarantee a stable iteration order * Or as a perf opt, `UnordSet` (a thin wrapper around `FxHashSet`) in cases where we never iterate over the set. * Yes, we do make use of `swap_remove` but that shouldn't matter since all the callers are deterministic. It does make the output less “predictable” but it's still better than before. Ofc, I rely on `rustc_infer` being deterministic. I hope that holds. * Utilizing `clean::simplify` over the custom “bounds cleaning” routines wipes out the last reference to `collect_referenced_late_bound_regions` in rustdoc (`simplify` uses `bound_vars`) which was a source of instability / unpredictability (cc rust-lang#116388) * Remove the types `RegionTarget` and `RegionDeps` from `librustdoc`. They were duplicates of the identical types found in `rustc`. Just import them from `rustc`. For some reason, they were duplicated when splitting `auto_trait` in two in rust-lang#49711. * Get rid of the useless “type namespace” `AutoTraitFinder` in `librustdoc` * The struct only held a `DocContext`, it was over-engineered * Turn the associated functions into free ones * Eliminates rightward drift; increases legibility * `rustc` also contains a useless `AutoTraitFinder` struct but I plan on removing that in a follow-up PR * Rename a bunch of methods to be way more descriptive * Eliminate `use super::*;` * Lead to `clean/mod.rs` accumulating a lot of unnecessary imports * Made `auto_traits` less modular * Eliminate a custom `TypeFolder`: We can just use the rustc helper `fold_regions` which does that for us I plan on adding extensive documentation to `librustdoc`'s `auto_trait` in follow-up PRs. I don't want to do that in this PR because further refactoring & bug fix PRs may alter the overall structure of `librustdoc`'s & `rustc`'s `auto_trait` modules to a great degree. I'm slowly digging into the dark details of `rustc`'s `auto_trait` module again and once I have the full picture I will be able to provide proper docs. --- While this PR does indeed touch `rustc`'s `auto_trait` — mostly tiny refactorings — I argue this PR doesn't need any compiler reviewers next to rustdoc ones since that module falls under the purview of rustdoc — it used to be part of `librustdoc` after all (rust-lang#49711). Sorry for not having split this into more commits. If you'd like me to I can try to split it into more atomic commits retroactively. However, I don't know if that would actually make reviewing easier. I think the best way to review this might just be to place the master version of `auto_trait` on the left of your screen and the patched one on the right, not joking. r? `@GuillaumeGomez` [^1]: Or even 4 depending on the way you're counting.
These commits transfer some of the functionality introduced in #47833 to librustc with the intention of making the tools to work with auto traits accessible to third-party code, for example rust-semverver.
Some rough edges remain, and I'm certain some of the FIXMEs introduced will need some discussion, most notably the fairly ugly overall approach to pull out the core logic into librustc, which was previously fairly tightly coupled with various bits and bobs from librustdoc.
cc @Aaron1011