lint / ImproperCTypes: better handling of indirections, take 2

[niacdoial]

This PR tries to re-add the changes in #131669 (which were reverted in #134064 after one (1) nightly),
and adds better coverage of ty_kinds:

• in the take-1-added TypeSizedness enum and its construction
• in a new test file

The changes in the original PR aim to make ImproperCTypes/ImproperCTypesDefinitions produce better warnings when dealing with indirections (Box, &T, *T), especially for those to DSTs.

r? workingjubilee (because you reviewed the first attempt)

[niacdoial]

(
hi Jubilee, I'm back at this again!
I know this is not the best time of year to add PRs, so I'm fine with postponing this if you don't feel like tackling it these upcoming weeks.
In any case, have some nice end-of-year festivities, if you celebrate any!
)

[niacdoial]

ah, and before I forget: a small part of the new test file is commented out because it hits ICE #134587, but there should be more than decent coverage anyway

[workingjubilee]

unfortunately the lint needs to be gutted and rewritten.

[workingjubilee]

Also while I was possibly having a mild case of get-there-itis and thus mostly tried to just make sure things were coherent, I would prefer all new code for the lint be in compiler/rustc_lint/src/types/improper_ctypes.rs.

[workingjubilee]

The version cut happened so there will be less time pressure now.

[niacdoial]

I have a first version for compiler/rustc_lint/src/types/improper_ctypes.rs if you want.
it's less of a from-the-ground-up rewrite as it is scrapping the original for parts, if the analogy makes sense.

you probably have things to say about its architecture, even if the whole thing still have a bunch of TODO comments
the progress so far looks like this:

• completely separate the type-checking and reporting systems
• (part of the way there) remove some of the special cases and integrate them to the "main logic"
  • check_for_opaque_types is still a "special case" part of the checking logic
  • Cstr and Cstring are also somewhat special-cased because the advice for them depends on the type around them, if any
  • the unit type is handled in multiple places, see if this can be fixed
• (almost complete) compile, pass existing tests
  • only failed tests are for Cstring, due to different error messages
  • one unrelated test had to have a second "#[allow(improper_ctypes)]" added, but it makes more sense for it to need that anyway
