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.

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)

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

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 added 3 commits September 19, 2025 23:15

ImproperCTypes: split type visiting into subfunctions

071fc71

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

359cb79

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

ImproperCTypes: add recursion limit

c74c323

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

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

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

Uh oh!

tgross35 left a comment •

edited

Loading

Uh oh!

tgross35 Sep 7, 2025

Uh oh!

niacdoial Sep 7, 2025 •

edited

Loading

Uh oh!

tgross35 Sep 7, 2025

Uh oh!

niacdoial Sep 7, 2025

Uh oh!

tgross35 Sep 7, 2025

Uh oh!

niacdoial Sep 7, 2025

Uh oh!

tgross35 Sep 7, 2025

Uh oh!

bors commented Sep 7, 2025

Uh oh!

niacdoial commented Sep 11, 2025 •

edited

Loading

Uh oh!

This comment has been minimized.

rustbot commented Sep 22, 2025

Uh oh!

Uh oh!

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

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!

tgross35 Sep 7, 2025

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!

tgross35 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

niacdoial Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

niacdoial Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Sep 7, 2025

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!

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