rust icon indicating copy to clipboard operation
rust copied to clipboard

[rustdoc-json] Partially remove `paths` and introduce `external_index`

Open Urgau opened this issue 1 year ago • 6 comments

From [rustdoc-json] paths is inconsistent and questionably useful:

Problem: Currently, the items paths includes is just the list of paths used in the rustdoc cache. This is implementation-dependent, misses some items that should possibly be included such as re-export source items, and generally imperfect.

Solution: paths inclusion should be standardized, and separated from the internal cache implementation.

To solve this issue this PR introduce a new external_index that only contains a ExternalItem struct from the previous ItemSummary. This "new" index only contains external item and is here so that no ID are not dangling and to still provide a minimum of informations to those external IDs (crate_id, name, kind).

This PR also rename path to name (from ExternalItem) because it's not clear at all what path represent (is it one of the public paths but which one or is it a private one but why) so to simplify all this we will now just give the first external name of the item.

Fixes https://github.com/rust-lang/rust/issues/93522 r? @aDotInTheVoid cc @Enselic

Urgau avatar Oct 15 '22 10:10 Urgau

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

rustbot avatar Oct 15 '22 10:10 rustbot

While I agree that the current situation with paths isn't good, I'm not convinced that this is better, as theirs now no way to go from an external ID to that item it the json docs in that crate (you can't just use the id). This is an operation that we should support, untill https://github.com/rust-lang/rust/issues/99513 is resolved. (And even then, you'd need to add not only reexported foreign items, but all foreign items appearing in the public API, and all items exposed via them, and so in.)

An alternative would be to give all publicly accessible paths to an item, but it turns out this is impossible in the general case, as some items can have an infinite number of paths, some of which are infinitely long😱

aDotInTheVoid avatar Oct 17 '22 15:10 aDotInTheVoid

What about we keep path at least until https://github.com/rust-lang/rust/issues/99513 is resolved and after it is resolved we see if it's still useful to keep it. Would this address your concern ?

Urgau avatar Oct 17 '22 16:10 Urgau

Yes

aDotInTheVoid avatar Oct 17 '22 16:10 aDotInTheVoid

an infinite number of paths, some of which are infinitely long😱

Yikes! Excellent and terrifying example. This feels like it might be worth a lint in clippy? It wasn't obvious to me that this can happen, and I wouldn't be surprised if many tools (cargo-semver-checks included) assume the set of paths is finite.

obi1kenobi avatar Oct 17 '22 16:10 obi1kenobi

cargo public-api handles recursive re-exports, and also has tests in place to prevent regressing on that 😎

Enselic avatar Oct 17 '22 17:10 Enselic

Two Comments:

  1. I Don't think the format churn is worth it, as it's just renames. I'm tracking all the renames I want to do in #100961, and will do in in one big bunch to minimize the amount of new format versions.

  2. I'm not sure it's worth removing local items from paths/external_index. What advantage to users of the format does it acheive?

aDotInTheVoid avatar Nov 03 '22 13:11 aDotInTheVoid

Two Comments:

  1. I Don't think the format churn is worth it, as it's just renames. I'm tracking all the renames I want to do in Rustdoc-Json: More inconsistant names #100961, and will do in in one big bunch to minimize the amount of new format versions.

  2. I'm not sure it's worth removing local items from paths/external_index. What advantage to users of the format does it acheive?

The end goal was to completely remove the path field but at least for now it's not possible. The rest is inherent to this:

  • the renaming is inherent to non inclusion of local items, leaving it as paths would be misleading it could be names external_paths but it does include more that the path (crate_id and kind), external_items makes it very clear that only external items are included in it and that it's similar to an item.
  • removing the non local items would be the first step towards removing path, we cannot yet remove it as it's currently needed for workaround some issues.

Note that @jyn514 proposed on Zulip adding a method in rustdoc-json-types to get a full path of an Item. This would potential be the replacement for the path field.

Urgau avatar Nov 03 '22 13:11 Urgau