-
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] partially fix invalid files creation #112836
[rustdoc] partially fix invalid files creation #112836
Conversation
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
This comment has been minimized.
This comment has been minimized.
0eefe84
to
7e9529a
Compare
This comment has been minimized.
This comment has been minimized.
Found out the issue: it's because of re-exports, as usual. |
Awhile ago, I looked through a bunch of diagnostics code and found the infrastructure that the compiler uses to print visible and trimmed paths. Here’s the function that does it: It seems like, to use this for something other than printing, rustdoc will need to look in the Visible Parent Map, instead of using the “true” parent, to compute the path for linking: |
I found the solution: |
7e9529a
to
b574466
Compare
As long as you're sure it's only going to require splitting on |
Such types don't have their own page generated yet, so we can worry about it later on. Also I forgot to update some tests so CI will fail. ^^' |
This comment has been minimized.
This comment has been minimized.
src/librustdoc/clean/inline.rs
Outdated
once(crate_name) | ||
.chain(cx.tcx.def_path_str(did).split("::").map(|entry| Symbol::intern(entry)).skip(1)) | ||
.collect() |
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.
I don't think this function should ever be invoked with an associated item, but just to be on the safe side:
once(crate_name) | |
.chain(cx.tcx.def_path_str(did).split("::").map(|entry| Symbol::intern(entry)).skip(1)) | |
.collect() | |
let path_str = cx.tcx.def_path_str(did); | |
// This function should never be invoked on a trait method. | |
assert!(!path_str.contains('<')); | |
// This function should never be invoked on a closure or impl. | |
assert!(!path_str.contains('{')); | |
once(crate_name) | |
.chain(path_str.split("::").map(|entry| Symbol::intern(entry)).skip(1)) | |
.collect() |
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.
I reverted this part until the extern re-exports are correctly handled as mentioned above.
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.
You mean def_path_str
is not returning the path that would actually be used for accessing the item in the dependent crate?
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.
It actually returns the path used to import the item. Which doesn't work in case like:
pub mod quux {
extern crate issue_30109_1 as bar;
use self::bar::Bar;
pub trait Foo {}
// @has issue_30109/quux/trait.Foo.html \
// '//a/@href' '../issue_30109_1/struct.Bar.html'
impl Foo for Bar {}
}
In here, Bar
path will include bar
but also self
(in the form of the current crate name) whereas it definitely shouldn't.
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.
I see. Ick.
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.
Here's the PR and feature which should allow to fix this definitely at some point: #112853 (the PR doesn't fix it, just adds tests for the feature).
b574466
to
ad049fa
Compare
ad049fa
to
03be346
Compare
I updated the top-comment. Currently we still need redirection files until external re-exports are correctly "exported" by the compiler. Hopefully, it shouldn't take too long. :) |
CI passed this time. |
03be346
to
3ad595a
Compare
@bors r+ |
…e-creation, r=notriddle [rustdoc] partially fix invalid files creation Part of rust-lang#111249. It only removes generation for modules which shouldn't exist. For files, we need the compiler to keep re-export information alive for external items so we can actually have the right path to their location as it's currently not generating them correctly. In case the item is inlined, it shouldn't (and neither should its children) get a file generated. r? `@notriddle`
…e-creation, r=notriddle [rustdoc] partially fix invalid files creation Part of rust-lang#111249. It only removes generation for modules which shouldn't exist. For files, we need the compiler to keep re-export information alive for external items so we can actually have the right path to their location as it's currently not generating them correctly. In case the item is inlined, it shouldn't (and neither should its children) get a file generated. r? ``@notriddle``
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#99587 (Document memory orderings of `thread::{park, unpark}`) - rust-lang#112836 ([rustdoc] partially fix invalid files creation) - rust-lang#112853 (Add `lazy_type_alias` feature gate) - rust-lang#112863 (Fix copy-paste typo in `eprint(ln)` docs) - rust-lang#112883 (Make queries traceable again) - rust-lang#112885 (Fix msg passed to span_bug) - rust-lang#112886 (Revert 'Rename profile=user to profile=dist') r? `@ghost` `@rustbot` modify labels: rollup
Part of #111249. It only removes generation for modules which shouldn't exist. For files, we need the compiler to keep re-export information alive for external items so we can actually have the right path to their location as it's currently not generating them correctly.
In case the item is inlined, it shouldn't (and neither should its children) get a file generated.
r? @notriddle