Rewrite the improper_ctypes lint. #26583

eefriedman · Jun 26, 2015

Makes the lint a bit more accurate, and improves the quality of the diagnostic
messages by explicitly returning an error message.

rust-highfive · Jun 26, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

eefriedman · Jun 26, 2015

With some help on IRC, I got part of the way handling projection types (54e183f). Still doesn't work completely correctly, though; not sure what's missing.

bors · Jun 27, 2015

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

eefriedman · Jul 2, 2015

Rebased, and updated with some handling for late-bound lifetimes. Ready to be reviewed, I think?

bors · Jul 4, 2015

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

eefriedman · Jul 4, 2015

Rebased.

eefriedman · Jul 21, 2015

Ping.

alexcrichton · Jul 21, 2015

r? @nrc

nrc · Jul 21, 2015

nrc reviewed Jul 21, 2015

View changes

nrc · Jul 21, 2015

src/librustc/middle/ty.rs

-    pub fn is_ffi_safe(&'tcx self, cx: &ctxt<'tcx>) -> bool {
-        !self.type_contents(cx).intersects(TC::ReachesFfiUnsafe)
-    }
-


Does this mean we no longer use ReachesFfiUnsafe at all? If so, it should be removed, not just this helper function.

nrc · Jul 21, 2015

nrc reviewed Jul 21, 2015

View changes

nrc · Jul 21, 2015

src/librustc_lint/builtin.rs

+/// "nullable pointer optimization". Currently restricted
+/// to function pointers and references, but could be
+/// expanded to cover NonZero raw pointers and newtypes.
+fn is_repr_nullable_ptr<'tcx>(variants: &Vec<Rc<ty::VariantInfo<'tcx>>>) -> bool {


Can we share this code with trans? I'm pretty uncomfortable with having two different ways of answering this question.

Pulling the whole algorithm out of trans into librustc is rather complicated... I can do it, but I think I'll submit it as a separate PR (it's a few hundred lines of code).

nrc · Jul 21, 2015

nrc reviewed Jul 21, 2015

View changes

nrc · Jul 21, 2015

src/librustc_lint/builtin.rs

+            }
+            ty::TyTuple(ref tys) => {
+                // The layout of tuples is implicitly repr(C).
+                // FIXME: Is that correct?


It is currently correct and unlikely to change (although technically, no guarantees).

After some discussion on IRC - why are tuples ever OK by the FFI lint? Since C has no tuples, it seems we should lint against passing them to C.

This follows what the old FFI lint did; I guess I can try changing it.

nrc · Jul 21, 2015

nrc reviewed Jul 21, 2015

View changes

nrc · Jul 21, 2015

src/librustc_lint/builtin.rs

+                self.check_type_for_ffi(cache, m.ty)
+            }
+
+            // FIXME: Maybe we want to prohibit arrays of zero-size types?


You should probably check for ZSTs at the 'top level' of this function - I believe it would be an error to pass a struct with a aero-sized field across FFI too?

nrc · Jul 21, 2015

nrc reviewed Jul 21, 2015

View changes

nrc · Jul 21, 2015

src/librustc_lint/builtin.rs

+
+            ty::TyBareFn(..) => {
+                // FIXME: Implement
+                FfiSafe


Better to be unsafe until this is implemented. Or perhaps add FfiUnimplemented

nrc · Jul 21, 2015

nrc reviewed Jul 21, 2015

View changes

nrc · Jul 21, 2015

src/librustc_lint/builtin.rs

+            }
+
+            // Primitive types with a stable representation.
+            ty::TyBool | ty::TyInt(..) | ty::TyUint(..) |


Shouldn't usize and isize be FfiUnsafe?

They are; I'll rearrange the function to make that more obvious.

Why are they considered unsafe for FFI? Are they not guaranteed to be defined the same as (one of) size_t/ssize_t, uintptr_t/intptr_t, on all our supported platforms?

On Jul 21, 2015, at 14:38, Nick Cameron notifications@github.com wrote:

In src/librustc_lint/builtin.rs:

ty::TyRawPtr(ref m) | ty::TyRef(_, ref m) => {

self.check_type_for_ffi(cache, m.ty)

}

// FIXME: Maybe we want to prohibit arrays of zero-size types?

ty::TyArray(ty, _) => {

self.check_type_for_ffi(cache, ty)

}

ty::TyBareFn(..) => {

// FIXME: Implement

FfiSafe

}

// Primitive types with a stable representation.

ty::TyBool | ty::TyInt(..) | ty::TyUint(..) |
Shouldn't usize and isize be FfiUnsafe?

—
Reply to this email directly or view it on GitHub.

If you were writing a malloc replacement in Rust, for example, you'd likely use size/usize internally, and it seems likely that you'd want to use those as well in your extern signature for tjmalloc or whatever.