• better separation of the different checks in different visit_* methods of ImproperCTypesVisitor
• better tracking of how the currently-checked type is used (static, function argument, function return's inner type, etc...)
  • raises questions about the separation of improper_ctypes and improper_ctypes_definitions versus declared/defined functions, especially when FnPtr:s are involved
• allow single argument check to emit multiple errors (for fnptr:s, structures with multiple FFI-unsafe fields, etc)
• review what is considered FFI-safe or not (once everything else is complete)

If you want to take a look in this state, should I just commit it here? (possibly put the PR in draft mode while I'm at it?)
or send you the files in a different way?

[bors]

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

[niacdoial]

aaaand I think I have something that's "first draft" material! (should I put this pull in the "draft" state?)
There's still a bunch of TODOs (...well, they were changed to FIXMEs to be pushed here) for questions I didn't manage to answer (and a bunch of failing tests because I don't know if the error should be here), but yeah.

here's a list of some of my remaining questions and concerns:

• visit_numeric seems too x86_64-specific

• should we revisit the distinction between ImproperCTypes and ImproperCTypesDefinitions?

  • part 1: the output ("external fn" or vs "external block" vs other possibilities)
  • part 2: handling opaque types (there's a high correlation between ImproperCTypesDefinitions and places where we allow FFI-opaque types to be fully specified. do we want this correlation to be 1?)

• more on FFI-opaque types: how do we handle that in the context of the "context switch" between functions and possible FnPtr arguments? The answer that seems correct currently prevents a stage1 compiler from being built

  • should we introduce a std::ffi::FfiOpaquePtr type? (which would be a *const c_void and some phantomdata, on first approximation)

• for indirections whose values may be supplied by non-rust code: do we only allow pointers (and Optionstd::ptr::NonNull), or do we also allow Option<&T> and Option<Box<T>>?

• not sure if the new error messages are intelligible in all cases (especially if there's a type param like Self or <Self as ::std::ops::Add<Self>>::Output that gets resolved in the error message).

• it feels like the current handling of CStr/Cstring and Option-like enums uses special casing, since those are tested for in multiple places.

• if we deny references and boxes in defined functions, what of &self in methods? We don't allow *const Self, last I checked.

[bors]

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

[workingjubilee]

hmm.

[workingjubilee]

aaaand I think I have something that's "first draft" material! (should I put this pull in the "draft" state?)

Yes, it's a good marker for "I don't want this merged yet, even if it looks done".

[workingjubilee]

visit_numeric seems too x86_64-specific

It probably is.

should we revisit the distinction between ImproperCTypes and ImproperCTypesDefinitions?

Yes, but in particular, not to just repartition them between: I think breaking them into as many conceptually-smaller lints as possible is good, as long as each one is a distinct idea (no splitting just for the sake of splitting!).

more on FFI-opaque types: how do we handle that in the context of the "context switch" between functions and possible FnPtr arguments? The answer that seems correct currently prevents a stage1 compiler from being built

I'm not sure what you mean?

for indirections whose values may be supplied by non-rust code: do we only allow pointers (and Option<std::ptr::NonNull>), or do we also allow Option<&T> and Option<Box<T>>?

We must allow Rust code to declare a pointer in a C signature to be Option<&T> or a number of things about our FFI story fall apart.

it feels like the current handling of CStr/CString and Option-like enums uses special casing, since those are tested for in multiple places.

Yes, probably.

[workingjubilee]

some initial nits on a first pass

[workingjubilee]

1. Because unsized function parameters are something we may want to support.
2. The code may not be well-formed: as you may have noticed at some point, you get warnings even if you get errors (usually), and this is because we lint even on "bad" code. This is because rustc didn't use to, once upon a time, and it was a bad debugging experience.

[workingjubilee]

etc... please encase the magic numbers.

[niacdoial]

alright, sorry for taking a while!

I'm currently planning what changes I'll do in terms of splitting the lint(s)
my current idea is to separate based on the nature of the thing being (presumably) propped up against a FFI boundary

• improper_ctypes: what's definitely an interface to an outside library (extern statics, extern function declarations)
• improper_ctypes_fn_definitions: functions written in rust intended to be exported
• improper_ctypes_callbacks: FnPtr arguments, no matter in what function they are being used
• improper_ctypes_ty_definitions: repr(C) structs/enums(/unions)?

more on FFI-opaque types: how do we handle that in the context of the "context switch" between functions and possible FnPtr arguments? The answer that seems correct currently prevents a stage1 compiler from being built

I'm not sure what you mean?

well, this is more or less answered in what I said before that, but my question was about how to deal with "switching" from checking arguments for, say, a function definition, to checking the arguments of a FnPtr argument?

1. should the nature of the lint change?
   (temptative answer: yes)
2. how should FFI-Safe-pointers-to-FFI-Unsafe-pointees work in FnPtr arguments? Should it be the rules for extern fn declarations? (throw the lint because one should use *const c_void, an extern type declaration, etc...) or the rules for extern fn definitions? (allow that, the function's body needs the full type even if it's opaque to the other side of the FFI boundary)
   (temptative answer: it should be the former, but parts of the rustc codebase doesn't follow this rule, so I can't get a stage1 compiler if I make that the rule)

---

// you would think that int-range pattern types that exclude 0 would have Option layout optimisation
// they don't (see tests/ui/type/pattern_types/range_patterns.stderr)
// so there's no need to allow Option<pattern_type!(u32 in 1..)>.

oh, I should fix that probably

I... maybe? I can't for the life of me find the link to that again but I think I saw a discussion about that and type covariance/contravariance,
where i32 is 1.. is a subtype of i32 (well that was under consideration), meaning fn(Option<i32>) is a type of fn(Option<i32 is 1..>) and it might have impacts on whether there should be an optimisation because of transmutation?

Though you'll definitely know more than me on all the moving parts.
Especially assuming you might have looked at this more in the past week.

---

As for the rest of your advice, I already took all this in!
thanks for shedding light on my code, one nit at a time!

[workingjubilee]

...Regardless, I think any reworking should start with scaling as far as possible back to "this is definitely inappropriate" versus "this is trying to catch a pattern that can be used correctly but usually isn't".

And I think that anything that assumes that a C-ABI-compatible pointer with an ABI-incompatible pointee is not merely being used opaquely is an example of the latter, of course.

[niacdoial]

it has been. two months.
er... here, it's what I currently have. it still needs to be rebased on top of the main branch, but if for some reason you wanted to review it early (or if you wanted proof that I didn't just give up), here it is.

I think I made quite a few decisions, but here are the most important ones I remember:

• repr(C) on a struct/union/enum is now seen as enough of a signal for the lint to look closer
• the "this type's value can come from the other side of the FFI boundary and might break the nonzero assumption" part is deactivated when linting on FnPtrs (I'm banking on the warning showing up in another place anyway, eventhough there's a specific case where it might not)
• uninhabited types are now linted against in argument position and static variables, and only ! and empty enums are allowed as uninhabited return types.
• &self in methods will be considered just as unsafe as other un-Option'd references, eventhough last I checked self: Option<&Self> is not something that compiles

[niacdoial]

alright, now the latest commit does (locally) pass tests and build stage2.
(it also did yesterday before that impromptu rebase)
I'll check the previous commits weren't degraded tomorrow, and then I'll make a summary of what is done and what still needs to be done

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

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

#16 2.930 Building wheels for collected packages: reuse
#16 2.931   Building wheel for reuse (pyproject.toml): started
#16 3.148   Building wheel for reuse (pyproject.toml): finished with status 'done'
#16 3.149   Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132719 sha256=d2a2565e7037ad3883fb9337653f2e25bbb588534fbef3697286cbc26d1bf634
#16 3.149   Stored in directory: /tmp/pip-ephem-wheel-cache-1icaauqr/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#16 3.151 Successfully built reuse
#16 3.152 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#16 3.554 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#16 3.554 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#16 DONE 3.6s
---
    Checking rustc_mir_transform v0.0.0 (/checkout/compiler/rustc_mir_transform)
error: mutable borrow from immutable input(s)
    --> compiler/rustc_codegen_llvm/src/llvm/ffi.rs:1309:68
     |
1309 |     |wrap pub(crate) fn LLVMCreateBuilderInContext(C: &Context) -> &mut Builder<'_>;
     |                                                                    ^^^^^^^^^^^^^^^^
     |
note: immutable borrow here
    --> compiler/rustc_codegen_llvm/src/llvm/ffi.rs:1309:55
     |
1309 |     |wrap pub(crate) fn LLVMCreateBuilderInContext(C: &Context) -> &mut Builder<'_>;
     |                                                       ^^^^^^^^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref
     = note: `-D clippy::mut-from-ref` implied by `-D clippy::correctness`
     = help: to override `-D clippy::correctness` add `#[allow(clippy::mut_from_ref)]`

error: mutable borrow from immutable input(s)
    --> compiler/rustc_codegen_llvm/src/llvm/ffi.rs:2569:69
     |
2569 |     |wrap pub(crate) fn LLVMRustArchiveIteratorNew(AR: &Archive) -> &mut ArchiveIterator<'_>;
     |                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: immutable borrow here
    --> compiler/rustc_codegen_llvm/src/llvm/ffi.rs:2569:56
     |
2569 |     |wrap pub(crate) fn LLVMRustArchiveIteratorNew(AR: &Archive) -> &mut ArchiveIterator<'_>;
     |                                                        ^^^^^^^^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref

error: mutable borrow from immutable input(s)
    --> compiler/rustc_codegen_llvm/src/llvm/ffi.rs:2690:58
     |
2690 |     |wrap pub(crate) fn LLVMRustLinkerNew(M: &Module) -> &mut Linker<'_>;
     |                                                          ^^^^^^^^^^^^^^^
     |
note: immutable borrow here
    --> compiler/rustc_codegen_llvm/src/llvm/ffi.rs:2690:46
     |
2690 |     |wrap pub(crate) fn LLVMRustLinkerNew(M: &Module) -> &mut Linker<'_>;
     |                                              ^^^^^^^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref

[RUSTC-TIMING] rustc_codegen_llvm test:false 5.433
error: could not compile `rustc_codegen_llvm` (lib) due to 3 previous errors