make `mem::Discriminant` invariant by lcnr · Pull Request #71814 · rust-lang/rust

lcnr · 2020-05-02T22:06:31Z

mem::Discriminant is currently covariant over T.
#70705 changes this to invariant, which is a breaking change.

This PR can be used for a crater run to check if more crates than expected depend on the variance of Discriminant, recommended in #70705 (comment)

cc @RalfJung @eddyb

Example breakage:

use std::mem::Discriminant;

pub fn variance<'a>(v: Discriminant<&'static ()>) -> Discriminant<&'a ()> {
    v
}

rust-highfive · 2020-05-02T22:06:36Z

r? @shepmaster

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

RalfJung · 2020-05-02T22:14:18Z

Why is this a "do not merge" experiment? Seems perfectly fine for me to land this first (after crater it happy). Smaller incremental changes are usually better than landing them all at once. :)

@bors try

bors · 2020-05-02T22:14:30Z

⌛ Trying commit 30d9462055452393a73eae5b52dee2cfe131734a with merge d027640e8e31338d002dd0c57ffd1e6d0c16834f...

lcnr · 2020-05-02T22:16:06Z

We still don't want to land this before the crater run, do we 😆

RalfJung · 2020-05-02T22:41:28Z

Sure, but "DO NOT MERGE" is usually for PRs that we do not want to merge period (say, CI experiments). If it just needs a crater run, no need to use all-caps, I'll just set the appropriate label.

RalfJung · 2020-05-02T22:42:50Z

Ah, CI shows that *mut T is not a good choice:

error[E0277]: `*mut mem::test_discriminant_send_sync::Regular` cannot be shared between threads safely
   --> src/libcore/../libcore/tests/mem.rs:129:5
    |
127 |     fn is_send_sync<T: Send + Sync>() {}
    |                               ---- required by this bound in `mem::test_discriminant_send_sync::is_send_sync`
128 | 
129 |     is_send_sync::<Discriminant<Regular>>();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut mem::test_discriminant_send_sync::Regular` cannot be shared between threads safely
    |
    = help: within `std::mem::Discriminant<mem::test_discriminant_send_sync::Regular>`, the trait `std::marker::Sync` is not implemented for `*mut mem::test_discriminant_send_sync::Regular`

Maybe use fn(T) -> T instead? That's invariant, too, and on top of that it's Send+Sync.

rust-highfive · 2020-05-02T23:27:04Z

The job x86_64-gnu-llvm-8 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/be4ed1c0-fd7e-4b08-914d-c1bb79967565.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/71814/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/71814/merge:refs/remotes/pull/71814/merge
---
 ---> cb2676f08729
Step 5/8 : ENV RUST_CONFIGURE_ARGS       --build=x86_64-unknown-linux-gnu       --llvm-root=/usr/lib/llvm-8       --enable-llvm-link-shared       --set rust.thin-lto-import-instr-limit=10
 ---> Using cache
 ---> df25ce111862
Step 6/8 : ENV SCRIPT python2.7 ../x.py test --exclude src/tools/tidy &&            python2.7 ../x.py test src/test/mir-opt --pass=build                                   --target=armv5te-unknown-linux-gnueabi &&            python2.7 ../x.py test src/tools/tidy
 ---> 599b9ac96b27
Step 7/8 : ENV NO_DEBUG_ASSERTIONS=1
 ---> Using cache
 ---> 091087e35a36
---
   Compiling rustc_feature v0.0.0 (/checkout/src/librustc_feature)
   Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_attr v0.0.0 (/checkout/src/librustc_attr)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
   Compiling rustc_feature v0.0.0 (/checkout/src/librustc_feature)
   Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_attr v0.0.0 (/checkout/src/librustc_attr)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
.................i.................................................................................. 1800/9966
.................................................................................................... 1900/9966
.................................i.................................................................. 2000/9966
.................................................................................................... 2100/9966
.......................iiiii........................................................................ 2200/9966
.................................................................................................... 2400/9966
.................................................................................................... 2500/9966
.................................................................................................... 2600/9966
.................................................................................................... 2700/9966
---
.......i...............i............................................................................ 5100/9966
.................................................................................................... 5200/9966
.....................................................i.............................................. 5300/9966
............................................i....................................................... 5400/9966
..............................................ii.ii........i...i.................................... 5500/9966
.............................................................................................i...... 5700/9966
.................................................................................................... 5800/9966
............................ii.....................................i................................ 5900/9966
.................................................................................................... 6000/9966
.................................................................................................... 6000/9966
.................................................................................................... 6100/9966
...............................................................ii..i..ii...........i................ 6200/9966
.................................................................................................... 6400/9966
.................................................................................................... 6500/9966
.................................................................................................... 6500/9966
..............................................................................................i..ii. 6600/9966
.................................................................................................... 6800/9966
...............................................................................................i.... 6900/9966
.................................................................................................... 7000/9966
.................................................................................................... 7100/9966
---
.................................................................................................... 7900/9966
.................................................................................................... 8000/9966
.................................................................................................... 8100/9966
.....i.............................................................................................. 8200/9966
......................................................iiiiii.iiiii.i................................ 8300/9966
.......i............................................................................................ 8500/9966
.................................................................................................... 8600/9966
.................................................................................................... 8700/9966
.................................................................................................... 8800/9966
---
Suite("src/test/codegen") not skipped for "bootstrap::test::Codegen" -- not in ["src/tools/tidy"]
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 186 tests
iiii......i.............ii.i..........i.............................i..i..................i....i.... 100/186
........i.i.i...iii..iiiiiii.iiiiiiiii......................iii...............ii......

 finished in 5.991
Suite("src/test/codegen-units") not skipped for "bootstrap::test::CodegenUnits" -- not in ["src/tools/tidy"]
Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 9 tests
iiiiiiiii

 finished in 0.163
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 115 tests
iiiii..i.....i..i...i..i.i.i..i..i..ii....i.i....ii..........iiii.........i.....i..i.......ii.i.ii.. 100/115
...iiii.....ii.

 finished in 15.833
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
 finished in 112.599
Set({"src/libcore"}) not skipped for "bootstrap::test::Crate" -- not in ["src/tools/tidy"]
Testing core stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
   Compiling core v0.0.0 (/checkout/src/libcore)
error[E0277]: `*mut mem::test_discriminant_send_sync::Regular` cannot be sent between threads safely
   --> src/libcore/../libcore/tests/mem.rs:129:5
    |
127 |     fn is_send_sync<T: Send + Sync>() {}
128 | 
128 | 
129 |     is_send_sync::<Discriminant<Regular>>();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut mem::test_discriminant_send_sync::Regular` cannot be sent between threads safely
    |
    = help: within `std::mem::Discriminant<mem::test_discriminant_send_sync::Regular>`, the trait `std::marker::Send` is not implemented for `*mut mem::test_discriminant_send_sync::Regular`
    = note: required because it appears within the type `std::marker::PhantomData<*mut mem::test_discriminant_send_sync::Regular>`
    = note: required because it appears within the type `std::mem::Discriminant<mem::test_discriminant_send_sync::Regular>`

