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

🔬 refactor structure of ty::Predicate #48539

Closed
nikomatsakis opened this issue Feb 25, 2018 · 6 comments
Closed

🔬 refactor structure of ty::Predicate #48539

nikomatsakis opened this issue Feb 25, 2018 · 6 comments
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-low Low priority T-lang Relevant to the language team, which will review and decide on the PR/issue. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804

Comments

@nikomatsakis
Copy link
Contributor

The current Predicate conservatively assumes that every variant may bind higher-ranked lifetimes:

rust/src/librustc/ty/mod.rs

Lines 905 to 941 in 322d7f7

#[derive(Clone, Copy, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum Predicate<'tcx> {
/// Corresponds to `where Foo : Bar<A,B,C>`. `Foo` here would be
/// the `Self` type of the trait reference and `A`, `B`, and `C`
/// would be the type parameters.
Trait(PolyTraitPredicate<'tcx>),
/// where `T1 == T2`.
Equate(PolyEquatePredicate<'tcx>),
/// where 'a : 'b
RegionOutlives(PolyRegionOutlivesPredicate<'tcx>),
/// where T : 'a
TypeOutlives(PolyTypeOutlivesPredicate<'tcx>),
/// where <T as TraitRef>::Name == X, approximately.
/// See `ProjectionPredicate` struct for details.
Projection(PolyProjectionPredicate<'tcx>),
/// no syntax: T WF
WellFormed(Ty<'tcx>),
/// trait must be object-safe
ObjectSafe(DefId),
/// No direct syntax. May be thought of as `where T : FnFoo<...>`
/// for some substitutions `...` and T being a closure type.
/// Satisfied (or refuted) once we know the closure's kind.
ClosureKind(DefId, ClosureSubsts<'tcx>, ClosureKind),
/// `T1 <: T2`
Subtype(PolySubtypePredicate<'tcx>),
/// Constant initializer must evaluate successfully.
ConstEvaluatable(DefId, &'tcx Substs<'tcx>),
}

This forces us to account for higher-ranked binders in a lot more places in the code than we really should have to. It would be nice to restructure Predicate to avoid that.

In the past, I tried two approaches. In one, I separated out Predicate into Predicate and PredicateAtom. In the other, I introduced a Poly variant into Predicate. In the latter case, this made the Predicate type recursive, so as an initial refactoring, I converted Predicate to be a &'tcx PredicateKind, introducing indirection.

It's been long enough that my memory is a bit dim, but I think that the second refactoring was done precisely because the first one turned out be rather intrusive. I am not sure just now which one I prefer, but I think we will want to do one of them sooner or later!

(NB. This will conflict with the work that @sgrif is doing in #48536, though the conflicts ought to be possible to resolve.)

@nikomatsakis nikomatsakis added the WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 label Feb 25, 2018
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 2, 2018

I think that the way to start here would be first to convert Predicate to be interned, so that it can have a recursive structure. The starting point would be to cherry-pick these commits from my repo:

We would probably want to measure the performance of this, though. =)

@sgrif
Copy link
Contributor

sgrif commented Mar 4, 2018

Given that this conflicts with what I'm already working on, I'd like to take this one.

@sgrif sgrif self-assigned this Mar 4, 2018
@ishitatsuyuki
Copy link
Contributor

@sgrif Do you have any updates on this?

@XAMPPRocky XAMPPRocky added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 14, 2018
@leoyvens
Copy link
Contributor

This forces us to account for higher-ranked binders in a lot more places in the code than we really should have to.

Do you remember examples of places where this is a problem, to help guide the refactoring?

@leoyvens
Copy link
Contributor

This was discussed in a WG-traits meeting. @nikomatsakis didn't remember why he originally wanted this and it seems less relevant with chalk, making it a low priority.

@leoyvens leoyvens added the P-low Low priority label Jun 4, 2018
@nikomatsakis
Copy link
Contributor Author

I'm going to close this issue altogether, since it seems like we are going more the route of building up a "parallel structure" for Chalk purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-low Low priority T-lang Relevant to the language team, which will review and decide on the PR/issue. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804
Projects
None yet
Development

No branches or pull requests

5 participants