Add cargo semver-checks testsuite to rustdoc JSON output

[jyn514]

Rustdoc has a ... not-very-comprensive test suite. cargo-semver-checks has much more extensive tests. It would be really nice if we could test that we don't accidentally regress the JSON output (or the other output for that matter; I suspect most breakage affects both backends).

I think the easiest way to do this is to add a new tests/rustdoc-semver-checks submodule with a dependency on https://github.com/obi1kenobi/cargo-semver-checks. Then in src/bootstrap/test.rs, we'll add a new test suite that runs cargo test on cargo-semver-checks using the in-tree version of rustdoc to make sure there's no incompatibilities.

I am not sure how this should deal with breaking changes to the JSON format - maybe we can use toolstate which allows the tests to be broken until semver-checks supports the new version?

cc @obi1kenobi @rust-lang/rustdoc

[Manishearth]

Big +1

[Manishearth]

I am not sure how this should deal with breaking changes to the JSON format - maybe we can use toolstate which allows the tests to be broken until semver-checks supports the new version?

We could, though semver-checks isn't an official tool. I'm also fine with having an internal toggle that we can use, and we try and coordinate with semver-checks as much as possible.

[jyn514]

I think if we're going to have some "it's ok to break this testsuite" state we should reuse the toolstate mechanisms. We don't need to rewrite our own const LET_SEMVER_CHECKS_BREAK = true; mechanism and integrate it with CI somehow.

[obi1kenobi]

Adding support for new format versions is generally extremely straightforward. I'd be happy to give folks involved with this effort push privs on cargo-semver-checks / trustfall-rustdoc-adapter / trustfall-rustdoc and write up the steps to follow so new formats can be supported ASAP. I obviously already try my best to add support quickly, but it's a known result in systems engineering that "min of multiple latencies" improves overall latency.

[Manishearth]

Yeah the mechanism seems fine, my worry is more about what we show publicly on toolstate.

[jyn514]

ah, gotcha - I think it should be possible to add some toggle to the toolstate website that hides semver-checks from the public site.

[aDotInTheVoid]

Talked about this on zulip, but TLDR:

• This can't be enforced by CI, as it makes breaking changes onerous
• If it's not in CI, unclear how much better is is than getting issues when these suites chatch them on nightly
• Would be nice to know how to run the test suites locally, with stage2 rustdoc
• Possible to use these testsuites to find gaps in our testsuite's coverage, and it's a parallel + good first issue to port them, after I write docs.

[Manishearth]

Actually this is a reason why I'm even more skeptical toolstate is the right tool for the job here: having I feel like "enforced by CI by easily disabled" is the best option. Toolstate is very much for cases where you expect breakage and want a dashboard of breakage, here we also want to catch unintentional breakage. For that we need CI, and the CI needs to be something you can disable when you do intend to break things.

[obi1kenobi]

From a purely technical perspective, I think "enforced by CI so long as the rustdoc format version number is still X" where X is a hardcoded constant representing the largest version supported by trustfall-rustdoc-adapter / cargo-semver-checks could be done. When the version number changes, this would auto-disable that CI, and it would need to be manually re-enabled after new format versions are released. I'd be happy to open PRs for that re-enabling, and it should also be quite easy to write a bot to do it automatically as well.

[Manishearth]

When you say rustdoc version do you mean rustdoc-json format version?

But yes that would work.

[obi1kenobi]

Yes, format version — edited above to add that.

[obi1kenobi]

If anyone is curious about the kinds of test cases that are part of trustfall-rustdoc-adapter and cargo-semver-checks, here I'm adding more cursed Rust: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/pull/172/files#diff-2f839cd5fedffb289957c24c103dbe0f8ac63ed3158f177549fafe83e9781806R1

The bug in this case was in my code, not in rustdoc. It was found by running cargo-semver-checks across all versions of a large number of Rust crates in the ecosystem, followed by a combination of automatic and manual triage of the results. This process is ongoing and I'm happy to share more information if you're curious.

For an example of a Rust (maybe-)bug found as a result of the same run, see #110831.

It seems that "run lots of rustdoc-based queries on lots of crates then look for weird outcomes" is a viable strategy for proactive bug-finding in Rust.