error[E0277]: `*mut mem::test_discriminant_send_sync::Regular` cannot be shared between threads safely
   --> src/libcore/../libcore/tests/mem.rs:129:5
    |
127 |     fn is_send_sync<T: Send + Sync>() {}
128 | 
128 | 
129 |     is_send_sync::<Discriminant<Regular>>();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut mem::test_discriminant_send_sync::Regular` cannot be shared between threads safely
    |
    = help: within `std::mem::Discriminant<mem::test_discriminant_send_sync::Regular>`, the trait `std::marker::Sync` is not implemented for `*mut mem::test_discriminant_send_sync::Regular`
    = note: required because it appears within the type `std::marker::PhantomData<*mut mem::test_discriminant_send_sync::Regular>`
    = note: required because it appears within the type `std::mem::Discriminant<mem::test_discriminant_send_sync::Regular>`

error[E0277]: `*mut mem::test_discriminant_send_sync::NotSendSync` cannot be sent between threads safely
   --> src/libcore/../libcore/tests/mem.rs:130:5
    |
127 |     fn is_send_sync<T: Send + Sync>() {}
...
...
130 |     is_send_sync::<Discriminant<NotSendSync>>();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut mem::test_discriminant_send_sync::NotSendSync` cannot be sent between threads safely
    |
    = help: within `std::mem::Discriminant<mem::test_discriminant_send_sync::NotSendSync>`, the trait `std::marker::Send` is not implemented for `*mut mem::test_discriminant_send_sync::NotSendSync`
    = note: required because it appears within the type `std::marker::PhantomData<*mut mem::test_discriminant_send_sync::NotSendSync>`
    = note: required because it appears within the type `std::mem::Discriminant<mem::test_discriminant_send_sync::NotSendSync>`

error[E0277]: `*mut mem::test_discriminant_send_sync::NotSendSync` cannot be shared between threads safely
   --> src/libcore/../libcore/tests/mem.rs:130:5
    |
