Uplift clippy::fn_null_check lint

[Urgau]

This PR aims at uplifting the clippy::fn_null_check lint into rustc.

incorrect_fn_null_checks

(warn-by-default)

The incorrect_fn_null_checks lint checks for expression that checks if a function pointer is null.

Example

let fn_ptr: fn() = /* somehow obtained nullable function pointer */

if (fn_ptr as *const ()).is_null() { /* ... */ }

Explanation

Function pointers are assumed to be non-null, checking for their nullity is incorrect.

---

Mostly followed the instructions for uplifting a clippy lint described here: #99696 (review)

@rustbot label: +I-lang-nominated
r? compiler

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[est31]

This is a bit similar to (but not the same as) #110166 , especially see this comment where it is pointed out that maybe all comparisons of function pointers should be linted for. But I guess it should be two lints still, one for when you do any fn pointer comparison, and one for when you compare it with null, which is especially bad.

[asquared31415]

Should there be tests for fn_ptr as fn() as *const () or similar, since you can do as <same fn type> forever before doing the cast to a raw pointer.

[Urgau]

Should there be tests for fn_ptr as fn() as *const () or similar, since you can do as <same fn type> forever before doing the cast to a raw pointer.

👍 I've tweaked the code to account for repeated casting and added a test.

[bors]

☔ The latest upstream changes (presumably #111799) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

@rfcbot fcp merge

Discussed in lang team, we approve, and think warn is appropriate (this is not insta-UB to execute, just suspicious).

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[RalfJung]

warning: function pointers cannot be null, so checking for their nullability will always come out as non-null

The code in question is checking their nullness, not their nullability. (Example: all raw pointers are nullable, but only some are null.) But "nullness" sounds awkward...

[est31]

I wonder if one should make a more general lint out of this. E.g. ideally this should also warn:

    let v: &u8 = &0;
    (v as *const u8).is_null();

At that point, it should be renamed. Maybe to incorrect_ptr_null_checks?

[Urgau]

I wonder if one should make a more general lint out of this. E.g. ideally this should also warn:

    let v: &u8 = &0;
    (v as *const u8).is_null();

At that point, it should be renamed. Maybe to incorrect_ptr_null_checks?

Sure, sound like a good idea. As for the naming, maybe useless_ptr_null_checks instead? (since for refs it's not really incorrect per-say, just useless)

---

Does T-lang have any objection extending the lint to mutable and immutable references ?

[scottmcm]

We can always tweak lints later -- they're not under the strong stability promise -- so I think extending the lint like this to something that's essentially the same check just in a different context is completely fine, without needing a fresh FCP.

(I wish fn had been &'static fn, to help make this parallel more obvious.)

[est31]

As for the naming, maybe useless_ptr_null_checks instead? (since for refs it's not really incorrect per-say, just useless)

The name useless_ptr_null_checks is fine IMO as well, but note that references aren't allowed to be null just as fn pointers aren't, so if you say that comparing fn pointers with null is incorrect, then comparing refs should be incorrect, too. It's certainly not unsound to do so in either case, it's just code that LLVM might optimize out, as Rust tells LLVM that the pointer has to be dereferencable. i.e. this Rust code (playground):

pub fn foo(v: &u8, w: fn()) {}

makes this LLVM IR:

; playground::foo
; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn uwtable
define void @_ZN10playground3foo17he73a2191a67f3498E(ptr noalias nocapture noundef readonly align 1 dereferenceable(1) %v, ptr nocapture noundef nonnull readnone %w) unnamed_addr #0 {
start:
  ret void
}

Note the nonnull for w an the dereferenceable(1) for v, where latter more or less implies nonnull:

The nonnull attribute does not imply dereferenceability (consider a pointer to one element past the end of an array), however dereferenceable(<n>) does imply nonnull in addrspace(0) (which is the default address space) [...]

[Urgau]

@rustbot label -I-lang-nominated

[oli-obk]

@bors r+

[bors]

📌 Commit c0fbeea has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit c0fbeea with merge df520ef2a45c0e48a49d7ef586847b6a5d962b8a...

[rust-log-analyzer]

The job x86_64-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

failures:

---- [rustdoc] tests\rustdoc\issue-35488.rs stdout ----

error: rustdoc failed!
status: exit code: 0xc00000fd
command: PATH="C:\a\rust\rust\build\x86_64-pc-windows-gnu\stage2\bin;C:\a\rust\rust\build\x86_64-pc-windows-gnu\stage0-bootstrap-tools\x86_64-pc-windows-gnu\release\deps;C:\a\rust\rust\build\x86_64-pc-windows-gnu\stage0\bin;C:\a\rust\rust\ninja;C:\a\rust\rust\mingw64\bin;C:\hostedtoolcache\windows\Python\3.11.4\x64\Scripts;C:\hostedtoolcache\windows\Python\3.11.4\x64;C:\msys64\usr\bin;C:\a\rust\rust\sccache;C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\cf-cli;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.11.1\x64;C:\cabal\bin;C:\ghcup\bin;C:\Program Files\dotnet;C:\mysql\bin;C:\Program Files\R\R-4.3.1\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;C:\Program Files (x86)\sbt\bin;C:\Program Files (x86)\GitHub CLI;C:\Program Files\Git\bin;C:\Program Files (x86)\pipx_bin;C:\npm\prefix;C:\hostedtoolcache\windows\go\1.20.5\x64\bin;C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts;C:\hostedtoolcache\windows\Python\3.7.9\x64;C:\hostedtoolcache\windows\Ruby\2.5.9\x64\bin;C:\Program Files\OpenSSL\bin;C:\tools\kotlinc\bin;C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.372-7\x64\bin;C:\Program Files\ImageMagick-7.1.1-Q16-HDRI;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\Program Files\Eclipse Foundation\jdk-8.0.302.8-hotspot\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\ProgramData\Chocolatey\bin;C:\Program Files\PowerShell\7;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\150\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\160\DTS\Binn;C:\Strawberry\c\bin;C:\Strawberry\perl\site\bin;C:\Strawberry\perl\bin;C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin;C:\Program Files\TortoiseSVN\bin;C:\Program Files\CMake\bin;C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.7\bin;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files\nodejs;C:\Program Files\Git\cmd;C:\Program Files\Git\mingw64\bin;C:\Program Files\Git\usr\bin;C:\Program Files\GitHub CLI;C:\tools\php;C:\Program Files (x86)\sbt\bin;C:\SeleniumWebDrivers\ChromeDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files (x86)\Microsoft BizTalk Server;C:\Program Files\LLVM\bin;C:\Users\runneradmin\.dotnet\tools;C:\Users\runneradmin\.cargo\bin;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\stage2\\bin\\rustdoc.exe" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\stage2\\lib\\rustlib\\x86_64-pc-windows-gnu\\lib" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\rustdoc\\issue-35488\\auxiliary" "-o" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\rustdoc\\issue-35488" "--deny" "warnings" "C:\\a\\rust\\rust\\tests\\rustdoc\\issue-35488.rs"
stdout: none

thread 'main' has overflowed its stack
------------------------------------------

[bors]

💔 Test failed - checks-actions

[oli-obk]

@bors retry Rustdoc stack overflow

[bors]

⌛ Testing commit c0fbeea with merge 63ef74b...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 63ef74b to master...

[rust-timer]

Finished benchmarking commit (63ef74b): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-1.2%, -1.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-1.2%, -1.2%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	1.4%	[0.7%, 2.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-4.8%, -3.1%]	2
All ❌✅ (primary)	2.9%	[2.9%, 2.9%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.463s -> 655.866s (-0.24%)