Skip to content
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

Rustdoc search by type #23289

Merged
merged 1 commit into from Mar 15, 2015
Merged

Conversation

mihneadb
Copy link
Contributor

This adds search by type (for functions/methods) support to Rustdoc. Target issue is at rust-lang/rfcs#658.

I've described my approach here: rust-lang/rfcs#658 (comment). I'll copy the text in here as well:


Hi, it took me longer than I wished, but I have implemented this in a not-too-complex way that I think can be extended to support more complex features (like the ones mentioned here).

The idea is to generate a JSON representation of the types of methods/functions in the existing index, and then make the JS understand when it should look by type (and not by name).

I tried to come up with a JSON representation that can be extended to support generics, bounds, ref/mut annotations and so on. Here are a few samples:

Function:

fn to_uppercase(c: char) -> char
{
    "inputs": [
        {"name": "char"}
    ],
    "output": {
        "name": "char",
    }
}

Method (implemented or defined in trait):

// in struct Vec
// self is considered an argument as well
fn capacity(&self) -> usize
{
    "inputs": [
        {"name": "vec"}
    ],
    "output": {
        "name": "usize"
    }
}

This simple format can be extended by adding more fields, like generic: bool, a bounds mapping and so on.

I have a working implementation in master...mihneadb:rustdoc-search-by-type. You can check out a live demo here.

screenshot from 2015-02-28 00 54 00

The feature list is not that long:

  • search by types (you can use generics as well, as long as you use the exact name - e.g. vec,t ->)
  • order of arguments does not matter
  • self is took into account as well (e.g. search for vec -> usize)
  • does not use "complex" annotations (e.g. you don't search for &char -> char but for char -> char)

My goal is to get a working, minimal "base" merged so that others can build upon it. How should I proceed? Do I open a PR (badly in need of code review since this is my first non "hello world"-ish rust code)?


@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@mihneadb
Copy link
Contributor Author

cc @steveklabnik

@steveklabnik
Copy link
Member

I am not great with Rustdoc internals, but really want this as a feature. 🎊

@mihneadb
Copy link
Contributor Author

Addressing the make tidy issues.

@jdm
Copy link
Contributor

jdm commented Mar 11, 2015

This is totally excellent! 🎃

@killercup
Copy link
Member

This looks really good!

A next step might be fuzzy matching on types. E.g. -> Result or File ->.

(Your demo page took forever to load, by the way. 5 1/2 min—I'm not kidding! Is that server on Mars? You might also want enable gzip; the raw search index is 3.5MB.)

@mihneadb
Copy link
Contributor Author

@killercup definitely! I'd rather do it in a further iteration if that's OK.

Regarding the server, I sometimes wonder the same thing. Sorry about that! From my tests the load time was OK.

Since the docs can be accessed in many ways (i.e. not necessarily by a server that can do gzip), I'm thinking of actually gzipping the index from the Rust code and have it decompressed in JS. What do you think?

@killercup
Copy link
Member

@mihneadb these were just queries I wanted to do that don't work right now. Launch this as is, and we can add more stuff later.

I have no idea how performant compressing this stuff in JS is. pako looks like a nice library (the deflate part is 26k minified). You might want to test how well that works. Don't forget that these docs are also served locally; I don't know how to load a compressed file so that it can be read in JS without using AJAX requests.

If you are concerned about the file size, it might help to restructure the JSON output a bit. E.g. change

{
  "inputs": [
    {"name": "char"},
    {"name": "i32"}
  ],
  "output": {
    "name": "char",
  }
}

to:

[
  ["char", "i32"],
  "char"
]

@mihneadb
Copy link
Contributor Author

@killercup for the record, you can search for no input / no output (e.g. vec -> and -> vec), but it's not fuzzy. Thanks for the suggestions!

}
} else {
None
};
Copy link
Member

Choose a reason for hiding this comment

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

I believe our typical form of indentation doesn't indent the body of the if over one extra (e.g. this should be tabbed left one).

I also think that this may be better expressed as self.parent_stack.first().and_then(|..| { ... }) as it's already got the Option there.

@mihneadb
Copy link
Contributor Author

@alexcrichton thanks a lot for the input, I learned a lot!

/// Formats type as {name: $name}.
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
try!(write!(f, "{{"));
try!(write!(f, "\"name\":\"{}\"", self.name.clone().unwrap_or("UNKNOWN".to_string())));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton any way to make this shorter/without using clone?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps! I was thinking though that we may want to use null here instead of UNKNOWN (as searching for UNKNOWN probably isn't what you want).

I wonder, could entries with None be omitted entirely from the search index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They shouldn't be omitted entirely from the search index, since you might still be able to search them by name. However, the search_type of such items should be serialized as null (JS) alltogether. On it!

@jstasiak
Copy link

+1, I'm new to Rust and this is a very desired feature

@alexcrichton
Copy link
Member

This is looking really great, nice work! I can think of a number of cases that aren't quite covered by this, but I feel like this is a great improvement regardless and it can always be iterated on so I'm fine landing with a few known cases where it may not work :)

@mihneadb
Copy link
Contributor Author

Thanks for the kind words, I totally agree that it can be improved (I want to try adding some things in future iterations, after v0 gets landed).

@mihneadb
Copy link
Contributor Author

@alexcrichton Please take another look. The code is now ignoring types it can't handle and it just writes null for the search_type of the corresponding entry. I think the PR is ready now.

try!(write!(f, "{{"));
// Wrapping struct fmt should never call us when self.name is None,
// but just to be safe we write `null` in that case.
try!(write!(f, "\"name\":\"{}\"", self.name.as_ref().unwrap_or(&"null".to_string())));
Copy link
Member

Choose a reason for hiding this comment

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

If the name is None, I think this will output "null" instead of null (which may confuse the JS)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops!

@mihneadb
Copy link
Contributor Author

@alexcrichton addressed your comments.

@alexcrichton
Copy link
Member

Looks good to me, thanks @mihneadb! Could you squash all the commits together as well? After that I'll hand it off to bors

@mihneadb
Copy link
Contributor Author

@alexcrichton thanks! I squashed the commits.

@steveklabnik
Copy link
Member

@bors: r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 14, 2015

@bors r=alexcrichton 7b7b938

@bors
Copy link
Contributor

bors commented Mar 14, 2015

Testing commit 7b7b938 with merge 30e1f9a...

bors added a commit that referenced this pull request Mar 14, 2015
This adds search by type (for functions/methods) support to Rustdoc. Target issue is at rust-lang/rfcs#658.

I've described my approach here: rust-lang/rfcs#658 (comment). I'll copy the text in here as well:

---

