-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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: Stop textually replacing Self
in doc links before resolving them
#93805
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
Looks good to me, thanks! Better have @camelid take a look too. |
This comment has been minimized.
This comment has been minimized.
29017ca
to
07f3429
Compare
r? @camelid |
Thanks for doing this! I've been wanting to fix this hack for a while. I'll review this soon. |
…g them Resolve it directly to a type / def-id instead. Also never pass `Self` to `Resolver`, it is useless because it's guaranteed that no resolution will be found.
07f3429
to
25c5e39
Compare
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this took so long. I only have one question; other than that, this looks great!
@@ -343,6 +346,7 @@ impl ItemFragment { | |||
|
|||
#[derive(Clone, Debug, Hash, PartialEq, Eq)] | |||
struct ResolutionInfo { | |||
item_id: ItemId, | |||
module_id: DefId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, could we remove module_id
now that we have item_id
? Not necessary to address now, but I wanted to bring it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without replacing it with some extra data, I tried it but wasn't able to do it right away.
The first issue is that rustdoc (unlike rustc) resolves paths differently in inner and outer attributes.
So item_id
will be the same for inner and outer paths, but their module_id
s will be different.
There are also may be nuances with how item_id
and module_id
are assigned for reexports, but I didn't investigate it.
I agree that ideally we would have an item_id
and some additional orthogonal flags here instead of the module_id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
@bors r=camelid,GuillaumeGomez |
📌 Commit 25c5e39 has been approved by |
Finished benchmarking commit (1661e4c): comparison url. Summary: This benchmark run shows 13 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
Wow, that regression is way higher than I expected. I thought this PR might improve perf. I still think this change is worth the regression, but do you know what caused the regression? Maybe the extra |
I'll try to benchmark this, but not earlier than landing the last part of #83761. |
It looks to me like collect_intra_doc_links got 8.8% slower on serde doc. It took 0.137 s before this PR, and 0.149 s after this PR. If I had to guess, I would assume the slowdown is due to the extra Its funny that the slowdown only registered on serde doc though. If you want to look further, @petrochenkov , then that would be great. But I don't think we need to spend too much time dissecting this, and I'm going to treat it as triaged. @rustbot label: +perf-regression-triaged |
|
This issue was most likely fixed by rust-lang#93805.
Add regression test for rust-lang#93205 Closes rust-lang#93205. This issue was most likely fixed by rust-lang#93805.
The report at rust-lang/rust#93205 was closed as it has presumably been fixed in rust-lang/rust#93805 which has long trickled down into stable releases, and I cannot reproduce the issue on `1.62.1` anymore (latest stable as of writing) 🎉 This workaround was originally added in #559.
The report at rust-lang/rust#93205 was closed as it has presumably been fixed in rust-lang/rust#93805 which has long trickled down into stable releases, and I cannot reproduce the issue on `1.62.1` anymore (latest stable as of writing) 🎉 This workaround was originally added in #559.
The report at rust-lang/rust#93205 was closed as it has presumably been fixed in rust-lang/rust#93805 which has long trickled down into stable releases, and I cannot reproduce the issue on `1.62.1` anymore (latest stable as of writing) 🎉 This workaround was originally added in #559.
The report at rust-lang/rust#93205 was closed as it has presumably been fixed in rust-lang/rust#93805 which has long trickled down into stable releases, and I cannot reproduce the issue on `1.62.1` anymore (latest stable as of writing) 🎉 This workaround was originally added in #559.
The report at rust-lang/rust#93205 was closed as it has presumably been fixed in rust-lang/rust#93805 which has long trickled down into stable releases, and I cannot reproduce the issue on `1.62.1` anymore (latest stable as of writing) 🎉 This workaround was originally added in #559.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
…omez rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang#93805 anyway.
rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. Fixes rust-lang/rust#108501. That issue could be fixed in a more local way, but this refactoring is something that I wanted to do since rust-lang/rust#93805 anyway.
Resolve it directly to a type / def-id instead.
Also never pass
Self
toResolver
, it is useless because it's guaranteed that no resolution will be found.This is a pre-requisite for #83761.