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 a map in the TypeckTables to get all Upvars given the a closure ID #56905

Closed
blitzerr opened this issue Dec 17, 2018 · 4 comments
Closed
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@blitzerr
Copy link
Contributor

It is a recurring pattern in the rustc compiler base to iterate thorough all the free-variables (or Upvars as they are also known as) using with_freevars. It might be better to have a list of all the Upvars given a closure id.

@varkor varkor added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Dec 18, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 20, 2018
Issue rust-lang#56905

Adding a map to TypeckTables to get the list of all the Upvars
given a closureID. This is help us get rid of the recurring
pattern in the codebase of iterating over the free vars
using with_freevars.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 21, 2018
Issue rust-lang#56905

Adding a map to TypeckTables to get the list of all the Upvars
given a closureID. This is help us get rid of the recurring
pattern in the codebase of iterating over the free vars
using with_freevars.
@nikomatsakis
Copy link
Contributor

Leaving some notes here:

It's clear that we want to remove the use of freevars wherever possible, since all that code is basically assuming that the set of captures for a closure is equal to the set of variables referenced from within that closure.

In most of these cases, it should be fairly straightforward: typeck can instead produce a list of captures and the code can use that list.

However, there are a few cases that are going to be more complex. The most notable is the place where we determine the "generics" (set of generic parameters) for a closure type. The problem is that, currently, the set of generic parameters include one generic parameter for each free variable.

I'm not really sure how best to fix this. I suppose the ideal would be if we could avoid generating TyClosure types until the initial type check is done. This may be plausible.

Hmm, perhaps an alternative could be to make the "upvar types" always be a tuple of the actual upvar types, with the arity not known until after type-check is done. During type-check, this would just be an inference variable (just as, today, all the types of the upvars are inference variables). After upvar inference, we would unify that variable with a tuple with the types of each upvar (just as, today, we unify each of the individual upvar variables with its final type).

A quick inspection of the users of upvar_tys suggests that this could work. We could do this refactoring today.

@nikomatsakis
Copy link
Contributor

I suppose we could even, but we probably don't want to, make closures always store their upvars in a tuple, so that we access field #x as closure.upvars.x instead of closure.#x. Hmm. This might turn out more convenient, though it would also complicate some of the code in the borrow-checker which would now have to work harder to figure out if a given field reference is an "upvar field" reference.

(It would have the advantage of mapping the closure more closely to something a user could have written)

@blitzerr
Copy link
Contributor Author

blitzerr commented Jan 9, 2019

@nikomatsakis Can we close this issue as we can treat this as a logical wrapup. For the tuple introduction, I created another issue with the next steps ?

Centril added a commit to Centril/rust that referenced this issue Jan 12, 2019
Issue rust-lang#56905

Adding a map to TypeckTables to get the list of all the Upvars
given a closureID. This is help us get rid of the recurring
pattern in the codebase of iterating over the free vars
using with_freevars.
bors added a commit that referenced this issue Jan 12, 2019
Rollup of 26 pull requests

Successful merges:

 - #56425 (Redo the docs for Vec::set_len)
 - #56906 (Issue #56905)
 - #57042 (Don't call `FieldPlacement::count` when count is too large)
 - #57175 (Stabilize `let` bindings and destructuring in constants and const fn)
 - #57192 (Change std::error::Error trait documentation to talk about `source` instead of `cause`)
 - #57296 (Fixed the link to the ? operator)
 - #57368 (Use CMAKE_{C,CXX}_COMPILER_LAUNCHER for ccache)
 - #57400 (Rustdoc: update Source Serif Pro and replace Heuristica italic)
 - #57417 (rustdoc: use text-based doctest parsing if a macro is wrapping main)
 - #57433 (Add link destination for `read-ownership`)
 - #57434 (Remove `CrateNum::Invalid`.)
 - #57441 (Supporting backtrace for x86_64-fortanix-unknown-sgx.)
 - #57450 (actually take a slice in this example)
 - #57459 (Reference tracking issue for inherent associated types in diagnostic)
 - #57463 (docs: Fix some 'second-edition' links)
 - #57466 (Remove outdated comment)
 - #57493 (use structured suggestion when casting a reference)
 - #57498 (make note of one more normalization that Paths do)
 - #57499 (note that FromStr does not work for borrowed types)
 - #57505 (Remove submodule step from README)
 - #57510 (Add a profiles section to the manifest)
 - #57511 (Fix undefined behavior)
 - #57519 (Correct RELEASES.md for 1.32.0)
 - #57522 (don't unwrap unexpected tokens in `format!`)
 - #57530 (Fixing a typographical error.)
 - #57535 (Stabilise irrefutable if-let and while-let patterns)

Failed merges:

r? @ghost
@blitzerr
Copy link
Contributor Author

blitzerr commented Jan 12, 2019

Related to #53488

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

3 participants