lint/ctypes: fix `()` return type checks #113457

davidtwco · 2023-07-07T18:32:04Z

() is normally FFI-unsafe, but is FFI-safe when used as a return type. It is also desirable that a transparent newtype for () is FFI-safe when used as a return type.

In order to support this, when a type was deemed FFI-unsafe, because of a () type, and was used in return type - then the type was considered FFI-safe. However, this was the wrong approach - it didn't check that the () was part of a transparent newtype! The consequence of this is that the presence of a () type in a more complex return type would make it the entire type be considered safe (as long as the () type was the first that the lint found) - which is obviously incorrect.

Instead, this logic is removed, and after consultation with t-lang, I've fixed the bugs and inconsistencies and made () FFI-safe within types.

I also refactor a function, but that's not too exciting.

rustbot · 2023-07-07T18:32:10Z

r? @oli-obk

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

davidtwco · 2023-07-07T18:32:29Z

Probably needs a crater run, quite confident this is correct, but it doubles down on a regression.

compiler-errors · 2023-07-07T18:42:49Z

@bors try

bors · 2023-07-07T18:42:59Z

⌛ Trying commit 550c0126541435f5837c0778f1899349c322943c with merge b67f81b43b417bb7d00053f011fd9a7d6817a6bc...

lukas-code · 2023-07-07T19:18:48Z

Why is this considered FFI-safe:

#[repr(C)]
pub struct Foo {
    a: u8,
    b: core::marker::PhantomData<()>,
}

... but this is not:

#[repr(C)]
pub struct Foo {
    a: u8,
    b: (),
}

Aren't they both guaranteed to have the same layout and ABI as this C struct?

struct Foo {
    uint8_t a;
};

bors · 2023-07-07T20:01:03Z

☀️ Try build successful - checks-actions
Build commit: b67f81b43b417bb7d00053f011fd9a7d6817a6bc (b67f81b43b417bb7d00053f011fd9a7d6817a6bc)

davidtwco · 2023-07-07T20:34:07Z

Why is this considered FFI-safe:
#[repr(C)]
pub struct Foo {
    a: u8,
    b: core::marker::PhantomData<()>,
}
... but this is not:
#[repr(C)]
pub struct Foo {
    a: u8,
    b: (),
}
Aren't they both guaranteed to have the same layout and ABI as this C struct?
struct Foo {
    uint8_t a;
};

See #113436 (comment) for some thoughts on this - this patch will change the behaviour to fix this, if that's what we decide we want.

davidtwco · 2023-07-10T09:26:20Z

I've asked for some feedback on Zulip about which direction to go here.

`()` is normally FFI-unsafe, but is FFI-safe when used as a return type. It is also desirable that a transparent newtype for `()` is FFI-safe when used as a return type. In order to support this, when an type was deemed FFI-unsafe, because of a `()` type, and was used in return type - then the type was considered FFI-safe. However, this was the wrong approach - it didn't check that the `()` was part of a transparent newtype! The consequence of this is that the presence of a `()` type in a more complex return type would make it the entire type be considered safe (as long as the `()` type was the first that the lint found) - which is obviously incorrect. Instead, this logic is removed, and a unit return type or a transparent wrapper around a unit is checked for directly for functions and fn-ptrs. Signed-off-by: David Wood <david@davidtw.co>

Simplify this function a bit, it was quite hard to reason about. Signed-off-by: David Wood <david@davidtw.co>

Consider `()` within types to be FFI-safe, and `()` to be FFI-safe as a return type (incl. when in a transparent newtype). Signed-off-by: David Wood <david@davidtw.co>

davidtwco · 2023-07-19T09:01:17Z

This should now be ready for review.

oli-obk · 2023-07-19T09:01:32Z

@bors try

bors · 2023-07-19T09:01:41Z

⌛ Trying commit 24f90fd with merge e58ab0492e70733577a2aa8f09aa11a81ccf8005...

bors · 2023-07-19T10:17:53Z

☀️ Try build successful - checks-actions
Build commit: e58ab0492e70733577a2aa8f09aa11a81ccf8005 (e58ab0492e70733577a2aa8f09aa11a81ccf8005)

oli-obk · 2023-07-20T07:43:53Z

@craterbot check

craterbot · 2023-07-20T07:44:07Z

👌 Experiment pr-113457 created and queued.
🤖 Automatically detected try build e58ab0492e70733577a2aa8f09aa11a81ccf8005
🔍 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 · 2023-07-20T08:48:03Z

🚧 Experiment pr-113457 is now running

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

craterbot · 2023-07-21T16:47:19Z

🎉 Experiment pr-113457 is completed!
📊 21 regressed and 7 fixed (327819 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

davidtwco · 2023-07-24T08:47:30Z

All of these seem unrelated to the changes in this PR, I think the crater run is okay :)

