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

Type-based search does not work for primitives since March #74780

Closed
jyn514 opened this issue Jul 26, 2020 · 8 comments · Fixed by #81557
Closed

Type-based search does not work for primitives since March #74780

jyn514 opened this issue Jul 26, 2020 · 8 comments · Fixed by #81557
Labels
A-type-based-search Area: Searching rustdoc pages using type signatures C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Jul 26, 2020

searched nightlies: from nightly-2019-05-02 to nightly-2020-07-17
regressed nightly: nightly-2020-03-20
searched commits: from f509b26 to f4c675c
regressed commit: f4c675c

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --script ./search_works.sh --start 2019-05-02 --preserve 

search_works.sh:

#!/bin/sh
set -eu
rustdoc search_function_type.rs
node ~/rustc/src/tools/rustdoc-js/tester.js --doc-folder doc --crate-name search_function_type --test-file ~/rustc/src/test/rustdoc-js/tmp/search-function-type.js

search_function_type.rs:

/// f
pub fn f(u: usize) -> usize {
    u
}

search-function-type.js:

const QUERY = 'usize -> usize';

const EXPECTED = {
    'in_args': [
        { 'path': 'search_function_type', 'name': 'f' },
    ],
};

This requires a git clone of rustc as well as the following modification to tester.js:

diff --git a/src/tools/rustdoc-js/tester.js b/src/tools/rustdoc-js/tester.js
index 139e6f73f42..8a3ab056f80 100644
--- a/src/tools/rustdoc-js/tester.js
+++ b/src/tools/rustdoc-js/tester.js
@@ -182,7 +182,7 @@ function loadThings(thingsToLoad, kindOfLoad, funcToCall, fileContent) {
         var tmp = funcToCall(fileContent, thingsToLoad[i]);
         if (tmp === null) {
             console.log('unable to find ' + kindOfLoad + ' "' + thingsToLoad[i] + '"');
-            process.exit(1);
+            continue;
         }
         content += tmp;
         content += 'exports.' + thingsToLoad[i] + ' = ' + thingsToLoad[i] + ';';

Originally reported in #60485 (comment).

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. C-bug Category: This is a bug. labels Jul 26, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 26, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jul 26, 2020

Relevant PR: #69402

@jyn514
Copy link
Member Author

jyn514 commented Jul 26, 2020

The function is missing from the search index after that PR. Before:

var N=null,E="",T="t",U="u",searchIndex={};

searchIndex["search_function_type"]={"doc":E,"i":[[5,"f","search_function_type","f",N,[[["usize"]],["usize"]]],[0,"m",E,E,N,N]],"p":[]};
initSearch(searchIndex);addSearchOptions(searchIndex);

After:

searchIndex["search_function_type"] = {"doc":"","i":[[5,"f","search_function_type","f",null,[[]]],[0,"m","","",null,null]],"p":[]};
addSearchOptions(searchIndex);initSearch(searchIndex);

@jyn514
Copy link
Member Author

jyn514 commented Jul 26, 2020

Actually it looks like the function is still there, it's just missing the types for the arguments and return type.

@jyn514
Copy link
Member Author

jyn514 commented Jul 26, 2020

@jyn514
Copy link
Member Author

jyn514 commented Jul 26, 2020

Yeah, I don't think arguments have def ids.

@GuillaumeGomez
Copy link
Member

To be exact, the problem seems to come from:

Primitive(p) => crate::html::render::cache().primitive_locations.get(&p).cloned(),

coming from:

impl GetDefId for Type {
    fn def_id(&self) -> Option<DefId> {
        ...
    }
}

in the librustdoc/clean/types.rs file.

@Manishearth
Copy link
Member

Through some chats in discord it seems like a good approach to this would be to:

  • compute a separate list of primitives before starting the whole cleaning rigmarole. For efficiency it might be worth tagging std as doc(has_primitives) or something if we'd like, but this wouldn't be a perf loss since this work is being done anyway.
  • use that list in GetDefId for Type

Eventually I'd like to stop having the cache be a global parameter and instead passing it around as much as possible so that such problems are easier to avoid.

@jyn514 jyn514 changed the title Type-based search does not work since March Type-based search does not work for primitives since March Aug 10, 2020
@jyn514 jyn514 added the A-type-based-search Area: Searching rustdoc pages using type signatures label Aug 17, 2020
@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 2, 2020
@camelid
Copy link
Member

camelid commented Nov 2, 2020

Labeling as P-high as per the guidelines for rustdoc regressions.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 1, 2021
…ollie27

Fix primitive search in parameters and returned values

Part of rust-lang#60485.
Fixes rust-lang#74780.

Replacing rust-lang#74879.

cc `@camelid` `@jyn514` `@CraftSpider`
r? `@ollie27`
@bors bors closed this as completed in 461cbe4 Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-based-search Area: Searching rustdoc pages using type signatures C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
5 participants