lint ImproperCTypes: refactor linting architecture (part 3) #146275

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

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.

rustbot · 2025-10-25T20:04:13Z

This PR was rebased onto a different master 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.

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

ImproperCTypes: split type visiting into subfunctions

f5a441e

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.

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 added 2 commits October 25, 2025 22:13

ImproperCTypes: merge outer_ty information into VisitorState

ee32bf7

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

ImproperCTypes: add recursion limit

85b72da

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

lint ImproperCTypes: refactor linting architecture (part 3) #146275

lint ImproperCTypes: refactor linting architecture (part 3) #146275

niacdoial commented Sep 6, 2025 •

edited by rustbot

Loading

tgross35 left a comment •

edited

Loading

tgross35 Sep 7, 2025

niacdoial Sep 7, 2025 •

edited

Loading

tgross35 Sep 7, 2025

niacdoial Sep 7, 2025

tgross35 Sep 7, 2025

niacdoial Sep 7, 2025

tgross35 Oct 21, 2025

tgross35 Sep 7, 2025

bors commented Sep 7, 2025

niacdoial commented Sep 11, 2025 •

edited

Loading

This comment has been minimized.

rustbot commented Sep 22, 2025

tgross35 Oct 21, 2025

niacdoial Oct 21, 2025

tgross35 Oct 22, 2025

tgross35 Oct 21, 2025

tgross35 Oct 21, 2025

tgross35 Oct 21, 2025

niacdoial Oct 21, 2025

niacdoial Oct 21, 2025

rustbot commented Oct 25, 2025

This comment has been minimized.

Labels

5 participants

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

lint ImproperCTypes: refactor linting architecture (part 3) #146275

Are you sure you want to change the base?

lint ImproperCTypes: refactor linting architecture (part 3) #146275

Conversation

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 7, 2025

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

Uh oh!

This comment has been minimized.

rustbot commented Sep 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Oct 25, 2025

This comment has been minimized.

Labels

5 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