The alternative is to use libc's size_t type alias in those signatures, but that seems to just be shoving the problem elsewhere.

On Jul 21, 2015, at 14:38, Nick Cameron notifications@github.com wrote:

In src/librustc_lint/builtin.rs:

ty::TyRawPtr(ref m) | ty::TyRef(_, ref m) => {

self.check_type_for_ffi(cache, m.ty)

}

// FIXME: Maybe we want to prohibit arrays of zero-size types?

ty::TyArray(ty, _) => {

self.check_type_for_ffi(cache, ty)

}

ty::TyBareFn(..) => {

// FIXME: Implement

FfiSafe

}

// Primitive types with a stable representation.

ty::TyBool | ty::TyInt(..) | ty::TyUint(..) |
Shouldn't usize and isize be FfiUnsafe?

—
Reply to this email directly or view it on GitHub.

They aren't really unsafe, per se. We just want to discourage people from using them; it's usually a good idea to use types from the libc crate in FFI signatures.

Granted, it used to be more of a concern when isize was named int.

The alternative is to use libc's size_t type alias in those signatures, but that seems to just be shoving the problem elsewhere.

By that I meant, by disallowing usize or size in FFI signatures: there are now two potential problems, that usize/size don't correspond to size_t/ssize_t, and that the libc::size_t and libc::ssize_t aliases are wrong. You'd need a cast somewhere, probably inside the FFI function, from libc::size_t (which is something like u64) to usize, which could cause hidden errors in the case that the alias in libc is wrong.

You might be right. That said, the old improper_ctypes lint did exactly the same thing, so I don't really want to change it here. Please file a bug, or open a thread on https://internals.rust-lang.org.

eefriedman · Jul 22, 2015

Updated to address review comments.

Function pointers are now checked. Tuples are now disallowed. Zero-size structs are now disallowed. is_repr_nullable_ptr is still a separate function; unifying it with the trans enum handling is going to be a big task.

tomjakubowski · Jul 22, 2015

Any chance fixes (or tests, if these commits already fix them) for #20098 and #19834 could slip in? :-)

eefriedman · Jul 22, 2015

This already fixes and tests #20098 (Box was affected by the same issue as String). #19834 is essentially orthogonal; I don't want to make too many changes at once.

nrc · Jul 22, 2015

Pulling the whole algorithm out of trans into librustc is rather complicated... I can do it, but I think I'll submit it as a separate PR (it's a few hundred lines of code).

Seems reasonable

nrc · Jul 22, 2015

@bors r+

bors · Jul 22, 2015

📌 Commit 484d071 has been approved by nrc

bors · Jul 22, 2015

⌛️ Testing commit 484d071 with merge d7e2f6f...

bors · Jul 23, 2015

📌 Commit 8d47c3d has been approved by alexcrichton

bors · Jul 23, 2015

⌛️ Testing commit 8d47c3d with merge 3825fc1...

bors · Jul 23, 2015

💔 Test failed - auto-mac-64-nopt-t

eefriedman · Jul 23, 2015

Fixed again...

nrc · Jul 23, 2015

@bors r+

bors · Jul 23, 2015

📌 Commit 858a1b9 has been approved by nrc

bors · Jul 23, 2015

⌛️ Testing commit 858a1b9 with merge 48ef346...

bors · Jul 23, 2015

💔 Test failed - auto-win-msvc-64-opt

eefriedman · Jul 24, 2015

Fixed again...

alexcrichton · Jul 24, 2015

@bors: r=nrc 6fa17b4

bors · Jul 24, 2015

⌛️ Testing commit 6fa17b4 with merge 68e0d13...

bors · Jul 24, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned pcwalton Jun 26, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch 2 times, most recently from 060b6eb to f951898 Jun 27, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch from f951898 to 09cfcba Jul 4, 2015

rust-highfive assigned nrc and unassigned pcwalton Jul 21, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch 3 times, most recently from dcc0cbb to 484d071 Jul 22, 2015

bors added a commit that referenced this pull request Jul 23, 2015

Auto merge of #26583 - eefriedman:lint-ffi, r=alexcrichton
Makes the lint a bit more accurate, and improves the quality of the diagnostic messages by explicitly returning an error message.

3825fc1

eefriedman force-pushed the eefriedman:lint-ffi branch from 8d47c3d to 858a1b9 Jul 23, 2015

bors added a commit that referenced this pull request Jul 23, 2015

Auto merge of #26583 - eefriedman:lint-ffi, r=nrc
Makes the lint a bit more accurate, and improves the quality of the diagnostic messages by explicitly returning an error message.

48ef346

eefriedman force-pushed the eefriedman:lint-ffi branch from 858a1b9 to 6fa17b4 Jul 24, 2015

bors added a commit that referenced this pull request Jul 24, 2015

