Cleanup crate structure and add features for SLG/recursive solvers #459

AzureMarker · 2020-05-17T19:40:03Z

Closes #456

The chalk-rust-ir crate is moved into chalk-solve as the rust_ir module.
chalk-macros is renamed to chalk-base and some types are moved there from chalk-engine so that the recursive solver does not depend on chalk-engine.
The SLG and recursive solvers have their own feature flags in chalk-solve so that only one solver needs to be compiled. By default, both features are enabled.

AzureMarker · 2020-05-17T19:40:41Z

chalk-solve/Cargo.toml

+chalk-engine = { version = "0.10.1-dev", path = "../chalk-engine", optional = true }
+chalk-ir = { version = "0.10.1-dev", path = "../chalk-ir", default-features = false }

 [dev-dependencies]
 chalk-integration = { version = "0.10.1-dev", path = "../chalk-integration" }
+
+[features]
+default = ["slg-solver", "recursive-solver"]
+
+slg-solver = ["chalk-engine", "chalk-ir/slg-solver"]
+recursive-solver = []


I ran into rust-lang/cargo#7916 while testing the feature flags, but I don't think it affects downstream users (like RA). The issue is that a cargo check --no-default-features --features recursive-solver in chalk-solve would still compile chalk-engine because the dev-dependency chalk-integration implicitly enables chalk-ir's slg-solver feature. If the dev-dependency features are not unified, this issue goes away (tested on nightly with -Z features=dev_dep).

Hmm. So what are the implications of this? We just always have to compile with both features?

I think it works as expected for downstream crates (with recursive-solver, chalk-engine is not compiled and vice versa). However, when running commands in this repo like cargo test/cargo check, both solvers will get built.

Yup, had some experience with it. This is okay.

nikomatsakis

Can you update the book's coverage of the chalk crates to reflect the changes?

I think I'd also like to see a directed graph (in a hackmd or in a book) that shows all the crates we intend to wind up with and summarizes their purpose. I think it'd be helpful.

I'm starting to lean mildly towards "let's have fewer crates" versus a bunch of really narrow ones, but haven't made up my mind on that point.

jackh726

This definitely makes me wonder if it's just worth moving chalk-engine code into chalk-solve. In a separate module, like recursive. I'm not entirely sure how I feel about this. Since they API boundary between the chalk-engine and chalk-solve seemed helpful for me when I was first getting into it. But I think maybe just it being in a separate module would be enough. And it means that the idea that the engine is a "standalone prolog solver" kind of goes away.

jackh726 · 2020-05-18T18:14:21Z

chalk-engine-base/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
-name = "chalk-macros"
+name = "chalk-engine-base"


I don't particularly like the name chalk-engine-base. Maybe just chalk-base?

Or, since chalk-ir is getting a feature flag. Just have this be part of chalk-engine with all SLG stuff under a feature flag.

Not sure how I feel about this one.

If it's part of chalk-engine, 90% of the crate will be under one feature flag.

Yeah, exactly. Which is probably not a great tradeoff to just get rid of a small crate.

jackh726 · 2020-05-18T18:17:38Z

chalk-solve/Cargo.toml

+chalk-engine = { version = "0.10.1-dev", path = "../chalk-engine", optional = true }
+chalk-ir = { version = "0.10.1-dev", path = "../chalk-ir", default-features = false }

 [dev-dependencies]
 chalk-integration = { version = "0.10.1-dev", path = "../chalk-integration" }
+
+[features]
+default = ["slg-solver", "recursive-solver"]
+
+slg-solver = ["chalk-engine", "chalk-ir/slg-solver"]
+recursive-solver = []


Hmm. So what are the implications of this? We just always have to compile with both features?

nikomatsakis · 2020-05-18T18:46:16Z

So -- I tend to think that the engine boundary is useful conceptually, but what I don't think is all that useful is the fact that the engine is totally generic over all the types. I feel like one of the following two options makes sense:

move chalk-engine into chalk-solve, but try to have a clear conceptual boundary between the modules, ideally still using a trait to guide "callbacks"
make chalk-engine depend on chalk-ir and move the recursive solver into it

If we did the latter, there would still need to be a kind of "context" trait, but it would be much thinner, I imagine?

Conceptually, we could go further, and move the unification code and some other things from chalk-solve into chalk-ir as well, which would probably narrow the interface further. However, I might not want to do that, or at least I might want to wait until the sem-vs-syn split proceeds a bit, because I think I'd like it if the "semantic unification" remained in chalk-solve, as it is kind of "Rust-specific semantics" and feels like a better fit for a callback.

jackh726 · 2020-05-18T19:05:09Z

I think either of your ideas - moving chalk-engine into chalk-solve, or making chalk-engine depend on chalk-ir - would be good.

ideally still using a trait to guide "callbacks"

I think this would be the same trait that we would get by having it in a separate crate

Moving unification and such code into chalk-ir I'm less inclined to do. I think we should strive to make chalk-ir as "type library"-like as possible, and less "trait solver library"-like. I guess unification can fall under types, but I'm not sure.

I'm more or less thinking long-term in regards to rustc integration, what we might want things to look like. Specifically, right now chalk-ir is a dependency in rustc_middle. Later on, the shared type library will probably also be a dependency of rustc_middle.

AzureMarker · 2020-05-18T19:15:05Z

