Refactor TraitObject to Slice<ExistentialPredicate> #37965

Mark-Simulacrum · 2016-11-23T18:34:05Z

For reference, the primary types changes in this PR are shown below. They may add in the understanding of what is discussed below, though they should not be required.

We change TraitObject into a list of ExistentialPredicates to allow for a couple of things:

Principal (ExistentialPredicate::Trait) is now optional.
Region bounds are moved out of TraitObject into TyDynamic. This permits wrapping only the ExistentialPredicate list in Binder.
BuiltinBounds and BuiltinBound are removed entirely from the codebase, to permit future non-constrained auto traits. These are replaced with ExistentialPredicate::AutoTrait, which only requires a DefId. For the time being, only Send and Sync are supported; this constraint can be lifted in a future pull request.
Binder-related logic is extracted from ExistentialPredicate into the parent (Binder<Slice<EP>>), so PolyXs are inside TraitObject are replaced with X.

The code requires a sorting order for ExistentialPredicates in the interned Slice. The sort order is asserted to be correct during interning, but the slices are not sorted at that point.

ExistentialPredicate::Trait are defined as always equal; This may be wrong; should we be comparing them and sorting them in some way?
ExistentialPredicate::Projection: Compared by ExistentialProjection::sort_key.
ExistentialPredicate::AutoTrait: Compared by TraitDef.def_path_hash.

Construction of ExistentialPredicates is conducted through TyCtxt::mk_existential_predicates, which interns a passed iterator as a Slice. There are no convenience functions to construct from a set of separate iterators; callers must pass an iterator chain. The lack of convenience functions is primarily due to few uses and the relative difficulty in defining a nice API due to optional parts and difficulty in recognizing which argument goes where. It is also true that the current situation isn't significantly better than 4 arguments to a constructor function; but the extra work is deemed unnecessary as of this time.

// before this PR
struct TraitObject<'tcx> {
    pub principal: PolyExistentialTraitRef<'tcx>,
    pub region_bound: &'tcx ty::Region,
    pub builtin_bounds: BuiltinBounds,
    pub projection_bounds: Vec<PolyExistentialProjection<'tcx>>,
}

// after
pub enum ExistentialPredicate<'tcx> {
    // e.g. Iterator
    Trait(ExistentialTraitRef<'tcx>),
    // e.g. Iterator::Item = T
    Projection(ExistentialProjection<'tcx>),
    // e.g. Send
    AutoTrait(DefId),
}

eddyb

LGTM modulo comments. cc @nikomatsakis about Binder discipline.

eddyb · 2016-11-23T18:59:20Z

src/librustc/traits/coherence.rs

You shouldn't need ref in positions by this, the match is on an rvalue anyway.

eddyb · 2016-11-23T19:00:43Z

src/librustc/traits/coherence.rs

Same extraneous ref. It might be worth using map_or in cases like this, I'm not sure.

eddyb · 2016-11-23T19:03:15Z

src/librustc/traits/select.rs

This should be an if-else chain.

eddyb · 2016-11-23T19:04:09Z

src/librustc/traits/select.rs

Same extraneous ref.

eddyb · 2016-11-23T19:04:16Z

src/librustc/traits/select.rs

Same extraneous ref.

eddyb · 2016-11-23T20:06:05Z

src/librustc_typeck/check/regionck.rs

Same odd reg.

eddyb · 2016-11-23T20:21:58Z

src/librustc_typeck/coherence/overlap.rs

This can be written using map_or.

eddyb · 2016-11-23T20:22:59Z

src/librustc_typeck/collect.rs

I'm not sure this partitioning should even exist until a TyDynamic is created.

eddyb · 2016-11-23T20:23:31Z

src/librustc_typeck/variance/constraints.rs

This can be wrapped in an if let Some instead of using .unwrap().

eddyb · 2016-11-23T20:26:12Z

src/librustc/ty/sty.rs

This shouldn't .unwrap().

bors · 2016-11-24T09:40:52Z

☔ The latest upstream changes (presumably #37890) made this pull request unmergeable. Please resolve the merge conflicts.

Mark-Simulacrum · 2016-11-25T22:31:06Z

@eddyb Ready for the crater run, I think, but might want to review before?

Mark-Simulacrum · 2016-11-26T04:32:49Z

src/librustc_trans/trans_item.rs

Shouldn't allocate.

Mark-Simulacrum · 2016-11-26T04:36:37Z

src/librustc_typeck/check/wfcheck.rs

Replace with require_lang_item.

Mark-Simulacrum · 2016-11-26T04:39:06Z

src/librustc_typeck/variance/constraints.rs

Mark-Simulacrum · 2016-11-26T13:19:11Z

Expanded the description with discussion and elaboration on what this PR does.

@eddyb Let me know what parts of that should be transferred into documentation comments in the code.

Mark-Simulacrum · 2016-11-27T17:07:33Z

Squashed the remaining fix commits. r? @eddyb

eddyb · 2016-11-27T22:01:07Z

Started crater run.

eddyb · 2016-11-28T00:33:38Z

src/librustc/infer/mod.rs

copy_trait or copy_def_id.

eddyb · 2016-11-28T00:34:34Z

src/librustc/traits/coherence.rs

This (and other places) could use map_or for brevity.

eddyb · 2016-11-28T00:36:01Z

src/librustc/traits/select.rs

Comment is unnecessary/misleading.

eddyb · 2016-11-28T00:39:31Z

src/librustc/traits/select.rs

Not necessary in this PR, but these push calls could be replaced with collecting an iterator to a Vec.

eddyb · 2016-11-28T00:40:36Z

src/librustc/ty/context.rs

Maybe rename this to mk_dynamic?

eddyb · 2016-11-28T01:33:49Z

src/librustc_typeck/astconv.rs

Use if let Some here.

eddyb · 2016-11-28T01:36:19Z

src/librustc_typeck/check/wfcheck.rs

Is this check necessary anymore? AFAIK trait_has_default_impl returns true for Send and Sync.

eddyb · 2016-11-28T01:37:32Z

src/librustc_typeck/collect.rs

No need to .clone().

eddyb · 2016-11-28T01:37:41Z

src/librustc_typeck/collect.rs

Unnecessary ref.

eddyb · 2016-11-28T01:38:28Z

src/librustc_typeck/diagnostics.rs

s/type/trait

eddyb · 2016-11-28T01:59:18Z

Crater report contains no legitimate regressions (ignoring uses of TyTrait in lints).

bors · 2016-11-28T13:06:06Z

☔ The latest upstream changes (presumably #37676) made this pull request unmergeable. Please resolve the merge conflicts.

…Option.

Mark-Simulacrum · 2016-11-28T14:11:44Z

@eddyb Rebased, but haven't run tests--started those locally (and Travis will test too, of course).

eddyb · 2016-11-28T19:44:12Z

src/librustc/ty/walk.rs

My suggestion was to move the .types().rev() here.

eddyb · 2016-11-28T19:44:53Z

src/librustc/traits/mod.rs

There's a few indentation aberrations, here and in a few other places. Let me know if you want a list.

eddyb · 2016-11-28T19:50:54Z

src/librustc/traits/select.rs

Hmm, to make this nicer I think you should call .skip_binder on data_a and data_b (with a comment that it's reintroduced below).

eddyb · 2016-11-28T19:52:12Z

src/librustc_typeck/astconv.rs

At least use .skip_binder() instead of .0.

Renames TyTrait to TyDynamic.

Replaces instances of tcx.lang_items.require(..) with fatal unwrap with this method.

Mark-Simulacrum · 2016-11-29T02:28:44Z

Fixed review comments.

r? @eddyb

eddyb · 2016-11-29T04:59:37Z

@bors r+

bors · 2016-11-29T04:59:39Z

📌 Commit 8b82fd7 has been approved by eddyb

…=eddyb Refactor TraitObject to Slice<ExistentialPredicate> For reference, the primary types changes in this PR are shown below. They may add in the understanding of what is discussed below, though they should not be required. We change `TraitObject` into a list of `ExistentialPredicate`s to allow for a couple of things: - Principal (ExistentialPredicate::Trait) is now optional. - Region bounds are moved out of `TraitObject` into `TyDynamic`. This permits wrapping only the `ExistentialPredicate` list in `Binder`. - `BuiltinBounds` and `BuiltinBound` are removed entirely from the codebase, to permit future non-constrained auto traits. These are replaced with `ExistentialPredicate::AutoTrait`, which only requires a `DefId`. For the time being, only `Send` and `Sync` are supported; this constraint can be lifted in a future pull request. - Binder-related logic is extracted from `ExistentialPredicate` into the parent (`Binder<Slice<EP>>`), so `PolyX`s are inside `TraitObject` are replaced with `X`. The code requires a sorting order for `ExistentialPredicate`s in the interned `Slice`. The sort order is asserted to be correct during interning, but the slices are not sorted at that point. 1. `ExistentialPredicate::Trait` are defined as always equal; **This may be wrong; should we be comparing them and sorting them in some way?** 1. `ExistentialPredicate::Projection`: Compared by `ExistentialProjection::sort_key`. 1. `ExistentialPredicate::AutoTrait`: Compared by `TraitDef.def_path_hash`. Construction of `ExistentialPredicate`s is conducted through `TyCtxt::mk_existential_predicates`, which interns a passed iterator as a `Slice`. There are no convenience functions to construct from a set of separate iterators; callers must pass an iterator chain. The lack of convenience functions is primarily due to few uses and the relative difficulty in defining a nice API due to optional parts and difficulty in recognizing which argument goes where. It is also true that the current situation isn't significantly better than 4 arguments to a constructor function; but the extra work is deemed unnecessary as of this time. ```rust // before this PR struct TraitObject<'tcx> { pub principal: PolyExistentialTraitRef<'tcx>, pub region_bound: &'tcx ty::Region, pub builtin_bounds: BuiltinBounds, pub projection_bounds: Vec<PolyExistentialProjection<'tcx>>, } // after pub enum ExistentialPredicate<'tcx> { // e.g. Iterator Trait(ExistentialTraitRef<'tcx>), // e.g. Iterator::Item = T Projection(ExistentialProjection<'tcx>), // e.g. Send AutoTrait(DefId), } ```

bors · 2016-11-30T02:41:39Z

⌛ Testing commit 8b82fd7 with merge 8e373b4...

bors · 2016-11-30T06:29:51Z

@eddyb

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> We refactor this in order to achieve the following wins: - Decrease the size of `FnSig` (`Vec` + `bool`: 32, `&Slice` + `bool`: 24). - Potentially decrease total allocated memory due to arena-allocating `FnSig` inputs/output; since they are allocated in the type list arena, other users of type lists can reuse the same allocation for an equivalent type list. - Remove the last part of the type system which needs drop glue (#37965 removed the other remaining part). This makes arenas containing `FnSig` faster to drop (since we don't need to drop a Vec for each one), and makes reusing them without clearing/dropping potentially possible. r? @eddyb

tbg · 2017-04-23T13:13:24Z

@Mark-Simulacrum: Very late to the party, but I just ran into the the sort order while spelunking for #41444, and I'm curious about:

The sort order is asserted to be correct during interning, but the slices are not sorted at that point.

What does "correct" mean? I can see from the code that the only time the assertion runs is when it is passed something that has previously been sorted using the same cmp. But I'm not sure what the point of this ordering is.

ExistentialPredicate::Trait are defined as always equal; This may be wrong; should we be comparing them and sorting them in some way?

Why not just tie-break by DefId? Or rather, why does it not matter which way two Traits compare? For interning, it seems to matter because when you have the "same" slice interned twice but the order differs, to my untrained eye it seems easy enough to introduce subtle bugs when doing pointer-comparisons, etc.

Mark-Simulacrum · 2017-04-24T18:05:39Z

What does "correct" mean? I can see from the code that the only time the assertion runs is when it is passed something that has previously been sorted using the same cmp. But I'm not sure what the point of this ordering is.

Since we now represent TraitObject as a slice of ExistentialPredicates, we need an efficient method to access the parts, which means that we sort into a particular order. As long as the ExistentialPredicate variants are in the correct order, we don't care about the specific order of the variants within each "set."

I think I answered the second question as well. You do make a good point about possibly more efficient interning if we sorted with tie-breaking by DefId; that sounds like a potential optimization, though I don't know how good the wins (if any) will be.

Let me know if you have any more questions!

eddyb · 2017-04-24T18:07:32Z

You simply can't have more than one Trait right now, so ignoring DefId is an optimization.

tbg · 2017-04-27T01:12:43Z

@Mark-Simulacrum: Thanks for the explanation. I have one follow-up question on the following (and it's not really a question about this code in particular any more):

Since we now represent TraitObject as a slice of ExistentialPredicates, we need an efficient method to access the parts, which means that we sort into a particular order. As long as the ExistentialPredicate variants are in the correct order, we don't care about the specific order of the variants within each "set."

The comment on Slice explains:

/// A wrapper for slices with the additional invariant
/// that the slice is interned and no other slice with
/// the same contents can exist in the same context.
/// This means we can use pointer + length for both
/// equality comparisons and hashing.

I guess it depends on what is meant by "same contents", but it would seem to me that at this point you could wind up interning both of the slices &[a,b] and &[b,a]. And, I assume you're fine with that in your use case, but in general if pointer+length is used for equality and you consider things "equal" under certain permutations, isn't there something weird that can happen (afaict, if there's only ever one Trait(_), it seems such permutations can't happen for yo in practice, but still)?

Mark-Simulacrum · 2017-04-27T01:16:58Z

Slices can be compared by pointer and length because they're always interned, which means that &[a,b] and &[b,a] will not compare equally, which is "correct"; arguably if and when we add multiple Traits in the ExistentialPredicate list we should probably sort them. I'm not sure, though. @eddyb might be able to explain more clearly....

rust-highfive assigned eddyb Nov 23, 2016

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 23, 2016

eddyb suggested changes Nov 23, 2016

View reviewed changes

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch 2 times, most recently from b475ea0 to 2c1ec2f Compare November 24, 2016 03:54

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch 4 times, most recently from 3c63d7c to 8d76e7e Compare November 25, 2016 04:40

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch 2 times, most recently from 90d69cd to 72f54e0 Compare November 26, 2016 04:22

Mark-Simulacrum commented Nov 26, 2016

View reviewed changes

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch from fdb5b7b to c0eca6a Compare November 26, 2016 12:57

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch from c0eca6a to eb7bb54 Compare November 27, 2016 17:06

eddyb suggested changes Nov 28, 2016

View reviewed changes

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch 2 times, most recently from 6b84098 to 17ac55e Compare November 28, 2016 05:07

Mark-Simulacrum added 3 commits November 28, 2016 06:37

Privatize TraitObject.principal and add a method accessor, returning …

46c7a11

…Option.

Refactor BuiltinBounds to Vec<DefId> on TraitObject.

607af72

Remove BuiltinBound and BuiltinBounds.

64e97d9

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch from 17ac55e to 0ae852c Compare November 28, 2016 14:10

eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 28, 2016

eddyb reviewed Nov 28, 2016

View reviewed changes

src/librustc/ty/walk.rs Outdated

Copy link

Member

eddyb Nov 28, 2016

Choose a reason for hiding this comment

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

My suggestion was to move the .types().rev() here.

eddyb reviewed Nov 28, 2016

View reviewed changes

src/librustc_typeck/astconv.rs Outdated

Copy link

Member

eddyb Nov 28, 2016

Choose a reason for hiding this comment

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

At least use .skip_binder() instead of .0.

Mark-Simulacrum added 4 commits November 28, 2016 18:09

Refactor TyTrait to contain a interned ExistentialPredicate slice.

bb35d50

Renames TyTrait to TyDynamic.

Remove auto_traits from PartitionedBounds

9b803ec

Adds TyCtxt::require_lang_item(LangItem) to simplify lang item requires.

a79c80d

Replaces instances of tcx.lang_items.require(..) with fatal unwrap with this method.

Remove allocation in push_type_params

8b82fd7

Mark-Simulacrum force-pushed the trait-obj-to-exis-predicate branch from 0ae852c to 8b82fd7 Compare November 29, 2016 01:09

eddyb approved these changes Nov 29, 2016

View reviewed changes

bors merged commit 8b82fd7 into rust-lang:master Nov 30, 2016

This was referenced Nov 30, 2016

struct field reordering and optimization #37429

Merged

Detect missing ; on methods with return type () #36409

Closed

Update the bootstrap compiler #37800

Merged

Mark-Simulacrum mentioned this pull request Dec 1, 2016

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> #38097

Merged

Mark-Simulacrum deleted the trait-obj-to-exis-predicate branch December 2, 2016 01:27

compiler-errors mentioned this pull request Oct 13, 2022

Sort elaborated existential predicates in object_ty_for_trait #102947

Merged

Refactor TraitObject to Slice<ExistentialPredicate> #37965

Refactor TraitObject to Slice<ExistentialPredicate> #37965

Uh oh!

Conversation

Mark-Simulacrum commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 24, 2016

Uh oh!

Mark-Simulacrum commented Nov 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Nov 26, 2016

Uh oh!

Mark-Simulacrum commented Nov 27, 2016

Uh oh!

eddyb commented Nov 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Nov 28, 2016

Uh oh!

bors commented Nov 28, 2016

Uh oh!

Mark-Simulacrum commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Nov 23, 2016 •

edited

Loading