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] partially fix invalid files creation #112836

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jun 20, 2023

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2023

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Found out the issue: it's because of re-exports, as usual. ParseFloatError is re-exported from dec2flt because this module is private. However, dependencies don't know about that. The usual issue in short. Trying to find if there is a way to go around that, otherwise I'll have to revert some parts of this PR.

@notriddle
Copy link
Contributor

However, dependencies don't know about that.

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:

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/print/pretty/trait.PrettyPrinter.html#method.try_print_visible_def_path

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:

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.visible_parent_map

@GuillaumeGomez
Copy link
Member Author

I found the solution: def_path_str. Unless we want to rewrite the implementation in rustdoc directly (which I'm not a big fan of), I think it's better to simply parse the string.

@notriddle
Copy link
Contributor

Unless we want to rewrite the implementation in rustdoc directly (which I'm not a big fan of), I think it's better to simply parse the string.

As long as you're sure it's only going to require splitting on ::, and will never return a complex, fully-qualified path such as <MyType as Display>::fmt

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jun 20, 2023

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. ^^'

@rust-log-analyzer

This comment has been minimized.

src/librustdoc/html/render/context.rs Outdated Show resolved Hide resolved
tests/rustdoc/issue-111249-file-creation.rs Outdated Show resolved Hide resolved
Comment on lines 208 to 210
once(crate_name)
.chain(cx.tcx.def_path_str(did).split("::").map(|entry| Symbol::intern(entry)).skip(1))
.collect()
Copy link
Contributor

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:

Suggested change
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()

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 reverted this part until the extern re-exports are correctly handled as mentioned above.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Ick.

Copy link
Member Author

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).

@GuillaumeGomez GuillaumeGomez changed the title [rustdoc] fix invalid files creation [rustdoc] partially fix invalid files creation Jun 20, 2023
@GuillaumeGomez
Copy link
Member Author

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. :)

@GuillaumeGomez
Copy link
Member Author

CI passed this time.

@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 21, 2023

📌 Commit 3ad595a has been approved by notriddle

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 21, 2023
…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`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 21, 2023
…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``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 21, 2023
…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
@bors bors merged commit e100df9 into rust-lang:master Jun 21, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 21, 2023
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-invalid-file-creation branch June 21, 2023 17:23
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. 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.

5 participants