rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.34k stars 12.72k forks source link

[rustdoc-json] `paths` is inconsistent and questionably useful #93522

Open CraftSpider opened 2 years ago

CraftSpider commented 2 years ago

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.

dsherret commented 2 years ago

questionably useful

I was using paths to get the fully resolved name of types by their id. Within the past two months it seems this has gotten really unstable. For example, doing something like...

// lib.rs
mod expr;

pub use expr::Expr;

pub enum Main {
    Test,
}

// expr.rs
pub enum Expr {
  Test,
}

...will now only have the Main enum in "paths" and not also Expr, though this previously included both. I guess it has something to do with the cache. I guess I should compute this upfront by traversing everything in index instead (this is very difficult to do).

Urgau commented 2 years ago

I would be in favor of removing it completely. Handling re-export is very complicated and even if we were to handle them I still don't see how it could distinguiesh between the "real" location and what the "user" might want.

It could see be re-introduce later but in the mean time I would be I favor of just removing it.

Urgau commented 2 years ago

@CraftSpider @Enselic I'm planning on submitting a PR to remove paths, any objection from your part ?

Enselic commented 2 years ago

@Urgau Sounds good to me! I don't use it and don't plan to.

obi1kenobi commented 1 year ago

https://github.com/obi1kenobi/cargo-semver-checks/issues/202 ran into an interesting real-world edge case that might be useful for guiding future work in this area.

The affected crate, libp2p-dcutr, includes three different pub enum UpgradeError types in three different modules. One of those modules is public, and the other two are private. The root lib.rs file publicly re-exports and renames the UpgradeError types from the private modules, so all three are accessible under different names.

Here's one of those enums: https://github.com/libp2p/rust-libp2p/blob/1c2712c1bc288dc608aaec5fc3458b0d07181feb/protocols/dcutr/src/protocol/outbound.rs#L118 Here's how it's publicly re-exported and renamed: https://github.com/libp2p/rust-libp2p/blob/1c2712c1bc288dc608aaec5fc3458b0d07181feb/protocols/dcutr/src/lib.rs#L31

Currently, rustdoc JSON v22 and v23 do not include the re-exported items in paths, which I believe is a known issue.

They do however include the renaming re-export:

"0:311": {
    "id": "0:311",
    "crate_id": 0,
    "name": null,
    "span": {
        "filename": "protocols/dcutr/src/lib.rs",
        "begin": [
            31,
            50
        ],
        "end": [
            31,
            96
        ]
    },
    "visibility": "public",
    "docs": null,
    "links": {},
    "attrs": [],
    "deprecation": null,
    "kind": "import",
    "inner": {
        "source": "protocol::outbound::UpgradeError",
        "name": "OutboundUpgradeError",
        "id": "0:377:1572",
        "glob": false
    }
}

The https://github.com/obi1kenobi/cargo-semver-checks/issues/202 is caused by the difficulty in matching up "the same item" across two different versions. Our current approach fails since it doesn't account for renames, and ends up comparing two enums that happen to be defined with the same name but are "not the same enum" across versions. This is certainly a bug in cargo-semver-checks alone and not in rustdoc JSON, and nothing I write here should be construed to imply otherwise. Matching up "the same items" across versions is quite a tricky problem, and I wanted to put it on your radar as a thing to consider while building the future of rustdoc.