rustdoc-json: Output target feature information

[willglynn]

#[target_feature] attributes refer to a target-specific list of features. Enabling certain features can imply enabling other features. Certain features are always enabled on certain targets, since they are required by the target's ABI. Features can also be enabled indirectly based on other compiler flags.

Feature information is ultimately known to rustc. Rather than force external tools to track it – which may be wildly impractical due to -C target-cpu – have rustdoc output rustc's feature data.

This change is motivated by obi1kenobi/cargo-semver-checks#1246, which intends to detect semver hazards caused by #[target_feature].

try-job: aarch64-gnu
try-job: armhf-gnu
try-job: test-various
try-job: x86_64-msvc-1
try-job: i686-msvc-1
try-job: x86_64-mingw-1
try-job: aarch64-apple

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

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

[willglynn]

I'm unsure about two topics here:

1. Does adding a field count as a backwards incompatible change for rustdoc-json-types purposes? I decided no due to the absence of #[serde(deny_unknown_fields)], so I did not increment FORMAT_VERSION.
2. Is it acceptable to output information about unstable target features from a stable toolchain? I decided yes so I unconditionally output both stable and unstable user-specifiable target features, and I decided that the stability distinction was important so I provide the associated language feature for any unstable target features.

Besides stable and unstable, there is also rustc_target::target_features::Stability::Forbidden:

    /// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be
    /// set in the target spec. It is never set in `cfg(target_feature)`. Used in
    /// particular for features are actually ABI configuration flags (not all targets are as nice as
    /// RISC-V and have an explicit way to set the ABI separate from target features).
    Forbidden { reason: &'static str },

I decided forbidden features are effectively internal implementation details (unreachable by the user) and so I drop them from the list.

[Enselic]

A new struct field is backwards compatible (edit: if it's Option or uses something like #[serde(default)] but we probably want neither in this case) but also a new FORMAT_VERSION.

[willglynn]

Got it. Is there a better way to phrase this requirement, and if so, would it belong in this PR?

rust/src/rustdoc-json-types/lib.rs

Lines 28 to 33 in 58fd7b4

	/// The version of JSON output that this crate represents.
	///
	/// This integer is incremented with every breaking change to the API,
	/// and is returned along with the JSON blob as [`Crate::format_version`].
	/// Consuming code should assert that this value matches the format version(s) that it supports.
	pub const FORMAT_VERSION: u32 = 44;

[Enselic]

Looking closer at your code I think this is actually a breaking change since the new field is not Option and does not use something like #[serde(default)] but I don't think we want anything like that in this case.

To be clear: I expect JSON from an old nightly to fail to deserialize into the new struct, but I expect JSON from a new nightly to successfully deserialize into the old struct.

[willglynn]

Oh, sure, I was thinking about the old-reading-new case and not the new-reading-old case. You're absolutely right that this is a breaking change.

[aDotInTheVoid]

Does adding a field count as a backwards incompatible change for rustdoc-json-types purposes? I decided no due to the absence of #[serde(deny_unknown_fields)], so I did not increment FORMAT_VERSION.

Yep, FORMAT_VERSION needs to be bumped here. We should probably write down the rules in more detail somewhere.

Is it acceptable to output information about unstable target features from a stable toolchain?

rustdoc-json is an unstable feature, and like all unstable features is not available/supported on a stable toolchain.

[aDotInTheVoid]

r? @aDotInTheVoid

[aDotInTheVoid]

Thanks for working on this! I think the overall approach makes sense.

This needs to have some tests, which will be in the src/rustdoc-json suite. It's not yet documented, sorry. It's probably best to start by looking at some examples.

[willglynn]

I have a tests/rustdoc-json/target.rs which passes and spot checks the output on my machine:

// Target tests are necessarily target-specific

// aarch64-*-*:
//   `sve2` is stable and implies `sve`
//@[aarch64] is "$.target.target_features[?(@.name=='sve2')].unstable_feature_gate" null
//@[aarch64] has "$.target.target_features[?(@.name=='sve2')].implies_features[*]" \"sve\"

// aarch64-apple-darwin:
//   We have the right triple, and `sve2` is enabled by default
//@[aarch64-apple-darwin] is "$.target.triple" \"aarch64-apple-darwin\"
//@[aarch64-apple-darwin] is "$.target.target_features[?(@.name=='sve2')].globally_enabled true

I'll add similar coverage for other tier 1 targets.

[rustbot]

Some changes occurred in tests/rustdoc-json

cc @aDotInTheVoid

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[willglynn]

🤦‍♂️

@bors try

[bors]

⌛ Trying commit 8c50f95 with merge 1b20903...

[bors]

☀️ Try build successful - checks-actions
Build commit: 1b20903 (1b20903de7b2a1fe2d9ba0ef0c8d982b7a3cbe25)

[willglynn]

@aDotInTheVoid 🙌 It's green.

CI terminating the build after the first failure wasn't ideal, but I don't otherwise have access to all the tier 1 targets I intended to cover. Setting aside my personal embarrassment and the sense of tumbling down some stairs, I think we're good 😄

[aDotInTheVoid]

@bors r+

[bors]

📌 Commit 8c50f95 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[obi1kenobi]

Thanks for all the awesome work here!

@willglynn absolutely no pressure, but on the off chance you're interested in driving this forward the rest of the way in cargo-semver-checks, I'd be thrilled to help you do that. You already took care of the hardest part 😁