wf: check foreign fn decls for well-formedness#73323

davidtwco · 2020-06-13T18:58:57Z

Fixes #73252 and fixes #73253.

This PR extends current well-formedness checking to apply to foreign function declarations, re-using the existing machinery for regular functions. In doing this, later parts of the compiler (such as the improper_ctypes lint) can rely on being operations not failing as a result of invalid code which would normally be caught earlier.

rust-highfive · 2020-06-13T18:59:00Z

r? @ecstatic-morse

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

lcnr · 2020-06-14T07:22:20Z

Does this also fix #73253?

If so, this is a breaking change (which we should still land IMO) and may require a crater run 😅

Even this PR doesn't change its output, can you still add this as a test?

pub trait Unsatisfied {}

#[repr(transparent)]
pub struct Foo<T: Unsatisfied>(T);

extern "C" {
    pub fn foo() -> Foo<u32>;
}

davidtwco · 2020-06-14T12:59:13Z

Does this also fix #73253?

Even this PR doesn't change its output, can you still add this as a test?
pub trait Unsatisfied {}

#[repr(transparent)]
pub struct Foo<T: Unsatisfied>(T);

extern "C" {
    pub fn foo() -> Foo<u32>;
}

This also fixes #73253, I've added that to the test.

If so, this is a breaking change (which we should still land IMO) and may require a crater run 😅

Probably, I'll wait to see what the reviewer thinks - not entirely sure what the protocol is here.

lcnr

impl looks good 👍 requires crater run IMO

jonas-schievink · 2020-06-25T11:36:13Z

@bors try

Crater seems borked at the moment, but we can prepare a build.

bors · 2020-06-25T11:36:29Z

⌛ Trying commit 9fc57fb950edf74e880a1348a2860f41d481def1 with merge aef0c51c752d86410f02b64fbda721f083f33c39...

bors · 2020-06-25T14:01:41Z

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

ecstatic-morse · 2020-06-25T17:31:15Z

Sorry. I seem to have missed this one. The implementation looks good, and it seems like something that we clearly want to do. I would be surprised if this broke real code, but I feel a bit uncomfortable approving unilaterally since the possibility exists. @rust-lang/compiler, does anyone object to this PR? Otherwise, r=me if there are no crater regressions.

estebank · 2020-06-25T19:15:19Z

Code looks good and this is a clear fix of invalid code, but having tried something similar out for types, there is a possibility of fallout in crater run. If we find that that is the case for this as well, could we phase this in as a lint then?

nikomatsakis · 2020-06-26T20:11:35Z

Definitely do a crater run. We should be able to manage a future-compatibility warning. We may want to do it anyway, but a crater run is a good first step.

davidtwco · 2020-07-08T11:52:33Z

Rebased this to make sure it was still working - will do a try build then crater run.

@bors try

bors · 2020-07-08T11:52:46Z

⌛ Trying commit 822fe7f00cc35bdaae86add5fe18de20631a2cfd with merge 85cc1486e1f000dce4bb8e1b3cfab2b654b102a8...

bors · 2020-07-08T14:34:28Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 85cc1486e1f000dce4bb8e1b3cfab2b654b102a8 (85cc1486e1f000dce4bb8e1b3cfab2b654b102a8)

davidtwco · 2020-07-08T14:35:17Z

@craterbot check

craterbot · 2020-07-08T14:35:29Z

👌 Experiment pr-73323 created and queued.
🤖 Automatically detected try build 85cc1486e1f000dce4bb8e1b3cfab2b654b102a8
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-07-14T21:17:04Z

🚧 Experiment pr-73323 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-07-19T00:45:09Z

🎉 Experiment pr-73323 is completed!
📊 7 regressed and 10 fixed (112019 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr · 2020-07-19T07:37:32Z

It looks like all regressions are spurious to me.

This commit extends current well-formedness checking to apply to foreign function declarations, re-using the existing machinery for regular functions. In doing this, later parts of the compiler (such as the `improper_ctypes` lint) can rely on being operations not failing as a result of invalid code which would normally be caught earlier. Signed-off-by: David Wood <david@davidtw.co>

davidtwco · 2020-07-20T09:54:16Z

Rebased this PR; I agree with @lcnr that the crater report looks good, so I'll r=ecstatic-morse after CI passes.

davidtwco · 2020-07-20T11:56:39Z

@bors r=ecstatic-morse

bors · 2020-07-20T11:56:41Z

📌 Commit ceac273 has been approved by ecstatic-morse

@ghost

…arth Rollup of 13 pull requests Successful merges: - rust-lang#72714 (Fix debug assertion in typeck) - rust-lang#73197 (Impl Default for ranges) - rust-lang#73323 (wf: check foreign fn decls for well-formedness) - rust-lang#74051 (disallow non-static lifetimes in const generics) - rust-lang#74376 (test caching opt_const_param_of on disc) - rust-lang#74501 (Ayu theme: Use different background color for Run button) - rust-lang#74505 (Fix search input focus in ayu theme) - rust-lang#74522 (Update sanitizer docs) - rust-lang#74546 (Fix duplicate maybe_uninit_extra attribute) - rust-lang#74552 (Stabilize TAU constant.) - rust-lang#74555 (Improve "important traits" popup display on mobile) - rust-lang#74557 (Fix an ICE on an invalid `binding @ ...` in a tuple struct pattern) - rust-lang#74561 (update backtrace-rs) Failed merges: r? @ghost

rust-highfive assigned ecstatic-morse Jun 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2020

davidtwco force-pushed the issue-73252-wfcheck-foreign-fn-decl branch from c83d10f to 9fc57fb Compare June 14, 2020 12:57

lcnr approved these changes Jun 25, 2020

View reviewed changes

davidtwco force-pushed the issue-73252-wfcheck-foreign-fn-decl branch from 9fc57fb to 822fe7f Compare July 8, 2020 11:52

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2020

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 19, 2020

davidtwco force-pushed the issue-73252-wfcheck-foreign-fn-decl branch from 822fe7f to ceac273 Compare July 20, 2020 09:53

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2020

Manishearth mentioned this pull request Jul 20, 2020

Rollup of 13 pull requests #74569

Merged

bors merged commit 0897bc2 into rust-lang:master Jul 21, 2020

davidtwco deleted the issue-73252-wfcheck-foreign-fn-decl branch July 21, 2020 09:21

dtolnay mentioned this pull request Dec 29, 2020

ICE in rustc_infer error reporting on elided lifetime in extern signature #80468

Closed

cuviper added this to the 1.47.0 milestone May 2, 2024

Uh oh!

Conversation

davidtwco commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jun 13, 2020

Uh oh!

lcnr commented Jun 14, 2020

Uh oh!

davidtwco commented Jun 14, 2020

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

jonas-schievink commented Jun 25, 2020

Uh oh!

bors commented Jun 25, 2020

Uh oh!

bors commented Jun 25, 2020

Uh oh!

ecstatic-morse commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Jun 25, 2020

Uh oh!

nikomatsakis commented Jun 26, 2020

Uh oh!

davidtwco commented Jul 8, 2020

Uh oh!

bors commented Jul 8, 2020

Uh oh!

bors commented Jul 8, 2020

Uh oh!

davidtwco commented Jul 8, 2020

Uh oh!

craterbot commented Jul 8, 2020

Uh oh!

craterbot commented Jul 14, 2020

Uh oh!

craterbot commented Jul 19, 2020

Uh oh!

lcnr commented Jul 19, 2020

Uh oh!

davidtwco commented Jul 20, 2020

Uh oh!

davidtwco commented Jul 20, 2020

Uh oh!

bors commented Jul 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

davidtwco commented Jun 13, 2020 •

edited

Loading

ecstatic-morse commented Jun 25, 2020 •

edited

Loading