Hi, it took me longer than I wished, but I have implemented this in a not-too-complex way that I think can be extended to support more complex features (like the ones mentioned [here](#12866 (comment))).

The idea is to generate a JSON representation of the types of methods/functions in the existing index, and then make the JS understand when it should look by type (and not by name).

I tried to come up with a JSON representation that can be extended to support generics, bounds, ref/mut annotations and so on. Here are a few samples:

Function:

```rust
fn to_uppercase(c: char) -> char
```

```json
{
    "inputs": [
        {"name": "char"}
    ],
    "output": {
        "name": "char",
    }
}
```

Method (implemented or defined in trait):

```rust
// in struct Vec
// self is considered an argument as well
fn capacity(&self) -> usize
```

```json
{
    "inputs": [
        {"name": "vec"}
    ],
    "output": {
        "name": "usize"
    }
}
```

This simple format can be extended by adding more fields, like `generic: bool`, a `bounds` mapping and so on.

I have a working implementation in master...mihneadb:rustdoc-search-by-type. You can check out a live demo [here](http://data.mihneadb.net/doc/std/index.html?search=charext%20-%3E%20char).

![screenshot from 2015-02-28 00 54 00](https://cloud.githubusercontent.com/assets/643127/6422722/7e5374ee-bee4-11e4-99a6-9aac3c9d5068.png)


The feature list is not that long:
- search by types (you *can* use generics as well, as long as you use the exact name - e.g. [`vec,t -> `](http://data.mihneadb.net/doc/std/index.html?search=vec%2C%20t%20-%3E))
- order of arguments does not matter
- `self` is took into account as well (e.g. search for `vec -> usize`)
- does not use "complex" annotations (e.g. you don't search for `&char -> char` but for `char -> char`)

My goal is to get a working, minimal "base" merged so that others can build upon it. How should I proceed? Do I open a PR (badly in need of code review since this is my first non "hello world"-ish rust code)?

---
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 14, 2015
…alexcrichton

 This adds search by type (for functions/methods) support to Rustdoc. Target issue is at rust-lang/rfcs#658.

I've described my approach here: rust-lang/rfcs#658 (comment). I'll copy the text in here as well:

---

Hi, it took me longer than I wished, but I have implemented this in a not-too-complex way that I think can be extended to support more complex features (like the ones mentioned [here](rust-lang#12866 (comment))).

The idea is to generate a JSON representation of the types of methods/functions in the existing index, and then make the JS understand when it should look by type (and not by name).

I tried to come up with a JSON representation that can be extended to support generics, bounds, ref/mut annotations and so on. Here are a few samples:

Function:

```rust
fn to_uppercase(c: char) -> char
```

```json
{
    \"inputs\": [
        {\"name\": \"char\"}
    ],
    \"output\": {
        \"name\": \"char\",
    }
}
```

Method (implemented or defined in trait):

```rust
// in struct Vec
// self is considered an argument as well
fn capacity(&self) -> usize
```

```json
{
    \"inputs\": [
        {\"name\": \"vec\"}
    ],
    \"output\": {
        \"name\": \"usize\"
    }
}
```

This simple format can be extended by adding more fields, like `generic: bool`, a `bounds` mapping and so on.

I have a working implementation in rust-lang/rust@master...mihneadb:rustdoc-search-by-type. You can check out a live demo [here](http://data.mihneadb.net/doc/std/index.html?search=charext%20-%3E%20char).

![screenshot from 2015-02-28 00 54 00](https://cloud.githubusercontent.com/assets/643127/6422722/7e5374ee-bee4-11e4-99a6-9aac3c9d5068.png)

The feature list is not that long:
- search by types (you *can* use generics as well, as long as you use the exact name - e.g. [`vec,t -> `](http://data.mihneadb.net/doc/std/index.html?search=vec%2C%20t%20-%3E))
- order of arguments does not matter
- `self` is took into account as well (e.g. search for `vec -> usize`)
- does not use \"complex\" annotations (e.g. you don't search for `&char -> char` but for `char -> char`)

My goal is to get a working, minimal \"base\" merged so that others can build upon it. How should I proceed? Do I open a PR (badly in need of code review since this is my first non \"hello world\"-ish rust code)?

---
@bors bors merged commit 7b7b938 into rust-lang:master Mar 15, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 7, 2015
…bnik

I noticed this feature added in rust-lang#23289 was missing from the `Search tricks`. Thanks!

r? @steveklabnik
steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 7, 2015
…bnik

I noticed this feature added in rust-lang#23289 was missing from the `Search tricks`. Thanks!

r? @steveklabnik
bors added a commit that referenced this pull request Jun 15, 2015
…crichton

In #23289, I did not include the type information for searching for orphan methods (methods that are defined in a module and implemented in another - doing this causes rustdoc sometimes to first find the impl before the actual type).

This PR fixes this. By merging this, searches for `char -> bool` will also find `is_alphabetic`, for example (which otherwise was only found for `charext -> bool`).


cc @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants