Skip to content

Null fn lints#10099

Merged
bors merged 10 commits into
rust-lang:masterfrom
Niki4tap:null_fn_lints
Dec 19, 2022
Merged

Null fn lints#10099
bors merged 10 commits into
rust-lang:masterfrom
Niki4tap:null_fn_lints

Conversation

@Niki4tap

@Niki4tap Niki4tap commented Dec 18, 2022

Copy link
Copy Markdown
Contributor

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

rustbot commented Dec 18, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

@llogiq llogiq left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread clippy_lints/src/fn_null_check.rs Outdated
Comment thread clippy_lints/src/fn_null_check.rs Outdated
Comment thread clippy_lints/src/fn_null_check.rs Outdated
Comment thread clippy_lints/src/fn_null_check.rs
Comment thread clippy_lints/src/transmute/mod.rs
Comment thread clippy_lints/src/transmute/transmute_null_to_fn.rs Outdated
@Niki4tap

Copy link
Copy Markdown
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.

Comment thread clippy_lints/src/transmute/transmute_null_to_fn.rs Outdated
@Niki4tap

Copy link
Copy Markdown
Contributor Author

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

@llogiq

llogiq commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

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

bors commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

📌 Commit 691df70 has been approved by llogiq

It is now in the queue for this repository.

@bors

bors commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

⌛ Testing commit 691df70 with merge b3145fe...

@bors

bors commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

☀️ 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