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

Ban multi-trait objects via trait aliases #59445

Merged
merged 12 commits into from
May 22, 2019

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Mar 26, 2019

Obviously, multi-trait objects are not normally supported, so they should not be supported via trait aliases.

This has been factored out from the previous PR #55994 (see point 1).

r? @Centril

CC @nikomatsakis


RELNOTES:

We now allow dyn Send + fmt::Debug with equivalent semantics to dyn fmt::Debug + Send.
That is, the order of the mentioned traits does not matter wrt. principal/not-principal traits.
This is a small change that might deserve a mention in the blog post because it is a language change but most likely not.

See https://github.com/rust-lang/rust/blob/ce2ee305f9165c037ecddddb5792588a15ff6c37/src/test/ui/traits/wf-trait-object-reverse-order.rs.

// @Centril

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2019
@rust-highfive

This comment has been minimized.

@crlf0710

This comment has been minimized.

@alexreg

This comment has been minimized.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from ce751e4 to e728b8d Compare March 27, 2019 01:49
@crlf0710

This comment has been minimized.

@alexreg

This comment has been minimized.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks mostly good aside from comments below... The diagnostics look great.

However, I think some more tests are needed. In particular, it would be nice to exercise the recursive behavior here including things like dyn A + B where trait A = C + D;, trait B = E + F, trait F = G + 'static; and various other combinations including auto traits, region constraints, where clauses on the trait alias, and so on.