127 |     fn is_send_sync<T: Send + Sync>() {}
...
...
130 |     is_send_sync::<Discriminant<NotSendSync>>();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut mem::test_discriminant_send_sync::NotSendSync` cannot be shared between threads safely
    |
    = help: within `std::mem::Discriminant<mem::test_discriminant_send_sync::NotSendSync>`, the trait `std::marker::Sync` is not implemented for `*mut mem::test_discriminant_send_sync::NotSendSync`
    = note: required because it appears within the type `std::marker::PhantomData<*mut mem::test_discriminant_send_sync::NotSendSync>`
    = note: required because it appears within the type `std::mem::Discriminant<mem::test_discriminant_send_sync::NotSendSync>`
error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0277`.
error: could not compile `core`.
---
  local time: Sat May  2 23:26:59 UTC 2020
  network time: Sat, 02 May 2020 23:26:59 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/71814/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/71814/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3642) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

bors · 2020-05-03T00:29:13Z

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

lcnr · 2020-05-03T07:08:33Z

Changed it to fn(T) -> T and added a short comment

RalfJung · 2020-05-03T09:19:05Z

@bors try

bors · 2020-05-03T09:19:17Z

⌛ Trying commit 57c4db3 with merge 5fae8f15a4dcfce640ee800a4d495673524b0a4d...

bors · 2020-05-03T11:54:12Z

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

RalfJung · 2020-05-03T11:58:52Z

@craterbot check

craterbot · 2020-05-03T11:58:55Z

👌 Experiment pr-71814 created and queued.
🤖 Automatically detected try build 5fae8f15a4dcfce640ee800a4d495673524b0a4d
🔍 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

shepmaster · 2020-05-03T14:51:58Z

Seems perfectly fine for me to land this

and

which is a breaking change

Seem to be contradictory points?

RalfJung · 2020-05-03T15:21:04Z

Sorry, I meant after a crater run and with team approval.

It seems very unlikely that anyone relies on this being covariant.

craterbot · 2020-05-13T07:32:51Z

🚧 Experiment pr-71814 is now running

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

craterbot · 2020-05-16T06:20:38Z

🎉 Experiment pr-71814 is completed!
📊 3 regressed and 2 fixed (102560 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-05-16T07:18:33Z

As far as I can tell we have no actual regressions here 🎉

#70705 is already ready for merge, so it's probably better to just close this and merge #70705 directly.

rust-highfive assigned shepmaster May 2, 2020

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

lcnr mentioned this pull request May 2, 2020

Use T's discriminant type in mem::Discriminant<T> instead of u64. #70705

Merged

RalfJung added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 2, 2020

RalfJung reviewed May 2, 2020

View reviewed changes

Comment thread src/libcore/mem/mod.rs Outdated

lcnr changed the title ~~[DO NOT MERGE] make mem::Discriminant invariant~~ make mem::Discriminant invariant May 3, 2020

make mem::Discriminant invariant

57c4db3

lcnr force-pushed the discriminantus_invariantus branch from 30d9462 to 57c4db3 Compare May 3, 2020 07:07

craterbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2020

craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 16, 2020

craterbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2020

lcnr closed this May 21, 2020

lcnr deleted the discriminantus_invariantus branch May 21, 2020 08:04

Aaron1011 mentioned this pull request Jun 2, 2020

Add an attribute for asserting variance of generic parameters #72926

Open

Uh oh!

Uh oh!

Conversation

lcnr commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 2, 2020

Uh oh!

RalfJung commented May 2, 2020

Uh oh!

bors commented May 2, 2020

Uh oh!

lcnr commented May 2, 2020

Uh oh!

RalfJung commented May 2, 2020

Uh oh!

Uh oh!

RalfJung commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 2, 2020

Uh oh!

bors commented May 3, 2020

Uh oh!

lcnr commented May 3, 2020

Uh oh!

RalfJung commented May 3, 2020

Uh oh!

bors commented May 3, 2020

Uh oh!

bors commented May 3, 2020

Uh oh!

RalfJung commented May 3, 2020

Uh oh!

craterbot commented May 3, 2020

Uh oh!

shepmaster commented May 3, 2020

Uh oh!

RalfJung commented May 3, 2020

Uh oh!

craterbot commented May 13, 2020

Uh oh!

craterbot commented May 16, 2020

Uh oh!

lcnr commented May 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lcnr commented May 2, 2020 •

edited

Loading

RalfJung commented May 2, 2020 •

edited

Loading