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

work on traits chapters #70

Merged
merged 13 commits into from Mar 10, 2018
Merged

work on traits chapters #70

merged 13 commits into from Mar 10, 2018

Conversation

nikomatsakis
Copy link
Contributor

This work begins to document the new-style trait solving system. This is documenting code that doesn't really exist yet -- my goal is to have the rustc-guide serve as a sort of living design document that we keep updating.

@mark-i-m
Copy link
Member

mark-i-m commented Mar 2, 2018

Thanks again @nikomatsakis! Perhaps we have enough to make a traits/ subdirectory in the repo?

I will take a deeper dive soon.

@nikomatsakis
Copy link
Contributor Author

@mark-i-m I could make a traits directory if you like. Also, I'm sort of inclined to land these without much review and keep iterating, since I want them to be easily accessible to people (i.e., easy for people to read).

@nikomatsakis
Copy link
Contributor Author

I think I initially removed the directories for other pages because I found that I was shuffling around the "groups" sometimes -- i.e., creating or removing "subchapters" -- and I wanted to be able to have "more stable" URLs (in many cases, the file name still made sense, even if it was no longer a subdirectory). Those concerns may not apply here.

@nikomatsakis
Copy link
Contributor Author

Splitting into directories proved harder than expected. I got weird errors from mdbook. Going to leave it for now.

@mark-i-m
Copy link
Member

mark-i-m commented Mar 7, 2018

@nikomatsakis Sorry for the delay. Things have been busy... I had actually started reviewing. I think there are some places in this chapter where the uninitiate (like me 😛) would have a hard time following. If you want, I can try to move my review comments to an issue or something, but frankly, on GH, it's a lot easier to leave comments on a PR than on raw code... your call.

Splitting into directories proved harder than expected. I got weird errors from mdbook. Going to leave it for now.

That's fine. Thanks anyway :D

@nikomatsakis nikomatsakis force-pushed the traits branch 2 times, most recently from 0a1af6f to 901e533 Compare March 7, 2018 14:54
@nikomatsakis
Copy link
Contributor Author

rebased, pushed an updated chapter

@nikomatsakis
Copy link
Contributor Author

@scalexm I realized along the way that I don't know how implied bounds for GATs ought to work. I left some XXX in the chapter on lowering rules at the point of my confusion.

elsewhere.

// XXX how exactly should we set this up? Have to be careful;
// presumably this has to be a kind of `FromEnv` setup.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scalexm here =)

@nikomatsakis
Copy link
Contributor Author

@mark-i-m

frankly, on GH, it's a lot easier to leave comments on a PR than on raw code...

Yeah, totally. I guess we can leave it this way for a bit longer. The text is approaching "code complete" status.

Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience @nikomatsakis! I left a bunch of comments. Overall, I think this is very well-written.

I think the canonicalization section was the hardest for me to follow and fit into the system. I left a number of more detailed comments there.

Also, I had one more question: how do const-generics fit in? I'm assuming they are basically just another kind, but other than that everything is the same?

Thanks again!

src/SUMMARY.md Outdated
@@ -17,10 +17,16 @@
- [The HIR (High-level IR)](./hir.md)
- [The `ty` module: representing types](./ty.md)
- [Type inference](./type-inference.md)
- [Trait resolution](./trait-resolution.md)
- [Trait bsolving (old-style)](./trait-resolution.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"bsolving"

**Note:** This chapter (and its subchapters) describe how the trait
solver **currently** works. However, we are in the process of
designing a new trait solver. If you'd prefer to read about *that*,
see [the traits chapter](./traits.html).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/the traits chapter/_this_ traits chapter/

src/traits.md Outdated
the process of being implemented; this chapter serves as a kind of
in-progress design document. If you would prefer to read about how the
current trait solver works, check out
[this chapter](./trait-resolution.html).🚧
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a link to the tracking issue for the new solver?

[inference variable](./type-inference.html#vars) is always in one of two
states: either it is **unbound**, in which case we don't know yet what
type it is, or it is **bound**, in which case we do. So to isolate
some thing T from its environment, we just walk down and find the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use a more precise word than "thing"?

a canonical order (left to right, for the most part, but really it
doesn't matter as long as it is consistent).

So, for example, if we have the type `X = (?T, ?U)`, where `?T` and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to introduce the existential ? before this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. I sort of do, by saying "where ?T are inference variables", but it does feel like there is some missing introductory material. And maybe a notation section. I have to mull on this.

forall<Self, P1..Pn, Pn+1..Pm> {
ProjectionEq(
<Self as Trait<P1..Pn>>::AssocType<Pn+1..Pm> =
(Trait::AssocType)<Self, P1..Pn, Pn+1..Pm>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skolemized associated type projections are not introduced until the next chapter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more forward refs


The next rule covers implied bounds for the projection. In particular,
the `Bounds` declared on the associated type must be proven to hold to
show that the impl is well-formed, and hence we can rely on them from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/from elsewhere/elsewhere/

impl<P0..Pn> Trait<A1..An> for A0
where WC
{
type AssocType<Pn+1..Pm>: Bounds where WC1 = T;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um... do we generally have the bounds on the impl? Or is it here for pedagogy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good...point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I was just conflating trait + impl somehow

@@ -0,0 +1,267 @@
# Lowering rules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is pretty accessible, but there are a few places where Normalize is used, and it isn't really defined until a later section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be that this should move until after both of them; this is sort of a "reference" section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reorganized a bit

will create the following rules:

forall<P0..Pn> {
Implemented(TraitRef) :- WC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that if there is no where clause, WC is Top of something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... "true"

@mark-i-m
Copy link
Member

mark-i-m commented Mar 8, 2018

Note that GH is hiding 23 comments again...

@nikomatsakis nikomatsakis mentioned this pull request Mar 8, 2018
20 tasks

Ok, so far so good. Let's move on to type-checking a more complex function.

## Type-checking generic functions: beyond Horn clauses
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an example of implied bounds "in practice" in this section, where we in fact assume FromEnv(T: Eq) when type-checking the function below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think we want to add a new "implied bounds" section -- beyond what's there now -- that kind of explains in a more "conversational" style. I'll add a placeholder for now, I'm sick of editing.

@nikomatsakis
Copy link
Contributor Author

Ok, did some massive restructuring in response to @mark-i-m's feedback.

@nikomatsakis
Copy link
Contributor Author

OK, I'm going to merge this -- but feel free to leave more comments and I will address.

@nikomatsakis nikomatsakis merged commit c207e4c into rust-lang:master Mar 10, 2018

Canonical(QR) = for<T, L> {
certainty: Proven,
var_values: [Vec<?0>, '?1, ?2]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ?2 be ?0 here? Same in region constraints?


for<T, L> {
certainty: Proven,
var_values: [Vec<?0>, '?1, ?2]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants