rustdoc: Disambiguate anchors #32985
Conversation
|
r? @cmr (rust_highfive has picked a reviewer for you, use r? to override) |
|
I'm still learning how to write idiomatic Rust, so feel free to offer suggestions. Changing to |
|
|
| @@ -2506,6 +2507,11 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi | |||
| if !is_static || render_static { | |||
| let id = derive_id(format!("{}.{}", shortty, name)); | |||
| write!(w, "<h4 id='{}' class='{}'>", id, shortty)?; | |||
| let assoc_link = &AssocItemLink::Anchor(Some(id)); | |||
| let link = match link { | |||
| &AssocItemLink::Anchor(None) => assoc_link, | |||
There was a problem hiding this comment.
This is fine, but since you asked for suggestions... 😄
You can write that as something like if let AssocItemLink::Anchor(ref mut id) = link { *id = Some(id) } (changing the function parameter to mut link: AssocItemLink).
You can also remove those pesky & from match arms by doing match *foo.
There was a problem hiding this comment.
Could you fill in the actual id for the other items/match arms too?
You may want to move the filling-in of the id into a inherent method of AssocItemLink then.
|
Looking good! Could you add a test in |
| @@ -2109,15 +2110,16 @@ fn render_assoc_item(w: &mut fmt::Formatter, | |||
| g: &clean::Generics, | |||
| selfty: &clean::SelfTy, | |||
| d: &clean::FnDecl, | |||
| link: AssocItemLink) | |||
| link: &AssocItemLink) | |||
| -> fmt::Result { | |||
| use syntax::abi::Abi; | |||
|
|
|||
| let name = meth.name.as_ref().unwrap(); | |||
| let anchor = format!("#{}.{}", shortty(meth), name); | |||
| let href = match link { | |||
There was a problem hiding this comment.
More usual style would be to write match *link here and simply remove the & prefix on each pattern.
|
☔ The latest upstream changes (presumably #33005) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@alexcrichton
|
|
Thanks for the review, @mitaa! I've pushed a new commit that incorporates your suggestions, things are indeed much cleaner this way. I had to work out the right lifetime for the Regarding adding a test, what is actually required to hit this edge case? I'm making these changes with only a partial understanding of the context. 😄 Something like the trait impls you wrote in this test was my first thought, but I guess that's not sufficient? |
|
I think that should be ok, we just need to create an id collision on an inherent impl block. (but to not trigger #31925 you should use concrete types for Another option would've been to use the same id for a header inside a doc block, but apparently the |
|
Sorry, I guess I still don't see how the collisions introduced by these methods aren't exactly what we're trying to address here. As a matter of fact, why isn't that test failing right now? I think there must be more to it than just a collision on an inherent impl, but I'm not sure what. |
|
The id attributes on the page are correct, which is what that test tests - what's wrong is that links on the very same element have the wrong link fragment when the target id collides with another one. So what we need to check here are the href attributes. (each |
|
Ah, of course. Lost sight of the forest for the trees there. Thanks for the explanation! I'll get a new test up shortly. |
|
@mitaa , @alexcrichton : updated commit with test. |
…excrichton rustdoc: Disambiguate anchors Closes #32890
Closes #32890