Rustdoc-Json: Document HRTB's on DynTrait #99787

aDotInTheVoid · 2022-07-27T00:06:17Z

Probably best reviewed commit by commit.

@rustbot modify labels: +A-rustdoc-json

aDotInTheVoid · 2022-07-27T00:07:22Z

src/rustdoc-json-types/lib.rs

+}
+
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
+pub struct PolyTrait {


I wasn't sure what the best name for this is. The compiler seems to use PolyTrait everywhere for this, but I don't think it's ever used outside that context.

Do you know what it means ?

A Trait with possible HRTB params

Don't hesitate to add doc comments to explain what it stands for. Always appreciated.

GuillaumeGomez · 2022-07-28T14:32:57Z

Looks good to me. Considering how big this change is, a second opinion would be very appreciated though. cc @camelid @notriddle

Enselic · 2022-07-28T17:18:29Z

Thanks you so much for fixing this. I have skimmed through the change and it looks good to me.

FYI, I have run the cargo-public-api test suite against this change and it found no regressions. The cargo-public-api test suite covers some cases the in-tree test suite does not (See e.g. cargo-public-api/cargo-public-api#67) so it's always nice to make sure nothing breaks for big changes like these, I think.

bors · 2022-07-29T22:23:59Z

☔ The latest upstream changes (presumably #99577) made this pull request unmergeable. Please resolve the merge conflicts.

Because python doesn't have lexical scope, loop variables persist after the loop is exited, set to the value of the last itteration ``` >>> i = 0 >>> for i in range(10): pass ... >>> i 9 ``` This causes the `ty` variable to be changed, causing unexpected crashes on ``` pub type RefFn<'a> = &'a dyn for<'b> Fn(&'a i32) -> i32; ```

aDotInTheVoid · 2022-07-30T19:25:41Z

Rebased onto master, updated documentation, renamed trait_ to trait with #[serde(rename)].

@rustbot ready

GuillaumeGomez · 2022-07-31T11:20:15Z

src/librustdoc/json/conversions.rs

@@ -130,11 +140,11 @@ impl FromWithTcx<clean::GenericArgs> for GenericArgs {
        use clean::GenericArgs::*;
        match args {
            AngleBracketed { args, bindings } => GenericArgs::AngleBracketed {
-                args: args.into_vec().into_iter().map(|a| a.into_tcx(tcx)).collect(),
-                bindings: bindings.into_iter().map(|a| a.into_tcx(tcx)).collect(),
+                args: args.into_vec().into_tcx(tcx),


It seems to be doing the same thing but instead of creating one vector, it creates two. Did I miss something?

Ah no nevermind. But then I wonder why the into_vec is needed at all...

Because it’s not a Vec. It’s a boxed slice.

Probably it could skip the into_vec because of the impl for IntoIterator right?

Like @camelid said. :)

args is a Box<[GenericArg]>, which doesn't impl IntoIterator, but Vec does and the conversion is zero cost. I guess you could impl <T, U: FromWithTcx<T>> FromWithTcx<Box<[T]>> for Vec<U>, but I don't think thats worth it as it only comes up twice.

Huh strange. Well if it doesn't work don't worry about it then.

camelid

The code changes overall look good to me, though there are a few things I'd like changed. I haven't reviewed the tests.

camelid · 2022-07-31T17:24:03Z

src/librustdoc/json/conversions.rs

@@ -130,11 +140,11 @@ impl FromWithTcx<clean::GenericArgs> for GenericArgs {
        use clean::GenericArgs::*;
        match args {
            AngleBracketed { args, bindings } => GenericArgs::AngleBracketed {
-                args: args.into_vec().into_iter().map(|a| a.into_tcx(tcx)).collect(),
-                bindings: bindings.into_iter().map(|a| a.into_tcx(tcx)).collect(),
+                args: args.into_vec().into_tcx(tcx),


Probably it could skip the into_vec because of the impl for IntoIterator right?

camelid · 2022-07-31T17:31:02Z

src/librustdoc/json/conversions.rs

+        tcx: TyCtxt<'_>,
+    ) -> Self {
+        PolyTrait {
+            trait_: clean::Type::Path { path: trait_ }.into_tcx(tcx),


Ideally we keep it in Path form rather than allowing it to be any type. (Type definition needs to be updated too.)

Suggested change

trait_: clean::Type::Path { path: trait_ }.into_tcx(tcx),

trait_: trait_.into_tcx(tcx),

We don't yet have a ResolvedPath type in the json output, so I think that should be done in a followup change, as theirs a couple of other places we'd also want to use it (

rust/src/librustdoc/json/conversions.rs

Line 423 in e141246

// FIXME: should `trait_` be a clean::Path equivalent in JSON?

,

rust/src/librustdoc/json/conversions.rs

Line 500 in e141246

let trait_ = clean::Type::Path { path: trait_ }.into_tcx(tcx);

)

Ok, makes sense to hold off then.

src/rustdoc-json-types/lib.rs

Closes rust-lang#99118

aDotInTheVoid · 2022-08-03T13:19:11Z

@rustbot ready

camelid · 2022-08-08T21:41:44Z

Ok @GuillaumeGomez this should be ready for your final review :)

GuillaumeGomez · 2022-08-09T09:26:25Z

Looks good to me, thanks!

@bors r=camelid,notriddle,GuillaumeGomez

bors · 2022-08-09T09:26:27Z

📌 Commit 6290f92 has been approved by camelid,notriddle,GuillaumeGomez

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#96478 (Implement `#[rustc_default_body_unstable]`) - rust-lang#99787 (Rustdoc-Json: Document HRTB's on DynTrait) - rust-lang#100181 (add method to get the mutability of an AllocId) - rust-lang#100221 (Don't document impossible to call default trait items on impls) - rust-lang#100228 (Don't ICE while suggesting updating item path.) - rust-lang#100301 (Avoid `&str` to `String` conversions) - rust-lang#100305 (Suggest adding an appropriate missing pattern excluding comments) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-lang/rust#99787

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 27, 2022

rust-highfive assigned CraftSpider Jul 27, 2022

rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Jul 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2022

aDotInTheVoid commented Jul 27, 2022

View reviewed changes

aDotInTheVoid mentioned this pull request Jul 27, 2022

rustdoc-json: Guard agains 2 commits making the same modification to FORMAT_VERSION #94591

Open

GuillaumeGomez approved these changes Jul 28, 2022

View reviewed changes

This was referenced Jul 28, 2022

update.sh: Make user, repo, and branch easy to change rust-lang/rustdoc-types#14

Merged

Properly render dyn ... (requires nightly-2022-08-10) cargo-public-api/cargo-public-api#86

Merged

aDotInTheVoid force-pushed the rdj-dyn branch from 1ffccb4 to f89660d Compare July 30, 2022 19:09

aDotInTheVoid added 2 commits July 30, 2022 20:13

Rustdoc-Json: Add tests for dyn and HRTBs

2143e48

aDotInTheVoid force-pushed the rdj-dyn branch from f89660d to e6c8b85 Compare July 30, 2022 19:23

GuillaumeGomez reviewed Jul 31, 2022

View reviewed changes

camelid requested changes Jul 31, 2022

View reviewed changes

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2022

aDotInTheVoid mentioned this pull request Aug 3, 2022

Rustdoc-Json: Add ResolvedPath type #100106

Closed

aDotInTheVoid added 3 commits August 3, 2022 14:17

Rustdoc-Json: Document HRTB's on DynTrait

a856e57

Closes rust-lang#99118

Rustdoc-Json: Extract convert_lifetime to function

625c4d7

Rustdoc-Json: Add and use FromWithTcx for Vec

6290f92

aDotInTheVoid force-pushed the rdj-dyn branch from e6c8b85 to 6290f92 Compare August 3, 2022 13:17

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2022

GuillaumeGomez approved these changes Aug 9, 2022

View reviewed changes

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 Aug 9, 2022

aDotInTheVoid mentioned this pull request Aug 9, 2022

new releases rust-lang/rustdoc-types#15

Closed

4 tasks

Dylan-DPC mentioned this pull request Aug 9, 2022

Rollup of 7 pull requests #100318

Merged

bors merged commit e41be25 into rust-lang:master Aug 9, 2022

rustbot added this to the 1.65.0 milestone Aug 9, 2022

aDotInTheVoid added a commit to rust-lang/rustdoc-types that referenced this pull request Aug 9, 2022

v0.13.0

94ce4c4

rust-lang/rust#99787

Enselic mentioned this pull request Aug 10, 2022

Support for nightly-2022-08-10 and later cargo-public-api/cargo-public-api#92

Closed

aDotInTheVoid mentioned this pull request Aug 10, 2022

Rustdoc-Json: Add Path type for traits. #100335

Merged

weiznich mentioned this pull request Aug 16, 2022

Rustdoc fails to build diesel documentation on current nightly #100620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustdoc-Json: Document HRTB's on DynTrait #99787

Rustdoc-Json: Document HRTB's on DynTrait #99787

aDotInTheVoid commented Jul 27, 2022

aDotInTheVoid Jul 27, 2022

Urgau Jul 27, 2022

aDotInTheVoid Jul 27, 2022

GuillaumeGomez Jul 28, 2022

GuillaumeGomez commented Jul 28, 2022

Enselic commented Jul 28, 2022

bors commented Jul 29, 2022

aDotInTheVoid commented Jul 30, 2022

GuillaumeGomez Jul 31, 2022

GuillaumeGomez Jul 31, 2022

notriddle Jul 31, 2022

camelid Jul 31, 2022

GuillaumeGomez Jul 31, 2022

aDotInTheVoid Aug 3, 2022

camelid Aug 8, 2022

camelid left a comment

camelid Jul 31, 2022

camelid Jul 31, 2022

aDotInTheVoid Aug 3, 2022 •

edited

Loading

camelid Aug 8, 2022

aDotInTheVoid commented Aug 3, 2022

camelid commented Aug 8, 2022

GuillaumeGomez commented Aug 9, 2022

bors commented Aug 9, 2022

	trait_: clean::Type::Path { path: trait_ }.into_tcx(tcx),
	trait_: trait_.into_tcx(tcx),

Rustdoc-Json: Document HRTB's on DynTrait #99787

Rustdoc-Json: Document HRTB's on DynTrait #99787

Conversation

aDotInTheVoid commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 28, 2022

Enselic commented Jul 28, 2022

bors commented Jul 29, 2022

aDotInTheVoid commented Jul 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aDotInTheVoid Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aDotInTheVoid commented Aug 3, 2022

camelid commented Aug 8, 2022

GuillaumeGomez commented Aug 9, 2022

bors commented Aug 9, 2022

aDotInTheVoid Aug 3, 2022 •

edited

Loading