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

Improve searching in rustdoc and add tests #64094

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

ayuusweetfish
Copy link
Contributor

👋 I have made searching in rustdoc more intuitive, added a couple more tests and made a little shell script to aid testing. Closes #63005.

It took me quite a while to figure out how to run the tests for rustdoc (instead of running tests for other crates with rustdoc); the only pointer I found was hidden in the rustc book. Maybe this could be better documented? I shall be delighted to help if it is desirable.

@jonas-schievink
Copy link
Contributor

r? @GuillaumeGomez

@@ -3,6 +3,8 @@ const QUERY = 'Vec::new';
const EXPECTED = {
'others': [
{ 'path': 'std::vec::Vec', 'name': 'new' },
{ 'path': 'std::rc::Rc', 'name': 'new' },
Copy link
Member

Choose a reason for hiding this comment

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

This is less good result. We specified Vec explicitly in here after all.

@@ -0,0 +1,19 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand the point of this little script (even with your explanations). You can already do it using the x.py script (which could be better documented I guess).

@GuillaumeGomez
Copy link
Member

Thanks for the PR! Could you describe a bit more what you're trying to achieve in here? A test seemed like having less good results so I just want to be sure to fully understand what you have in mind with this PR.

@ayuusweetfish
Copy link
Contributor Author

Ah sure. I intended to prioritize methods with the same name (final word). This could be helpful in that it would be possible get accurate results even if we only remember the last word (#59287 shows such a use case). If we want to know about methods in a particular type or module, we can always go to its own page instead.

It was my carelessness to have missed the x.py test command, my apologies (^ ^;) I am trying that right now, but the --keep-stage flag somehow breaks here. Do I need to rebuild without it?

$ RUST_BACKTRACE=1 ./x.py test src/test/rustdoc-js --keep-stage 1
Updating only changed submodules
Submodules updated in 0.06 seconds
    Finished dev [unoptimized] target(s) in 0.13s
Building stage0 std artifacts (x86_64-apple-darwin -> x86_64-apple-darwin)
    Finished release [optimized] target(s) in 0.20s
Copying stage0 std from stage0 (x86_64-apple-darwin -> x86_64-apple-darwin / x86_64-apple-darwin)
Building stage0 compiler artifacts (x86_64-apple-darwin -> x86_64-apple-darwin)
    Finished release [optimized] target(s) in 0.21s
Copying stage0 rustc from stage0 (x86_64-apple-darwin -> x86_64-apple-darwin / x86_64-apple-darwin)
Building stage0 codegen artifacts (x86_64-apple-darwin -> x86_64-apple-darwin, llvm)
    Finished release [optimized] target(s) in 0.22s
Assembling stage1 compiler (x86_64-apple-darwin)
Warning: Using a potentially old libstd. This may not behave well.
Copying stage1 std from stage1 (x86_64-apple-darwin -> x86_64-apple-darwin / x86_64-apple-darwin)
Warning: Using a potentially old librustc. This may not behave well.
Copying stage1 rustc from stage1 (x86_64-apple-darwin -> x86_64-apple-darwin / x86_64-apple-darwin)
thread 'main' panicked at 'fs::read(stamp) failed with No such file or directory (os error 2)', src/bootstrap/lib.rs:1129:24
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /Users/vsts/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.34/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /Users/vsts/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.34/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:200
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:214
   6: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:477
   7: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:384
   8: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:339
   9: bootstrap::Build::read_stamp_file
             at src/bootstrap/lib.rs:1129
  10: bootstrap::compile::add_to_sysroot
             at src/bootstrap/compile.rs:941
  11: <bootstrap::compile::RustcLink as bootstrap::builder::Step>::run
             at src/bootstrap/compile.rs:509
  12: bootstrap::builder::Builder::ensure
             at src/bootstrap/builder.rs:1238
  13: <bootstrap::compile::Rustc as bootstrap::builder::Step>::run
             at src/bootstrap/compile.rs:396
  14: bootstrap::builder::Builder::ensure
             at src/bootstrap/builder.rs:1238
  15: <bootstrap::compile::Assemble as bootstrap::builder::Step>::run
             at src/bootstrap/compile.rs:871
  16: bootstrap::builder::Builder::ensure
             at src/bootstrap/builder.rs:1238
  17: bootstrap::builder::Builder::compiler
             at src/bootstrap/builder.rs:564
  18: <bootstrap::test::RustdocJSNotStd as bootstrap::builder::Step>::make_run
             at src/bootstrap/test.rs:696
  19: bootstrap::builder::StepDescription::maybe_run
             at src/bootstrap/builder.rs:183
  20: bootstrap::builder::StepDescription::run
             at src/bootstrap/builder.rs:226
  21: bootstrap::builder::Builder::run_step_descriptions
             at src/bootstrap/builder.rs:556
  22: bootstrap::builder::Builder::execute_cli
             at src/bootstrap/builder.rs:546
  23: bootstrap::Build::build
             at src/bootstrap/lib.rs:449
  24: bootstrap::main
             at src/bootstrap/bin/main.rs:15
  25: std::rt::lang_start::{{closure}}
             at /rustc/e450539c2a8d7f791268d699cbe45ab3e57d43a1/src/libstd/rt.rs:64
  26: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:49
  27: std::panicking::try::do_call
             at src/libstd/panicking.rs:296
  28: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:80
  29: std::panicking::try
             at src/libstd/panicking.rs:275
  30: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  31: std::rt::lang_start_internal
             at src/libstd/rt.rs:48
  32: std::rt::lang_start
             at /rustc/e450539c2a8d7f791268d699cbe45ab3e57d43a1/src/libstd/rt.rs:64
  33: bootstrap::main

@GuillaumeGomez
Copy link
Member

Ah sure. I intended to prioritize methods with the same name (final word). This could be helpful in that it would be possible get accurate results even if we only remember the last word (#59287 shows such a use case). If we want to know about methods in a particular type or module, we can always go to its own page instead.

I don't really agree with this: if you specified the parent item (so vec in vec::new), vec elements shouldn't be lower, on the contrary.

On another case, if you're looking for a type and you make a typo, you want both the type and the method to be somewhat at the same level, not all methods then the type (based on the fact that they have the same levenhstein level result).

It was my carelessness to have missed the x.py test command, my apologies (^ ^;) I am trying that right now, but the --keep-stage flag somehow breaks here. Do I need to rebuild without it?

No worries, x.py isn't easy to get started with (I was lucky to see it evolving so that made learning using it way easier hehe). Otherwise, I actually never used the --keep-stage flag... I usually run with --stage 1 and that's pretty much it.

@ayuusweetfish
Copy link
Contributor Author

Previously x.py filtered out all tests in rustdoc-js; I have pushed a fix and now ./x.py test src/test/rustdoc-js --stage 1 works normally. Please kindly point out if anything can be done in better ways!

As for results ordering, we can keep only the most intuitive requirements in tests and get rid of others, but I'd like to insist on this implementation. (tl;dr ends here 😝)

if you're looking for a type and you make a typo, you want both the type and the method to be somewhat at the same level, not all methods then the type

I think it's usually easy for the user to fix the typo themself, and it should be fine as long as we are returning meaningful results.

When we type Vec::new correctly, how other results are ordered is actually irrelevant. However, prioritizing final word equivalence is particularly helpful for short names. Also, the current calculation does not simply take the actual Levenshtein distance, but a rounded average over namespaces plus minor adjustments instead. This value may be biased in corner cases and can disarrange the results if used as a primary keyword.

Many other documentation sites are doing it this way as well: Hoogle (List.init), cppreference.com (vector::size).

@GuillaumeGomez
Copy link
Member

You should send a new PR for the x.py fix. Not a good idea to mix too much things.

Otherwise, I still have issues to see how these changes improve the search... Normally, exact matches are put up by adding a small advantage. Or did I miss something?

@ayuusweetfish
Copy link
Contributor Author

ayuusweetfish commented Sep 4, 2019

Okay, I shall separate the x.py fix.

This mainly aims at two use cases that current nightly does not handle well:

  1. Very short names (rustdoc search should ensure it shows exact matches #59287, regression)
  2. Exact matches (rustdoc: Searching for path (including a ::) does not work #63005)

Edit: relevant logic ↓

} else if (searchWords[j] === val) {
// Small trick to fix when you're looking for a one letter type
// and there are other short named types.
lev = -1;
}

This advantage is by no means small, and overwrites the Levenshtein biases completely.

// sort by exact match (mismatch goes later)
a = (aaa.word !== valLower);
b = (bbb.word !== valLower);
if (a !== b) { return a - b; }

These have no effect in many cases, because valLower is the path (vec::new) but aaa.word is only the last word (new).

I should have explained it earlier, sorry for the confusion ._.

@ayuusweetfish
Copy link
Contributor Author

Is it better now?

@GuillaumeGomez
Copy link
Member

Yes, I prefer the current result. Can you squash your commits please? Once done, I'll approve the PR.

@ayuusweetfish
Copy link
Contributor Author

Accidentally messed up, fixing 😓

@ayuusweetfish
Copy link
Contributor Author

I think it's ready now!

@ayuusweetfish ayuusweetfish changed the title Improve searching in rustdoc; tests included Improve searching in rustdoc and add tests Sep 4, 2019
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

👍

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 4, 2019

📌 Commit cb84aa4 has been approved by GuillaumeGomez

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 4, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
…umeGomez

Improve searching in rustdoc and add tests

👋 I have made searching in rustdoc more intuitive, added a couple more tests and made a little shell script to aid testing. Closes rust-lang#63005.

It took me quite a while to figure out how to run the tests for rustdoc (instead of running tests for other crates with rustdoc); the only pointer I found was [hidden in the rustc book](https://rust-lang.github.io/rustc-guide/rustdoc.html#cheat-sheet). Maybe this could be better documented? I shall be delighted to help if it is desirable.
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
…umeGomez

Improve searching in rustdoc and add tests

👋 I have made searching in rustdoc more intuitive, added a couple more tests and made a little shell script to aid testing. Closes rust-lang#63005.

It took me quite a while to figure out how to run the tests for rustdoc (instead of running tests for other crates with rustdoc); the only pointer I found was [hidden in the rustc book](https://rust-lang.github.io/rustc-guide/rustdoc.html#cheat-sheet). Maybe this could be better documented? I shall be delighted to help if it is desirable.
bors added a commit that referenced this pull request Sep 5, 2019
Rollup of 5 pull requests

Successful merges:

 - #63676 (Use wasi crate for Core API)
 - #64094 (Improve searching in rustdoc and add tests)
 - #64111 (or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve)
 - #64156 (Assume non-git LLVM is fresh if the stamp file exists)
 - #64175 (Fix invalid span generation when it should be div)

Failed merges:

 - #63806 (Upgrade rand to 0.7)

r? @ghost
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 6, 2019
…umeGomez

Improve searching in rustdoc and add tests

👋 I have made searching in rustdoc more intuitive, added a couple more tests and made a little shell script to aid testing. Closes rust-lang#63005.

It took me quite a while to figure out how to run the tests for rustdoc (instead of running tests for other crates with rustdoc); the only pointer I found was [hidden in the rustc book](https://rust-lang.github.io/rustc-guide/rustdoc.html#cheat-sheet). Maybe this could be better documented? I shall be delighted to help if it is desirable.
Centril added a commit to Centril/rust that referenced this pull request Sep 6, 2019
…umeGomez

Improve searching in rustdoc and add tests

👋 I have made searching in rustdoc more intuitive, added a couple more tests and made a little shell script to aid testing. Closes rust-lang#63005.

It took me quite a while to figure out how to run the tests for rustdoc (instead of running tests for other crates with rustdoc); the only pointer I found was [hidden in the rustc book](https://rust-lang.github.io/rustc-guide/rustdoc.html#cheat-sheet). Maybe this could be better documented? I shall be delighted to help if it is desirable.
bors added a commit that referenced this pull request Sep 6, 2019
Rollup of 10 pull requests

Successful merges:

 - #63676 (Use wasi crate for Core API)
 - #64094 (Improve searching in rustdoc and add tests)
 - #64111 (or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve)
 - #64156 (Assume non-git LLVM is fresh if the stamp file exists)
 - #64161 (Point at variant on pattern field count mismatch)
 - #64174 (Add missing code examples on Iterator trait)
 - #64175 (Fix invalid span generation when it should be div)
 - #64186 (std: Improve downstream codegen in `Command::env`)
 - #64190 (fill metadata in rustc_lexer's Cargo.toml)
 - #64198 (Add Fuchsia to actually_monotonic)

Failed merges:

r? @ghost
@bors bors merged commit cb84aa4 into rust-lang:master Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: Searching for path (including a ::) does not work
5 participants