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] Partially remove paths and introduce external_index #103085

Closed

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Oct 15, 2022

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.

r? @aDotInTheVoid
cc @Enselic

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 15, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2022

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

src/librustdoc/json/mod.rs Outdated Show resolved Hide resolved
@aDotInTheVoid
Copy link
Member

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 #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😱

@Urgau
Copy link
Member Author

Urgau commented Oct 17, 2022

What about we keep path at least until #99513 is resolved and after it is resolved we see if it's still useful to keep it. Would this address your concern ?

@aDotInTheVoid
Copy link
Member

Yes

@obi1kenobi
Copy link
Member

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.

@Enselic
Copy link
Member

Enselic commented Oct 17, 2022

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

@Urgau Urgau force-pushed the rustdoc-json-partialy-remove-paths branch 2 times, most recently from 80bc412 to 331a0d9 Compare October 19, 2022 15:15
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 29, 2022
…au,GuillaumeGomez

rustdoc-json-types: Improve ItemSummary::path docs

Somewhat inspired by the doc changes from rust-lang#103085 (cc `@Urgau)`

r? `@GuillaumeGomez`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 29, 2022
…au,GuillaumeGomez

rustdoc-json-types: Improve ItemSummary::path docs

Somewhat inspired by the doc changes from rust-lang#103085 (cc ``@Urgau)``

r? ``@GuillaumeGomez``
Introduce a new `external_index` that only contains a `ExternalItem`
struct from the previous `ItemSummary`. This "new" index only contains
external item and is only here so that IDs continue to referring to
an "item".

A previous version of this patch also removed `path` but at least until
rust-lang#99513 is fixed it is
unfortunately still required (as a workaround).
@Urgau Urgau force-pushed the rustdoc-json-partialy-remove-paths branch from 331a0d9 to 5c9982d Compare October 29, 2022 12:20
@Urgau Urgau requested a review from aDotInTheVoid November 2, 2022 13:12
@aDotInTheVoid
Copy link
Member

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?

@Urgau
Copy link
Member Author

Urgau commented Nov 3, 2022

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.

@bors
Copy link
Contributor

bors commented Jan 3, 2023

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 3, 2023
@Urgau Urgau closed this Jan 10, 2023
@Urgau Urgau deleted the rustdoc-json-partialy-remove-paths branch May 5, 2023 16:47
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-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants