Fix deref impl typedef #68093
Merged
Fix deref impl typedef #68093
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GuillaumeGomez
force-pushed
the
fix-deref-impl-typedef
branch
2 times, most recently
from
360624b
to
bd734fc
Compare
|
I like that the unwraps have transitioned to expects. |
JohnTitor
added
the
S-waiting-on-review
|
@dns2utf8 This is a side-effect of my debugging (and I hate |
ollie27
reviewed
Jan 13, 2020
There was a problem hiding this comment.
This is now only including impls for the inner item_type but we actually need the impls for the typedef itself as well. Take for example:
pub struct FooA;
pub type FooB = FooA;
pub type FooC = FooB;
impl FooA {
pub fn foo_a(&self) {}
}
impl FooB {
pub fn foo_b(&self) {}
}
impl FooC {
pub fn foo_c(&self) {}
}
pub struct Bar;
impl std::ops::Deref for Bar {
type Target = FooC;
fn deref(&self) -> &Self::Target { unimplemented!() }
}Currently the docs for Bar show only method foo_c but with this PR it shows only foo_a but what we really need is all of foo_a, foo_b and foo_c to be included.
I'm not sure of the best way to do that but maybe Typedef could contain a Vec<Type> or Vec<DefId> containing each inner type rather than just a single Option<Type>.
src/librustdoc/html/render/cache.rs
Outdated
| if item.name.is_none() { | ||
| // this is most likely from a typedef | ||
| continue; | ||
| } |
There was a problem hiding this comment.
What is this change for? I can't see anything in this PR that would require this.
There was a problem hiding this comment.
It's not needed anymore, indeed! It was part of my debugging.
src/librustdoc/html/render/cache.rs
Outdated
| crate_paths.push((short, fqp.last().expect("no fqp").clone())); | ||
| } else { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Again, what is this change for?
|
@ollie27 This is a great point: I didn't think about checking the type alias itself for implementations. I'll need to update the PR to extend it a bit. |
kinnison
reviewed
Jan 14, 2020
There was a problem hiding this comment.
I also appreciate the .unwrap() -> .expect("...") changes, and other than wondering about the same points as Ollie did in cache.rs I think this is looking quite OK.
I wonder if, in the process of removing that additional debugging code you might fold the formatting fixes into the requisite commits though, to keep the history a bit cleaner?
kinnison
added
S-waiting-on-author
GuillaumeGomez
force-pushed
the
fix-deref-impl-typedef
branch
from
90e0a90
to
d66457a
Compare
|
The job 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 |
GuillaumeGomez
force-pushed
the
fix-deref-impl-typedef
branch
from
d66457a
to
e6ad49a
Compare
|
With the last changes:
So we currently have |
|
The job 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 |
GuillaumeGomez
force-pushed
the
fix-deref-impl-typedef
branch
from
3da3171
to
8a9b951
Compare
|
Ok, fixed all the issues encountered. It now returns all implementations made on the type aliases. |
oli-obk
reviewed
Jan 16, 2020
src/librustdoc/clean/mod.rs
Outdated
| Some(DefKind::TyAlias) => Some(cx.tcx.type_of(did).clean(cx)), | ||
| _ => None, | ||
| }); | ||
| if let Some(type_alias) = type_alias { |
There was a problem hiding this comment.
Can you try deduplicating the ret.push with the one afterwards? Maybe for for_ in type_alias.into_iter().chain(self.for_.clean(cx)) {
There was a problem hiding this comment.
If we do it this way, we'll have to clone trait_ and items one extra time. I'll write something else instead to avoid the duplication of code.
|
Updated! |
|
The job 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 |
|
Fixed formatting. |
|
@bors r+ |
|
📌 Commit 482dc77 has been approved by |
bors
added
S-waiting-on-bors
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Jan 17, 2020
…ef, r=oli-obk Fix deref impl typedef Fixes rust-lang#35295. r? @kinnison
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Jan 18, 2020
…ef, r=oli-obk Fix deref impl typedef Fixes rust-lang#35295. r? @kinnison
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Jan 18, 2020
…ef, r=oli-obk Fix deref impl typedef Fixes rust-lang#35295. r? @kinnison
bors
added a commit
that referenced
this pull request
Jan 18, 2020
Rollup of 9 pull requests Successful merges: - #66660 (Don't warn about snake case for field puns.) - #68093 (Fix deref impl typedef) - #68204 (Use named fields for `{ast,hir}::ItemKind::Impl`) - #68256 (Do not ICE on malformed suggestion spans) - #68279 (Clean up E0198 explanation) - #68291 (Update sanitizer tests) - #68312 (Add regression test for integer literals in generic arguments in where clauses) - #68314 (Stop treating `FalseEdges` and `FalseUnwind` as having semantic value for const eval) - #68317 (Clean up E0199 explanation) Failed merges: r? @ghost
jyn514
added
the
T-rustdoc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #35295.
r? @kinnison