Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improper_ctypes_definitions lint #72700

Merged

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented May 28, 2020

Addresses #19834, #66220, and #66373.

This PR takes another attempt at #65134 (reverted in #66378). Instead of modifying the existing improper_ctypes lint to consider extern "C" fn definitions in addition to extern "C" {} declarations, this PR adds a new lint - improper_ctypes_definitions - which only applies to extern "C" fn definitions.

In addition, the improper_ctype_definitions lint differs from improper_ctypes by considering *T and &T (where T: Sized) FFI-safe (addressing #66220).

There wasn't a clear consensus in #66220 (where the issues with #65134 were primarily discussed) on the approach to take, but there has been some discussion in Zulip. I fully expect that we'll want to iterate on this before landing.

cc @varkor + @shepmaster (from #19834) @hanna-kruppe (active in discussing #66220), @SimonSapin (#65134 caused problems for Servo, want to make sure that this PR doesn't)

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2020
@davidtwco davidtwco changed the title improper_ctypes_declarations lint improper_ctypes_definitions lint May 28, 2020
@SimonSapin
Copy link
Contributor

#65134 caused problems for Servo, want to make sure that this PR doesn't

A try build that I could install with rustup-toolchain-install-master would be good. Is the new lint warn by default? (Affected by -D warnings?)

@davidtwco
Copy link
Member Author

#65134 caused problems for Servo, want to make sure that this PR doesn't

A try build that I could install with rustup-toolchain-install-master would be good. Is the new lint warn by default? (Affected by -D warnings?)

The lint is warn-by-default.

@bors try

@bors
Copy link
Contributor

bors commented May 29, 2020

⌛ Trying commit 7cbdc2d58423511e6f336424955a17c891b13c03 with merge 672712eca084ef382c9505561aac6e15fbefe8ab...

@bors
Copy link
Contributor

bors commented May 29, 2020

☀️ Try build successful - checks-azure
Build commit: 672712eca084ef382c9505561aac6e15fbefe8ab (672712eca084ef382c9505561aac6e15fbefe8ab)

@SimonSapin
Copy link
Contributor

I am unable to test this build in Servo as the llvm-tools-preview component appears to not be available. It is now required for linking compiler plugins: #72594 (comment)

$ rustup-toolchain-install-master 672712eca084ef382c9505561aac6e15fbefe8ab -c rustc-dev -c llvm-tools-preview 
[…]
downloading <https://rust-lang-ci2.s3-us-west-1.amazonaws.com/rustc-builds/672712eca084ef382c9505561aac6e15fbefe8ab/llvm-tools-preview-nightly-x86_64-unknown-linux-gnu.tar.xz>...
error: missing component `llvm-tools-preview` on toolchain `672712eca084ef382c9505561aac6e15fbefe8ab` on channel `nightly` for target `x86_64-unknown-linux-gnu`

@hanna-kruppe
Copy link
Contributor

I don't have the time to actually review this at the moment, but after glancing over the implementation I remembered some unstated ideas underlying my past comments advocating for separate lints. The tl;dr is that I think the approach of adding more "modes" to the existing improper_ctypes visitor is not scalable as we add more lints that are concerned with layout/ABI questions (which we should), and the code could do with an untangling anyway. This definitely doesn't have to happen in this PR, though, so I wrote down the details in a new issue #72774 before I forget again.

@JohnCSimon JohnCSimon 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 Jun 6, 2020
@davidtwco davidtwco force-pushed the issue-66220-improper-ctypes-declarations branch from 7cbdc2d to dbc8a3a Compare June 9, 2020 10:23
@davidtwco
Copy link
Member Author

r? @varkor

@varkor
Copy link
Member

varkor commented Jun 9, 2020

Will try to review soon.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the issue-66220-improper-ctypes-declarations branch from 3d9e5ba to 379486c Compare June 19, 2020 13:12
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the issue-66220-improper-ctypes-declarations branch from 379486c to 055377c Compare June 20, 2020 11:06
@davidtwco davidtwco 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 Jun 20, 2020
@Aaron1011
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Jun 25, 2020

⌛ Testing commit 4504648 with merge ae3f308cf38eb55957fed008aa952276ac949b3e...

@bors
Copy link
Contributor

bors commented Jun 25, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 25, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#72700 (`improper_ctypes_definitions` lint)
 - rust-lang#73516 (Allow dynamic linking for iOS/tvOS targets)
 - rust-lang#73616 (Liballoc minor hash import tweak)
 - rust-lang#73634 (Add UI test for issue 73592)
 - rust-lang#73688 (Document the self keyword)
 - rust-lang#73698 (Add procedure for prioritization notifications on Zulip)

Failed merges:

r? @ghost
@bors bors merged commit 67db7a2 into rust-lang:master Jun 25, 2020
@davidtwco davidtwco deleted the issue-66220-improper-ctypes-declarations branch July 2, 2020 12:28
josephlr added a commit to josephlr/compiler-builtins that referenced this pull request Jul 8, 2020
rust-lang/rust#72700 caused the existing
`allow(improper_ctypes)` guard to stop working, we now need
`allow(improper_ctypes_definitions)` instead.

We keep the old one to avoid any issues with older nightlies.

Signed-off-by: Joe Richey <[email protected]>
josephlr added a commit to josephlr/compiler-builtins that referenced this pull request Jul 8, 2020
rust-lang/rust#72700 caused the existing
`allow(improper_ctypes)` guard to stop working, we now need
`allow(improper_ctypes_definitions)` instead.

We keep the old one to avoid any issues with older nightlies.

Signed-off-by: Joe Richey <[email protected]>
alexcrichton pushed a commit to rust-lang/compiler-builtins that referenced this pull request Jul 8, 2020
rust-lang/rust#72700 caused the existing
`allow(improper_ctypes)` guard to stop working, we now need
`allow(improper_ctypes_definitions)` instead.

We keep the old one to avoid any issues with older nightlies.

Signed-off-by: Joe Richey <[email protected]>
@alexcrichton
Copy link
Member

Sorry for being a bit late to the punch on this, but this feels like it's still noisy in some cases where it probably shouldn't be. For example:

warning: `extern` fn uses type `std::option::Option<std::boxed::Box<error::wasmtime_error_t>>`, which is not FFI-safe

where in this case wasmtime_error_t is a #[repr(C)] structure, but the internals are intentionally not FFI-safe. We're returning an opaque pointer across the FFI boundary so we don't want to make the contents all FFI-safe. This warning also seems to be about Option and Box which are perhaps missing being allowed since I believe they have defined ABIs?

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jul 16, 2020
This was enabled in rust-lang/rust#72700 but it looks like it's still
too noisy for it to be useful to us.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jul 16, 2020
This was enabled in rust-lang/rust#72700 but it looks like it's still
too noisy for it to be useful to us.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jul 16, 2020
This was enabled in rust-lang/rust#72700 but it looks like it's still
too noisy for it to be useful to us.
@alexcrichton
Copy link
Member

FWIW this is actually a bit extra onerous because if you write #![allow(improper_ctypes_definitions)] you also have to write #![allow(unknown_lints)] since the lint doesn't exist on stable right now.

bnjbvr pushed a commit to bytecodealliance/wasmtime that referenced this pull request Jul 16, 2020
This was enabled in rust-lang/rust#72700 but it looks like it's still
too noisy for it to be useful to us.
@CryZe
Copy link
Contributor

CryZe commented Jul 16, 2020

Yeah I'm now also getting these warnings all over the place for Rust types in Boxes, which should be FFI safe (in fact that's specifically documented to be FFI safe)

@davidtwco
Copy link
Member Author

I'll have a PR up to resolve this soon.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 17, 2020
…ns-boxes, r=davidtwco

improper_ctypes_definitions: allow `Box`

Addresses rust-lang#72700 (comment).

This PR stops linting against `Box` in `extern "C" fn`s for the `improper_ctypes_definitions` lint - boxes are documented to be FFI-safe.

cc @alexcrichton @CryZe
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…ns-boxes, r=davidtwco

improper_ctypes_definitions: allow `Box`

Addresses rust-lang#72700 (comment).

This PR stops linting against `Box` in `extern "C" fn`s for the `improper_ctypes_definitions` lint - boxes are documented to be FFI-safe.

cc @alexcrichton @CryZe
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…ns-boxes, r=davidtwco

improper_ctypes_definitions: allow `Box`

Addresses rust-lang#72700 (comment).

This PR stops linting against `Box` in `extern "C" fn`s for the `improper_ctypes_definitions` lint - boxes are documented to be FFI-safe.

cc @alexcrichton @CryZe
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…ns-boxes, r=davidtwco

improper_ctypes_definitions: allow `Box`

Addresses rust-lang#72700 (comment).

This PR stops linting against `Box` in `extern "C" fn`s for the `improper_ctypes_definitions` lint - boxes are documented to be FFI-safe.

cc @alexcrichton @CryZe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.