[rustdoc] Improve "in parameters" search and search more generally by GuillaumeGomez · Pull Request #59004 · rust-lang/rust

GuillaumeGomez · 2019-03-07T22:49:53Z

Fixes #58230.

r? @QuietMisdreavus

rust-highfive · 2019-03-07T22:49:58Z

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

rust-highfive · 2019-03-07T22:50:00Z

r? @steveklabnik

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

GuillaumeGomez · 2019-03-07T23:17:20Z

r? @QuietMisdreavus

Dylan-DPC-zz · 2019-03-18T19:58:43Z

ping from triage @QuietMisdreavus waiting for your review on this

QuietMisdreavus

Looks good, just a couple small comments. This may clash with #59170 whenever that lands.

QuietMisdreavus · 2019-03-18T21:34:12Z

src/librustdoc/clean/mod.rs

Can you write doc comments for the functions you added? For example, this could be something like "Extract the types from a function's parameters and generics" with maybe a note about trait bounds since you've already added the comment for it.

QuietMisdreavus · 2019-03-18T21:34:50Z

src/librustdoc/clean/mod.rs

If you use a BTreeSet instead, it comes pre-sorted - otherwise it comes out in an arbitrary order. I'm not sure if you wanted it to be properly sorted or just deduplicated, though.

Dedup, the order doesn't matter. ;)

QuietMisdreavus · 2019-03-18T21:35:45Z

src/librustdoc/clean/mod.rs

You can replace this loop with res.extend(adds), since the sets implement the Extend trait.

bors · 2019-03-21T12:20:01Z

