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

compiler/rustc_lint/src/types/improper_ctypes.rs

+        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

compiler/rustc_lint/src/types/improper_ctypes.rs

+        // 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

compiler/rustc_lint/src/types/improper_ctypes.rs

 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

compiler/rustc_lint/src/types/improper_ctypes.rs

+        // 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

tests/ui/lint/improper-ctypes/mustpass-irregular-recursive-types.rs

+//@ 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.

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

compiler/rustc_lint/src/types/improper_ctypes.rs

 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

tests/ui/lint/improper-ctypes/mustpass-non-recursion-limit.rs

+//@ 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

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

Same as at #146273 (comment),

r? compiler

Another interal change that shouldn't impact rustc users. The goal is to break apart the gigantic visit_type function into more managable and easily-editable bits that focus on specific parts of FFI safety.

Another user-transparent change, unifying outer-type information and the existing VisitorState flags.

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

rustbot · 2026-01-24T16:44:06Z

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.

rust-log-analyzer · 2026-01-24T17:36:08Z

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

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

REPOSITORY                                   TAG       IMAGE ID       CREATED      SIZE
ghcr.io/dependabot/dependabot-updater-core   latest    354d02aa29ac   5 days ago   783MB
=> Removing docker images...
Deleted Images:
untagged: ghcr.io/dependabot/dependabot-updater-core:latest
untagged: ghcr.io/dependabot/dependabot-updater-core@sha256:596da3f22bcbdff2c96fd7126001278022c834c1621c5efa2ad1a7794590636c
deleted: sha256:354d02aa29acf525570c732b6e006ecf138de6d63ca525d552eb4b24880ddc6c
deleted: sha256:8b7af0e426bc2cbeeacfd96b8354d3b80016991520977197e62090e47abaede8
deleted: sha256:cadf11ef1de7fdd5eab563757942353684047f09b212dc99d6ed48e8acf34d62
deleted: sha256:569b0caf9d5285db44ccd2629a3470139eea755be423a33a54d8a24cb3926bfa
deleted: sha256:f9dc5feb048d8f9fd43137e3998f59e9acfbd76c47a4e14984d109654119e282
---
Command `git clone --depth=1 https://github.com/rust-lang-nursery/rust-toolstate rust-toolstate` failed with exit code 128
Created at: src/bootstrap/src/core/build_steps/toolstate.rs:314:5
Executed at: src/bootstrap/src/core/build_steps/toolstate.rs:319:10

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:02:17
  local time: Sat Jan 24 17:33:46 UTC 2026
  network time: Sat, 24 Jan 2026 17:33:46 GMT
##[error]Process completed with exit code 1.
##[group]Run echo "disk usage:"

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 added 3 commits January 24, 2026 17:43

ImproperCTypes: split type visiting into subfunctions

cbda993

Another interal change that shouldn't impact rustc users. The goal is to break apart the gigantic visit_type function into more managable and easily-editable bits that focus on specific parts of FFI safety.

ImproperCTypes: merge outer_ty information into VisitorState

bce41ce

Another user-transparent change, unifying outer-type information and the existing VisitorState flags.

ImproperCTypes: add recursion limit

975b603

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

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

	/// 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!

This comment has been minimized.

This comment has been minimized.

tgross35 commented Jan 23, 2026

Uh oh!

rustbot commented Jan 24, 2026

Uh oh!

rust-log-analyzer commented Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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