Skip to content

rustdoc_json: improve handling of generic args #142502

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

This PR fixes some inconsistencies and inefficiencies in how generic args are handled by rustdoc-json-types.

r? @aDotInTheVoid

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend 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 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2025

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, @Enselic, @obi1kenobi

@nnethercote
Copy link
Contributor Author

nnethercote commented Jun 15, 2025

Local measurements indicate some sub-1% instruction count improvements, and some max-rss improvements of up to 2%, in rustdoc-json perf.

@bors
Copy link
Collaborator

bors commented Jun 15, 2025

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

@nnethercote nnethercote force-pushed the rustdoc-json-GenericArgs branch from f232c4e to 27eee69 Compare June 15, 2025 04:17
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.

Behavioral changes make sense, but all of them needs tests.

They live in tests/rustdoc-json. Sadly, I've not finished the docs for it yet (rust-lang/rustc-dev-guide#2422), but there's a draft here

type/generic_default.rs and gats.rs are probably good examples

@aDotInTheVoid aDotInTheVoid 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 Jun 15, 2025
@nnethercote nnethercote force-pushed the rustdoc-json-GenericArgs branch from 27eee69 to f3c20ca Compare June 16, 2025 00:10
@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2025

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

  • It's intentional.
  • The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

@nnethercote
Copy link
Contributor Author

I added a test as a separate commit early in the sequence, so the changes to GenericArgs are visible in the changes to the test.

@rustbot ready

@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 Jun 16, 2025
@aDotInTheVoid
Copy link
Member

r=me after #142601 lands, and then this gets rebased to change the "Latest Change" comment.

@bors
Copy link
Collaborator

bors commented Jun 17, 2025

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

@nnethercote nnethercote force-pushed the rustdoc-json-GenericArgs branch from f3c20ca to 81a2aec Compare June 17, 2025 23:10
@nnethercote
Copy link
Contributor Author

I fixed the conflicts and updated the FORMAT_VERSION message.

@bors r=aDotInTheVoid

@bors
Copy link
Collaborator

bors commented Jun 17, 2025

📌 Commit 81a2aec has been approved by aDotInTheVoid

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 17, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 18, 2025
…gs, r=aDotInTheVoid

rustdoc_json: improve handling of generic args

This PR fixes some inconsistencies and inefficiencies in how generic args are handled by rustdoc-json-types.

r? `@aDotInTheVoid`
bors added a commit that referenced this pull request Jun 18, 2025
Rollup of 10 pull requests

Successful merges:

 - #135656 (Add `-Z hint-mostly-unused` to tell rustc that most of a crate will go unused)
 - #138237 (Get rid of `EscapeDebugInner`.)
 - #140772 ({aarch64,x86_64}-pc-windows-gnullvm: build host tools)
 - #140774 (Affirm `-Cforce-frame-pointers=off` does not override)
 - #141610 (Stabilize `feature(generic_arg_infer)`)
 - #141864 (Handle win32 separator for cygwin paths)
 - #142384 (Bringing `rustc_rayon_core` in tree as `rustc_thread_pool`)
 - #142502 (rustdoc_json: improve handling of generic args)
 - #142571 (Reason about borrowed classes in CopyProp.)
 - #142591 (Add spawn APIs for BootstrapCommand to support deferred command execution)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Collaborator

bors commented Jun 18, 2025

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

@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 Jun 18, 2025
@rust-bors
Copy link

rust-bors bot commented Jun 18, 2025

💔 Test failed

@nnethercote
Copy link
Contributor Author

There was a failure in the new tests for the type sizes, likely caused by rustc_randomized_layout. I will move those checks from the test into static assertions in src/librustdoc/json/mod.rs. This is much better: the assertions are static so you get a compile-time error (earlier) on failure; they can handle rustc_randomized_layout because they use static_assert_size; they are done in the same way as size assertion all throughout the rest of the codebase. I should have done them that way in the first place.

@nnethercote nnethercote force-pushed the rustdoc-json-GenericArgs branch from a172d78 to 2f916c5 Compare June 18, 2025 22:21
@nnethercote
Copy link
Contributor Author

@bors2 try jobs=x86_64-mingw-2

@rust-bors
Copy link

rust-bors bot commented Jun 18, 2025

⌛ Trying commit 2f916c5 with merge 53e44a5

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 18, 2025
rustdoc_json: improve handling of generic args

This PR fixes some inconsistencies and inefficiencies in how generic args are handled by rustdoc-json-types.

r? `@aDotInTheVoid`
try-job: x86_64-mingw-2
@rust-bors
Copy link

rust-bors bot commented Jun 18, 2025

💔 Test failed

A lot of these are large! Lots of room for improvement in the future.
A path without generic args, like `Reader`, currently has JSON produced
like this:
```
{"path":"Reader","id":286,"args":{"angle_bracketed":{"args":[],"constraints":[]}}}
```
Even though `types::Path::args` is `Option` and allows for "no args",
instead it gets represented as "empty args". (More like `Reader<>` than
`Reader`.)

This is due to a problem in `clean::Path::from_clean`. It only produces
`None` if the path is an empty string. This commit changes it to also
produce `None` if there are no generic args. The example above becomes:
```
{"path":"Reader","id":286,"args":null}
```
I looked at a few examples and saw this reduce the size of the JSON
output by 3-9%.

