Rustdoc: disambiguate Implementors when the type name is not unique #38414

estebank · 2016-12-16T20:05:24Z

Presentation goes from:

to:

on cases where there're multiple implementors with the same name.

Fixes #37762.

rust-highfive · 2016-12-16T20:05:29Z

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

bluss · 2016-12-16T23:10:47Z

Ignore-whitespace diff: https://github.com/rust-lang/rust/pull/38414/files?w=1

bluss · 2016-12-16T23:19:37Z

src/librustdoc/html/render.rs

+            // full path, for example in `std::iter::ExactSizeIterator`
+            let mut dissambiguate = false;
+            for l in implementors.iter() {
+                match (k.impl_.for_.clone(), l.impl_.for_.clone()) {


These two .clone() can be removed, just use match (&k.impl_.for_, &l....) if not rewriting the loop entirely due to the other comment.

bluss · 2016-12-16T23:20:21Z

src/librustdoc/html/render.rs

+                match (k.impl_.for_.clone(), l.impl_.for_.clone()) {
+                    (clean::Type::ResolvedPath {path: path_a, ..},
+                     clean::Type::ResolvedPath {path: path_b, ..}) => {
+                        if k.def_id != l.def_id && path_a.last_name() == path_b.last_name() {


I noticed (not in this patch) that last_name creates a String where it could just be a &str, too.

bluss · 2016-12-16T23:20:41Z

src/librustdoc/html/render.rs

+                        }
+                    }
+                    _ => (),
+                }


The loops are quadratic (complexity) in the number of implementors, I don't think we can afford that. A trait like Clone only lists ~480 impls, but still.

Unless I'm missing something, you can change last_name() to return &str and create a HashMap<&str, usize> or something to count everything's duplicitly here.

bluss · 2016-12-19T22:11:56Z

src/librustdoc/html/render.rs

@@ -2110,24 +2110,27 @@ fn item_trait(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item,
        <h2 id='implementors'>Implementors</h2>
        <ul class='item-list' id='implementors-list'>
    ")?;
+    let mut implementor_count: FxHashMap<String, usize> = FxHashMap();
+    for (_, implementors) in cache.implementors.iter() {


It's counting far too many.. all implementors instead of just the ones for this trait(!). I tested a fix.

bluss · 2016-12-19T22:13:33Z

Can you apply this diff? Then it uses &str instead, and counts the right implementors. I wrote this up to make sure that it was possible (to not make suggestions that can't be).

diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs
index 28ca92f..a99d4dd 100644
--- a/src/librustdoc/clean/mod.rs
+++ b/src/librustdoc/clean/mod.rs
@@ -2205,8 +2205,8 @@ impl Path {
         }
     }
 
-    pub fn last_name(&self) -> String {
-        self.segments.last().unwrap().name.clone()
+    pub fn last_name(&self) -> &str {
+        &self.segments.last().unwrap().name
     }
 }
 
diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs
index 846c072..5839c46 100644
--- a/src/librustdoc/html/render.rs
+++ b/src/librustdoc/html/render.rs
@@ -2110,23 +2110,22 @@ fn item_trait(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item,
         <h2 id='implementors'>Implementors</h2>
         <ul class='item-list' id='implementors-list'>
     ")?;
-    let mut implementor_count: FxHashMap<String, usize> = FxHashMap();
-    for (_, implementors) in cache.implementors.iter() {
+    if let Some(implementors) = cache.implementors.get(&it.def_id) {
+        let mut implementor_count: FxHashMap<&str, usize> = FxHashMap();
         for implementor in implementors {
             if let clean::Type::ResolvedPath {ref path, ..} = implementor.impl_.for_ {
                 *implementor_count.entry(path.last_name()).or_insert(0) += 1;
             }
         }
-    }
-    if let Some(implementors) = cache.implementors.get(&it.def_id) {
-        for implementor in implementors.iter() {
+
+        for implementor in implementors {
             write!(w, "<li><code>")?;
             // If there's already another implementor that has the same abbridged name, use the
             // full path, for example in `std::iter::ExactSizeIterator`
             let dissambiguate = if let clean::Type::ResolvedPath {
                 ref path, ..
             } = implementor.impl_.for_ {
-                *implementor_count.get(&path.last_name()).unwrap_or(&0) > 1
+                implementor_count[path.last_name()] > 1
             } else {
                 false
             };

estebank · 2016-12-22T08:35:55Z

@bluss applied 👍

bluss

Thanks, we can integrate, let's just clean up that bool parameter's name to be consistent across the patch.

bluss · 2016-12-22T11:06:38Z

src/librustdoc/html/format.rs

@@ -433,7 +433,7 @@ pub fn href(did: DefId) -> Option<(String, ItemType, Vec<String>)> {
 /// Used when rendering a `ResolvedPath` structure. This invokes the `path`
 /// rendering function with the necessary arguments for linking to a local path.
 fn resolved_path(w: &mut fmt::Formatter, did: DefId, path: &clean::Path,
-                 print_all: bool) -> fmt::Result {
+                 print_all: bool, use_absolute: bool) -> fmt::Result {


Can you go through the patch and use the same name for this parameter everywhere (the use_absolute here)? That's one step that makes a bool parameter more bearable.

bluss · 2016-12-22T11:06:48Z

src/librustdoc/html/format.rs

-                // Paths like T::Output and Self::Output should be rendered with all segments
-                resolved_path(f, did, path, is_generic)?;
-                tybounds(f, typarams)
+fn fmt_type(t: &clean::Type, f: &mut fmt::Formatter, full_path: bool) -> fmt::Result {


This parameter is the same (full_path), so this and in other places.

bluss · 2016-12-22T11:08:01Z

src/librustdoc/html/render.rs

-            fmt_impl_for_trait_page(&i.impl_, w)?;
+            // If there's already another implementor that has the same abbridged name, use the
+            // full path, for example in `std::iter::ExactSizeIterator`
+            let dissambiguate = if let clean::Type::ResolvedPath {


Extra s in the spelling here. It's another of the ones that should have the same name across the patch, though.

steveklabnik · 2016-12-22T21:41:29Z

/cc @rust-lang/tools

steveklabnik · 2017-01-03T16:17:01Z

@bors: r+

thanks!

GuillaumeGomez · 2017-01-04T16:03:06Z

Excellent, thanks a lot for this!

japaric · 2017-01-04T16:22:48Z

@bors r=steveklabnik

bors · 2017-01-04T16:22:49Z

📌 Commit 346a442 has been approved by steveklabnik

bluss · 2017-01-04T16:24:06Z

@bors r=steveklabnik

bors · 2017-01-04T16:24:07Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Fix transmute::<T, U> where T requires a bigger alignment than U #38670

bors · 2017-01-04T16:24:08Z

📌 Commit 346a442 has been approved by steveklabnik

bors · 2017-01-04T17:10:44Z

⌛ Testing commit 346a442 with merge 05f4a75...

Rustdoc: disambiguate Implementors when the type name is not unique Presentation [goes from](https://doc.rust-lang.org/stable/std/iter/trait.ExactSizeIterator.html#implementors): <img width="492" alt="" src="https://cloud.githubusercontent.com/assets/1606434/21276752/b2b50474-c387-11e6-96e1-9766851da269.png"> to: <img width="787" alt="" src="https://cloud.githubusercontent.com/assets/1606434/21276763/bb37f6b0-c387-11e6-8596-9163cb254674.png"> on cases where there're multiple implementors with the same name. Fixes #37762.

bors · 2017-01-04T19:25:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: steveklabnik
Pushing 05f4a75 to master...

Rustdoc: disambiguate Implementors when the type name is not unique

2841bf3

rust-highfive assigned steveklabnik Dec 16, 2016

bluss reviewed Dec 16, 2016

View reviewed changes

remove O(n^2) to O(n) behavior

08aa825

bluss reviewed Dec 19, 2016

View reviewed changes

bluss and others added 2 commits December 21, 2016 21:27

use &str instead of String

bf7d7ae

fix String to &str cast

de69a3b

bluss suggested changes Dec 22, 2016

View reviewed changes

use same param name across methods

346a442

bluss approved these changes Dec 23, 2016

View reviewed changes

bors merged commit 346a442 into rust-lang:master Jan 4, 2017

QuietMisdreavus mentioned this pull request May 17, 2017

rustdoc: disambiguate between types of the same name #42066

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustdoc: disambiguate Implementors when the type name is not unique #38414

Rustdoc: disambiguate Implementors when the type name is not unique #38414

estebank commented Dec 16, 2016

rust-highfive commented Dec 16, 2016

bluss commented Dec 16, 2016

bluss Dec 16, 2016

bluss Dec 16, 2016

bluss Dec 16, 2016 •

edited

Loading

bluss Dec 16, 2016 •

edited

Loading

bluss Dec 19, 2016

bluss commented Dec 19, 2016

estebank commented Dec 22, 2016

bluss left a comment

bluss Dec 22, 2016

bluss Dec 22, 2016

bluss Dec 22, 2016

steveklabnik commented Dec 22, 2016

steveklabnik commented Jan 3, 2017

GuillaumeGomez commented Jan 4, 2017

japaric commented Jan 4, 2017

bors commented Jan 4, 2017

bluss commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

Rustdoc: disambiguate Implementors when the type name is not unique #38414

Rustdoc: disambiguate Implementors when the type name is not unique #38414

Conversation

estebank commented Dec 16, 2016

rust-highfive commented Dec 16, 2016

bluss commented Dec 16, 2016

bluss Dec 16, 2016

Choose a reason for hiding this comment

bluss Dec 16, 2016

Choose a reason for hiding this comment

bluss Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

bluss Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

bluss Dec 19, 2016

Choose a reason for hiding this comment

bluss commented Dec 19, 2016

estebank commented Dec 22, 2016

bluss left a comment

Choose a reason for hiding this comment

bluss Dec 22, 2016

Choose a reason for hiding this comment

bluss Dec 22, 2016

Choose a reason for hiding this comment

bluss Dec 22, 2016

Choose a reason for hiding this comment

steveklabnik commented Dec 22, 2016

steveklabnik commented Jan 3, 2017

GuillaumeGomez commented Jan 4, 2017

japaric commented Jan 4, 2017

bors commented Jan 4, 2017

bluss commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 4, 2017

bluss Dec 16, 2016 •

edited

Loading

bluss Dec 16, 2016 •

edited

Loading