-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add missing impl blocks for item reexported from private mod in JSON output #103653
Add missing impl blocks for item reexported from private mod in JSON output #103653
Conversation
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.
Looks good, just some small nits around the test
@@ -0,0 +1,24 @@ | |||
// Regression test for <https://github.com/rust-lang/rust/issues/102583>. |
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.
Nit: This should be in src/test/rustdoc-json/reexport.rs
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.
👍
@@ -0,0 +1,24 @@ | |||
// Regression test for <https://github.com/rust-lang/rust/issues/102583>. | |||
|
|||
// @has "$.index[*][?(@.name=='missing_from_rustdoc_json')]" |
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.
Nit: Could you also check that S has an impl block, and the impl block has just the expected method
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.
So, for whatever reason I can't do:
// @has "$.index[*][?(@.kind=='impl' && @.inner.for.inner.name=='S')]"
but this one works:
// @has "$.index[*][?(@.kind=='impl')].inner.for.inner.name" '"S"'
I wanted to use it to use to first count the number of impl blocks for S
and then count the number of items in this impl. But since it's not possible, I'll just leave it as is I guess...
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.
// @has "$.index[*][?(@.kind=='impl' && @.inner.for.inner.name=='S')]"
doesn't work due to freestrings/jsonpath#91
The way I to work around this is to add a doc comment to the impl and select on that, eg
pub struct S;
//// impl S
impl S {
pub fn is_present() {}
}
// @set impl_S = "$.index[*][?(@.docs=='impl S')].id"
// @set is_present = "$.index[*][?(@.name=='is_present')].id"
// @is "$.index[*][?(@.name=='S')].inner.impls[*]" $impl_S
// @is "$.index[*][?(@.docs=='impl S')].inner.items[*]" $is_present
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.
Smart trick! That doesn't allow to see how many impl blocks are implemented on S
unfortunately but it's much better than currently. :)
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.
If theirs a second impls on S
, then it'll fail because several things match to // @is "$.index[*][?(@.name=='S')].inner.impls[*]" $impl_S
, but because this in #[no_core]
, their are no auto traits.
Similarly, this checks theirs only 1 method in the impl.
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.
Makes sense, thanks for the explanation! Then I guess it's ready for final review. :)
2ebd3ea
to
9c10ef0
Compare
Updated! I'm a bit sad I couldn't extend the test as I wanted but I guess it's enough for now. If you find a way to do it, I'd be very interested! |
Updated! Thanks for the suggestion @aDotInTheVoid ! |
9c10ef0
to
2345cd3
Compare
@bors r+ |
📌 Commit 2345cd3155e481bd39ad36c91a2d190edca099ed has been approved by It is now in the queue for this repository. |
☔ The latest upstream changes (presumably #102233) made this pull request unmergeable. Please resolve the merge conflicts. |
2345cd3
to
0ef36b8
Compare
@bors r=notriddle rollup |
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#102634 (compiletest: Refactor test rustcflags) - rust-lang#102721 (Prevent foreign Rust exceptions from being caught) - rust-lang#103415 (filter candidates in pick probe for diagnostics) - rust-lang#103618 (Rename some `OwnerId` fields.) - rust-lang#103625 (Accept `TyCtxt` instead of `TyCtxtAt` in `Ty::is_*` functions) - rust-lang#103653 (Add missing impl blocks for item reexported from private mod in JSON output) - rust-lang#103699 (Emit proper error when casting to `dyn*`) - rust-lang#103719 (fix typo in `try_reserve` method from `HashMap` and `HashSet`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…e-json, r=notriddle Add missing impl blocks for item reexported from private mod in JSON output Fixes rust-lang#102583. Since we don't inline for the JSON output, the impl blocks from private modules are not present when we generate the output. To go around this limitation, in case the impl block doesn't have `#[doc(hidden)]` and is implementing a public item, we don't strip it. cc `@fmease` `@aDotInTheVoid` r? `@notriddle`
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#102634 (compiletest: Refactor test rustcflags) - rust-lang#102721 (Prevent foreign Rust exceptions from being caught) - rust-lang#103415 (filter candidates in pick probe for diagnostics) - rust-lang#103618 (Rename some `OwnerId` fields.) - rust-lang#103625 (Accept `TyCtxt` instead of `TyCtxtAt` in `Ty::is_*` functions) - rust-lang#103653 (Add missing impl blocks for item reexported from private mod in JSON output) - rust-lang#103699 (Emit proper error when casting to `dyn*`) - rust-lang#103719 (fix typo in `try_reserve` method from `HashMap` and `HashSet`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #102583.
Since we don't inline for the JSON output, the impl blocks from private modules are not present when we generate the output. To go around this limitation, in case the impl block doesn't have
#[doc(hidden)]
and is implementing a public item, we don't strip it.cc @fmease @aDotInTheVoid
r? @notriddle