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: Also index impl Traits and raw pointers #92339

Closed
wants to merge 5 commits into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 28, 2021

dyn Traits and references are indexed, so these should be as well.

r? @GuillaumeGomez

@camelid camelid added the A-rustdoc-search Area: Rustdoc's search feature label Dec 28, 2021
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 28, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021
@@ -226,18 +226,25 @@ fn get_index_type_name(clean_type: &clean::Type, accept_generic: bool) -> Option
let path = &bounds[0].trait_;
Some(path.segments.last().unwrap().name)
}
clean::ImplTrait(ref bounds) => {
let first_trait = bounds.iter().find_map(|b| match b {
clean::GenericBound::TraitBound(poly_trait, _) => Some(&poly_trait.trait_),
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this will always pick the first trait bound, even if it's ?Sized or ~const Drop.

Copy link
Member

@GuillaumeGomez GuillaumeGomez Dec 28, 2021

Choose a reason for hiding this comment

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

It should return all traits, not just the first one. I have no idea what ~const Drop means and can't find it on the internet... If it's like !Drop, we shouldn't list it. For ?Sized, I'm not sure if we should list it or not...

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, though note that the pre-existing code only returns the first trait of a dyn Trait1 + Trait2 + ... type. Making it return all traits would be a bigger change; would you like me to attempt that here?

~const Drop: #67792

Copy link
Member

Choose a reason for hiding this comment

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

Making it return all traits would be a bigger change; would you like me to attempt that here?

Yes please!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@camelid camelid 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 Dec 28, 2021
@camelid camelid marked this pull request as draft December 28, 2021 21:59
@camelid
Copy link
Member Author

camelid commented Dec 28, 2021

Blocked on #92340. Merged now.

@rust-log-analyzer

This comment has been minimized.

`dyn Trait`s and references are indexed, so these should be as well.
Comment on lines -111 to +112
name: Option<String>,
generics: Option<Vec<TypeWithKind>>,
name: String,
generics: Vec<TypeWithKind>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Can this and similar refactorings have any effect on behavior? I found it quite bizarre that the search-index representation of a type can exist without a name.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible in case you have something like T: Display + Whatever. In this case, since T isn't a type, we don't keep its name. Please revert this change.

Copy link
Member Author

@camelid camelid Jan 7, 2022

Choose a reason for hiding this comment

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

This change didn't cause any tests to fail though. Could you please write a test case that passes on master and fails with this refactoring? Otherwise it'll be hard to not break this code accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

Please add one then. Something like (in rustdoc-js):

pub fn foo<Something: Debug>(s: Something) {}

Then look for Something. You shouldn't have any result.

Copy link
Contributor

Choose a reason for hiding this comment

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

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2022
@camelid
Copy link
Member Author

camelid commented Jan 4, 2022

Ready for review!

@camelid camelid marked this pull request as ready for review January 4, 2022 03:44
@camelid
Copy link
Member Author

camelid commented Jan 4, 2022

The last two commits are not necessary for this PR. I tacked them on since they're fairly small and self-contained. Let me know if you'd like me to split them into another PR.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2022
@camelid
Copy link
Member Author

camelid commented Jan 25, 2022

@GuillaumeGomez could you please review this sometime soon? :)

if is_full_generic {
// We remove the name of the full generic because we have no use for it.
index_ty.name = Some(String::new());
Copy link
Contributor

Choose a reason for hiding this comment

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

@GuillaumeGomez

It seems like, in the multi-generic case you were worried about, the old code didn't actually set the value to None. It actually slided the empty string into there instead.

On the one hand, this means we can get rid of the Option wrapper without breaking anything. On the other hand, this is really strange code.

Copy link
Contributor

@notriddle notriddle Jan 26, 2022

Choose a reason for hiding this comment

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

@camelid

For the record, I approve of this refactoring. If the generated JSON uses the empty string as its sentinel here (and it should, because it's smaller), then RenderType should just use the empty string here to match.

My point here is that the old code, which uses Some(""), is very strange. It even seemed to confuse its original author!

Copy link
Member

Choose a reason for hiding this comment

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

There is another open PR which fixes this part but can't find it... T_T

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 27, 2022
…ulti-trait, r=GuillaumeGomez

rustdoc: add test case for multiple traits and erased names

rust-lang#92339 (comment)
@bors
Copy link
Contributor

bors commented Jan 30, 2022

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

@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 31, 2022
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2022
@JohnCSimon JohnCSimon added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. 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-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2022
…uillaumeGomez

rustdoc: also index impl trait and raw pointers

Revives rust-lang#92339
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2022
…uillaumeGomez

rustdoc: also index impl trait and raw pointers

Revives rust-lang#92339
@notriddle
Copy link
Contributor

This is obsoleted by #97592, right?

@notriddle notriddle closed this Aug 24, 2022
@camelid
Copy link
Member Author

camelid commented Aug 26, 2022

Ah, I didn't see that PR! Thanks for reviving it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-search Area: Rustdoc's search feature S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants