Skip to content

Dedup auto traits in trait objects. #51276

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

Merged
merged 3 commits into from
Jun 9, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use util::common::ErrorReported;
use util::nodemap::{FxHashSet, FxHashMap};
use errors::FatalError;

use std::cmp::Ordering;
use std::iter;
use syntax::ast;
use syntax::feature_gate::{GateIssue, emit_feature_err};
Expand Down Expand Up @@ -706,10 +707,16 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {
.emit();
}

// Dedup auto traits so that `dyn Trait + Send + Send` is the same as `dyn Trait + Send`.
let mut auto_traits =
auto_traits.into_iter().map(ty::ExistentialPredicate::AutoTrait).collect::<Vec<_>>();
auto_traits.sort_by(|a, b| a.cmp(tcx, b));
auto_traits.dedup_by(|a, b| (&*a).cmp(tcx, b) == Ordering::Equal);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The (&*a) is necessary here because std::cmp::Ord::cmp shadows ty::ExistentialPredicate::cmp for an &mut ty::ExistentialPredicate, but not necessary in the sort_by on the previous line because the shadowing is reversed for &ty::ExistentialPreciate (and the unborrowed version).

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's possible for you to just collect into an FxHashSet here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cmp used by a HashSet doesn't compare using the TyCtx, so would fail if the DefIds for the auto trait are different, which I think can happen through type or use aliases. Also, in the vast majority of cases, there's going to be less than three auto traits (and many times zero), so I don't think it would be much of an optimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

First off, I would recommend invoking ty::ExistentialPredicate::cmp — but maybe we should rename that method, if it is indeed not equivalent to Ord::cmp?

Copy link
Contributor

Choose a reason for hiding this comment

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

Put another way, why can't we invoke .dedup() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I thought that the DefIds might be different based on which use clause you use, but switching to just .dedup() works here. But then, why is there both ty::ExistentialPredicate::cmp and Ord::cmp if they give the same results? Are there actually cases where two DefIds could point to the same trait?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, that function (a) has a bad name and (b) needs a comment. I think the distinction is this:

  • ExistentialPredicate::cmp sorts via a stable ordering that will not change if modules are ordered or other changes are made to the tree. In particular, this ordering is preserved across incremental compilations.
  • Ord::Cmp sorts in an order that is not stable across compilations.

For purposes of equality, they are equivalent.

It should really be called ExistentialPredicate::stable_cmp -- would you be game to try and rename it? (And add a comment.)

That could be either part of this PR or outside of it...

...given the unfortunate shadowing, the way I would go about it would be to add stable_cmp and mark the existing method as deprecated (and maybe make it panic! for good measure). Then a ./x.py test should shake out all the callers. =)

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, I think we can also (in this PR) just call dedup. We probably don't need a stable ordering, either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, we don't need to put the auto traits into the ExistentialPredicate before sorting and deduping, so can just sort and dedup the returned auto traits. Which makes this change ultimately more readable than the whole wrapping and then comparing.

--

I'll tackle the refactor in a separate PR, since it has nothing to do with this one. Due to the argument count difference, anybody calling the inherent method will get an argument count error at compile time, so I don't think the old name needs to be deprecated.


// skip_binder is okay, because the predicates are re-bound.
let mut v =
iter::once(ty::ExistentialPredicate::Trait(*existential_principal.skip_binder()))
.chain(auto_traits.into_iter().map(ty::ExistentialPredicate::AutoTrait))
.chain(auto_traits.into_iter())
.chain(existential_projections
.map(|x| ty::ExistentialPredicate::Projection(*x.skip_binder())))
.collect::<AccumulateVec<[_; 8]>>();
Expand Down Expand Up @@ -1318,7 +1325,7 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {
}
}

/// Divides a list of general trait bounds into two groups: builtin bounds (Sync/Send) and the
/// Divides a list of general trait bounds into two groups: auto traits (e.g. Sync and Send) and the
/// remaining general trait bounds.
fn split_auto_traits<'a, 'b, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
trait_bounds: &'b [hir::PolyTraitRef])
Expand Down
53 changes: 53 additions & 0 deletions src/test/run-pass/trait-object-auto-dedup.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Test that duplicate auto trait bounds in trait objects don't create new types.
#[allow(unused_assignments)]

use std::marker::Send as SendAlias;

// A dummy trait for the non-auto trait.
trait Trait {}

// A dummy struct to implement Trait, Send, and .
struct Struct;

impl Trait for Struct {}

// These three functions should be equivalent.
fn takes_dyn_trait_send(_: Box<dyn Trait + Send>) {}
fn takes_dyn_trait_send_send(_: Box<dyn Trait + Send + Send>) {}
fn takes_dyn_trait_send_sendalias(_: Box<dyn Trait + Send + SendAlias>) {}

impl dyn Trait + Send + Send {
fn do_nothing(&self) {}
}

fn main() {
// 1. Moving into a variable with more Sends and back.
let mut dyn_trait_send = Box::new(Struct) as Box<dyn Trait + Send>;
let dyn_trait_send_send: Box<dyn Trait + Send + Send> = dyn_trait_send;
dyn_trait_send = dyn_trait_send_send;

// 2. Calling methods with different number of Sends.
let dyn_trait_send = Box::new(Struct) as Box<dyn Trait + Send>;
takes_dyn_trait_send_send(dyn_trait_send);

let dyn_trait_send_send = Box::new(Struct) as Box<dyn Trait + Send + Send>;
takes_dyn_trait_send(dyn_trait_send_send);

// 3. Aliases to the trait are transparent.
let dyn_trait_send = Box::new(Struct) as Box<dyn Trait + Send>;
takes_dyn_trait_send_sendalias(dyn_trait_send);

// 4. Calling an impl that duplicates an auto trait.
let dyn_trait_send = Box::new(Struct) as Box<dyn Trait + Send>;
dyn_trait_send.do_nothing();
}
29 changes: 29 additions & 0 deletions src/test/ui/trait-object-auto-dedup-in-impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Checks to make sure that `dyn Trait + Send` and `dyn Trait + Send + Send` are the same type.
// Issue: #47010

struct Struct;
impl Trait for Struct {}
trait Trait {}

type Send1 = Trait + Send;
type Send2 = Trait + Send + Send;

fn main () {}

impl Trait + Send {
fn test(&self) { println!("one"); } //~ ERROR duplicate definitions with name `test`
}

impl Trait + Send + Send {
fn test(&self) { println!("two"); }
}
12 changes: 12 additions & 0 deletions src/test/ui/trait-object-auto-dedup-in-impl.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0592]: duplicate definitions with name `test`
--> $DIR/trait-object-auto-dedup-in-impl.rs:24:5
|
LL | fn test(&self) { println!("one"); } //~ ERROR duplicate definitions with name `test`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definitions for `test`
...
LL | fn test(&self) { println!("two"); }
| ----------------------------------- other definition for `test`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0592`.