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

🛠 introduce abstraction layer between trait solver and type checker #48895

Closed
nikomatsakis opened this issue Mar 9, 2018 · 9 comments
Closed
Assignees
Labels
A-traits Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler 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

nikomatsakis commented Mar 9, 2018

The purpose of this issue is to setup a trait-solving structure that can change between the current trait solver and the type checker, allowing for us to easily switch to the chalk-style solving when a command line flag is given.

Currently, the interface between the two is the FulfillmentContext, so we have a relationship like this:

[ type checker ] --> [ fulfillment context ] --> [ existing trait solver ]

The first phase then is to introduce another layer in between, let's call it the TraitEngine:

[ type checker ] --> [ TraitEngine ] --> [ fulfillment context ] --> [ existing trait solver ]

When we're done with this phase, everything should work exactly the same, but that the type checker never interacts directly with the fulfillment context.

@nikomatsakis
Copy link
Contributor Author

Mentoring instructions (part 1)

How it works today

The main interaction that the type checker has with the fulfillment context is to "register" obligations with it and try to solve them. Registering is primarily done with this method:

pub fn register_predicate_obligation(&mut self,
infcx: &InferCtxt<'a, 'gcx, 'tcx>,
obligation: PredicateObligation<'tcx>)

Solving is done with select_where_possible, which selects obligations when it can, but defers in the case of ambiguity:

pub fn select_where_possible(&mut self,
infcx: &InferCtxt<'a, 'gcx, 'tcx>)
-> Result<(),Vec<FulfillmentError<'tcx>>>

And select_all_or_error, which selects obligations and reports errors if anything comes up as ambiguous (maybe true, maybe false):

pub fn select_all_or_error(&mut self,
infcx: &InferCtxt<'a, 'gcx, 'tcx>)
-> Result<(),Vec<FulfillmentError<'tcx>>>

Step 1: Introduce TraitEngine trait

We should introduce a new module, let's call it rustc::traits::engine. In there would be a trait TraitEngine that (to start) encapsulates a FulfilllmentContext:

pub trait TraitEngine<'tcx> {
    fn register_predicate_obligation(
        &mut self,
        infcx: &InferCtxt<'a, 'gcx, 'tcx>,
        obligation: PredicateObligation<'tcx>,
    );

    // ... mirror the other fulfillment cx methods as needed ...
}

Then we can have FulfillmentContext implement this trait. This could be done by forwarding to the inherent methods, or by moving them out from the inherent impl:

impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
    fn register_predicate_obligation(
        &mut self,
        infcx: &InferCtxt<'a, 'gcx, 'tcx>,
        obligation: PredicateObligation<'tcx>,
    ) {
        // forward to the inherent method
        self.register_predicate_obligation(infcx, obligation);
    }
}

Finally, we create a factory method on TraitEngine; for now it can always create a fulfillment context. The important part is that it returns a Box<dyn TraitEngine<'tcx>>:

impl dyn TraitEngine<'tcx> {
    pub fn new(_tcx: TyCtxt<'_, '_, 'tcx>) -> Box<Self> {
        Box::new(FulfillmentContext::new())
    }
}

Next, we want to modify typeck to use this. For that, we would modify this field from having type RefCell<traits::FulfillmentContext<'tcx>> to RefCell<Box<dyn TraitEngine<'tcx>>>:

fulfillment_cx: RefCell<traits::FulfillmentContext<'tcx>>,

There may be other uses of FulfillmentContext within librustc_typeck, you can ripgrep around. We should be able to convert them all.

Step 2: Add compiler flag and alternative implementation

I'll leave this step for later. =)

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler 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 A-traits Area: Trait system labels Mar 9, 2018
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 9, 2018

cc @rust-lang/wg-traits -- this is the refactoring step that will allow us to introduce -Zchalk and change implementations. Mentoring instructions are included, looking for someone to pick it up!

@sgrif
Copy link
Contributor

sgrif commented Mar 9, 2018

This is the sort of structural change that is right up my alley (but I also have a non-zero number of things already on my plate so if someone else is looking for a thing to do, go for it)

@cuviper cuviper added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Mar 10, 2018
@csmoe
Copy link
Member

csmoe commented Mar 11, 2018

This work looks not so hard for a rustlang newbie from the mentoring instructions up to now.

If I didn’t underestimate the difficulty, please consider me for this.

@nikomatsakis
Copy link
Contributor Author

@csmoe I don't think this will be very hard! Please feel free to leave questions here (but expect longer latency) or ping me on gitter (shorter latency)

@nikomatsakis nikomatsakis changed the title introduce abstraction layer between trait solver and type checker 🛠 introduce abstraction layer between trait solver and type checker Mar 12, 2018
@nikomatsakis
Copy link
Contributor Author

@csmoe How goes? Have you had a chance to take a look at this yet? It would be very useful. =)

@csmoe
Copy link
Member

csmoe commented Mar 20, 2018

@nikomatsakis I have done mostly, but stuck at the lifetime errors.

@nikomatsakis
Copy link
Contributor Author

@csmoe I see #49202 -- I'll take a look!

bors added a commit that referenced this issue Mar 27, 2018
Introduce trait engine

address #48895 step 1: introduce trait engine
@nikomatsakis
Copy link
Contributor Author

This is basically done enough for now I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler 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

4 participants