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-json: Include GenericParamDefKind::Type::synthetic in JSON #94150

Merged
merged 2 commits into from
Mar 12, 2022

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Feb 19, 2022

The rustdoc JSON for

pub fn f(_: impl Clone) {}

will effectively be

pub fn f<impl Clone: Clone>(_: impl Clone) {}

where a synthetic generic parameter called impl Clone with generic trait bound
Clone is added to the function declaration.

The generated HTML filters out these generic parameters by doing
self.params.iter().filter(|p| !p.is_synthetic_type_param()), because the
synthetic generic paramter is not of interest to regular users.

For the same reason, we should expose whether or not a generic parameter is
synthetic or not also in the rustdoc JSON, so that rustdoc JSON clients can also
have the option to hide syntehtic generic parameters.

@rustbot modify labels: +A-rustdoc-json

@rust-highfive
Copy link
Collaborator

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider,@aDotInTheVoid

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 19, 2022
@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2022
@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Feb 19, 2022
Copy link
Member

@aDotInTheVoid aDotInTheVoid left a comment

Choose a reason for hiding this comment

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

This is a good change, and might help with #93803

@@ -329,7 +329,7 @@ pub struct GenericParamDef {
#[serde(rename_all = "snake_case")]
pub enum GenericParamDefKind {
Lifetime { outlives: Vec<String> },
Type { bounds: Vec<GenericBound>, default: Option<Type> },
Type { bounds: Vec<GenericBound>, default: Option<Type>, synthetic: bool },
Copy link
Member

Choose a reason for hiding this comment

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

This should be documented for what it means, because I can't find any reference to synthetic generic params outside the compiller.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I added docs now. I considered documenting other fields, but I think the structure and naming is pretty clear by themselves to someone that is familiar with the Rust type system. The diff is a bit big due to rustfmt though.

Also, big thanks for making the test much more robust.

It is also worth thinking about if we instead of adding synthetic should simply omit synthetic params from JSON. But, I suspect that if we go that route, that will cascade into current or future problems, such as the need to special case other code that relies on synthetic generic args being available.

Analogous to how it appears to be the case that omitting some private modules was a mistake (see #93518)

Copy link
Member

Choose a reason for hiding this comment

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

It is also worth thinking about if we instead of adding synthetic should simply omit synthetic params from JSON.

Yeah, the long term plan is to make params only be the names, and put all constraints in the where_predicates (#93803). But thats a larger change and we can land this now, to also improve the shape of the tests for when it lands

src/test/rustdoc-json/fns/generics.rs Outdated Show resolved Hide resolved
@Enselic Enselic force-pushed the synthetic-generic-parameters-in-json branch 2 times, most recently from 955bab5 to 1d4b30a Compare February 19, 2022 15:01
@rust-log-analyzer

This comment has been minimized.

@Enselic Enselic force-pushed the synthetic-generic-parameters-in-json branch 2 times, most recently from 9446484 to 9c43789 Compare February 25, 2022 19:40
@CraftSpider
Copy link
Contributor

Looks good, thank you for the PR.
@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2022

📌 Commit 9c43789 has been approved by CraftSpider

@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 Mar 3, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 4, 2022
…s-in-json, r=CraftSpider

rustdoc-json: Include GenericParamDefKind::Type::synthetic in JSON

The rustdoc JSON for

```
pub fn f(_: impl Clone) {}
```

will effectively be

```
pub fn f<impl Clone: Clone>(_: impl Clone) {}
```

where a synthetic generic parameter called `impl Clone` with generic trait bound
`Clone` is added to the function declaration.

The generated HTML filters out these generic parameters by doing
`self.params.iter().filter(|p| !p.is_synthetic_type_param())`, because the
synthetic generic paramter is not of interest to regular users.

For the same reason, we should expose whether or not a generic parameter is
synthetic or not also in the rustdoc JSON, so that rustdoc JSON clients can also
have the option to hide syntehtic generic parameters.

````@rustbot```` modify labels: +A-rustdoc-json
@aDotInTheVoid
Copy link
Member

aDotInTheVoid commented Mar 4, 2022

#94009 (being merged now) has become version 12, this will need to be version 13

@Dylan-DPC
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 4, 2022
Enselic and others added 2 commits March 4, 2022 05:54
The rustdoc JSON for

```
pub fn f(_: impl Clone) {}
```

will effectively be

```
pub fn f<impl Clone: Clone>(_: impl Clone)
```

where a synthetic generic parameter called `impl Clone` with generic
trait bound `Clone` is added to the function declaration.

The generated HTML filters out these generic parameters by doing
`self.params.iter().filter(|p| !p.is_synthetic_type_param())`, because
the synthetic generic parameter is not of interest to regular users.

For the same reason, we should expose whether or not a generic parameter
is synthetic or not also in the rustdoc JSON, so that rustdoc JSON
clients can also have the option to hide synthetic generic parameters.
@Enselic Enselic force-pushed the synthetic-generic-parameters-in-json branch from 9c43789 to a424f42 Compare March 4, 2022 04:58
@Enselic
Copy link
Member Author

Enselic commented Mar 4, 2022

I have now rebased and bumped FORMAT_VERSION.

You (and me) can confirm that FORMAT_VERSION is the only change compared to the approved commit, by using git range-diff.

% git range-diff 9c4378918d0^^..9c4378918d0 a424f42e97f^^..a424f42e97f
1:  87943cd3a31 ! 1:  aa763fcf421 rustdoc-json: Include GenericParamDefKind::Type::synthetic in JSON
    @@ src/rustdoc-json-types/lib.rs
      use serde::{Deserialize, Serialize};
      
      /// rustdoc format-version.
    --pub const FORMAT_VERSION: u32 = 11;
    -+pub const FORMAT_VERSION: u32 = 12;
    +-pub const FORMAT_VERSION: u32 = 12;
    ++pub const FORMAT_VERSION: u32 = 13;
      
      /// A `Crate` is the root of the emitted JSON blob. It contains all type/documentation information
      /// about the language items in the local crate, as well as info about external items to allow
2:  9c4378918d0 = 2:  a424f42e97f rustdoc-json: Make the `fns/generics.rs` test much more robust

9c4378918d0 comes from

📌 Commit 9c43789 has been approved by CraftSpider

and a424f42e97f comes from being HEAD of this PR. And ^^ comes from that this PR consists of two commits.

I didn't do anything for #94591 yet since it not clear what the consensus is on how to solve it, but I am fully on board with the need for some kind of solution eventually.

@Enselic
Copy link
Member Author

Enselic commented Mar 6, 2022

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot 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 Mar 6, 2022
@CraftSpider
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2022

📌 Commit a424f42 has been approved by CraftSpider

@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 Mar 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2022
…askrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#94150 (rustdoc-json: Include GenericParamDefKind::Type::synthetic in JSON)
 - rust-lang#94833 ([2/2] Implement macro meta-variable expression)
 - rust-lang#94863 (Remove redundant slicing of whole ranges in `bootstrap`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27e674d into rust-lang:master Mar 12, 2022
@bors
Copy link
Contributor

bors commented Mar 12, 2022

⌛ Testing commit a424f42 with merge f8a29bd...

@rustbot rustbot added this to the 1.61.0 milestone Mar 12, 2022
@Enselic Enselic deleted the synthetic-generic-parameters-in-json branch March 13, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend 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.

8 participants