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

Simplify chalk-engine a bit #264

Merged
merged 9 commits into from Oct 28, 2019

Conversation

@jackh726
Copy link
Contributor

jackh726 commented Oct 20, 2019

cc #255

This removes the two different Contexts from Strand and the &'table mut dyn InferenceTable<C, I> field (and consequently the lifetime parameter 'table) has been removed, in lieu of passing infer directly to function arguments as needed. WithInstantiatedUCanonicalGoal and WithInstantiatedExClause were also removed in favor of just FnOnces. And then the with_instantiated_strand indirection has also been removed.

CanonicalStrands are still stored in Table, since I haven't yet figured out the best way to simplify that. But this gets us one step closer.

I'll probably still be playing around to try to remove CanonicalStrand, but I figured it would be better to at least make this PR since it seems fairly straightforward.

@jackh726 jackh726 force-pushed the jackh726:simplify-engine branch from 0e8882d to 5916370 Oct 21, 2019
Copy link
Collaborator

nikomatsakis left a comment

This is definitely in the right direction -- but it's probably worth looking back a bit in the history. I think that the way the code originally worked was that we had each strand store an inference table, and that persisted as long as the strand was in use.

It's maybe a bit debatable if this is the best structure, but that was how we also got rid of the canonicalization when "storing" strands.

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Oct 21, 2019

Yep, it looks like this is the commit to look at

5d8bc33

@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Oct 21, 2019

@nikomatsakis Yes, I went through the history and found that. The problem is now we can't store the InferenceTable in the Strand, since it now has a lifetime associated with it (from &'me dyn RustIrDatabase).

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Oct 21, 2019

@jackh726 I see. Interesting! Well, I think we can land this PR for now and look into this question as follow-up. The only thing I'm not sure about is whether it makes sense to remove the inference table from Strand, as I think we might likely want to put it back again.

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Oct 21, 2019

I suspect that the right answer here is going to be:

  • move program_clauses from the InferenceTable trait to ContextOps -- which is enable by your changes here
  • then we can remove the field program from the TruncatingInferenceTable
  • then we can store it in Strand
@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Oct 21, 2019

@nikomatsakis I can definitely try that later tonight. I can also start topic on Zulip if you want to discuss more in sync.

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Oct 21, 2019

@jackh726 zulip topic sounds good =)

@jackh726 jackh726 force-pushed the jackh726:simplify-engine branch from d810d69 to 511b4f2 Oct 25, 2019
@@ -401,3 +374,9 @@ pub trait AnswerStream<C: Context> {
fn any_future_answer(&mut self, test: impl FnMut(&C::InferenceNormalizedSubst) -> bool)
-> bool;
}

pub trait Canonical<T, C: Context> {

This comment has been minimized.

Copy link
@jackh726

jackh726 Oct 25, 2019

Author Contributor

I was looking at some of the git history since writing this, and it seems like there was a push to reduce the traits in Context, so I'm not sure this trait makes since.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

Yeah, not sure, I did push away from these sorts of traits at some point. I think it was because, for real flexibility, you wind up wanting to thread &C arguments around anyway, since some crates may need access to fields.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

Oh and I think it was because of coherence reasons, too -- this way, you only needed to own the "context" type, but the other types might be from other crates

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

I think I'd prefer to have the methods collected on the Context type, but we could introduce "indirection helpers" if desired to make the code read more nicely. Still, I'm not convinced these methods are needed at all.

pub(crate) infer: &'table mut dyn InferenceTable<C, I>,

pub(super) ex_clause: ExClause<I>,
pub(super) ex_clause: C::CanonicalExClause,

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

I think this should not be canonical -- I think the Strand type should be

struct Strand<C: Context> {
    pub(super) infer: C::InferenceTable,
    pub(super) ex_clause: ExClause<C>,
    pub(crate) selected_subgoal: Option<SelectedSubgoal<C>>,
}
chalk-engine/src/context.rs Show resolved Hide resolved
@@ -401,3 +374,9 @@ pub trait AnswerStream<C: Context> {
fn any_future_answer(&mut self, test: impl FnMut(&C::InferenceNormalizedSubst) -> bool)
-> bool;
}

pub trait Canonical<T, C: Context> {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

Yeah, not sure, I did push away from these sorts of traits at some point. I think it was because, for real flexibility, you wind up wanting to thread &C arguments around anyway, since some crates may need access to fields.

@@ -401,3 +374,9 @@ pub trait AnswerStream<C: Context> {
fn any_future_answer(&mut self, test: impl FnMut(&C::InferenceNormalizedSubst) -> bool)
-> bool;
}

pub trait Canonical<T, C: Context> {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

Oh and I think it was because of coherence reasons, too -- this way, you only needed to own the "context" type, but the other types might be from other crates

@@ -401,3 +374,9 @@ pub trait AnswerStream<C: Context> {
fn any_future_answer(&mut self, test: impl FnMut(&C::InferenceNormalizedSubst) -> bool)
-> bool;
}

pub trait Canonical<T, C: Context> {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

I think I'd prefer to have the methods collected on the Context type, but we could introduce "indirection helpers" if desired to make the code read more nicely. Still, I'm not convinced these methods are needed at all.

*canonical_strand = delayed_strand;

for strand in self.tables[table].strands_mut() {
// FIXME: do we need to uncanonicalize this

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Oct 26, 2019

Collaborator

I don't think so -- but I have to review this logic -- in any case I actually want to remove this "delaying" stuff, see #250, so maybe that too would be a good step to take before doing the rest of this PR, just to simplify things?

@jackh726 jackh726 force-pushed the jackh726:simplify-engine branch 2 times, most recently from 03dd96c to 3a6ed17 Oct 27, 2019
@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Oct 27, 2019

@nikomatsakis Ok, so I removed the "remove CanonicalStrand" commit, and added a couple other smaller cleanup changes. I do think that any followup work for removing CanonicalStrand should be in another PR, possibly after removing the delaying stuff.

Let me know if you disagree with the other smaller changes I made and I can just remove them.

@jackh726 jackh726 force-pushed the jackh726:simplify-engine branch from 3a6ed17 to 6f34e3c Oct 27, 2019
@jackh726 jackh726 mentioned this pull request Oct 28, 2019
@nikomatsakis nikomatsakis merged commit 30556d2 into rust-lang:master Oct 28, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@jackh726 jackh726 deleted the jackh726:simplify-engine branch Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.