Merge and reframe strict_provenance_lints

[hanna-kruppe]

View all comments

There does not seem to be any good reason to have separate lints for ptr2int and int2ptr casts. The main reason to enable this lint is to help with replacing as casts with strict provenance APIs (if possible) or explicit provenance APIs (when needed). That applies equally to both directions.

Merging the lints requires coming up with new name and lint explanation. This is a good chance to reconsider the purpose and messaging of the lints which have been essentially unchanged since the early days of the "strict provenance experiment". For reasons described below, I called the merged lint implicit_provenance_casts, rewrote its explanation from scratch, and made some changes to diagnostics.

First, the lint is not (only) about strict provenance any more. While strict provenance is often the best fix, migrating as casts to exposed provenance APIs also silences the lint. The exposed provenance APIs aren't any better for Miri and CHERI, but at least provenance considerations are made explicit, not picked up as side effect of the as keyword. (Banning use of exposed provenance APIs can be done with clippy's existing disallowed_methods lint.)

Second, provenance is now officially part of the language and prominently explained in the std::ptr documentation. The new lint refers to those docs and is more consistent with them.

Third, while strict provenance is encouraged whenever possible, exposed provenance is also part of the language and here to stay. Indeed, if we eventually enable the lint by default and make it cargo fix-able to keep the ecosystem migration manageable, we may want to suggest exposed provenance APIs to err on the side of not introducing UB. Thus, I made the diagnostics more neutral and descriptive. I've kept the suggestions that introduce strict provenance APIs, but we may want to reconsider this later.

Tracking issue: #130351

[rustbot]

r? @mu001999

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[hanna-kruppe]

Not sure what the right process for renaming unstable lints. Does it need a lang FCP or is that only for new stable lints?

Also: cc @RalfJung

[RalfJung]

Since this is unstable it doesn't need heavy process.

However, we should ensure that the direction we are taking this is aligned with where t-lang wants this to go. Can you extend the PR description so it briefly recaps what the existing lints are and what they do? Then we can nominate that for t-lang to get their feedback.

Actually an easier approach might be for me to become the champion for these lints, so I can approve changes to them. I'll ask about that on Zulip.

[RalfJung]

Thanks for your patience! I'm finally back from my trip and can start catching up with my backlog.

Mostly minor comments, but I am kind of concerned about the suggestions these lints emit. That's pre-existing though -- it does not block this PR, but I am not sure we want to stabilize this as-is.

@rustbot author

View changes since this review

[RalfJung]

This suggestion seems kind of dangerous -- it is almost always wrong to just apply this, because the cast back to a pointer somewhere else in the code also needs to be adjusted. Do we really want such a suggestions?

[hanna-kruppe]

I don't think it's almost always wrong (e.g., in #156832, it's correct in all but one instance because most cases just inspected the address and never cast back to pointer). There's also the mitigating factor that the lint will frequently also point out the corresponding int2ptr cast so you can consider them at roughly the same time.

I agree it would be safer to suggest exposed provenance APIs, because there are cases where applying the suggestion leads to subtle UB. But I'd like to propose that change separately in a follow-up PR.

[RalfJung]

Agreed with postponing this discussion/change to a later PR. I'll add a note to the tracking issue.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[hanna-kruppe]

@rustbot ready

[RalfJung]

@bors r+ rollup

[rust-bors]

📌 Commit 3d31306 has been approved by RalfJung

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: RalfJung
Duration: 3h 29m 41s
Pushing b4486ca to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 1182320 (parent) -> b4486ca (this PR)

Test differences

Show 10 test diffs

10 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard b4486cacf58926f02e451d96e71e20882faf5453 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-i686-msvc: 2h 3m -> 1h 7m (-45.4%)
2. x86_64-gnu-llvm-21-2: 1h 39m -> 56m 4s (-43.7%)
3. dist-x86_64-llvm-mingw: 1h 53m -> 1h 5m (-42.2%)
4. x86_64-gnu-llvm-22-1: 53m 46s -> 1h 11m (+33.6%)
5. x86_64-gnu-next-trait-solver-polonius: 40m 26s -> 52m 1s (+28.6%)
6. dist-ohos-aarch64: 1h 15m -> 54m 39s (-27.8%)
7. x86_64-gnu-stable: 2h 26m -> 1h 47m (-26.6%)
8. dist-x86_64-apple: 1h 36m -> 2h 1m (+25.6%)
9. dist-x86_64-freebsd: 1h 33m -> 1h 12m (-22.7%)
10. aarch64-apple: 2h 56m -> 3h 23m (+15.5%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (b4486ca): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary 2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	8.9%	[8.9%, 9.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-5.4%, -1.6%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 485.632s -> 486.992s (0.28%)
Artifact size: 393.58 MiB -> 393.11 MiB (-0.12%)