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

Null fn lints #10099

Merged
merged 10 commits into from
Dec 19, 2022
Merged

Null fn lints #10099

merged 10 commits into from
Dec 19, 2022

Conversation

Niki4tap
Copy link
Contributor

@Niki4tap Niki4tap commented Dec 18, 2022

Adds lints to check for code, that assumes nullable fn().

Lint examples:

transmute_null_to_fn:

error: transmuting a known null pointer into a function pointer
  --> $DIR/transmute_null_to_fn.rs:9:23
   |
LL |         let _: fn() = std::mem::transmute(std::ptr::null::<()>());
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this transmute results in undefined behavior
   |
   = help: try wrapping your function pointer type in `Option<T>` instead, and using `None` as a null pointer value

fn_null_check:

error: function pointer assumed to be nullable, even though it isn't
  --> $DIR/fn_null_check.rs:13:8
   |
LL |     if (fn_ptr as *mut ()).is_null() {}
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value

Closes #1644


changelog: Improvement: [transmuting_null]: Now detects const pointers to all types
#10099
changelog: New lint: [transmute_null_to_fn]
#10099
changelog: New lint: [fn_null_check]
#10099

@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 18, 2022
@Niki4tap
Copy link
Contributor Author

@rustbot label: +A-lint, +C-enhancement, +L-correctness

@rustbot rustbot added A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages L-correctness Lint: Belongs in the correctness lint group labels Dec 18, 2022
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This looks good in general, I have a few suggestions regarding code style.

clippy_lints/src/fn_null_check.rs Outdated Show resolved Hide resolved
clippy_lints/src/fn_null_check.rs Outdated Show resolved Hide resolved
clippy_lints/src/fn_null_check.rs Outdated Show resolved Hide resolved
clippy_lints/src/fn_null_check.rs Show resolved Hide resolved
clippy_lints/src/transmute/mod.rs Show resolved Hide resolved
clippy_lints/src/transmute/transmute_null_to_fn.rs Outdated Show resolved Hide resolved
@Niki4tap
Copy link
Contributor Author

@llogiq I believe all code style issues have been resolved, one thing I would still like to discuss though, is whether fn_null_check lint should be in a different category.

Everything I've said in the suggestion still stands, and I just mostly wanted to mention this concern here specifically, as this is not just a code style issue, and might need to be discussed outside of a suggestion. My opinion is that, while fn_null_check might not seem as impactful as transmute_null_to_fn, they both are checking for the same root issue, which is assuming that fn() can be null, so I believe that their severity should not be separated.

@Niki4tap
Copy link
Contributor Author

Thanks for the review @llogiq! Is this now ready for merge?

@llogiq
Copy link
Contributor

llogiq commented Dec 19, 2022

Thank you!

I'm a bit wary about the correctness group for the null check lint, but that may be overcautious. We can revisit this if there are any reports of false positives or something.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 19, 2022

📌 Commit 691df70 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 19, 2022

⌛ Testing commit 691df70 with merge b3145fe...

@bors
Copy link
Contributor

bors commented Dec 19, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing b3145fe to master...

@bors bors merged commit b3145fe into rust-lang:master Dec 19, 2022
@bors bors mentioned this pull request Dec 19, 2022
@Niki4tap Niki4tap deleted the null_fn_lints branch December 19, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages L-correctness Lint: Belongs in the correctness lint group 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.

Lint null function pointers
4 participants