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

remove synthetic closure substs #92617

Open
lcnr opened this issue Jan 6, 2022 · 2 comments
Open

remove synthetic closure substs #92617

lcnr opened this issue Jan 6, 2022 · 2 comments
Assignees
Labels
A-closures Area: closures (`|args| { .. }`) C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jan 6, 2022

right now, we add synthetic generic parameters to closures to represent a bunch of additional information about the closure, see
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/sty/struct.ClosureSubsts.html

These substs don't behave like ordinary generic parameters, negatively impact perf and are difficult to understand or deal with in some cases.

We should try to remove these synthetic parameters, for example by moving them into TypeckResults and treating closures a lot more similar to ordinary adts. The same approach should also be used for generators and inline consts.

Going to look into this myself for now

cc @nikomatsakis

@lcnr lcnr added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-closures Area: closures (`|args| { .. }`) labels Jan 6, 2022
@lcnr lcnr self-assigned this Jan 6, 2022
@lcnr lcnr changed the title move synthetic closure substs into TypeckResults remove synthetic closure substs Jan 6, 2022
@eddyb
Copy link
Member

eddyb commented Jan 6, 2022

How can this soundly interact with inference? IIRC, the reason they're like this today, is so that parts of that information can be left as inference variables, in a way that interfaces with everything that handles inference variables (that is, I remember them being stored in an ad-hoc way in InferCtxt and that having issues).

The only thing I can think of, that would work with e.g. canonicalization, would be to put the information in ParamEnv, something like a foo::{closure#0}: FnOnce<$123> bound - but are inference variables in ParamEnv supported?

@eddyb
Copy link
Member

eddyb commented Jan 6, 2022

Also, regarding upvars, ADTs differ a lot from tuples, in how their fields interact with various aspects of traits and lifetimes, and tuples are much more straight-forward IMO - you would have to look for every place in the compiler where either ty::Adt or ty::Tuple are handled, and consider the implications of switching closures from one approach to the other.

But still, it would have to be in something like ParamEnv, instead of always being able to do a query for "field types" the way ADTs can, otherwise how can you reason about e.g. foo::{closure#0}: Sync during inference?

(Also, I am not aware of any precedent where some trait system interaction with a type can involve inference variables that are not in the type itself - anything that relies on type flags may be broken).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

2 participants