oli-obk · 2023-07-26T09:11:39Z

@bors r+

bors · 2023-07-26T09:11:42Z

📌 Commit 24f90fd has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-07-26T15:27:39Z

⌛ Testing commit 24f90fd with merge 601a34d...

bors · 2023-07-26T17:23:44Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 601a34d to master...

bors · 2023-07-26T17:23:45Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 601a34d to master...

rust-timer · 2023-07-26T19:50:27Z

Finished benchmarking commit (601a34d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.5%	[-6.5%, -6.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.721s -> 650.146s (-0.09%)

apiraino · 2023-07-28T12:40:45Z

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

[beta] backport * Restrict linker version script of proc-macro crates to just its two symbols rust-lang#114470 * bootstrap: config: fix version comparison bug rust-lang#114440 * lint/ctypes: only try normalize rust-lang#113921 * Avoid tls access while iterating through mpsc thread entries rust-lang#113861 * Substitute types before checking inlining compatibility. rust-lang#113802 * Revert "fix: bug etc/bash_complettion -> src/etc/... to avoid copy error" rust-lang#113579 * lint/ctypes: fix () return type checks rust-lang#113457 * Rename and allow cast_ref_to_mut lint rust-lang#113422 * Ignore flaky clippy tests. rust-lang#113621 r? cuviper

rustbot assigned oli-obk Jul 7, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 7, 2023

davidtwco mentioned this pull request Jul 7, 2023

nightly/beta regression: fnptrs with types containing () is warned to be not FFI-safe, while it is before #113436

Closed

davidtwco added 3 commits July 19, 2023 09:44

lint: refactor check_variant_for_ffi

99b1897

Simplify this function a bit, it was quite hard to reason about. Signed-off-by: David Wood <david@davidtw.co>

lint/ctypes: allow () within types

24f90fd

Consider `()` within types to be FFI-safe, and `()` to be FFI-safe as a return type (incl. when in a transparent newtype). Signed-off-by: David Wood <david@davidtw.co>

davidtwco changed the title ~~lint/ctypes: stricter () return type checks~~ lint/ctypes: fix () return type checks Jul 19, 2023

davidtwco force-pushed the lint-ctypes-issue-113436 branch from 550c012 to 24f90fd Compare July 19, 2023 08:59

oli-obk 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 19, 2023

oxalica mentioned this pull request Jul 20, 2023

[ICE] when compiling abi_stable: Failed to normalize ..., maybe try to call try_normalize_erasing_regions instead #113900

Closed

4 tasks

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 21, 2023

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 26, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 26, 2023

bors merged commit 601a34d into rust-lang:master Jul 26, 2023

rustbot added this to the 1.73.0 milestone Jul 26, 2023

davidtwco deleted the lint-ctypes-issue-113436 branch July 27, 2023 08:23

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 28, 2023

cuviper mentioned this pull request Aug 12, 2023

[beta] backport #114748

Merged

cuviper modified the milestones: 1.73.0, 1.72.0 Aug 12, 2023

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 12, 2023

lint/ctypes: fix () return type checks #113457

lint/ctypes: fix () return type checks #113457

Uh oh!

Conversation

davidtwco commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 7, 2023

Uh oh!

davidtwco commented Jul 7, 2023

Uh oh!

compiler-errors commented Jul 7, 2023

Uh oh!

bors commented Jul 7, 2023

Uh oh!

lukas-code commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jul 7, 2023

Uh oh!

davidtwco commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidtwco commented Jul 10, 2023

Uh oh!

davidtwco commented Jul 19, 2023

Uh oh!

oli-obk commented Jul 19, 2023

Uh oh!

bors commented Jul 19, 2023

Uh oh!

bors commented Jul 19, 2023

Uh oh!

oli-obk commented Jul 20, 2023

Uh oh!

craterbot commented Jul 20, 2023

Uh oh!

craterbot commented Jul 20, 2023

Uh oh!

craterbot commented Jul 21, 2023

Uh oh!

davidtwco commented Jul 24, 2023

Uh oh!

oli-obk commented Jul 26, 2023

Uh oh!

bors commented Jul 26, 2023

Uh oh!

bors commented Jul 26, 2023

Uh oh!

bors commented Jul 26, 2023

Uh oh!

bors commented Jul 26, 2023

Uh oh!

rust-timer commented Jul 26, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

apiraino commented Jul 28, 2023

Uh oh!

Uh oh!

lint/ctypes: fix `()` return type checks #113457

lint/ctypes: fix `()` return type checks #113457

davidtwco commented Jul 7, 2023 •

edited

Loading

lukas-code commented Jul 7, 2023 •

edited

Loading

davidtwco commented Jul 7, 2023 •

edited

Loading