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 Program to separating lowering from solving #19

Closed
nikomatsakis opened this issue Mar 11, 2017 · 0 comments
Closed

refactor Program to separating lowering from solving #19

nikomatsakis opened this issue Mar 11, 2017 · 0 comments
Labels
good first issue A good issue to start working on Chalk with

Comments

@nikomatsakis
Copy link
Contributor

Right now the ir::Program struct contains a lot of fields that encode the lowered Rust program:

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Program {
    /// From type-name to item-id. Used during lowering only.
    pub type_ids: HashMap<Identifier, ItemId>,

    /// For each struct/trait:
    pub type_kinds: HashMap<ItemId, TypeKind>,

    /// For each impl:
    pub impl_data: HashMap<ItemId, ImplDatum>,

    /// For each trait:
    pub trait_data: HashMap<ItemId, TraitDatum>,

    /// For each trait:
    pub associated_ty_data: HashMap<ItemId, AssociatedTyDatum>,

    /// Compiled forms of the above:
    pub program_clauses: Vec<ProgramClause>,
}

Since the trait solving routines in solve get access to an Arc<Program>, this suggests that they need all of this information. But in fact they do not. The goal is that they only need program_clauses, although it may be useful to have a bit more (basically, it depends a bit on how much code we lower to explicit program-clauses and how much code we choose to keep in Rust code).

However, these fields are used during lowering. And in particular we lower the input program and then, in the unit tests, save that state and come back and lower various goals.

I think we should introduce a new struct, let's call it the SharedEnvironment or ProgramEnvironment, which is used by the solving routines and contains only those fields needed by solving.

Looking through the code that would be two fields:

So we could refactor Program like so:

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Program {
    /// From type-name to item-id. Used during lowering only.
    pub type_ids: HashMap<Identifier, ItemId>,

    /// For each struct/trait:
    pub type_kinds: HashMap<ItemId, TypeKind>,

    /// For each impl:
    pub impl_data: HashMap<ItemId, ImplDatum>,

    /// For each trait:
    pub associated_ty_data: HashMap<ItemId, AssociatedTyDatum>,

    /// Data used by the solver routines:
    pub program_env: Arc<ProgramEnvironment>
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct ProgramEnvironment {
    /// For each trait:
    pub trait_data: HashMap<ItemId, TraitDatum>,

    /// Compiled forms of the above:
    pub program_clauses: Vec<ProgramClause>,
}

Here I moved the trait_data field into the program-env. Another option would be to duplicate it. In that case, I suspect that Program would not need to have a link to the program-env at all. Instead, lowering could return a pair of (Program, ProgramEnvironment).

We would then also change the solve routines to take an Arc<ProgramEnvironment> instead of Arc<Program>.

I'm tagging this as help-wanted since it seems like a relatively good "intro bug".

@nikomatsakis nikomatsakis added the good first issue A good issue to start working on Chalk with label Mar 11, 2017
withoutboats added a commit to withoutboats/chalk that referenced this issue Mar 13, 2017
Closes rust-lang#19. I took a slightly different approach from your suggestion.

ir::Program and ProgramEnvironment are distinct types with no ownership
relationship, but instead of lowering returning a tuple, ir::Program
has a method to construct a ProgramEnvironment.

In addition to the trait_data, the associated_type_data also needs to
be cloned over, and the `split_projection` method is duplicated for
both types, because that method is invoked in `elaborated_clauses`.

It seems like once we have a new way of solving rust-lang#12, ProgramEnvironment
would just be a set of ProgramClauses, and that would be the whole
context necessary for the solver to run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A good issue to start working on Chalk with
Projects
None yet
Development

No branches or pull requests

1 participant