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

Add closures #519

Merged
merged 7 commits into from
Jun 16, 2020
Merged

Add closures #519

merged 7 commits into from
Jun 16, 2020

Conversation

jackh726
Copy link
Member

@jackh726 jackh726 commented Jun 11, 2020

c.c. #368

This works but isn't rebased.

@jackh726 jackh726 marked this pull request as ready for review June 12, 2020 01:17
@jackh726
Copy link
Member Author

Okay I just applied my changes in a new commit against master, since that was easier.

@nikomatsakis I would appreciate a review. The biggest "missing" part of this PR is tests. Not sure the best was to test closures currently.

@jackh726
Copy link
Member Author

Downloaded lalrpop v0.19.0
error: failed to parse manifest at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/lalrpop-0.19.0/Cargo.toml

The heck. I'm just gonna restart CI.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks basically right. I left a few nits. I guess that for testing we'd have to have a way to declare closure types in the source, similar to our fn foo declarations. Not 100% sure what the obvious syntax for that is, maybe we should just do closure foo(...); declarations that are otherwise very much like fn declarations.

There was occasional talk of merging fns and closures in rustc, but I don't know of any concrete plans to do that. I guess that until we do so we can leave things as they are in chalk, though it does seem like you can imagine merging them and having a notion of some way to

(a) get the upvar type for a "callable" thing (for fn-def, returns (), for closures returns the tuple)
(b) get the fn-kind for a "callable" thing (for fn-def, returns Fn, for closures returned whatever is inferred)
(c) get the signature for the callable thing

Certainly would fit in with our general trend in chalk of trying to group together things that are "really quite similar", but I kind of think I'd rather land your PR as is (modulo changes I suggested) and then re-approach this later.

chalk-solve/src/clauses/builtin_traits/copy.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses/builtin_traits/fn_family.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses/builtin_traits/fn_family.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses/builtin_traits/fn_family.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member Author

So, testing closures with no upvars is fairly easy (like you said, a closure foo(...) syntax isn't terrible). The question is more of how would we test upvars?

We could do something like

closure foo[...]()

where the [...] would be the upvars. But even still, the question is how do we write the upvars? [Foo, &mut Bar]?

@nikomatsakis
Copy link
Contributor

I was imagining

closure foo<...>(self|&self|&mut self, t0...tn) -> treturn {
   t_upvar1,
   t_upvar2,
   ...
}

where the self part captures fn/fn-mut/fn-once.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

👍 so far

chalk-solve/src/clauses/builtin_traits/fn_family.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member Author

This should be ready to review

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks awesome! I left some comments, I'm a bit confused about the handling of generic type parameters.

chalk-solve/src/clauses/builtin_traits/fn_family.rs Outdated Show resolved Hide resolved
chalk-solve/src/lib.rs Outdated Show resolved Hide resolved
chalk-solve/src/lib.rs Show resolved Hide resolved
chalk-solve/src/rust_ir.rs Outdated Show resolved Hide resolved
chalk-solve/src/rust_ir.rs Show resolved Hide resolved
tests/test/closures.rs Outdated Show resolved Hide resolved
tests/test/closures.rs Outdated Show resolved Hide resolved
chalk-solve/src/rust_ir.rs Outdated Show resolved Hide resolved
tests/test/closures.rs Show resolved Hide resolved
chalk-integration/src/lowering.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member Author

Ok @nikomatsakis I addressed your comments. I wasn't handling the generic types correctly. I also just moved from the single ClosureDatum struct to just functions on RustIrDatabase, since that seems to be the direction we want to go. Can you look over again? I wrote some doc comments, but they could use a second pair of eyes for clarity.

@jackh726
Copy link
Member Author

Btw there might be a bug in here somewhere. Trying to update rustc to use this causes the closure tests to fail. So don't merge yet even if it looks good. Debugging...

@jackh726
Copy link
Member Author

False alarm :) this is good for review

@nikomatsakis nikomatsakis merged commit a27bfd0 into rust-lang:master Jun 16, 2020
@jackh726 jackh726 deleted the closure branch June 16, 2020 21:59
@jackh726 jackh726 restored the closure branch December 15, 2020 19:40
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.

2 participants