Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new lint: add call_missing_target_feature lint #13240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

changelog: [call_missing_target_feature]: was added

This PR adds a lint for missing target features.

Motivation

A function that requires a target feature (e.g. #[target_feature(enable = "avx2")] to indicate that avx2 instructions can be used) must be called from a context where these instructions are actually available. That check is part of the safety contract of the function. For correctness and performance, any function calling a function with a target feature must either

  • also declare that target feature
  • assert that the target feature is available (e.g. with the is_x86_feature_detected macro, or similar for the current target platform)

However, currently no warning is emitted when the user forgets to correctly annotate target features. My personal experience with zlib-rs is that it's just really easy to forget the annotation on some helper function, and to forget to check for target features in code review. Hence, this check.

Implementation

The implementation looks for function calls, then looks up if that function (the callee) has any target features, and if so, checks whether the caller also has these target features. Any missing target features are reported.

This lint is not always accurate, and I think should be off by default, but is very useful in e.g. simd-heavy codebases. Some #[cfg(allow(...)] are always required though in the function where the feature detection macro is called.

Questions

  • The tests run into

    error: test got exit status: 1, but expected 0
     --> tests/ui/call_missing_target_feature.fixed:1:1
      |
    1 | #[target_feature(enable = "avx2")]
      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ after rustfix is applied, all errors should be gone, but weren't
    

    despite the suggestion being Applicability::MaybeIncorrect. I must be missing something here. The problem is that the function must also be marked as unsafe in this case. Still I think the suggestion is useful. Also inserting the unsafe seems complicated, but maybe that is possible?

cc @bjorn3 maybe you have phrasing suggestions?

@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

r? @dswij

rustbot has assigned @dswij.
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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 8, 2024
@folkertdev folkertdev force-pushed the call-missing-target-feature branch 2 times, most recently from de11ecb to 503517a Compare August 8, 2024 20:44
@folkertdev
Copy link
Contributor Author

probably rust-lang/rust#128852 is needed to fix the suggestion here and correctly insert the unsafe keyword

@folkertdev folkertdev force-pushed the call-missing-target-feature branch 4 times, most recently from bda8541 to 711a22c Compare August 26, 2024 17:30
@Jarcho
Copy link
Contributor

Jarcho commented Dec 6, 2024

r? @Jarcho

I'll take a look at this within a couple of days.

@rustbot rustbot assigned Jarcho and unassigned dswij Dec 6, 2024
Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did an initial review. When ordering and performing checks try to make sure to optimize for the common case. For this it would be something like:

  • Not a function call
  • Called function has no target_features
  • Not from an external macro (Not always true, but true for most crates)

clippy_lints/src/call_missing_target_feature.rs Outdated Show resolved Hide resolved
clippy_lints/src/call_missing_target_feature.rs Outdated Show resolved Hide resolved
clippy_lints/src/call_missing_target_feature.rs Outdated Show resolved Hide resolved
clippy_lints/src/call_missing_target_feature.rs Outdated Show resolved Hide resolved
@folkertdev folkertdev force-pushed the call-missing-target-feature branch 2 times, most recently from 3ea48d3 to 43f1f1b Compare December 8, 2024 12:12
@folkertdev folkertdev force-pushed the call-missing-target-feature branch from 43f1f1b to cf88b49 Compare December 8, 2024 12:41
@folkertdev
Copy link
Contributor Author

Thanks for the review!

I've handled the comments and also rebased because some CI things had changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants