`UnsafeCell` blocks niches inside its nested type from being available outside #99011

oli-obk · 2022-07-07T10:49:27Z

This implements the plan by @eddyb in #87341 (comment)

Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): #94527

rustbot · 2022-07-07T10:49:30Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

rust-highfive · 2022-07-07T10:49:30Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_lint/src/types.rs

library/core/src/cell.rs

RalfJung · 2022-07-07T11:48:36Z

Please make sure this is covered by tests. #87341 has a nice test set at the top.

eddyb

LGTM (modulo the comment I left about handling all Abis, and tests ofc).

IMO limiting this to UnsafeCell is more reasonable than #[repr(no_niche)].

The question I ask myself is: why would anyone want #[repr(no_niche)] without the semantics of UnsafeCell and/or MaybeUninit?

I can only think of layout/ABI misuses (that would be defeated by layout randomization), I can't imagine that there's a way to have unsafe code that would only need #[repr(no_niche)] (and nothing else) to both have no UB, and work under layout randomization.

eddyb · 2022-07-07T12:02:13Z

compiler/rustc_middle/src/ty/layout.rs

-
-                            // Update `largest_niche` if we have introduced a larger niche.
-                            let niche = if def.repr().hide_niche() {
-                                None
+                            if def.is_unsafe_cell() {
+                                match scalar {
+                                    Scalar::Initialized { value, valid_range } => {
+                                        *valid_range = WrappingRange::full(value.size(dl))
+                                    }
+                                    // Already doesn't have any niches
+                                    Scalar::Union { .. } => {}
+                                }
+                                st.largest_niche = None;


Looks like the original code got misplaced into the #[rustc_layout_scalar_valid_range] handling, instead of being separate.
I think it should be moved out and made to exhaustively handle all Abis.

More specifically, Abi::{Scalar,ScalarPair,Vector}) all have Scalars in them - and even if ScalarPair appears to be handled, note that you're only operating its first Scalar.

So when testing, make sure to also check UnsafeCell around pairs with niches in both sides of the pair and custom #[repr(simd)] structs with niche'd element types.

custom #[repr(simd)] structs with niche'd element types.

I tried, we have checks preventing such datastructures from existing at all. So this can't happen.

The checks are not perfect: https://godbolt.org/z/W8o3Gxhdo.

davidtwco · 2022-07-08T08:15:10Z

r? @eddyb

oli-obk · 2022-07-08T14:24:58Z

has a nice test set at the top.

I didn't add those as we already had equivalent tests in

rust/src/test/ui/layout/unsafe-cell-hides-niche.rs

Lines 20 to 28 in 2a899dc

    
           assert_eq!(size_of::<Option<Wrapper<u32>>>(),     8); 
        
           assert_eq!(size_of::<Option<Wrapper<N32>>>(),     4); 
        
           assert_eq!(size_of::<Option<Transparent<u32>>>(), 8); 
        
           assert_eq!(size_of::<Option<Transparent<N32>>>(), 4); 
        
           assert_eq!(size_of::<Option<NoNiche<u32>>>(),     8); 
        
           assert_eq!(size_of::<Option<NoNiche<N32>>>(),     8); 
        
           assert_eq!(size_of::<Option<UnsafeCell<u32>>>(),  8); 
        
           assert_eq!(size_of::<Option<UnsafeCell<N32>>>(),  8);

I'll add the libstd type tests for completeness now

…n the layout_scalar_valid_range logic

oli-obk · 2022-07-08T14:49:00Z

@rustbot review

RalfJung · 2022-07-08T17:46:43Z

 I didn't add those as we already had equivalent tests in

That test already passed before this PR. So it cannot be testing the right thing.

src/test/ui/layout/unsafe-cell-hides-niche.rs

compiler/rustc_middle/src/ty/layout.rs

eddyb

r=me after adding Cell versions of several testcases (as per #99011 (comment))

(and maybe fixing the commas, if they're not intentional - if you want a more distinct separator, maybe something like => could work better than ,?)

src/test/ui/layout/unsafe-cell-hides-niche.rs

oli-obk · 2022-07-12T07:29:49Z

@bors r=eddyb

bors · 2022-07-12T07:29:51Z

📌 Commit 7269196 has been approved by eddyb

It is now in the queue for this repository.

`UnsafeCell` blocks niches inside its nested type from being available outside fixes rust-lang#87341 This implements the plan by `@eddyb` in rust-lang#87341 (comment) Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527

`UnsafeCell` blocks niches inside its nested type from being available outside fixes rust-lang#87341 This implements the plan by ``@eddyb`` in rust-lang#87341 (comment) Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527

Dylan-DPC · 2022-07-13T08:38:39Z

failed in rollup

@bors r-

oli-obk · 2022-07-13T10:12:02Z

@bors r=eddyb

bors · 2022-07-13T10:12:04Z

📌 Commit 519c07b has been approved by eddyb

It is now in the queue for this repository.

Rollup of 5 pull requests Successful merges: - rust-lang#98574 (Lower let-else in MIR) - rust-lang#99011 (`UnsafeCell` blocks niches inside its nested type from being available outside) - rust-lang#99030 (diagnostics: error messages when struct literals fail to parse) - rust-lang#99155 (Keep unstable target features for asm feature checking) - rust-lang#99199 (Refactor: remove an unnecessary `span_to_snippet`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

`UnsafeCell` blocks niches inside its nested type from being available outside fixes rust-lang#87341 This implements the plan by `@eddyb` in rust-lang#87341 (comment) Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527

Rollup of 5 pull requests Successful merges: - rust-lang#98574 (Lower let-else in MIR) - rust-lang#99011 (`UnsafeCell` blocks niches inside its nested type from being available outside) - rust-lang#99030 (diagnostics: error messages when struct literals fail to parse) - rust-lang#99155 (Keep unstable target features for asm feature checking) - rust-lang#99199 (Refactor: remove an unnecessary `span_to_snippet`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

UnsafeCell now has no niches, ever.

2a899dc

rust-highfive assigned davidtwco Jul 7, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2022

oli-obk changed the title ~~UnsafeCell now has no niches, ever.~~ UnsafeCell now has no niches, not even transitively Jul 7, 2022

oli-obk changed the title ~~UnsafeCell now has no niches, not even transitively~~ UnsafeCell blocks niches inside its nested type from being available outside Jul 7, 2022

Nilstrieb reviewed Jul 7, 2022

View reviewed changes

compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved

Comment update

8d9f609

RalfJung reviewed Jul 7, 2022

View reviewed changes

library/core/src/cell.rs Show resolved Hide resolved

eddyb requested changes Jul 7, 2022

View reviewed changes

rust-highfive assigned eddyb and unassigned davidtwco Jul 8, 2022

oli-obk added 2 commits July 8, 2022 14:28

Add tests for libstd types

4bfba76

Create a custom layout path for UnsafeCell instead of piggy backing o…

69b1b3c

…n the layout_scalar_valid_range logic

RalfJung reviewed Jul 8, 2022

View reviewed changes

src/test/ui/layout/unsafe-cell-hides-niche.rs Outdated Show resolved Hide resolved

eddyb reviewed Jul 9, 2022

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

eddyb reviewed Jul 9, 2022

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

oli-obk added 5 commits July 11, 2022 10:11

More obvious closure name

4239155

Hide niches in SIMD types, too

984db78

Move checks to compile-time

3f4cf59

Simplify assertion macro

af8536e

Rename assertion macro

fef596f

eddyb requested changes Jul 11, 2022

View reviewed changes

src/test/ui/layout/unsafe-cell-hides-niche.rs Outdated Show resolved Hide resolved

src/test/ui/layout/unsafe-cell-hides-niche.rs Outdated Show resolved Hide resolved

src/test/ui/layout/unsafe-cell-hides-niche.rs Outdated Show resolved Hide resolved

oli-obk added 2 commits July 12, 2022 07:22

Use some more visible sigils than ,

24e8796

Always check Cell alongside with UnsafeCell

7269196

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 12, 2022

Dylan-DPC mentioned this pull request Jul 12, 2022

Rollup of 5 pull requests #99187

Closed

Dylan-DPC mentioned this pull request Jul 13, 2022

Rollup of 6 pull requests #99202

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2022

Limit test to x86 targets for reproducability

519c07b

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 13, 2022

Dylan-DPC mentioned this pull request Jul 13, 2022

Rollup of 5 pull requests #99210

Merged

bors merged commit 1e7d04b into rust-lang:master Jul 13, 2022

rustbot added this to the 1.64.0 milestone Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`UnsafeCell` blocks niches inside its nested type from being available outside #99011

`UnsafeCell` blocks niches inside its nested type from being available outside #99011

oli-obk commented Jul 7, 2022

rustbot commented Jul 7, 2022

rust-highfive commented Jul 7, 2022

RalfJung commented Jul 7, 2022

eddyb left a comment

eddyb Jul 7, 2022

oli-obk Jul 8, 2022

eddyb Jul 9, 2022

davidtwco commented Jul 8, 2022

oli-obk commented Jul 8, 2022

oli-obk commented Jul 8, 2022

RalfJung commented Jul 8, 2022

eddyb left a comment

oli-obk commented Jul 12, 2022

bors commented Jul 12, 2022

Dylan-DPC commented Jul 13, 2022

oli-obk commented Jul 13, 2022

bors commented Jul 13, 2022

UnsafeCell blocks niches inside its nested type from being available outside #99011

UnsafeCell blocks niches inside its nested type from being available outside #99011

Conversation

oli-obk commented Jul 7, 2022

rustbot commented Jul 7, 2022

rust-highfive commented Jul 7, 2022

RalfJung commented Jul 7, 2022

eddyb left a comment

Choose a reason for hiding this comment

eddyb Jul 7, 2022

Choose a reason for hiding this comment

oli-obk Jul 8, 2022

Choose a reason for hiding this comment

eddyb Jul 9, 2022

Choose a reason for hiding this comment

davidtwco commented Jul 8, 2022

oli-obk commented Jul 8, 2022

oli-obk commented Jul 8, 2022

RalfJung commented Jul 8, 2022

eddyb left a comment

Choose a reason for hiding this comment

oli-obk commented Jul 12, 2022

bors commented Jul 12, 2022

Dylan-DPC commented Jul 13, 2022

oli-obk commented Jul 13, 2022

bors commented Jul 13, 2022

`UnsafeCell` blocks niches inside its nested type from being available outside #99011

`UnsafeCell` blocks niches inside its nested type from being available outside #99011