[aDotInTheVoid]

Relevant to if we ever do this: rust-lang/compiler-team#845. But I don't think this is particularly urgent. To my knowledge there's never been a regression that csc's test-suite would have caught.

That said, I'd still love to have a way written down to run the csc tests against in-tree rustdoc.

[obi1kenobi]

Over the last few days I've been boosting our test coverage around attributes, so that the list of attributes cargo-semver-checks relies on is fully tested between the test suites of cargo-semver-checks and trustfall-rustdoc-adapter. I felt it would make it easier to ensure compatibility with the upcoming attribute changes, and could plausibly help developing rustdoc JSON too.

For both cargo-semver-checks and trustfall-rustdoc-adapter, running the tests is a two-step process:

• Run ./scripts/regenerate_test_rustdocs.sh to produce rustdoc JSON in a local (git-ignored) directory.
• Run cargo test which uses that generated JSON.

Running with in-tree rustdoc would just require figuring out how to amend the script to invoke the in-tree rustdoc instead of the installed one. I imagine that shouldn't be difficult?

[aDotInTheVoid]

and could plausibly help developing rustdoc JSON too.

❤ yeah, this would be.

• Run ./scripts/regenerate_test_rustdocs.sh to produce rustdoc JSON in a local (git-ignored) directory.

Ah, that should make it easy.

Running with in-tree rustdoc would just require figuring out how to amend the script to invoke the in-tree rustdoc instead of the installed one. I imagine that shouldn't be difficult?

Yeah, you create a rustup toolchain that points to your in-tree, either with https://rustc-dev-guide.rust-lang.org/building/how-to-build-and-run.html#creating-a-rustup-toolchain or link_rust.sh. Then you can use rustdoc +rust_stage2 ... / cargo +rust_stage2 ...

[obi1kenobi]

Nice! The script already supports calling with a custom toolchain (like ./scripts/regenerate_test_rustdocs.sh +nightly which passes the arg to cargo) so barring something unforeseen, it should be as simple as ./scripts/regenerate_test_rustdocs.sh +rust_stage2 && cargo test.

[obi1kenobi]

While opening PRs to test item attribute representation in rustdoc JSON, I seem to have stumbled upon an example of something rustdoc JSON tests cannot seem to test today, but cargo-semver-checks and trustfall-rustdoc-adapter can and do test already.

Consider the following code:

/// The generated `impl Clone` has `#[automatically_derived]` applied on it.
#[derive(Clone)]
pub struct Example(i64);

Ideally, we'd use a statement like the following to create a rustdoc JSON test that ensures the attribute is indeed present on the generated impl:

//@ is "$.index[*][?(@.inner.impl.trait.path=='Clone')].attrs" '["#[automatically_derived]"]'
// possibly also with an additional clause near ^ here, to make sure it's `Example`'s `impl Clone`

Unfortunately, this doesn't work because of bug freestrings/jsonpath#91 — AFAICT any predicate with more than two dot operators just silently doesn't match anything.

Undeterred, we might try the opposite approach: "find the item with #[automatically_derived] and assert it's an impl Clone for Example". We'd try a rustdoc JSON test like this:

//@ is "$.index[*][?(@.attrs[0]=='#[automatically_derived]')].inner.impl.trait.path" '"Clone"'
//@ is "$.index[*][?(@.attrs[0]=='#[automatically_derived]')].inner.impl.for.resolved_path.id" $Example

This doesn't work either. I triple-checked that [0] is indeed the correct array indexing syntax. It just only seems to work for projections, and silently fails to match anything when used as a predicate. Attempting to match the entire @.attrs array in the predicate instead of just @.attrs[0] also seems to fail to match anything.

At this point, I got frustrated by the 3h of getting absolutely nowhere with this and gave up on writing this test in rustdoc JSON.

trustfall-rustdoc-adapter has a test that ensures #[automatically_derived] is applied when deriving a number of fundamental Rust traits. So until we can figure out a valid workaround for all the issues above, this is one possible regression that the test suites of the cargo-semver-checks cinematic universe will catch but rustdoc JSON's own tests would miss: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/pull/786/files