-
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
Correctly handle associated items of a trait inside a #[doc(hidden)]
item
#111095
Correctly handle associated items of a trait inside a #[doc(hidden)]
item
#111095
Conversation
This comment has been minimized.
This comment has been minimized.
f541f34
to
6998692
Compare
Fixed fmt issue. |
I don't think this entirely fixes the problem. It fixes the case where you specifically inline a trait from a hidden module, but these two test cases both fail: Test case for inlining a module that inherits hiddenness#![crate_name = "foo"]
// @!has 'foo/hidden/index.html'
// @!has 'foo/hidden/inner/index.html'
// @!has 'foo/hidden/inner/trait.Foo.html'
#[doc(hidden)]
pub mod hidden {
pub mod inner {
pub trait Foo {
/// Hello, world!
fn test();
}
}
}
// @has 'foo/visible/index.html'
// @has 'foo/visible/trait.Foo.html'
#[doc(inline)]
pub use hidden::inner as visible;
// @has 'foo/struct.Bar.html'
// @count - '//*[@id="impl-Foo-for-Bar"]' 1
pub struct Bar;
impl visible::Foo for Bar {
fn test() {}
} Test case for inlining an item that's two levels deep#![crate_name = "foo"]
// @!has 'foo/hidden/index.html'
// @!has 'foo/hidden/inner/index.html'
// @!has 'foo/hidden/inner/trait.Foo.html'
#[doc(hidden)]
pub mod hidden {
pub mod inner {
pub trait Foo {
/// Hello, world!
fn test();
}
}
}
// @has 'foo/trait.Foo.html'
#[doc(inline)]
pub use hidden::inner::Foo;
// @has 'foo/struct.Bar.html'
// @count - '//*[@id="impl-Foo-for-Bar"]' 1
pub struct Bar;
impl Foo for Bar {
fn test() {}
} |
@notriddle In the two tests you provided, you're underlining a limitation of this fix (which is great!) and another existing bug. I don't think they have the same origin though but I'll try to fix both here as well. After a lot of investigations, I'm still wondering whether or not we should store all reexports and then when we encounter an item that is inside a |
6998692
to
f664bb9
Compare
This comment has been minimized.
This comment has been minimized.
f664bb9
to
c9faa4b
Compare
This comment has been minimized.
This comment has been minimized.
c9faa4b
to
ac85277
Compare
@notriddle The first failing test you gave actually allowed me to uncover another bug: reexported modules were not handled in rustdoc. So that's two bugs fixed with this PR. For the extra files created, since the bug already exists, I'll fix it in a follow-up PR. I opened the issue for it here: #111249 |
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.
This seems better.
inherits_doc_hidden
's stop_at
parameter seems like a more complete fix. #[doc(hidden)]
is recursively applied, so it makes sense that a bug fix for it should impact the way the recursion is done and not just check one level. Thank you!
I have nits, but the approach seems sound.
local_def_id: LocalDefId, | ||
name: Symbol, | ||
import_id: Option<LocalDefId>, | ||
renamed: Option<Symbol>, |
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.
Why are name
and renamed
both needed? It seems like name
is completely ignored if renamed
is Some.
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 wondered if I should pass the "computed" name
or if I should do it in the function. In the end I picked this approach because it's not possible to pass the "non-computed" name since you need to pass both name
and renamed
.
ac85277
to
8de4308
Compare
Updated! |
@bors r+ rollup |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#110673 (Make alias bounds sound in the new solver (take 2)) - rust-lang#110747 (Encode types in SMIR) - rust-lang#111095 (Correctly handle associated items of a trait inside a `#[doc(hidden)]` item) - rust-lang#111381 (Keep encoding attributes for closures) - rust-lang#111408 (Fix incorrect implication of transmuting slices) - rust-lang#111410 (Switch to `EarlyBinder` for `thir_abstract_const` query) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #111064.
cc @compiler-errors
r? @notriddle