lint ImproperCTypes: refactor linting architecture (part 3) by niacdoial · Pull Request #146275 · rust-lang/rust

niacdoial · 2025-09-06T15:50:41Z

This is the third PR in an effort to split #134697 (refactor plus overhaul of the ImproperCTypes family of lints) into individually-mergeable parts.

Contains:

the changes of the first two PRs
other user-invisible changes,
the prevention of stack overflows while checking irregular recursive types.

Fixes: #130310
Superset of: #146271 and its superset #146273

tgross35

Looked at the final commit here "add recursion limit"; I think the method should be adjusted, but this can land pretty easily.

I don't think "add architecture for layered reasoning in lints" can land yet, there is a lot added here that is dead code and untested for now. Can it be moved to just before a commit that makes use of it?

Edit: actually, no need to move it anywhere specific yet since I'm still going through the commits at #134697 one-by-one. Just dropping it from this PR is sufficient, so the recursion limit can merge without being blocked.

View changes since this review

tgross35 · 2025-09-07T05:58:28Z

+        let _depth_guard = match self.can_enter_type(ty) {
+            Ok(guard) => guard,
+            Err(ffi_res) => return ffi_res,
+        };
        let tcx = self.cx.tcx;

-        // Protect against infinite recursion, for example
-        // `struct S(*mut S);`.
-        // FIXME: A recursion limit is necessary as well, for irregular
-        // recursive types.
-        if !self.cache.insert(ty) {
-            return FfiSafe;
-        }


I don't follow the original implementation here. If you have

extern "C" fn foo(a: NotFfiSafe, b: NotFfiSafe)

Why is it that the b doesn't get marked FfiSafe since the cache insert will fail?

this is because the cache is set up per visitor.
One of those gets set up for each extern static variable, each extern non-fnptr-function argument, each extern non-fnptr-function return, and each extern fnptr in a non-extern context.
Here specifically, a and b each get a visitor.

tgross35 · 2025-09-07T06:04:38Z

+        // borrows the refcell, outside of ImproperCTypesVisitorDepthGuard::drop()
+        let mut limiter_guard = self.recursion_limiter.borrow_mut();
+        let (ref mut cache, ref mut depth) = *limiter_guard;
+        if (!cache.insert(ty)) || *depth >= 1024 {


Why is the cache still necessary if you have the depth?

The cache allows to stop processing regular-recursive types (say, some LinkedListNode struct) after a single iteration rather than after 1024.
The depth limit is really needed for irregular recursive types, like in #130310.

tgross35 · 2025-09-07T06:27:21Z

 struct ImproperCTypesVisitor<'a, 'tcx> {
    cx: &'a LateContext<'tcx>,
    /// To prevent problems with recursive types,
-    /// add a types-in-check cache.
-    cache: FxHashSet<Ty<'tcx>>,
+    /// add a types-in-check cache and a depth counter.
+    recursion_limiter: RefCell<(FxHashSet<Ty<'tcx>>, usize)>,


It's probably cleaner to continue passing this around as &mut and avoiding the RefCell if possible, which will make it easier to store more in the visitor in the future if needed. Is it possible to just pass a depth argument to the needed functions? This also avoids the guard complexity, and makes it easy to avoid resetting the depth if you need to construct a new ImproperCTypesVisitor.

Alternatively you can make a type RecursionCount = Rc<()> and store it in the visitor and clone it for each guard, which allows Rc::strong_count(that_rc) to tell you the recursion count. But that's not quite as clean.

I guess? I thought it would be annoying, performance-wise, to add 8 bytes to each method's stack. which... I end up half-doing anyway. huh.
Sure, I'll add a "depth" argument.

For context, this suggestion was how it looks like we do this sort of thing in Miri and Clippy.

I thought it would be annoying, performance-wise, to add 8 bytes to each method's stack

Accounting for this would be an arch-specific extreme micro-optimization, no real need to think about that kind of thing :)

tgross35 · 2025-09-07T06:30:39Z

+        // borrows the refcell, outside of ImproperCTypesVisitorDepthGuard::drop()
+        let mut limiter_guard = self.recursion_limiter.borrow_mut();
+        let (ref mut cache, ref mut depth) = *limiter_guard;
+        if (!cache.insert(ty)) || *depth >= 1024 {


For the limit we may as well use the global recursion limit here, I think that's available via cx.tcx.recursion_limit(). Should give you a Limit, and you can use value_within_limit to check

rust/compiler/rustc_session/src/session.rs

Lines 65 to 87 in f4b2f68

/// New-type wrapper around `usize` for representing limits. Ensures that comparisons against

/// limits are consistent throughout the compiler.

#[derive(Clone, Copy, Debug, HashStable_Generic)]

pub struct Limit(pub usize);

impl Limit {

/// Create a new limit from a `usize`.

pub fn new(value: usize) -> Self {

Limit(value)

}

/// Create a new unlimited limit.

pub fn unlimited() -> Self {

Limit(usize::MAX)

}

/// Check that `value` is within the limit. Ensures that the same comparisons are used

/// throughout the compiler, as mismatches can cause ICEs, see #72540.

#[inline]

pub fn value_within_limit(&self, value: usize) -> bool {

value <= self.0

}

}

bors · 2025-09-07T08:49:36Z

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

niacdoial · 2025-09-11T22:01:49Z

I'm not sure I addressed everything you asked in reviews, but I figures I should push what I have so far (in both branches,
leaving aside the commit on layered lint messages), before I go offline for most of the weekend.

(also I know there are be two commits that should be squashed together in the other PR, but I'll need to resolve a bunch of conflicts in the rest of the commit chain before doing this)

rustbot · 2025-09-22T18:40:07Z

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

tgross35 · 2025-10-21T00:59:10Z

+//@ check-pass
+
+//! this test checks that irregular recursive types do not cause stack overflow in ImproperCTypes
+//! Issue: https://github.com/rust-lang/rust/issues/94223
+
+use std::marker::PhantomData;
+
+#[repr(C)]
+struct A<T> {
+    a: *const A<A<T>>,  // without a recursion limit, checking this ends up creating checks for
+                        // infinitely deep types the likes of `A<A<A<A<A<A<...>>>>>>`
+    p: PhantomData<T>,
+}
+
+extern "C" {
+    fn f(a: *const A<()>);
+}
+
+fn main() {}


It should also raise the lint here right? If so, a global #![deny(improper_ctypes)] with a local #[expect(improper_ctypes)] would be good to add.

Also I think the mustpass- test name prefix is only for tests named after issues (which we're trying to get rid of)

No, there shouldn't be an error here.

When the depth limit is reached, the type visitor assumes it managed to visit everything there was to visit. Therefore, if no error has be found until now, it must mean that the recursive pile of types is FFI-safe, somehow.

For the test names, this is something I fixed way down the commit chain, but yeah, I can at least give better names this for the lints I added myself.

Ah, you're right. I guess maybe it would just be good to deny(improper_ctypes, improper_ctypes_definitions) to verify that.

unless the recursion limit is reached traversing to many different types right? If you keep encountering more and more different types, the last one might make it ffi unsafe

not sure what statement you're tagging this "unless" onto but...
The assumption is that whenever the recursion depth is reached, it is because something technically recurses infinitely. (excluding tests where this limit is very low on purpose.)
So there wouldn't really be a "last" type to speak of? I cannot think of a way where the FFI-unsafety of a type would come from the [arbitrarily large N]th type coming from the same struct A<..> definition.

tgross35 · 2025-10-21T01:00:33Z

 struct ImproperCTypesVisitor<'a, 'tcx> {
    cx: &'a LateContext<'tcx>,
    /// To prevent problems with recursive types,
-    /// add a types-in-check cache.
-    cache: FxHashSet<Ty<'tcx>>,
+    /// add a types-in-check cache and a depth counter.
+    recursion_limiter: RefCell<(FxHashSet<Ty<'tcx>>, usize)>,


For context, this suggestion was how it looks like we do this sort of thing in Miri and Clippy.

I thought it would be annoying, performance-wise, to add 8 bytes to each method's stack

Accounting for this would be an arch-specific extreme micro-optimization, no real need to think about that kind of thing :)

tgross35 · 2025-10-21T01:02:06Z

+//@ check-pass
+
+#![recursion_limit = "5"]
+#![allow(unused)]
+#![deny(improper_ctypes)]
+
+#[repr(C)]
+struct F1(*const ());
+#[repr(C)]
+struct F2(*const ());
+#[repr(C)]
+struct F3(*const ());
+#[repr(C)]
+struct F4(*const ());
+#[repr(C)]
+struct F5(*const ());
+#[repr(C)]
+struct F6(*const ());
+
+#[repr(C)]
+struct B {
+    f1: F1,
+    f2: F2,
+    f3: F3,
+    f4: F4,
+    f5: F5,
+    f6: F6,
+}
+
+extern "C" fn foo(_: B) {}
+
+fn main() {}


What exactly is this expected to test? There doesn't seem to be any recursion here.

Maybe this would be good to run twice with different limits:

//@ revisions: limit5 limit10 #![deny(improper_ctypes)] #![cfg_attr(limit6, recursion_limit = "10")] #![cfg_attr(limit5, recursion_limit = "5")]

Then add something that passes with the limit of 10 but needs #[cfg_attr(limit5, expect(improper_ctypes))] to pass with a limit of 5.

not sure why git is saying I wrote this test, it comes from 9050b33.
let me check what it was about...

ah got it.
it's linked to pull #130758, so the test is here to check that the next attempt to add a recursion limit wouldn't make the same mistakes as the previous one

Like in this test, if the last type wasn't #[repr(C)] but we hit the recursion limit, the lint wouldn't trigger on an ffi-unsafe type right?

the goal of this test is to show that no matter how many fields we have in a single struct definition, we don't hit the recursion limit.
(the attempt from pull 130758 didn't count the recursion level properly, and the f6 field believed it was on the 7th recursion level rather than the 2nd)

tgross35 · 2026-01-23T20:29:32Z

Same as at #146273 (comment),

r? compiler

rustbot · 2026-05-07T23:35:54Z

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.

Simple change to stop irregular recursive types from causing infinitely-deep recursion in type checking.

niacdoial · 2026-05-08T08:12:59Z

@rustbot review

jdonszelmann

I left some comments before, related to the recursion limit. Otherwise the impl looks good to me, though no warning on recursion limit scares me. Sorry for the long review delay!

View changes since this review

rustbot · 2026-06-08T11:06:00Z

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

jdonszelmann · 2026-06-08T11:06:24Z

Don't think this conflicts with #156399, but good to be aware of

niacdoial · 2026-06-08T20:43:55Z

I answered your comments, let me know if this cleared things up!
And no worries for the delay, I don't have anything that's waiting on this.

And also thanks for the other PR link! I'm glad I managed to land my previous PR before that one started.
...wait a minute. ah. I was the one to cause that regression wasn't I.
I will keep the new solver in mind from now on.

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2025

tgross35 requested changes Sep 7, 2025

View reviewed changes

tgross35 mentioned this pull request Sep 7, 2025

lint ImproperCTypes: overhaul (take 2 of "better handling of indirections") #134697

Open

23 tasks

niacdoial force-pushed the improperctypes-refactor3 branch from 971f0ab to 1cc9c7e Compare September 11, 2025 21:59

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor3 branch from 1cc9c7e to cf704cf Compare September 11, 2025 22:10

tgross35 self-assigned this Sep 12, 2025

niacdoial force-pushed the improperctypes-refactor3 branch from cf704cf to c74c323 Compare September 19, 2025 21:15

niacdoial marked this pull request as ready for review September 22, 2025 18:40

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2025

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 22, 2025

tgross35 requested changes Oct 21, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2025

niacdoial force-pushed the improperctypes-refactor3 branch from c74c323 to 2ec7eab Compare October 25, 2025 20:04

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor3 branch from 2ec7eab to 85b72da Compare October 25, 2025 20:13

niacdoial force-pushed the improperctypes-refactor3 branch 2 times, most recently from 55b512c to 38bbcde Compare November 1, 2025 21:39

rustbot assigned jdonszelmann and unassigned tgross35 Jan 23, 2026

niacdoial force-pushed the improperctypes-refactor3 branch from 38bbcde to 975b603 Compare January 24, 2026 16:44

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor3 branch from 975b603 to 935b5d5 Compare March 20, 2026 17:46

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 20, 2026

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor3 branch from 935b5d5 to a219c4e Compare March 20, 2026 21:11

This comment has been minimized.

Sign in to view

theemathas mentioned this pull request Apr 19, 2026

[ICE]: "improper_ctypes: Option nonnull optimization not applied?" #155479

Open

niacdoial force-pushed the improperctypes-refactor3 branch from a219c4e to 4c78ff9 Compare May 7, 2026 23:35

ImproperCTypes: add recursion limit

298f51c

Simple change to stop irregular recursive types from causing infinitely-deep recursion in type checking.

niacdoial force-pushed the improperctypes-refactor3 branch from 4c78ff9 to 298f51c Compare May 7, 2026 23:36

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2026

jieyouxu removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 4, 2026

jdonszelmann requested changes Jun 8, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2026

	/// New-type wrapper around `usize` for representing limits. Ensures that comparisons against
	/// limits are consistent throughout the compiler.
	#[derive(Clone, Copy, Debug, HashStable_Generic)]
	pub struct Limit(pub usize);

	impl Limit {
	/// Create a new limit from a `usize`.
	pub fn new(value: usize) -> Self {
	Limit(value)
	}

	/// Create a new unlimited limit.
	pub fn unlimited() -> Self {
	Limit(usize::MAX)
	}

	/// Check that `value` is within the limit. Ensures that the same comparisons are used
	/// throughout the compiler, as mismatches can cause ICEs, see #72540.
	#[inline]
	pub fn value_within_limit(&self, value: usize) -> bool {
	value <= self.0
	}
	}

Uh oh!

Conversation

niacdoial commented Sep 6, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niacdoial Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Sep 7, 2025

Uh oh!

niacdoial commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

rustbot commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

tgross35 commented Jan 23, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented May 7, 2026

Uh oh!

niacdoial commented May 8, 2026

Uh oh!

jdonszelmann left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jun 8, 2026

Uh oh!

jdonszelmann commented Jun 8, 2026

Uh oh!

niacdoial commented Sep 6, 2025 •

edited by rustbot

Loading

tgross35 left a comment •

edited

Loading

niacdoial Sep 7, 2025 •

edited

Loading

niacdoial commented Sep 11, 2025 •

edited

Loading

jdonszelmann left a comment •

edited by rustbot

Loading