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: Not enough info after paths change. #135600

Closed
aDotInTheVoid opened this issue Jan 16, 2025 · 2 comments · Fixed by #135799
Closed

rustdoc-json: Not enough info after paths change. #135600

aDotInTheVoid opened this issue Jan 16, 2025 · 2 comments · Fixed by #135799
Assignees
Labels
A-rustdoc-json Area: Rustdoc JSON backend T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@aDotInTheVoid
Copy link
Member

aDotInTheVoid commented Jan 16, 2025

@aDotInTheVoid I'm running into a serious issue making cargo-semver-checks support this format. I'd vote for reverting this change while we look for a better alternative — possibly by reworking what path is in paths.

All this information is available in the paths field, and there it's in a more usable form, as it also includes the defining crate.

This turns out to not be true in practice. The paths field includes the originally-defined path of the item, which is often private and not accessible. For example:

  • Where Path::name previously contained Iterator (accessible via the prelude), the paths lookup produces core::iter::traits::iterator::Iterator which is private.
  • Where Path::name previously contained std::str::SplitAsciiWhitespace, the paths lookup produces core::str::iter::SplitAsciiWhitespace which is private.
    etc.

This is making cargo-semver-checks unable to properly print types, function signatures, trait bounds, etc. and effectively disables a bunch of lints.

In theory, we could run c-s-c's importable paths analysis to derive a valid, publicly-importable path for the item instead of using the paths field. In practice, this is blocked in two ways:

  • It requires cross-crate analysis, since most such items are in std/core/alloc etc. This is a goal, but as we've already chatted, it's many months out and blocked on rustc functionality etc.
  • Even if we had cross-crate analysis, it requires the ability to either download or generate rustdoc JSON for std/core/alloc. I don't know of a way to generate it client-side, and AFAIK we can download their rustdoc JSON for nightly Rust only. This would break c-s-c's ability to support multiple stable Rust versions.

Originally posted by @obi1kenobi in #134880 (comment)

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 16, 2025
@aDotInTheVoid aDotInTheVoid added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-json Area: Rustdoc JSON backend and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 16, 2025
@aDotInTheVoid
Copy link
Member Author

MCVE:

#![no_std]

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

Which produces (abridged):

{
  "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
}

And there's no way to produce core::str::SplitAsciiWhitespace from this, which is what a load of tools really want. (And core::str::iter::SplitAsciiWhitespace isn't publicly accessible)

@aDotInTheVoid
Copy link
Member Author

@rustbot claim.

I'm going to submit a revert for #134880, then later we can try to reland (while still having the needed info somewhere)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 24, 2025
…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`
@bors bors closed this as completed in 9c0e32b Jan 24, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 24, 2025
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`
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 T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants