rustdoc-json: Rename `Path::name` to `path`, and give it the path again. #135799

aDotInTheVoid · 2025-01-20T22:40:58Z

Reverts #134880 (Effectively, but not actually, as the FORMAT_VERSION needs to be bumped, changed docs/tests). CC @AS1100K.

Also CC @obi1kenobi @LukeMathWalker

Still needs before being merge-ready:

Tests for cross-crate paths
(Maybe) Document what the field does.
Decide if the field rename is good (rustdoc-json: Rename Path::name to path, and give it the path again. #135799 (comment))
Squash commits.

r? @GuillaumeGomez

obi1kenobi

Thank you for putting this together! 🙏

obi1kenobi · 2025-01-20T23:09:13Z

tests/rustdoc-json/path_name.rs

+//@ is "$.index[*][?(@.name=='U3')].inner.type_alias.type.resolved_path.name" '"pub_mod::InPubMod"'
+pub type U3 = InPubMod3;
+
+// Check we only have paths for structs at there origonal path


Suggested change

// Check we only have paths for structs at there origonal path

// Check we only have paths for structs at their original path

AS1100K · 2025-01-21T03:18:18Z

Thanks for this PR! A revert was necessary because we need to prevent #134880 from being included in the beta, as it would break tools and we cannot change paths.path from absolute path to public facing path because it is required for generating source URLs by rustdoc and other crates.

aDotInTheVoid · 2025-01-21T08:52:42Z

A revert was necessary because we need to prevent #134880 from being included in the beta, as it would break tools

N.B: Rustdoc-Json is currently unstable, and not usable on beta/stable. The purpose of the revert is to fix nightly.

we cannot change paths.path from absolute path to public facing path because it is required for generating source URLs by rustdoc and other crates.

I’m not sure i follow.

LukeMathWalker · 2025-01-21T08:58:02Z

I don't follow the last part either—rustdoc's HTML backend doesn't build on top of the JSON output AFAIK. Even if it did, it doesn't expose private paths for public items.

AS1100K · 2025-01-21T09:29:08Z

we cannot change paths.path from absolute path to public facing path because it is required for generating source URLs by rustdoc and other crates.

Sorry, I wasn't clear. What I meant was

For the code:

#![no_std]

pub type Foo = core::str::SplitAsciiWhitespace<'static>;

The json output will be

{
  "index": {
    "0": {
      "id": 0,
      "inner": {
        "type_alias": {
          "generics": {"params": [], "where_predicates": []},
          "type": {
            "resolved_path": {
              "args": {"angle_bracketed": {"args": [{"lifetime": "'static"}], "constraints": []}},
              "id": 1,
              "name": "SplitAsciiWhitespace"
            }
          }
        }
      },
      "name": "Foo"
    },
    "2": {
      "id": 2,
      "inner": {"module": {"is_crate": true, "is_stripped": false, "items": [0]}},
      "name": "mcve"
    }
  },
  "paths": {
    "0": {"crate_id": 0, "kind": "type_alias", "path": ["mcve", "Foo"]},
    "1": {"crate_id": 1, "kind": "struct", "path": ["core", "str", "iter", "SplitAsciiWhitespace"]}
  },
  "root": 2
}

Here, "path": ["core", "str", "iter", "SplitAsciiWhitespace"] was refered as path.path. I said it because this field is in ItemSummary struct which is constructed from Cache here which is used by HTML backend to get the absolute path i.e. "core", "str", "iter", "SplitAsciiWhitespace" as per above example to get the URL for source file that would be core/str/iter.rs.html and I thought of replacing this path with public facing path that would be core::str::SplitAsciiWhitespace but then thought absolute path could also be important for other libaries (like i am building cargo-wiki) to use this absolute path to generate URL on docs.rs or some other mechanism they might want.

I hope I made it clear but if not please ask me, I might be wrong so please let me know.

aDotInTheVoid · 2025-01-21T23:12:55Z

rustdoc's HTML backend doesn't build on top of the JSON output AFAIK.

Correct, both of them built of clean, a rustdoc internal IR (that's in turn built from HIR & MIR)

as per above example to get the URL for source file that would be ...

For source URL's, rustdoc-html goes through the Span infrastructure (as should rustdoc-json consumers): See https://github.com/rust-lang/rust/blob/ed43cbcb882e7c06870abdd9305dc1f17eb9bab9/src/librustdoc/html/sources.rs for details.

The issue is that paths comes from an implementation detail of HTML (see #93522), so it's kinda arbitrary what's there. As some point it'll need to be properly thought through (possibly alongside the long-awaited cross-crate id solution.

aDotInTheVoid · 2025-01-21T23:18:56Z

A though while doing the docs: Should this field be renamed to not name? Having name: String actually be a path seems really surprising and goofy. How'd people feel about it being path instead?

rustbot · 2025-01-21T23:34:22Z

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

aDotInTheVoid · 2025-01-21T23:36:17Z

@GuillaumeGomez the code can be reviewed as-is, it's ready. But don't merge it yet, I want to squash commits before that. (But don't want to yet, incase we decide to not do the field rename, as it'd be easier to revert).

LukeMathWalker · 2025-01-22T05:51:07Z

A though while doing the docs: Should this field be renamed to not name? Having name: String actually be a path seems really surprising and goofy. How'd people feel about it being path instead?

It'd definitely be a better match for what it currently contains. It works for me.

obi1kenobi · 2025-01-22T16:14:02Z

src/rustdoc-json-types/lib.rs

+    /// This will be the path where the type is *used*, so it may be a
+    /// re-export.


Suggestion, feel free to take it or leave it: I'm not 100% sure what "used" means in this context. Is it the path referenced in the code being documented, either directly or via an import? AKA "what path would we have to remove to break the code being documented"

Perhaps an example could help?

Added an example:

pub type Vec1 = std::vec::Vec<i32>; // path: "std::vec::Vec" pub type Vec2 = Vec<i32>; // path: "Vec" pub type Vec3 = std::prelude::v1::Vec<i32>; // path: "std::prelude::v1::Vec"

Closes rust-lang#135600 Effectivly reverts rust-lang#134880

aDotInTheVoid · 2025-01-22T19:33:15Z

Docs improved. Commits squashed.

obi1kenobi · 2025-01-23T20:45:42Z

Looks good to me, thank you! Seems ready to merge, if I'm not mistaken?

I'm eager to use it in cargo-semver-checks ASAP, so that we narrow the window of nightlies with which we're incompatible :)

GuillaumeGomez · 2025-01-23T22:39:26Z

I already approve so it's up to @aDotInTheVoid whenever they want. :)

aDotInTheVoid · 2025-01-23T22:48:10Z

@bors r=GuillaumeGomez rollup

(Sorry, I didn't realize github's green check meant r=you here. I don't think there's a norm here)

bors · 2025-01-23T22:48:12Z

📌 Commit 9c0e32b has been approved by GuillaumeGomez

It is now in the queue for this repository.

GuillaumeGomez · 2025-01-23T22:51:11Z

Ah sorry, miscommunication I think. You said you wanted my approval but not my merge because you wanted to make another small change. So I approved and for me it was all done. Should have precised. ^^'

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#133605 (Add extensive set of drop order tests) - rust-lang#135489 (remove pointless allowed_through_unstable_modules on TryFromSliceError) - rust-lang#135757 (Add NuttX support for AArch64 and ARMv7-A targets) - rust-lang#135799 (rustdoc-json: Rename `Path::name` to `path`, and give it the path again.) - rust-lang#135865 (For E0223, suggest associated functions that are similar to the path, even if the base type has multiple inherent impl blocks.) - rust-lang#135890 (Implement `VecDeque::pop_front_if` & `VecDeque::pop_back_if`) - rust-lang#135914 (Remove usages of `QueryNormalizer` in the compiler) - rust-lang#135936 (fix reify-intrinsic test) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#135799 - aDotInTheVoid:skrrt-skrrt-revrrt, r=GuillaumeGomez rustdoc-json: Rename `Path::name` to `path`, and give it the path again. Closes: rust-lang#135600. Reverts rust-lang#134880 (Effectively, but not actually, as the `FORMAT_VERSION` needs to be bumped, changed docs/tests). CC `@AS1100K.` Also CC `@obi1kenobi` `@LukeMathWalker` Still needs before being merge-ready: - [x] Tests for cross-crate paths - [x] (Maybe) Document what the field does. - [x] Decide if the field rename is good (rust-lang#135799 (comment)) - [ ] Squash commits. r? `@GuillaumeGomez`

…meGomez rustdoc-json: Rename `Path::name` to `path`, and give it the path again. Closes: #135600. Reverts #134880 (Effectively, but not actually, as the `FORMAT_VERSION` needs to be bumped, changed docs/tests). CC `@AS1100K.` Also CC `@obi1kenobi` `@LukeMathWalker` Still needs before being merge-ready: - [x] Tests for cross-crate paths - [x] (Maybe) Document what the field does. - [x] Decide if the field rename is good (rust-lang/rust#135799 (comment)) - [ ] Squash commits. r? `@GuillaumeGomez`

rustbot assigned GuillaumeGomez Jan 20, 2025

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 Jan 20, 2025

This comment has been minimized.

Sign in to view

obi1kenobi approved these changes Jan 20, 2025

View reviewed changes

aDotInTheVoid marked this pull request as ready for review January 21, 2025 23:34

aDotInTheVoid changed the title ~~[WIP] rustdoc-json: Made Path::name have the full name again.~~ rustdoc-json: Rename Path::name to path, and give it the path again. Jan 21, 2025

This comment has been minimized.

Sign in to view

obi1kenobi reviewed Jan 22, 2025

View reviewed changes

GuillaumeGomez approved these changes Jan 22, 2025

View reviewed changes

rustdoc-json: Rename Path::name to path, and give it path (again).

9c0e32b

Closes rust-lang#135600 Effectivly reverts rust-lang#134880

rust-cloud-vms bot force-pushed the skrrt-skrrt-revrrt branch from 707ef52 to 9c0e32b Compare January 22, 2025 19:32

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 Jan 23, 2025

matthiaskrgr mentioned this pull request Jan 24, 2025

Rollup of 8 pull requests #135978

Merged

bors merged commit 40e2858 into rust-lang:master Jan 24, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc-json: Rename `Path::name` to `path`, and give it the path again. #135799

rustdoc-json: Rename `Path::name` to `path`, and give it the path again. #135799

aDotInTheVoid commented Jan 20, 2025 •

edited

Loading

This comment has been minimized.

obi1kenobi left a comment

obi1kenobi Jan 20, 2025

AS1100K commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025 •

edited

Loading

LukeMathWalker commented Jan 21, 2025

AS1100K commented Jan 21, 2025 •

edited

Loading

aDotInTheVoid commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025 •

edited

Loading

rustbot commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025

This comment has been minimized.

LukeMathWalker commented Jan 22, 2025

obi1kenobi Jan 22, 2025

aDotInTheVoid Jan 22, 2025

obi1kenobi Jan 22, 2025

aDotInTheVoid commented Jan 22, 2025

obi1kenobi commented Jan 23, 2025

GuillaumeGomez commented Jan 23, 2025

aDotInTheVoid commented Jan 23, 2025

bors commented Jan 23, 2025

GuillaumeGomez commented Jan 23, 2025

	// Check we only have paths for structs at there origonal path
	// Check we only have paths for structs at their original path

		/// This will be the path where the type is used, so it may be a
		/// re-export.

rustdoc-json: Rename Path::name to path, and give it the path again. #135799

rustdoc-json: Rename Path::name to path, and give it the path again. #135799

Conversation

aDotInTheVoid commented Jan 20, 2025 • edited Loading

This comment has been minimized.

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi Jan 20, 2025

Choose a reason for hiding this comment

AS1100K commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025 • edited Loading

LukeMathWalker commented Jan 21, 2025

AS1100K commented Jan 21, 2025 • edited Loading

aDotInTheVoid commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025 • edited Loading

rustbot commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025

This comment has been minimized.

LukeMathWalker commented Jan 22, 2025

obi1kenobi Jan 22, 2025

Choose a reason for hiding this comment

aDotInTheVoid Jan 22, 2025

Choose a reason for hiding this comment

obi1kenobi Jan 22, 2025

Choose a reason for hiding this comment

aDotInTheVoid commented Jan 22, 2025

obi1kenobi commented Jan 23, 2025

GuillaumeGomez commented Jan 23, 2025

aDotInTheVoid commented Jan 23, 2025

bors commented Jan 23, 2025

GuillaumeGomez commented Jan 23, 2025

rustdoc-json: Rename `Path::name` to `path`, and give it the path again. #135799

rustdoc-json: Rename `Path::name` to `path`, and give it the path again. #135799

aDotInTheVoid commented Jan 20, 2025 •

edited

Loading

aDotInTheVoid commented Jan 21, 2025 •

edited

Loading

AS1100K commented Jan 21, 2025 •

edited

Loading

aDotInTheVoid commented Jan 21, 2025 •

edited

Loading