I think I'd also like to see a directed graph (in a hackmd or in a book) that shows all the crates we intend to wind up with and summarizes their purpose. I think it'd be helpful.

I'm not sure if I have the context to do this, but maybe it wasn't directed at me :)

AzureMarker · 2020-05-18T19:26:06Z

The book lint check is failing because the new chalk_solve::rust_ir module documentation is not built yet. I'm not sure if this can be solved without changing how the book references the documentation (reference current branch's docs instead of published master branch docs in PRs?).

nikomatsakis · 2020-05-18T21:34:06Z

@jackh726

I think this would be the same trait that we would get by having it in a separate crate

Yes -- and ideally it'd be the same trait for both solvers, too.

I'm more or less thinking long-term in regards to rustc integration, what we might want things to look like. Specifically, right now chalk-ir is a dependency in rustc_middle. Later on, the shared type library will probably also be a dependency of rustc_middle.

I'm happy to wait, though I was thinking of the same thing (that is, currently rustc has its unification code in rustc_middle::infer -- though I think that rustc's crate structure is not very deeply thought out right now). My main motivation was to try and make the trait smaller. But I think that if we have the solvers able to reference chalk-ir, the size of the trait will already go down considerably.

jackh726 · 2020-05-19T16:34:12Z

@Mcat12 how much do you want to try to experiment here? Would you like to try to turn this into a bit of a refactor to make chalk-engine depend on chalk-ir?

AzureMarker · 2020-05-19T17:18:02Z

I'm ok with experimenting but I'm afraid it might make this PR too large (there are other nice changes in this PR like moving chalk-rust-ir and chalk-macros, and merge conflicts can be annoying). Could we merge this and then experiment on further simplifying the SLG interface in a new PR?

jackh726 · 2020-05-19T17:22:33Z

I think the changes here are fine. @nikomatsakis do we want to merge this? Or wait for docs? Or wait for refactor?

AzureMarker · 2020-05-19T17:23:21Z

I think the docs are done, please feel free to point out if they are not completely updated.

jackh726 · 2020-05-19T17:23:49Z

I'm referring to Niko's suggestion of adding the directed graph

nikomatsakis · 2020-05-19T20:27:08Z

I think we can wait on the directed graph until we know what we want

nikomatsakis · 2020-05-19T20:27:16Z

I would be happy to edit in a hackmd to start

Enabling one but not the other avoids compiling an unnecessary solver.

AzureMarker commented May 17, 2020

View reviewed changes

nikomatsakis requested changes May 18, 2020

View reviewed changes

jackh726 reviewed May 18, 2020

View reviewed changes

AzureMarker added 6 commits May 19, 2020 18:23

Merge chalk-rust-ir into chalk-solve

81f76f6

Rename chalk-macros to chalk-engine-base and consolidate base types

896f786

Add cargo features to enable SLG or recursive solvers

8ecdac6

Enabling one but not the other avoids compiling an unnecessary solver.

Fix imports after rebase

192bb8b

Rename chalk-engine-base to chalk-base

4616da7

Update book with chalk_solve::rust_ir

849a295

AzureMarker requested a review from nikomatsakis May 20, 2020 18:47

nikomatsakis approved these changes May 23, 2020

View reviewed changes

nikomatsakis merged commit 1f9c529 into rust-lang:master May 23, 2020

AzureMarker deleted the cleanup/reorganize-crates branch May 23, 2020 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup crate structure and add features for SLG/recursive solvers #459

Cleanup crate structure and add features for SLG/recursive solvers #459

AzureMarker commented May 17, 2020 •

edited

Loading

AzureMarker May 17, 2020

jackh726 May 18, 2020

AzureMarker May 18, 2020

pksunkara May 20, 2020

nikomatsakis left a comment

jackh726 left a comment

jackh726 May 18, 2020

jackh726 May 18, 2020

AzureMarker May 18, 2020

jackh726 May 18, 2020

jackh726 May 18, 2020

nikomatsakis commented May 18, 2020

jackh726 commented May 18, 2020

AzureMarker commented May 18, 2020

AzureMarker commented May 18, 2020

nikomatsakis commented May 18, 2020 •

edited

Loading

jackh726 commented May 19, 2020 •

edited

Loading

AzureMarker commented May 19, 2020 •

edited

Loading

jackh726 commented May 19, 2020

AzureMarker commented May 19, 2020

jackh726 commented May 19, 2020

nikomatsakis commented May 19, 2020

nikomatsakis commented May 19, 2020

Cleanup crate structure and add features for SLG/recursive solvers #459

Cleanup crate structure and add features for SLG/recursive solvers #459

Conversation

AzureMarker commented May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 18, 2020

jackh726 commented May 18, 2020

AzureMarker commented May 18, 2020

AzureMarker commented May 18, 2020

nikomatsakis commented May 18, 2020 • edited Loading

jackh726 commented May 19, 2020 • edited Loading

AzureMarker commented May 19, 2020 • edited Loading

jackh726 commented May 19, 2020

AzureMarker commented May 19, 2020

jackh726 commented May 19, 2020

nikomatsakis commented May 19, 2020

nikomatsakis commented May 19, 2020

AzureMarker commented May 17, 2020 •

edited

Loading

nikomatsakis commented May 18, 2020 •

edited

Loading

jackh726 commented May 19, 2020 •

edited

Loading

AzureMarker commented May 19, 2020 •

edited

Loading