☔ The latest upstream changes (presumably #58927) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2019-03-23T19:33:38Z

@QuietMisdreavus I applied your comments.

QuietMisdreavus

Excellent, thanks for doing this! This will make the in-doc search much more powerful.

QuietMisdreavus · 2019-03-26T14:17:45Z

@bors r+

bors · 2019-03-26T14:17:46Z

📌 Commit befe9ca has been approved by QuietMisdreavus

@QuietMisdreavus

…=QuietMisdreavus [rustdoc] Improve "in parameters" search and search more generally Fixes rust-lang#58230. r? @QuietMisdreavus

@ghost

Rollup of 7 pull requests Successful merges: - #59004 ([rustdoc] Improve "in parameters" search and search more generally) - #59026 (Fix moving text in search tabs headers) - #59197 (Exclude old book redirect stubs from search engines) - #59330 (Improve the documentation for std::convert (From, Into, AsRef and AsMut)) - #59424 (Fix code block display in portability element in dark theme) - #59427 (Link to PhantomData in NonNull documentation) - #59432 (Improve some compiletest documentation) Failed merges: r? @ghost

@ghost

Rollup of 7 pull requests Successful merges: - #59004 ([rustdoc] Improve "in parameters" search and search more generally) - #59026 (Fix moving text in search tabs headers) - #59197 (Exclude old book redirect stubs from search engines) - #59330 (Improve the documentation for std::convert (From, Into, AsRef and AsMut)) - #59424 (Fix code block display in portability element in dark theme) - #59427 (Link to PhantomData in NonNull documentation) - #59432 (Improve some compiletest documentation) Failed merges: r? @ghost

eddyb · 2019-03-28T13:44:40Z

@GuillaumeGomez @QuietMisdreavus Please do not merge PRs that change the compiler (in this case, typeck) without a review from @rust-lang/compiler.

eddyb · 2019-03-28T13:48:30Z

src/librustdoc/clean/mod.rs

+    pub fn get_type(&self, cx: &DocContext<'_>) -> Option<Type> {
+        match *self {
+            GenericParamDefKind::Type { did, .. } => {
+                rustc_typeck::checked_type_of(cx.tcx, did, false).map(|t| t.clean(cx))


You didn't have to do any of the typeck changes, the default is already there:

GenericParamDefKind::Type { ref default, .. } => default.clone(),

eddyb · 2019-03-28T22:14:59Z

src/librustdoc/clean/mod.rs


+/// The point of this function is to replace bounds with types.
+///
+/// i.e. `[T, U]` when you have the following bounds: `T: Display, U: Option<T>` will return


Option<T> is not a bound... how does this make sense?

The point is to replace T by the type/trait if any. For example:

fn<T>(x: T) where T: Display

In there, T should be replaced with Display. In here, it's all about the rustdoc search, not about finding exact types. If we have two types with the same name, we don't care, we just remove one name from the list and that's it.

Sure, but you can get the desired effect without using confusing terminology.

So what you want is "types of arguments" and "bounds on parameters in those arguments"?

Why bother with that complication tho? Why not just record all paths in a signature, be it args/return, or bounds/where clauses? Getting all that is less work and much easier to reason about.

eddyb · 2019-03-28T22:25:14Z

src/librustdoc/clean/mod.rs

+
+    pub fn is_full_generic(&self) -> bool {
+        match *self {
+            Type::Generic(_) => true,


This should be called Param or GenericParam.

eddyb · 2019-03-28T22:25:22Z

src/librustdoc/clean/mod.rs

        }
    }
+
+    pub fn is_full_generic(&self) -> bool {


This should be called is_param or is_generic_param.

eddyb · 2019-03-28T22:25:56Z

src/librustdoc/clean/mod.rs

+/// wrapped types in here).
+fn get_real_types(
+    generics: &Generics,
+    arg: &Type,


This should probably be called ty?

eddyb · 2019-03-28T22:27:55Z

src/librustdoc/clean/mod.rs

+                                continue
+                            }
+                            if let Some(ty) = x.get_type(cx) {
+                                let adds = get_real_types(generics, &ty, cx);


This is unguarded recursion, can easily become infinite, probably the cause of #59502.

It was indeed.

eddyb · 2019-03-28T22:28:14Z

src/librustdoc/clean/mod.rs

+        }) {
+            for bound in bound.get_bounds().unwrap_or_else(|| &[]) {
+                if let Some(ty) = bound.get_trait_type() {
+                    let adds = get_real_types(generics, &ty, cx);


Same here, unguarded recursion.

eddyb · 2019-03-28T22:28:28Z

src/librustdoc/clean/mod.rs

+        if let Some(gens) = arg.generics() {
+            for gen in gens.iter() {
+                if gen.is_full_generic() {
+                    let adds = get_real_types(generics, gen, cx);


Same here, unguarded recursion.

eddyb · 2019-03-28T22:29:01Z

src/librustdoc/clean/mod.rs

+/// Return the full list of types when bounds have been resolved.
+///
+/// i.e. `fn foo<A: Display, B: Option<A>>(x: u32, y: B)` will return
+/// `[u32, Display, Option]`.


Same here, Option<A> is not a bound.

eddyb · 2019-03-28T22:41:16Z

src/librustdoc/clean/mod.rs

+                }
+            }
+        }
+        if let Some(bound) = generics.params.iter().find(|g| {


bound here seems to be a misnomer, this is a param.

IMO this should be above the where handling above, doing it this way is far more confusing.

eddyb · 2019-03-28T22:42:07Z

src/librustdoc/clean/mod.rs

+            }
+        }
+        if let Some(bound) = generics.params.iter().find(|g| {
+            g.is_type() && g.name == arg_s


Why is this by name instead of by DefId?

I don't care about DefId in here, I only care about names. Don't forget it's to generate rustdoc search array. I'm not looking for a given type but for a given type name.

You're searching for a type parameter though, not a struct/enum.
And you implemented the other search for type parameters by DefId, so it's inconsistent.

eddyb · 2019-03-28T22:55:40Z

src/librustdoc/clean/mod.rs

+                    let adds = get_real_types(generics, &ty, cx);
+                    if !adds.is_empty() {
+                        res.extend(adds);
+                    } else if !ty.is_full_generic() {


ty is a trait, not a type, so is_full_generic() should never be the case.

eddyb · 2019-03-28T22:56:44Z

src/librustdoc/clean/mod.rs

+/// i.e. `[T, U]` when you have the following bounds: `T: Display, U: Option<T>` will return
+/// `[Display, Option]` (we just returns the list of the types, we don't care about the
+/// wrapped types in here).
+fn get_real_types(


This name should be find_concrete_types, or, even better, find_type_paths.

eddyb · 2019-03-28T22:57:42Z

src/librustdoc/clean/mod.rs

+        res.insert(arg.clone());
+        if let Some(gens) = arg.generics() {
+            for gen in gens.iter() {
+                if gen.is_full_generic() {


So this means that get_real_types(Option<T>) will look at T's bounds... why?

eddyb · 2019-03-28T22:58:03Z

src/librustdoc/clean/mod.rs

+                    if !adds.is_empty() {
+                        res.extend(adds);
+                    } else if !ty.is_full_generic() {
+                        res.insert(ty.clone());


ty is a trait, so your returned set contains both types and traits?

eddyb · 2019-03-28T23:05:26Z

src/librustdoc/clean/mod.rs

@@ -1084,9 +1084,10 @@ impl GenericBound {

    fn get_trait_type(&self) -> Option<Type> {


The existence of a trait path as a value of Type was a mistake, IMO.

If you move DefId into Path, you can replace trait_: Type with path: Path in:

rust/src/librustdoc/clean/mod.rs

Lines 2428 to 2431 in 237bf32

pub struct PolyTrait {

pub trait_: Type,

pub generic_params: Vec<GenericParamDef>,

}

and this (and other places) with PolyTrait { path, generic_params: late_bounds }:

rust/src/librustdoc/clean/mod.rs

Lines 1196 to 1201 in 237bf32

trait_: ResolvedPath {

path,

param_names: None,

did: trait_ref.def_id,

is_generic: false,

},

Wait, Path has a Def which contains a DefId:

rust/src/librustdoc/clean/mod.rs

Lines 3421 to 3425 in 237bf32

pub struct Path {

pub global: bool,

pub def: Def,

pub segments: Vec<PathSegment>,

}

So Type::ResolvedPath's DefId is just redundant?!

Oh, you'd need this to take a Def, too (which can replace trait_did):

rust/src/librustdoc/clean/mod.rs

Lines 1153 to 1154 in 237bf32

fn external_path(cx: &DocContext<'_>, name: &str, trait_did: Option<DefId>, has_self: bool,

bindings: Vec<TypeBinding>, substs: SubstsRef<'_>) -> Path {

eddyb · 2019-04-08T07:07:48Z

Opened #59789.

@nikomatsakis

Revert two unapproved changes to rustc_typeck. There was a breakdown in process (#59004 (comment), #58894 (comment)) and two changes were made to `rustc_typeck`'s "collect" queries, for rustdoc, that were neither needed *nor* correct. I'm reverting them here, and will fix up rustdoc *somehow*, if necessary. cc @rust-lang/compiler How do we ensure this doesn't happen again? r? @nikomatsakis or @oli-obk

@nikomatsakis

Revert two unapproved changes to rustc_typeck. There was a breakdown in process (rust-lang#59004 (comment), rust-lang#58894 (comment)) and two changes were made to `rustc_typeck`'s "collect" queries, for rustdoc, that were neither needed *nor* correct. I'm reverting them here, and will fix up rustdoc *somehow*, if necessary. cc @rust-lang/compiler How do we ensure this doesn't happen again? r? @nikomatsakis or @oli-obk

rust-highfive assigned steveklabnik Mar 7, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2019

GuillaumeGomez changed the title ~~Generics handling~~ [rustdoc] Improve "in parameters" search and search more generally Mar 7, 2019

rust-highfive assigned QuietMisdreavus and unassigned steveklabnik Mar 7, 2019

GuillaumeGomez force-pushed the generics-handling branch from 37bcbce to 6efbcaa Compare March 7, 2019 23:19

GuillaumeGomez mentioned this pull request Mar 9, 2019

RustDoc: Search should "follow" trait generics for parameter and return tabs #56386

Closed

QuietMisdreavus reviewed Mar 18, 2019

View reviewed changes

Dylan-DPC-zz 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2019

GuillaumeGomez added 7 commits March 21, 2019 22:49

Greatly improve generics handling in rustdoc search

64382f4

Small generics search improvement

d611301

Improve bounds search

6ae73e2

Add bounds for return types as well

aefe750

Fix invalid returned types generation

6bce61c

cleanup

dc628b4

Add test

befe9ca

GuillaumeGomez force-pushed the generics-handling branch from 6efbcaa to befe9ca Compare March 23, 2019 13:58

QuietMisdreavus approved these changes Mar 26, 2019

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2019

GuillaumeGomez mentioned this pull request Mar 26, 2019

Rollup of 7 pull requests #59447

Merged

bors merged commit befe9ca into rust-lang:master Mar 27, 2019

GuillaumeGomez deleted the generics-handling branch March 27, 2019 09:10

eddyb reviewed Mar 28, 2019

View reviewed changes

eddyb mentioned this pull request Mar 28, 2019

rustdoc overflows stack on latest nightly #59502

Closed

eddyb reviewed Mar 28, 2019

View reviewed changes

src/librustdoc/clean/mod.rs

}

}

pub fn is_full_generic(&self) -> bool {

Copy link

Member

eddyb Mar 28, 2019

Choose a reason for hiding this comment

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

This should be called is_param or is_generic_param.

eddyb reviewed Mar 28, 2019

View reviewed changes

eddyb mentioned this pull request Apr 8, 2019

Revert two unapproved changes to rustc_typeck. #59789

Merged

		@@ -1084,9 +1084,10 @@ impl GenericBound {

		fn get_trait_type(&self) -> Option<Type> {

	pub struct PolyTrait {
	pub trait_: Type,
	pub generic_params: Vec<GenericParamDef>,
	}

	trait_: ResolvedPath {
	path,
	param_names: None,
	did: trait_ref.def_id,
	is_generic: false,
	},

	pub struct Path {
	pub global: bool,
	pub def: Def,
	pub segments: Vec<PathSegment>,
	}

	fn external_path(cx: &DocContext<'_>, name: &str, trait_did: Option<DefId>, has_self: bool,
	bindings: Vec<TypeBinding>, substs: SubstsRef<'_>) -> Path {

Uh oh!

Conversation

GuillaumeGomez commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 7, 2019

Uh oh!

rust-highfive commented Mar 7, 2019

Uh oh!

GuillaumeGomez commented Mar 7, 2019

Uh oh!

Dylan-DPC-zz commented Mar 18, 2019

Uh oh!

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 21, 2019

Uh oh!

GuillaumeGomez commented Mar 23, 2019

Uh oh!

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Uh oh!

QuietMisdreavus commented Mar 26, 2019

Uh oh!

bors commented Mar 26, 2019

Uh oh!

eddyb commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 7, 2019 •

edited

Loading

eddyb Mar 28, 2019 •

edited

Loading

eddyb Mar 28, 2019 •

edited

Loading