src/librustc/traits/mod.rs Outdated Show resolved Hide resolved
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Show resolved Hide resolved
@Centril Centril 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 Mar 27, 2019
@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from 47d6cd3 to c0ef111 Compare March 27, 2019 15:36
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@@ -1175,14 +1198,13 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
} else {
self.re_infer(span, None).unwrap_or_else(|| {
span_err!(tcx.sess, span, E0228,
"the lifetime bound for this object type cannot be deduced \
from context; please supply an explicit bound");
"the lifetime bound for this object type cannot be deduced \
Copy link
Contributor

@Centril Centril Mar 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"the lifetime bound for this object type cannot be deduced \
"the lifetime bound for this object type cannot be inferred \

while reading this PR again, I noticed this... we use "infer" elsewhere and so we should prefer that everywhere instead of C++isms like "deduce".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be a separate PR I think. You're welcome to make it though, and I can review. ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexreg Sure; leaving it here (unresolved) for visibility and as a reminder ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Centril With this PR merging now, just a note for you that this is unresolved, in case you want to create an issue or PR for it yourself.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from a54c090 to 50871cb Compare March 31, 2019 16:03
@rust-highfive

This comment has been minimized.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from 50871cb to d2993e9 Compare March 31, 2019 16:42
@rust-highfive

This comment has been minimized.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from d2993e9 to 0f94b88 Compare April 1, 2019 02:17
@Centril Centril 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 1, 2019
@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from 0f94b88 to 2590f72 Compare April 2, 2019 18:38
@rust-highfive

This comment has been minimized.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from 2590f72 to d408a1f Compare April 3, 2019 02:19
@rust-highfive

This comment has been minimized.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from d408a1f to a42c23b Compare April 3, 2019 16:42
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@alexreg alexreg force-pushed the ban-multi-trait-objects-via-aliases branch from a42c23b to 560a8de Compare April 29, 2019 03:06
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 21, 2019
@oli-obk
Copy link
Contributor

oli-obk commented May 21, 2019

is the beta cut already successfull even without #60999 having been merged?

@alexreg
Copy link
Contributor Author

alexreg commented May 21, 2019

Not 100% sure, but @Centril and I thought that by the time this PR works its way up the queue now, the toolstate problem should have disappeared at least, so it should be no problem.

@bors
Copy link
Contributor

bors commented May 22, 2019

⌛ Testing commit ce2ee30 with merge de414067da0f3f6c80530b9ba2a9f98e90c2f484...

@bors
Copy link
Contributor

bors commented May 22, 2019

💔 Test failed - checks-travis

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-distcheck of your PR failed on Travis (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.
[00:03:51]    Compiling textwrap v0.11.0
[00:03:53] error: failed to run custom build command for `rand_chacha v0.1.1`
[00:03:53] process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/debug/build/rand_chacha-264db25efa48e2e1/build-script-build` (exit code: 101)
[00:03:53] --- stdout
[00:03:53] cargo:rerun-if-changed=build.rs
[00:03:53] --- stderr
[00:03:53] thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
[00:03:53] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:03:53] thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
[00:03:53] thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
[00:03:53] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:03:53] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: Other("could not probe for `std`") }', src/libcore/result.rs:997:5
[00:03:53] 
[00:03:53] warning: build failed, waiting for other jobs to finish...
[00:03:53] error: failed to run custom build command for `rand_pcg v0.1.2`
[00:03:53] process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/debug/build/rand_pcg-77510ee48e3cc776/build-script-build` (exit code: 101)
[00:03:53] process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/debug/build/rand_pcg-77510ee48e3cc776/build-script-build` (exit code: 101)
[00:03:53] --- stdout
[00:03:53] cargo:rerun-if-changed=build.rs
[00:03:53] --- stderr
[00:03:53] thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
[00:03:53] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:03:53] thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
[00:03:53] thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
[00:03:53] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:03:53] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: Other("could not probe for `std`") }', src/libcore/result.rs:997:5
[00:03:53] 
[00:03:53] warning: build failed, waiting for other jobs to finish...
[00:03:53] warning: build failed, waiting for other jobs to finish...
[00:03:56] error: failed to compile `cargo-vendor v0.1.22`, intermediate artifacts can be found at `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools`
[00:03:56] Caused by:
[00:03:56]   build failed
[00:03:56] 
[00:03:56] 
[00:03:56] 
[00:03:56] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "install" "-j" "4" "--locked" "--color" "always" "--force" "--debug" "--vers" "0.1.22" "cargo-vendor"
[00:03:56] 
[00:03:56] 
[00:03:56] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test distcheck
[00:03:56] Build completed unsuccessfully in 0:00:34
---
travis_time:end:2065bca2:start=1558486191740556448,finish=1558486191752078901,duration=11522453
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:07ad57f0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1213ecc0
travis_time:start:1213ecc0
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0298c0e0
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@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 May 22, 2019
@mati865
Copy link
Contributor

mati865 commented May 22, 2019

#61027 already broke Clippy so for sure tools don't block it anymore.

@Centril Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label May 22, 2019
@Centril Centril added this to the 1.37 milestone May 22, 2019
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 22, 2019
@Centril
Copy link
Contributor

Centril commented May 22, 2019

@bors retry

@bors 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 May 22, 2019
@bors
Copy link
Contributor

bors commented May 22, 2019

⌛ Testing commit ce2ee30 with merge 37ff5d3...

bors added a commit that referenced this pull request May 22, 2019
…=oli-obk

Ban multi-trait objects via trait aliases

Obviously, multi-trait objects are not normally supported, so they should not be supported via trait aliases.

This has been factored out from the previous PR #55994 (see point 1).

r? @Centril

CC @nikomatsakis

------------------

### RELNOTES:

We now allow `dyn Send + fmt::Debug` with equivalent semantics to `dyn fmt::Debug + Send`.
That is, the order of the mentioned traits does not matter wrt. principal/not-principal traits.
This is a small change that might deserve a mention in the blog post because it is a language change but most likely not.

See https://github.com/rust-lang/rust/blob/ce2ee305f9165c037ecddddb5792588a15ff6c37/src/test/ui/traits/wf-trait-object-reverse-order.rs.

// @Centril
@bors
Copy link
Contributor

bors commented May 22, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 37ff5d3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 22, 2019
@bors bors merged commit ce2ee30 into rust-lang:master May 22, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jul 5, 2019
Pkgsrc changes:
 * NetBSD/sparc64 disabling of "packed" removed ("packed" removed upstream)
 * Adapt src_libstd_build.rs patch, update sed'ing of .cargo-checksum.json

Build verified on NetBSD 8.0/amd64.

Upstream changes:

Version 1.36.0 (2019-07-04)
==========================

Language
--------
- [Non-Lexical Lifetimes are now enabled on the 2015 edition.][59114]
- [The order of traits in trait objects no longer affects the semantics of that
  object.][59445] e.g. `dyn Send + fmt::Debug` is now equivalent to
  `dyn fmt::Debug + Send`, where this was previously not the case.

Libraries
---------
- [`HashMap`'s implementation has been replaced with `hashbrown::HashMap` implem
entation.][58623]
- [`TryFromSliceError` now implements `From<Infallible>`.][60318]
- [`mem::needs_drop` is now available as a const fn.][60364]
- [`alloc::Layout::from_size_align_unchecked` is now available as a const fn.][6
0370]
- [`String` now implements `BorrowMut<str>`.][60404]
- [`io::Cursor` now implements `Default`.][60234]
- [Both `NonNull::{dangling, cast}` are now const fns.][60244]
- [The `alloc` crate is now stable.][59675] `alloc` allows you to use a subset
  of `std` (e.g. `Vec`, `Box`, `Arc`) in `#![no_std]` environments if the
  environment has access to heap memory allocation.
- [`String` now implements `From<&String>`.][59825]
- [You can now pass multiple arguments to the `dbg!` macro.][59826] `dbg!` will
  return a tuple of each argument when there is multiple arguments.
- [`Result::{is_err, is_ok}` are now `#[must_use]` and will produce a warning if
  not used.][59648]

Stabilized APIs
---------------
- [`VecDeque::rotate_left`]
- [`VecDeque::rotate_right`]
- [`Iterator::copied`]
- [`io::IoSlice`]
- [`io::IoSliceMut`]
- [`Read::read_vectored`]
- [`Write::write_vectored`]
- [`str::as_mut_ptr`]
- [`mem::MaybeUninit`]
- [`pointer::align_offset`]
- [`future::Future`]
- [`task::Context`]
- [`task::RawWaker`]
- [`task::RawWakerVTable`]
- [`task::Waker`]
- [`task::Poll`]

Cargo
-----
- [Cargo will now produce an error if you attempt to use the name of a required
dependency as a feature.][cargo/6860]
- [You can now pass the `--offline` flag to run cargo without accessing the netw
ork.][cargo/6934]

You can find further change's in [Cargo's 1.36.0 release notes][cargo-1-36-0].

Clippy
------
There have been numerous additions and fixes to clippy, see [Clippy's 1.36.0 rel
ease notes][clippy-1-36-0] for more details.

Misc
----

Compatibility Notes
-------------------
- [`std::arch::x86::_rdtsc` returns `u64` instead of `i64`][stdsimd/559]
- [`std::arch::x86_64::_mm_shuffle_ps` takes an `i32` instead of `u32` for `mask
`][stdsimd/522]
- With the stabilisation of `mem::MaybeUninit`, `mem::uninitialized` use is no
  longer recommended, and will be deprecated in 1.38.0.

[60318]: rust-lang/rust#60318
[60364]: rust-lang/rust#60364
[60370]: rust-lang/rust#60370
[60404]: rust-lang/rust#60404
[60234]: rust-lang/rust#60234
[60244]: rust-lang/rust#60244
[58623]: rust-lang/rust#58623
[59648]: rust-lang/rust#59648
[59675]: rust-lang/rust#59675
[59825]: rust-lang/rust#59825
[59826]: rust-lang/rust#59826
[59445]: rust-lang/rust#59445
[59114]: rust-lang/rust#59114
[cargo/6860]: rust-lang/cargo#6860
[cargo/6934]: rust-lang/cargo#6934
[`VecDeque::rotate_left`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_left
[`VecDeque::rotate_right`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_right
[`Iterator::copied`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#tymethod.copied
[`io::IoSlice`]: https://doc.rust-lang.org/std/io/struct.IoSlice.html
[`io::IoSliceMut`]: https://doc.rust-lang.org/std/io/struct.IoSliceMut.html
[`Read::read_vectored`]: https://doc.rust-lang.org/std/io/trait.Read.html#method.read_vectored
[`Write::write_vectored`]: https://doc.rust-lang.org/std/io/trait.Write.html#method.write_vectored
[`str::as_mut_ptr`]: https://doc.rust-lang.org/std/primitive.str.html#method.as_mut_ptr
[`mem::MaybeUninit`]: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html
[`pointer::align_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset
[`future::Future`]: https://doc.rust-lang.org/std/future/trait.Future.html
[`task::Context`]: https://doc.rust-lang.org/beta/std/task/struct.Context.html
[`task::RawWaker`]: https://doc.rust-lang.org/beta/std/task/struct.RawWaker.html
[`task::RawWakerVTable`]: https://doc.rust-lang.org/beta/std/task/struct.RawWakerVTable.html
[`task::Waker`]: https://doc.rust-lang.org/beta/std/task/struct.Waker.html
[`task::Poll`]: https://doc.rust-lang.org/beta/std/task/enum.Poll.html
[clippy-1-36-0]: https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-136
[cargo-1-36-0]: https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-136-2019-07-04
[stdsimd/522]: rust-lang/stdarch#522
[stdsimd/559]: rust-lang/stdarch#559
@alexreg alexreg added A-traits Area: Trait system F-trait_alias `#![feature(trait_alias)]` labels Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system F-trait_alias `#![feature(trait_alias)]` merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants