rustdoc-json: Dangling ID when private struct used in bound for pub-on-pub impl

[aDotInTheVoid]

With the code:

#![feature(no_core)]
#![no_core]

pub trait Trait {}

pub struct Pub;
struct Priv;

impl Trait for Pub where Priv: Trait {}

fails to valididate in jsondoclint:

---- [rustdoc-json] tests/rustdoc-json/impls/private_generic_bound.rs stdout ----

error: jsondoclint failed!
status: exit status: 1
command: "/home/gh-aDotInTheVoid/rust/build/aarch64-unknown-linux-gnu/stage0-tools-bin/jsondoclint" "/home/gh-aDotInTheVoid/rust/build/aarch64-unknown-linux-gnu/test/rustdoc-json/impls/private_generic_bound/private_generic_bound.json"
stdout: none
--- stderr -------------------------------
0:4:1635 not in index or paths, but referred to at '$.index["0:6"].inner.impl.generics.where_predicates[0].bound_predicate.type.resolved_path.id'
Error: Errors validating json /home/gh-aDotInTheVoid/rust/build/aarch64-unknown-linux-gnu/test/rustdoc-json/impls/private_generic_bound/private_generic_bound.json
------------------------------------------

Because it produces (redacted)

    "0:6": {
      "inner": {
        "impl": {
          "blanket_impl": null,
          "for": {"resolved_path": {"id": "0:2:1634", "name": "Pub"}},
          "generics": {
            "where_predicates": [
              {
                "bound_predicate": {
                  "bounds": [
                    {
                      "trait_bound": {"modifier": "none", "trait": {"id": "0:1:1633", "name": "Trait"}}
                    }
                  ],
                  "type": {"resolved_path": {"id": "0:4:1635", "name": "Priv"}}
                }
              }
            ]
          },
          "is_unsafe": false,
          "items": [],
          "negative": false,
          "provided_trait_methods": [],
          "synthetic": false,
          "trait": {"id": "0:1:1633", "name": "Trait"}
        }
      },
      "name": null
    }
  },

This pattern comes up in practice eg:

rust/library/core/src/iter/adapters/flatten.rs

Lines 139 to 146 in a161ab0

	unsafe impl<I, U, F> TrustedLen for FlatMap<I, U, F>
	where
	I: Iterator,
	U: IntoIterator,
	F: FnMut(I::Item) -> U,
	FlattenCompat<Map<I, F>, <U as IntoIterator>::IntoIter>: TrustedLen,
	{
	}

where TrustedLen and FlatMap are part of cores public API, but FlattenCompat isn't.

HTML get's arround this by not linking to the item:

I have no idea what the right thing to do here is design-wise.

[jyn514]

@HeroicKatora @obi1kenobi @LukeMathWalker what do you think is the right thing to do here? i expect removing the bound altogether will cause issues, and i do like that it's consistent with the HTML output, but in the other hand i don't think the HTML side was ever thought through too well and this makes it very hard to test for correctness.

@aDotInTheVoid can you remind me the reason the "no broken links" rule was added to jsondoclint? the idea was that we were missing public items in the output? maybe we can add the equivalent of -Z ui-testing that adds a list of items that are private and therefore expected to have broken links?

[LukeMathWalker]

It's an interesting one.
A potential solution would be to include the private item in the output, although that breaks the assumption that all items are public if you didn't use --document-private-items. It can also be messy since we would need to do it recursively (e.g. if Priv references other private items, those should be included as well to ensure we don't have broken links).

Another potential solution would be to include tombstone items—the link resolves to an "Omitted"-style item, which allows the code using the JSON to understand that it was not included due to the visibility used to generate docs, allowing it to return a useful error.

To confirm: everything works as expected if we run with --document-private-items?

[obi1kenobi]

For semver-checking, for a variety of reasons we actually have to run with both private and hidden items included. One such reason: if adding a private field causes a semver break, we want our error message to point to the problematic field specifically by name and span. So our IDs rarely dangle — though they still sometimes do — and we've written our code to be robust to dangling IDs just in case.

To a degree, users of rustdoc JSON have to be mindful of various flavors of non-resolvable (or not-easily-resolvable) links to other items, whether because of a dangling ID or because of cross-crate items which are also not included in the JSON file etc. So while I understand that this feels quite broken and offends our general sense of propriety and correctness, I personally don't consider it a particularly high-priority issue to fix or even decide on a firm design for fixing later.

In an ideal world, we wouldn't pick an approach with "Omitted"-style items since it feels likely to add more complexity and edge cases that could be hard to foresee. I'd love to get a sense of how much larger JSON files would get if we tried recursively adding Priv and any relevant items it itself directly references, since that'd be my preferred solution if the size increase isn't too bad.