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

niacdoial · 2025-09-06T15:08:57Z

This is the second 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 PR, and splits the core type checking function into several bits, each focused on a specific aspect of FFI-safety.
Some logic which was outside of said core function was also moved into the new functions.

Superset of: #146271

tgross35

I think a lot of this makes sense, but aren't there behavior changes here? It looks like tuples and arrays may be treated slightly differently.

Which is probably fine, that would ideally just be split from the refactoring and come with test updates.

View changes since this review

niacdoial · 2025-09-06T20:33:01Z

I think a lot of this makes sense, but aren't there behavior changes here? It looks like tuples and arrays may be treated slightly differently.

Which is probably fine, that would ideally just be split from the refactoring and come with test updates.

I moved the actual change in behaviour in a later commit.
The rest of the changes here are just an exercise in moving more of the type-checking logic into the visit_* methods.

tgross35 · 2025-09-07T05:13:12Z

I moved the actual change in behaviour in a later commit.
The rest of the changes here are just an exercise in moving more of the type-checking logic into the visit_* methods.

"split type visiting into subfunctions" still has some changes right? Array went from just checking the type to checking whether or not it is in a function. Which is probably a reasonable change to make, it should just be its own thing (and come with a test update).

(Possible I'm missing something here)

niacdoial · 2025-09-07T11:27:26Z

Array went from just checking the type to checking whether or not it is in a function.

That's a bit of logic that was moved from check_type to visit_type

tgross35 · 2025-09-13T19:05:21Z

Btw if these are ready for a more final review, feel free to un-draft them (just gets them actually into my queue)

niacdoial · 2025-09-19T21:16:32Z

just double-checked:
I'm pretty sure I covered all the things you made reviews on
(the one change in this force-push is renaming IndirectionType->IndirectionKind)

tgross35 · 2025-09-22T18:35:19Z

@niacdoial what exactly are these waiting on? I assume they're close to ready based on your above comment, but they are still marked as drafts.

niacdoial · 2025-09-22T18:39:07Z

ah, I knew I was missing something (talking about the PR still being a draft)

tgross35

Sorry for the delay here, I've been behind for al little while. Should be catching up now, though

View changes since this review

JohnTitor · 2026-04-09T00:30:50Z

@rustbot reroll

nnethercote · 2026-04-09T02:10:52Z

Sorry to play hot potato, but this seems like the most realistic outcome:

r? @petrochenkov

petrochenkov · 2026-04-09T05:40:31Z

Ok, then we return to

I can gradually review and merge this piece-by-piece is you submit the changes in smaller single-purpose portions, each accompanied with motivation
feel free to start splitting and assigning to me

@rustbot author

rustbot · 2026-04-09T05:40:36Z

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

niacdoial · 2026-04-15T21:12:07Z

ok! all is split, with the two commits "before" this one being #155358 and #155359
sorry it took some time, I ended up being far more busy than I thought I would

petrochenkov · 2026-04-16T11:17:25Z

Blocked on #155359.
@rustbot blocked

….2, r=petrochenkov Improperctypes refactor2.2 This is "part 2/3 of 2/3 of 1/2" of the original pull request rust-lang#134697 (refactor plus overhaul of the ImproperCTypes family of lints) (all pulls of this series of pulls are supersets of the previous pulls.) previous pull: rust-lang#155358 next pull: rust-lang#146273 This commit splits the lint's `visit_type` function into multiple functions that focus on specific things: - visit_indirection (references, boxes, raw pointers) - visit_variant_fields (the list of fields of a struct, enum variant, or union) - visit_enum - visit_struct_or_union - visit_type (most "easy" decisions such as labeling `char` unsafe are here) since, during these visits, we often move from an "outer type" to an "inner type" (structs, arrays, pointers, etc...), two structs have been added to track the current state of a visit: - VisitorState tracks the state related to the "original type" being checked (function argument/return, static variable) - OuterTyData tracks the data related to the type "immediately outer to the current visited type" r? petrochenkov (because you asked me to)

Rollup merge of #155359 - niacdoial:improperctypes-refactor2.2, r=petrochenkov Improperctypes refactor2.2 This is "part 2/3 of 2/3 of 1/2" of the original pull request #134697 (refactor plus overhaul of the ImproperCTypes family of lints) (all pulls of this series of pulls are supersets of the previous pulls.) previous pull: #155358 next pull: #146273 This commit splits the lint's `visit_type` function into multiple functions that focus on specific things: - visit_indirection (references, boxes, raw pointers) - visit_variant_fields (the list of fields of a struct, enum variant, or union) - visit_enum - visit_struct_or_union - visit_type (most "easy" decisions such as labeling `char` unsafe are here) since, during these visits, we often move from an "outer type" to an "inner type" (structs, arrays, pointers, etc...), two structs have been added to track the current state of a visit: - VisitorState tracks the state related to the "original type" being checked (function argument/return, static variable) - OuterTyData tracks the data related to the type "immediately outer to the current visited type" r? petrochenkov (because you asked me to)

rustbot · 2026-04-21T21:31:01Z

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.

niacdoial · 2026-04-22T10:55:10Z

@rustbot review

petrochenkov · 2026-04-22T12:10:59Z

+            | ty::Tuple(..)
+            | ty::Array(..)
+            | ty::Slice(_) => Self::UnusedVariant,
+            k @ _ => bug!("Unexpected outer type {:?} of kind {:?}", ty, k),


Suggested change

k @ _ => bug!("Unexpected outer type {:?} of kind {:?}", ty, k),

_ => bug!("unexpected outer type {ty:?}),

Debug printing for Ty already redirects to TyKind printing.

View changes since the review

petrochenkov · 2026-04-22T12:11:41Z

+
+impl OuterTyKind {
+    /// Computes the relationship by providing the containing mir::Ty itself
+    fn from_outer_ty<'tcx>(ty: Ty<'tcx>) -> Self {


Suggested change

fn from_outer_ty<'tcx>(ty: Ty<'tcx>) -> Self {

fn from_ty<'tcx>(ty: Ty<'tcx>) -> Self {

Nit: already have "outer" in OuterTyKind.

View changes since the review

petrochenkov · 2026-04-22T12:13:33Z

+    /// but is needed because we don't change the lint's behavior yet
+    NoneThroughFnPtr,
+    /// Placeholder for properties that will be used eventually
+    UnusedVariant,


Suggested change

UnusedVariant,

Other,

It's sort of used, because it indicates that the parent type is one of the allowed ones (and we panic on non-allowed). The comment can be also updated to "other allowed outer type".

View changes since the review

petrochenkov · 2026-04-22T12:18:11Z

+impl VisitorState {
+    /// From an existing state, compute the state of any subtype of the current type.
+    /// (General case. For the case where the current type is a function pointer, see `get_next_in_fnptr`.)
+    fn get_next<'tcx>(&self, current_ty: Ty<'tcx>) -> Self {


Suggested change

fn get_next<'tcx>(&self, current_ty: Ty<'tcx>) -> Self {

fn next(&self, current_ty: Ty<'_>) -> Self {

get_ is usually omitted in typical naming conventions because it's kind of useless.

some of the lifetimes here and below are probably unnecessary.

View changes since the review

petrochenkov · 2026-04-22T12:21:22Z

+    /// (General case. For the case where the current type is a function pointer, see `get_next_in_fnptr`.)
+    fn get_next<'tcx>(&self, current_ty: Ty<'tcx>) -> Self {
+        assert!(!matches!(current_ty.kind(), ty::FnPtr(..)));
+        Self {


Suggested change

Self {

VisitorState {

Could you avoid Self in non-generic code? At least in constructors like this.

View changes since the review

OK! I'm also changing the type annotation for OuterTyKind::from_ty's return and the RootUseFlags consts. Is this something you want?
edit: x fmt undoes that so...

petrochenkov · 2026-04-22T12:21:44Z

+            root_use_flags: self.root_use_flags,
+            outer_ty_kind: OuterTyKind::from_outer_ty(current_ty),
        }
    }


Mega-nit: space between functions.

View changes since the review

petrochenkov · 2026-04-22T12:23:17Z

+    /// Generate the state for an "outermost" type that needs to be checked
+    fn entry_point(root_use_flags: RootUseFlags) -> Self {
+        Self { root_use_flags, outer_ty_kind: OuterTyKind::None }
+    }


I'd inline and remove this function, it's trivial and is only used twice.

View changes since the review

petrochenkov · 2026-04-22T12:25:35Z

+    }
+
+    /// Get the proper visitor state for a static variable's type
+    fn static_var() -> Self {


Suggested change

fn static_var() -> Self {

fn static_entry_point() -> Self {

Nit: I don't think these are called "variables" often in Rust.
Also for consistency with fn_entry_point.

View changes since the review

petrochenkov · 2026-04-22T12:29:17Z

+    }
+
+    /// Whether the type itself is the type of a function argument or return type.
+    fn is_direct_in_function(&self) -> bool {


I'd rather not introduce simple functions that are used once, this one and is_direct_function_return can be inlined and removed.

View changes since the review

petrochenkov · 2026-04-22T12:35:15Z

+    }
+
+    /// Get the proper visitor state for a given function's arguments or return type.
+    fn entry_point_from_fnmode(fn_mode: CItemKind, fn_pos: FnPos) -> Self {


Suggested change

fn entry_point_from_fnmode(fn_mode: CItemKind, fn_pos: FnPos) -> Self {

fn fn_entry_point(fn_mode: CItemKind, fn_pos: FnPos) -> Self {

For consistency with static_entry_point.
The _from_fnmode suffix would make sense if there were other simpler fn_entry, not from fn mode.

View changes since the review

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

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

niacdoial mentioned this pull request Sep 6, 2025

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

Open

tgross35 requested changes Sep 6, 2025

View reviewed changes

niacdoial force-pushed the improperctypes-refactor2 branch from 1dbb0e2 to f54061c Compare September 6, 2025 20:31

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor2 branch from f54061c to 66037fd Compare September 6, 2025 21:03

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor2 branch from 66037fd to 2781ebd Compare September 6, 2025 22:23

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

tgross35 reviewed Sep 7, 2025

View reviewed changes

Comment thread compiler/rustc_lint/src/types/improper_ctypes.rs Outdated

niacdoial force-pushed the improperctypes-refactor2 branch 2 times, most recently from efe195a to 69b0807 Compare September 11, 2025 21:56

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor2 branch from 69b0807 to 64106e6 Compare September 11, 2025 22:10

tgross35 self-assigned this Sep 12, 2025

niacdoial force-pushed the improperctypes-refactor2 branch from 64106e6 to 359cb79 Compare September 19, 2025 21:15

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

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 Sep 22, 2025

tgross35 requested changes Oct 21, 2025

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2026

rustbot assigned nnethercote and unassigned JohnTitor Apr 9, 2026

rustbot assigned petrochenkov and unassigned nnethercote Apr 9, 2026

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 Apr 9, 2026

niacdoial mentioned this pull request Apr 15, 2026

Improperctypes refactor2.2 #155359

Merged

niacdoial force-pushed the improperctypes-refactor2 branch from c52aa53 to 2150411 Compare April 15, 2026 21:10

rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2026

This comment has been minimized.

Sign in to view

niacdoial force-pushed the improperctypes-refactor2 branch from 2150411 to ac62a57 Compare April 21, 2026 21:30

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 22, 2026

petrochenkov reviewed Apr 22, 2026

View reviewed changes

petrochenkov 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 Apr 22, 2026

ImproperCTypes: merge outer_ty information into VisitorState

405a44a

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

niacdoial force-pushed the improperctypes-refactor2 branch from ac62a57 to 405a44a Compare April 24, 2026 06:29

	k @ _ => bug!("Unexpected outer type {:?} of kind {:?}", ty, k),
	_ => bug!("unexpected outer type {ty:?}),

	fn from_outer_ty<'tcx>(ty: Ty<'tcx>) -> Self {
	fn from_ty<'tcx>(ty: Ty<'tcx>) -> Self {

	fn get_next<'tcx>(&self, current_ty: Ty<'tcx>) -> Self {
	fn next(&self, current_ty: Ty<'_>) -> Self {

	fn entry_point_from_fnmode(fn_mode: CItemKind, fn_pos: FnPos) -> Self {
	fn fn_entry_point(fn_mode: CItemKind, fn_pos: FnPos) -> Self {

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niacdoial commented Sep 6, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgross35 commented Sep 7, 2025

Uh oh!

niacdoial commented Sep 7, 2025

Uh oh!

Uh oh!

This comment has been minimized.

tgross35 commented Sep 13, 2025

Uh oh!

niacdoial commented Sep 19, 2025

Uh oh!

tgross35 commented Sep 22, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JohnTitor commented Apr 9, 2026

Uh oh!

nnethercote commented Apr 9, 2026

Uh oh!

petrochenkov commented Apr 9, 2026

Uh oh!

rustbot commented Apr 9, 2026

Uh oh!

niacdoial commented Apr 15, 2026

Uh oh!

petrochenkov commented Apr 16, 2026 • 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 Apr 21, 2026

Uh oh!

niacdoial commented Apr 22, 2026

Uh oh!

petrochenkov Apr 22, 2026 • 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!

petrochenkov Apr 22, 2026 • 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!

petrochenkov Apr 22, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

niacdoial commented Sep 6, 2025 •

edited by rustbot

Loading

tgross35 left a comment •

edited by rustbot

Loading

niacdoial commented Sep 22, 2025 •

edited

Loading

tgross35 left a comment •

edited by rustbot

Loading

petrochenkov commented Apr 16, 2026 •

edited

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

niacdoial Apr 23, 2026 •

edited

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading