Add a Lint for Pointer to Integer Transmutes in Consts

[veera-sivarajan]

Fixes #87525

This PR adds a MirLint for pointer to integer transmutes in const functions and associated consts. The implementation closely follows this comment: #85769 (comment). More details about the implementation can be found in the comments.

Note: This could break some sound code as mentioned by RalfJung in #85769 (comment):

... technically const-code could transmute/cast an int to a ptr and then transmute it back and that would be correct -- so the lint will deny some sound code. Does not seem terribly likely though.

References:

1. https://doc.rust-lang.org/std/mem/fn.transmute.html
2. https://doc.rust-lang.org/reference/items/associated-items.html#associated-constants

[rustbot]

r? @estebank

rustbot has assigned @estebank.
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]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[scottmcm]

What's the motivation for this lint? https://doc.rust-lang.org/std/primitive.pointer.html#method.addr and https://doc.rust-lang.org/std/ptr/fn.without_provenance.html are transmutes, and I'd thought that this was considered basically fine now, albeit possibly not what you wanted if you need to preserve provenance.

[veera-sivarajan]

Transmuting a pointer to integer in const context is undefined behavior¹. Usually, the evaluator will abort and emit an error when it comes across such undefined transmutes.

But const functions and associated consts are evaluated only when referenced. This can result in undefined behavior in a library going unnoticed until the function or constant is actually used. Therefore, this lint specifically targets pointer to integer transmutes in const functions and associated consts.

From what I can tell, this lint is not related to the functions you have linked because:

1. addr() is not a const function.
2. without_provenance() transmutes an integer to pointer.

Footnotes

1. https://doc.rust-lang.org/std/mem/fn.transmute.html#transmutation-between-pointers-and-integers ↩

[scottmcm]

Oh, of course; thank you. Brain fart on my part.

Seem hard to lint on when it's not always UB, because things like NonNull::dangling().addr() is a transmute but isn't UB.

I'd be tempted to run it on optimized mir where we can have const-folded a bunch of those cases away already, but then it'd be super inconsistent which we probably don't want either :/

[RalfJung]

I'd be tempted to run it on optimized mir where we can have const-folded a bunch of those cases away already, but then it'd be super inconsistent which we probably don't want either :/

Also, we're not optimizing const MIR.

[RalfJung]

Seem hard to lint on when it's not always UB, because things like NonNull::dangling().addr() is a transmute but isn't UB.

Indeed, that's the tricky part. The lint triggers in this code which is exactly an example of that:

rust/library/core/src/ptr/mod.rs

Lines 1989 to 1993 in b5bd0fe

	// SAFETY: This is just an inlined `p.addr()` (which is not
	// a `const fn` so we cannot call it).
	// During const eval, we hook this function to ensure that the pointer never
	// has provenance, making this sound.
	let addr: usize = unsafe { mem::transmute(p) };

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[flip1995]

The Clippy changes remove a test case that tests that a Clippy lint doesn't trigger for a certain pattern and adds this new test case, that tests a rustc lint in the Clippy test suite.

I think the better thing to do here would be to keep the test case where it is and allow the new rustc lint with an outer attribute just on the issue_12402 function. Maybe give the allow attribute a reason = "". We still need to test that our other transmute lints do not trigger on this and this new test case doesn't reflect what we want to actually test with this.

[estebank]

Please address this request.

[veera-sivarajan]

Thank you for the detailed feedback.

[estebank]

The code change looks correct, but I have some questions.

Shouldn't we also account for as? Is align_offset invoking UB?

[estebank]

Please address this request.

[RalfJung]

Shouldn't we also account for as?

as from ptr to int is just forbidden in const contexts.

Is align_offset invoking UB?

No, that function is treated specially by the const interpreter to ensure that the pointer argument is always actually an integer.

[estebank]

@bors r+

[bors]

📌 Commit ab86735 has been approved by estebank

It is now in the queue for this repository.

[bors]

⌛ Testing commit ab86735 with merge daebce4...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing daebce4 to master...

[rust-timer]

Finished benchmarking commit (daebce4): comparison URL.

Overall result: ❌✅ regressions and 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.2%	[0.1%, 0.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.1%, secondary 1.6%)

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.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	1.6%	[0.5%, 3.6%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Cycles

Results (secondary 3.0%)

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)	-	-	0
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 774.072s -> 774.475s (0.05%)
Artifact size: 329.50 MiB -> 329.52 MiB (0.01%)