rustdoc-json: change item ID's repr from a string to an int

[its-the-shrimp]

Following this discussion on Zulip, I've changed the repr of rustdoc_json_types::Id from a String to a u32, by adding a clean::ItemId interner to JsonRenderer

r? @aDotInTheVoid

[rustbot]

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

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

[obi1kenobi]

I'm broadly speaking quite unfamiliar with the librustdoc portion of the codebase, so apologies for my ignorant questions here.

[obi1kenobi]

Is there a way to recover the string ID from the numeric ID given a rustdoc JSON file? Or are you confident that isn't a capability that would be useful in any use case, so it isn't necessary to add?

[aDotInTheVoid]

What do you mean "recover the string ID"? If you want to find what the Id would have been under the old schem, then no, this won't be possible, but I'm fine with that.

Rustdoc makes no guarantees about the inner value of Id’s. Applications should treat them as opaque keys to lookup items, and avoid attempting to parse them, or otherwise depend on any implementation details.

[aDotInTheVoid]

This PR's doing alot, and needs major rework, so I've only tried to review it at a high level, as I don't think the in-the-weeds stuff is worth it yet.

1. The changes to ItemId are intrusive to most of the rustdoc codebase, and not motivated at all. If these are nessessary, could that be explained, and send in a different, tighter-scoped, PR (that would be landed before this)
2. Making Cache be the only source of state for the clean->json lowering is quite a bad idea:
   • It requires adding a TyCtxt, which produces more changes to the rest of rustdoc.
   • It adds the interner field which isn't used for HTML, but is still present, which is unfortunate.
   • In present, it requires adding a Lock, but I don't think this is inherent.
3. There seem to be unrelated cleanups (like the removal of FormatRenderer::cache). These are should be merged into rustdoc, but not here! Again, the answer is a smaller, seperate PR, that just does one thing, and will be much easier to review.

[jalil-salame]

Some tests are failing to compile due to them expecting the id to be Id(String): https://github.com/rust-lang/rust/actions/runs/10769404806/job/29860558315?pr=130078#step:25:6101

[bors]

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

[its-the-shrimp]

@rustbot ready

This time the interner is restricted to the JsonRenderer, however I still couldn't get rid of the lock on the interner because of borrow checking errors

[its-the-shrimp]

It could be possible to get rid of it if we were to change the way formats::run_format drives the renderer forward, currently it clones the renderer at least once (and for JSON it'll do so only once), which means the ID interner has to be in an Rc, which is unfortunate since that clone is completely redundant (I'll probably do that after this commit is merged)

[its-the-shrimp]

Or I can do it in this PR, the change should be very minimal

[its-the-shrimp]

should the FORMAT_VERSION be bumped for this?

[jalil-salame]

should the FORMAT_VERSION be bumped for this?

Most likely yes, it is an API change (rustdoc-types will be unable to parse files with Id(u32)).

[its-the-shrimp]

should the FORMAT_VERSION be bumped for this?

Most likely yes, it is an API change (rustdoc-types will be unable to parse files with Id(u32)).

The representation of IDs doesn't have any guarantees associated with it though, and rustdoc-types can get a new major version without incrementing FORMAT_VERSION

[obi1kenobi]

FORMAT_VERSION is how one would choose which rustdoc-types major version to use to deserialize a rustdoc JSON file. It would be a major issue for example for cargo-semver-checks if the format number doesn't get bumped here. Existing precedent is to bump it any time ~anything about the format changes. It's been bumped over much smaller changes in the past. I'd say go ahead and bump it here as well.

…

On Wed, Sep 11, 2024, 5:42 AM Tim Kurdov ***@***.***> wrote: should the FORMAT_VERSION be bumped for this? Most likely yes, it is an API change (rustdoc-types will be unable to parse files with Id(u32)). The representation of IDs doesn't have any guarantees associated with it though, and rustdoc-types can get a new major version without incrementing FORMAT_VERSION — Reply to this email directly, view it on GitHub <#130078 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5MSQB7MZAOCQVBPBZW6TZWAF6VAVCNFSM6AAAAABN2ESABOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBTGE2TGMJWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[aDotInTheVoid]

Yeah, bumping the format version needs to happen whenever we do something that breaks deserialisation (and I don’t know any times it’s been done for other reasons, but would be interested).

The representation of IDs doesn't have any guarantees associated with it though

Previously we guaranteed they were strings.