Auto merge of #26583 - eefriedman:lint-ffi, r=nrc
Makes the lint a bit more accurate, and improves the quality of the diagnostic messages by explicitly returning an error message.

Loading status checks…

68e0d13

bors merged commit 6fa17b4 into rust-lang:master Jul 24, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

Eljay referenced this pull request Jul 26, 2015
Closed
FFI: improper_ctypes warning when using associated type in FFI signature #26230

tamird referenced this pull request Jul 26, 2015
Merged
Fix compile on nightly #1836

briansmith referenced this pull request Aug 30, 2015
Closed
Improve safety & usability of |size_t| and |ssize_t|. #28096

arielb1 referenced this pull request Sep 15, 2015
Merged
1.3 relnotes #28408

SSheldon added a commit to SSheldon/rust-objc that referenced this pull request Sep 20, 2015

Fix zero-size struct improper_ctypes warnings.
These warnings starting appearing from rust-lang/rust#26583.

Loading status checks…

dfa8b48

SSheldon added a commit to SSheldon/rust-dispatch that referenced this pull request Sep 21, 2015

Add a dispatch_object_s type to fix improper_ctypes warnings.
These warnings starting appearing from rust-lang/rust#26583.

d27d56a

SSheldon added a commit to SSheldon/rust-block that referenced this pull request Oct 10, 2015

Add a Class type to fix the improper_ctypes warning.
This warning starting appearing from rust-lang/rust#26583.

a1c5ef6

TimNN referenced this pull request Dec 20, 2015
Closed
Rust types in FFI lint #12608

rust-lang/rust

Join GitHub today

Rewrite the improper_ctypes lint. #26583

Conversation

eefriedman commented Jun 26, 2015

rust-highfive assigned pcwalton Jun 26, 2015

This comment has been minimized.

rust-highfive commented Jun 26, 2015

This comment has been minimized.

eefriedman commented Jun 26, 2015

This comment has been minimized.

bors commented Jun 27, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch 2 times, most recently from 060b6eb to f951898 Jun 27, 2015

This comment has been minimized.

eefriedman commented Jul 2, 2015

This comment has been minimized.

bors commented Jul 4, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch from f951898 to 09cfcba Jul 4, 2015

This comment has been minimized.

eefriedman commented Jul 4, 2015

This comment has been minimized.

eefriedman commented Jul 21, 2015

This comment has been minimized.

alexcrichton commented Jul 21, 2015

rust-highfive assigned nrc and unassigned pcwalton Jul 21, 2015

nrc reviewed Jul 21, 2015 View changes

This comment has been minimized.

nrc reviewed Jul 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nrc reviewed Jul 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nrc reviewed Jul 21, 2015 View changes

This comment has been minimized.

nrc reviewed Jul 21, 2015 View changes

This comment has been minimized.

nrc reviewed Jul 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eefriedman force-pushed the eefriedman:lint-ffi branch 3 times, most recently from dcc0cbb to 484d071 Jul 22, 2015

This comment has been minimized.

eefriedman commented Jul 22, 2015

This comment has been minimized.

tomjakubowski commented Jul 22, 2015

This comment has been minimized.

eefriedman commented Jul 22, 2015

This comment has been minimized.

nrc commented Jul 22, 2015

This comment has been minimized.

nrc commented Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

This comment has been minimized.

bors commented Jul 23, 2015

bors added a commit that referenced this pull request Jul 23, 2015

This comment has been minimized.

bors commented Jul 23, 2015

This comment has been minimized.

bors commented Jul 23, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch from 8d47c3d to 858a1b9 Jul 23, 2015

This comment has been minimized.

eefriedman commented Jul 23, 2015

This comment has been minimized.

nrc commented Jul 23, 2015

This comment has been minimized.

bors commented Jul 23, 2015

This comment has been minimized.

bors commented Jul 23, 2015

bors added a commit that referenced this pull request Jul 23, 2015

This comment has been minimized.

bors commented Jul 23, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch 2 times, most recently from `060b6eb` to `f951898` Jun 27, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch from `f951898` to `09cfcba` Jul 4, 2015

nrc reviewed Jul 21, 2015

View changes

nrc reviewed Jul 21, 2015

View changes

nrc reviewed Jul 21, 2015

View changes

nrc reviewed Jul 21, 2015

View changes

nrc reviewed Jul 21, 2015

View changes

nrc reviewed Jul 21, 2015

View changes

eefriedman force-pushed the eefriedman:lint-ffi branch 3 times, most recently from `dcc0cbb` to `484d071` Jul 22, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch from `8d47c3d` to `858a1b9` Jul 23, 2015

eefriedman force-pushed the eefriedman:lint-ffi branch from `858a1b9` to `6fa17b4` Jul 24, 2015

bors merged commit `6fa17b4` into rust-lang:master Jul 24, 2015
2 checks passed