New lints: `#[target_feature]` changes

[obi1kenobi]

As of Rust 1.86, #[target_feature] can be applied to safe functions, so we should look at what it means in terms of breaking changes.

Reading the reference page for this attribute is highly recommended before proceeding with this issue.

Adding #[target_feature] to a safe pub function that didn't have it before is a major breaking change, since the function no longer implements its corresponding Fn() trait — playground

• lint for functions (deny by default): Add safe_function_target_feature_added lint #1303
• lint for ImplOwner methods (deny by default): Add safe_inherent_method_target_feature_added lint #1306
• currently prohibited for safe functions in traits; no lint there

Adding #[target_feature] to a unsafe pub function that didn't have it before is usually a major breaking change, since the safety obligations for that function have changed. The exception is if the newly-requested feature was already enabled on that target, or it's implied by another already-requested feature. For example: x86-64 with any OS at all enables sse and sse2 by default, and avx2 implies avx. Information on whether required features are explicit or implied, or globally enabled, is available in the schema.

• lint for functions (deny by default): Add unsafe_function_target_feature_added lint #1302
• lint for ImplOwner methods (deny by default): Add unsafe_inherent_method_target_feature_added lint #1307
• lint for trait associated functions / methods (deny by default): Add unsafe_trait_method_target_feature_added lint #1304

If a safe pub function already had #[target_feature], adding a new feature is always a major breaking change. This lint shouldn't trigger if the function became unsafe as well — that breakage dominates.

• lint for functions (deny by default): Add safe_function_requires_more_target_features lint #1332
• lint for ImplOwner methods (deny by default): Add safe_inherent_method_requires_more_target_features lint #1333
• currently prohibited in safe functions in traits; no lint here

If an unsafe pub function already had #[target_feature], adding a new feature is only major breaking if the feature isn't implied nor already globally enabled, as above. This lint should trigger even if the function might have become safe in the meantime.

• lint for functions (deny by default): Add function_requires_more_target_features lint #1316
• lint for ImplOwner methods (deny by default): Add lint for inherent methods gaining target features #1311
• lint for trait associated functions / methods (deny by default): Add trait_method_requires_more_target_features lint #1315

If an unsealed trait's associated function has a feature removed from its existing #[target_feature] list, that's a major breaking change since Rust currently allows impls of that trait to specify a stricter (i.e. the previous) #[target_feature] without any warning. Such downstream impls may suddenly be hit with UB when a caller that uses impl Trait or dyn Trait with their impl (with stricter target feature requirements) is used based on the trait's looser declared target feature requirements. The exception to the breakage is when the removed feature continues to be implied by either the platform (see above x86-64 example) or by another feature that remains declared.

• lint for unsealed trait associated functions / methods (deny by default): Add trait_method_target_feature_removed lint #1337
• lint for public API sealed trait associated functions / methods (warn by default): Add pub_api_sealed_trait_method_target_feature_removed lint. #1339

Related false-positives

#[target_feature] also appears to trigger a bug in rustdoc JSON that causes false-positives in two of our lints: function_unsafe_added and inherent_method_unsafe_added. The cause is this: rust-lang/rust#142655

Open questions

• How do we find out which features are effectively implied by which target? This seems to be one of those things that "everyone knows" but isn't really written down directly in that explicit form. (answered below)
• How do we figure out which target's "implied" rules we should be looking at in cargo-semver-checks? We may have to ask rustdoc to describe the target while generating rustdoc JSON.
• How do we encode the "implies" rules? Do we put them in the schema (as "adapter magic"), or do we make some wild pile of regexes, or something else? (answered below)
• With regard to the above, how will we handle cases when new features are added to the list? We may only support target features that are considered stable (i.e. "unstable features don't auto-enable any other features and are never automatically enabled themselves"), and rely on stabilization announcements to be notified of changes.

[willglynn]

• How do we find out which features are effectively implied by which target? This seems to be one of those things that "everyone knows" but isn't really written down directly in that explicit form.
• How do we encode the "implies" rules? Do we put them in the schema (as "adapter magic"), or do we make some wild pile of regexes, or something else?

I think this is adapter magic.

From a linting perspective, we're less concerned with which features are specified in the source code and more concerned with which features are required by each item beyond the target's ABI. This is a different set – "enabled target features" vs "effective target features", say – but this distinction appears to be common to the rules above.

Specifically: begin with the set of features enabled from the #[target_feature(enable = "…")] attribute. For each specified feature, recursively add any features which that feature implies. (See also rust-analyzer's or rustc's implementation of this step.) Finally, remove any features required by the target triple's ABI. (See: rustc's abi_required_features(), which writes down these unwritten rules.) Expose this set as "effective target features".

This approach implies that specifying #[target_feature(enable = "sse")] is equivalent to omitting #[target_feature] on x86_64-unknown-linux-gnu. I think that's true, but I'm stating this implication as a way to refute this approach in case it isn't.

It seems unfortunate that #[target_feature] use imposes a requirement to evaluate these lints once for each target. On the other hand, conditional compilation already requires checking the entire matrix, and #[target_feature]'s behavior really does depend on the target triple's ABI, so this also seems unavoidable.

[obi1kenobi]

Amazing, thank you! Marking those questions as answered. In particular, the links you gave are exactly what I was hoping for here 💯

This approach implies that specifying #[target_feature(enable = "sse")] is equivalent to omitting #[target_feature] on x86_64-unknown-linux-gnu. I think that's true, but I'm stating this implication as a way to refute this approach in case it isn't.

I also believe this to be true, and at least one more person has told me the same thing. So at least we can all be wrong together if it turns out not to be the case :)

It seems unfortunate that #[target_feature] use imposes a requirement to evaluate these lints once for each target. On the other hand, conditional compilation already requires checking the entire matrix, and #[target_feature]'s behavior really does depend on the target triple's ABI, so this also seems unavoidable.

Yes, there's no way around this so I don't feel that bad about it.

Thank you so much for the excellent contribution! It took this issue in my mind from "maybe implement someday" to "probably even a good fit for new contributors right now."

Aside: if you'd like to join the project's Discord server (no pressure!), we'd love to have you — here's an invite: https://discord.gg/zk8MsRuV

[willglynn]

It turns out abi_required_features() is not authoritative for which features are globally enabled. The user may specify -C target-feature to enable or disable features as they see fit. The effective target features for each item – and thus the existence of #[target_feature] semver hazards – depends not just on the target triple, but also potentially on .cargo/config.toml or RUSTFLAGS.

But wait, it gets even more complex. There are indirect ways to specify target features. On an aarch64-apple-darwin machine, rustc normally enables 34 target features. When I compile with -C target-cpu=apple-m4, it enables 45. When I compile with -C target-cpu=generic, it prints a warning, and enables only 1:

warning: target feature `neon` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>

So, fine, maybe we have to consider the totality of rustc's configuration to figure out which target features are enabled everywhere, so that we can distinguish which #[target_features] enable features that are not already enabled. This would be a lot of machinery just to start answering the question and it would still not be enough. The user can specify -C target-cpu=native, in which case the existence of #[target_feature] semver hazards depends on the capabilities of the CPU running the compiler (rust-lang/rust#80749).

I see two alternatives:

1. Duplicate all the relevant logic and data, track it as it changes, and hope the user doesn't specify any compiler flags which change the results.
2. Expose rustc's target feature information – inclusive of any user-specified flags and any potential runtime CPU feature detection – via rustdoc JSON output.

Option 2 seems clearly better and I will have a PR in a moment.