The commit also adds an assertion that non-final segments don't have any
generics; something the old code was implicitly relying on.

Note: the original sin here is that `clean::PathSegment::args` is not an
`Option`, unlike `{ast,hir}::PathSegment::args`. I want to fix that, but
it can be done separately.
As per the previous commit, generic args here can only appear on the
final segment. So make the comments obey that constraint.
They show up in three places: once as `Option<Box<GenericArgs>>`, once
as `Box<GenericArgs>`, and once as `GenericArgs`. The first option is
best. It is more compact because generic args are often missing. This
commit changes the latter two to the former.

Example output, before and after, for the `AssocItemConstraint` change:
```
{"name":"Offset","args":{"angle_bracketed":{"args":[],"constraints":[]}},"binding":{...}}
{"name":"Offset","args":null,"binding":{...}}
```
Example output, before and after, for the `Type::QualifiedPath` change:
```
{"qualified_path":{"name":"Offset","args":{"angle_bracketed":{"args":[],"constraints":[]}}, ...}}
{"qualified_path":{"name":"Offset","args":null, ...}}
```
This reduces JSON output size, but not by much (e.g. 0.5%), because
`AssocItemConstraint` and `Type::QualifiedPath` are uncommon.
@nnethercote nnethercote force-pushed the rustdoc-json-GenericArgs branch from 2f916c5 to 91818ba Compare June 19, 2025 03:17
@nnethercote
Copy link
Contributor Author

There was a failure in the new tests for the type sizes, likely caused by rustc_randomized_layout

Turns out it was because PathBuf is different sizes on different OSes. (The switch to static assertions was still a good one.) I have adjusted the Item size assertion accordingly.

@bors try

@bors
Copy link
Collaborator

bors commented Jun 19, 2025

⌛ Trying commit 91818ba with merge 93c2176...

bors added a commit that referenced this pull request Jun 19, 2025
rustdoc_json: improve handling of generic args

This PR fixes some inconsistencies and inefficiencies in how generic args are handled by rustdoc-json-types.

r? `@aDotInTheVoid`
@Kobzol
Copy link
Contributor

Kobzol commented Jun 19, 2025

@bors2 try jobs=x86_64-mingw-2

@rust-bors
Copy link

rust-bors bot commented Jun 19, 2025

⌛ Trying commit 91818ba with merge 31df07f

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 19, 2025
rustdoc_json: improve handling of generic args

This PR fixes some inconsistencies and inefficiencies in how generic args are handled by rustdoc-json-types.

r? `@aDotInTheVoid`
try-job: x86_64-mingw-2
@bors
Copy link
Collaborator

bors commented Jun 19, 2025

☀️ Try build successful - checks-actions
Build commit: 93c2176 (93c2176d5e5c4a5fce647f345a930c72a017e9a2)

@rust-bors
Copy link

rust-bors bot commented Jun 19, 2025

☀️ Try build successful (CI)
Build commit: 31df07f (31df07f8fae14bfe655d5749fecd8e13105cd6fe, parent: d1d8e386c5e84c4ba857f56c3291f73c27e2d62a)

@nnethercote
Copy link
Contributor Author

@bors r=aDotInTheVoid

@bors
Copy link
Collaborator

bors commented Jun 19, 2025

📌 Commit 91818ba has been approved by aDotInTheVoid

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2025
Kobzol added a commit to Kobzol/rust that referenced this pull request Jun 19, 2025
…gs, r=aDotInTheVoid

rustdoc_json: improve handling of generic args

This PR fixes some inconsistencies and inefficiencies in how generic args are handled by rustdoc-json-types.

r? `@aDotInTheVoid`
bors added a commit that referenced this pull request Jun 19, 2025
Rollup of 6 pull requests

Successful merges:

 - #141990 (Implement send_signal for unix child processes)
 - #142331 (Add `trim_prefix` and `trim_suffix` methods for both `slice` and `str` types.)
 - #142502 (rustdoc_json: improve handling of generic args)
 - #142629 (Add config builder for bootstrap tests)
 - #142687 (Reduce uses of `hir_crate`.)
 - #142714 (add comment to `src/bootstrap/build.rs`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Jun 19, 2025
Rollup of 10 pull requests

Successful merges:

 - #138291 (rewrite `optimize` attribute to use new attribute parsing infrastructure)
 - #141990 (Implement send_signal for unix child processes)
 - #142331 (Add `trim_prefix` and `trim_suffix` methods for both `slice` and `str` types.)
 - #142478 (install docs for each target in different directory)
 - #142571 (Reason about borrowed classes in CopyProp.)
 - #142668 (vec_deque/fmt/vec tests: remove static mut)
 - #142687 (Reduce uses of `hir_crate`.)
 - #142690 (expand: Remove some unnecessary generic parameters)
 - #142699 (Update books)
 - #142714 (add comment to `src/bootstrap/build.rs`)

Failed merges:

 - #142502 (rustdoc_json: improve handling of generic args)

r? `@ghost`
`@rustbot` modify labels: rollup
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.

5 participants