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

return the correct type for closures in type_of #91055

Merged
merged 1 commit into from
Jan 7, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Nov 19, 2021

A bit unhappy about the way typeck::check_crate works rn. Would have preferred to not change CollectItemTypesVisitor in this way.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2021
@nikomatsakis
Copy link
Contributor

@lcnr This feels like a step backward to me. Can you say a bit more about what's motivating it? In general, I feel like the way things are expected to work is that type_of returns the "generic template" for the type.

@lcnr
Copy link
Contributor Author

lcnr commented Dec 7, 2021

In general, I feel like the way things are expected to work is that type_of returns the "generic template" for the type.

yes, that is also my mental model for type_of. But before this PR, that isn't actually the case.
Before this, type_of returned ty::Closure with substs containing the dummy params for closure_kind, closure_signature, and upvars. To my knowledge, there is not really anything sensible you can do with these substs.

With this change you can now actually call type_of(closure_def_id).subst(tcx, substs) and get the expected result¹.
I am using this in some wip rework of polymorphization where this is needed. If you want, we can talk more about this in sync.

¹: that is a more precise description of my mental model for type_of: I expect subst to result in a correct instance of the type

@nikomatsakis
Copy link
Contributor

@lcnr that makes sense, but I'm a bit confused. Why did that not work before? I guess maybe the question is "what are these substs you are using". I would have expected them to be the closure substs, which would then give a value for those closure type arguments and so forth, right?

@lcnr
Copy link
Contributor Author

lcnr commented Dec 21, 2021

here's an example in case that might help

fn foo<T: Into<i32>, U>(x: T) -> i32 {
    let x = |y: i32| y + x.into();
    x(3)
}

before this PR, the type returned by type_of for |y: i32| y + x.into() was

ty::Closure(def_id, [T, U, <closure_kind>, <closure_signature>, <upvars>])

while with this PR, the type returned by type_of for |y: i32| y + x.into() is

ty::Closure(def_id, [T, U, i32, extern "rust-call" fn((i32,)) -> i32, (T,)])

Before this PR, the only way subst would be correct for the type of x is that we first substitute <closure_kind>, <closure_signature>, <upvars> with their actual values and then , using the result, call subst on the type of x.

With this PR, we can simply call subst with arguments for T and U.

@lcnr
Copy link
Contributor Author

lcnr commented Jan 6, 2022

opened an issue for a more complex fix in #92617

@bors r=nikomatsakis rollup

@bors
Copy link
Contributor

bors commented Jan 6, 2022

📌 Commit 6bd534d has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 6, 2022
return the correct type for closures in `type_of`

A bit unhappy about the way `typeck::check_crate` works rn. Would have preferred to not change `CollectItemTypesVisitor` in this way.

r? `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#91055 (return the correct type for closures in `type_of`)
 - rust-lang#92207 (Delay remaining `span_bug`s in drop elaboration)
 - rust-lang#92417 (Fix spacing and ordering of words in pretty printed Impl)
 - rust-lang#92504 (Exit nonzero on rustc -Wall)
 - rust-lang#92559 (RustWrapper: adapt to new AttributeMask API)
 - rust-lang#92589 (Break the loop)
 - rust-lang#92607 (rustc_metadata: Some minor cleanups and optimizations)
 - rust-lang#92620 (Remove unused `ExtendDefault` struct)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cdc5c13 into rust-lang:master Jan 7, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 7, 2022
@lcnr lcnr deleted the type_of-closures branch January 7, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants