Lint #[inline(always)] closure in #[target_feature] functions

[calebzulawski]

Make #108655 a future-incompatible lint as suggested in #69098 (comment)

r? @RalfJung

Some context:

In Rust today, closures don't inherit target features from their enclosing function. This behavior might be surprising to users. The target_feature_11 feature changes this--otherwise, safe target feature 1.1 calls would still require unsafe within a closure.

Rust does not allow #[inline(always)] to be applied to #[target_feature] functions, so adding target features to closures causes errors in previously accepted code (#108655). Unfortunately, we can't simply allow both attributes for two reasons. First, it is possible for the closure to "escape" the function (e.g. an indirect call optimization) and attempt to inline into a function that doesn't have the target features, resulting in an ICE when inlining fails. Additionally, there are soundness concerns when combining #[inline(always)] with #[target_feature], see #116573.

This leaves us with a few options:

• Don't pass target features to closures. This prevents target feature 1.1 from working in closures. Inconsistent safety requirements inside and outside closures might be confusing. This also potentially affects performance: e.g. std::arch intrinsics don't inline into closures.
• Pass target features to closures, but don't enable them at codegen when #[inline(always)] is present. This is the current target_feature_11 behavior. This allows any existing code to still work, but it is potentially confusing for #[inline] to affect other codegen behavior.
• Merge this PR: add a future-incompatible warning, and reject #[inline(always)] on closures in #[target_feature] functions in a future version of rust.

[RalfJung]

I am in favor of this lint, but IMO it should get input from some other people as well, so let's

r? compiler

[RalfJung]

In fact since this is a new lint, we'll need approval from @rust-lang/lang. @calebzulawski can you make the PR description a bit more self-contained so t-lang has the required context here?

[RalfJung]

What if we don't have the span? Something is still wrong, so we should still emit a lint -- it can point at the closure instead of the attribute, maybe?

[RalfJung]

Given the example at #69098 (comment), I am not sure if we still want to do this.

[calebzulawski]

Does Fn::call in that example get inline, inline(always), or maybe neither? (#96929)

If for some reason Fn::call is not inlined, even if the closure is inlined, you don't get the target features. (Adding the target features to the closure doesn't even really fix this, it's more an issue with the fragility of inlining closures/Fn traits)

[RalfJung]

That seems like an entirely orthogonal question? I am very confused.

[calebzulawski]

I'm just suggesting that in this particular example, the closure's inlining might not actually matter, if the Fn trait is not inline(always). It might be acting more like inline anyway.

[RalfJung]

@hanna-kruppe indicated that the attribute did help though.

[hanna-kruppe]

Both the example in #96929 and the discussion there suggest to me that it’s a FnOnce specific problem, while I’m using Fn. Also, it sounds like it’s Just A Bug that rustc can easily fix, unlike the LLVM mess around inlining and target features.

[traviscross]

For the lang nomination, if someone could perhaps write up a short self-contained code example of this case, that would be helpful to us.

[calebzulawski]

@hanna-kruppe do you think this minimal example accurately represents your use case?
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=acf0a7af1bf877198c583faf3bf0dc93

[RalfJung]

Let's de-nominate for now, I am not sure we still want to go ahead with this lint.

[hanna-kruppe]

@hanna-kruppe do you think this minimal example accurately represents your use case? https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=acf0a7af1bf877198c583faf3bf0dc93

Broadly yes. It’s important that the intermediate non-target-feature function(s), for_each in this case, are also virtually guaranteed to be inlined, but for simple iterator usage that usually isn’t an issue.

For this specific example you probably want chunks_exact_mut (and skip handling the remainder for simplicity) so the closure can actually compile to SIMD instead of needing a bounds check for every element.

[calebzulawski]

This is pretty contrived, but I put together an example that has both inline and non-inline closures: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=58d03317abd8b2fe151e6c8d0bbf2b8d

I've written code that looks vaguely like this. I think outside of #[inline(always)], like this example, it could sometimes be unexpected if closures don't inherit target features.

[chenyukang]

I'm not sure about this lint
r? compiler

[RalfJung]

Yeah, I've been convinced we should close this in favor of clearly documenting the current rules for when target features are inherited by closures and when they are not. Those rules are surprising, but it is not clear that forbidding inline(always) is any less surprising.

[calebzulawski]

I agree. I don't think we want either type of closure to do anything differently, so the existing rules are probably the best we will get. I'm not totally sure where to document it, though.