Remove extra call to upvar_tys

[arora-aman]

We already visit the tuple of upvar_tys, we don't need to visit each individual type.

Fixes #78720

r? @ghost

[arora-aman]

Running tests

[arora-aman]

I'm curious if we will ever call Iterator for NeedDrop and ty/layout in case the types haven't been inferred yet?

I'm assuming this is fine, we won't be working with basic blocks if we have uninferred types.

[nikomatsakis]

@arora-aman the elaborate-drops code is fine, but for needs-drop I would change it to just enqueue the tupled upvars type. In general I would prefer to write code that operates on tupled_upvar_ty than code which invokes upvar_tys.

In fact, I think we could rewrite the layout code the same way -- the layout of the closure is just the same as self.layout_of(tupled_upvar_ty) (if you look at the code for ty::Tuple, you'll see that this is basically what we're already doing anyway).

[arora-aman]

I don't think this is necessarily an err ...

[roxelo]

I am using the Err not because it is an error, but to know whether to return or to pass as parameter the output of ty_and_layout_kind into layout_of

[roxelo]

Nvm, I see what you mean now: #78725 (comment)

[arora-aman]

I'd probably add an enum which is

// Name can probably be better.
enum TyMaybeWithLayout {
    Ty(C::Ty),
    TyAndLayout(C::TyAndLayout)
}

[roxelo]

Oh I see, I guess that way I don't need to use result and use "Err" as a workaround

[roxelo]

Other locations where upvar_tys() is called:

• rustc_trait_selection/src/traits/select/mod.rs

1609:                    Where(obligation.predicate.rebind(substs.as_closure().upvar_tys().collect()))

Not an issue since we shallow_resolve first and check if the type is ty::Infer before calling upvar_tys()

• rustc_mir_build/src/thir/cx/expr.rs

394:                .zip(substs.upvar_tys())

Should be fine since call takes place in mir

• rustc_middle/src/ty/print/pretty.rs

667:                        self = self.comma_sep(substs.as_generator().upvar_tys())?;
708:                        self = self.comma_sep(substs.as_closure().upvar_tys())?;

Not an issue since we call is_valid on the generator and closure first.

• rustc_mir/src/borrow_check/type_check/mod.rs

752:                    return match substs.as_closure().upvar_tys().nth(field.index()) {
755:                            field_count: substs.as_closure().upvar_tys().count(),
1948:                match substs.as_closure().upvar_tys().nth(field_index) {
1951:                        field_count: substs.as_closure().upvar_tys().count(),

Should be fine since call takes place in mir

• rustc_mir/src/borrow_check/diagnostics/var_name.rs

38:            self.universal_regions().defining_ty.upvar_tys().position(|upvar_ty| {
47:        let upvar_ty = self.universal_regions().defining_ty.upvar_tys().nth(upvar_index);

Should be fine since call takes place in mir

• rustc_mir_build/src/build/mod.rs

827:            let upvar_tys = upvar_substs.upvar_tys();
828:            let upvars_with_tys = upvars.iter().zip(upvar_tys);

Should be fine since call takes place in mir

• rustc_middle/src/ty/layout.rs

612:                let tys = substs.as_closure().upvar_tys();

This could be bad, there doesn't seem to be any check for ty::Infer
EDIT: Aman found the check for ty::Infer, we are good here too

• rustc_mir/src/transform/generator.rs

1256:                    substs.upvar_tys().collect(),

Should be fine since call takes place in mir

• rustc_mir/src/util/elaborate_drops.rs

853:                let tys: Vec<_> = substs.as_closure().upvar_tys().collect();
863:                let tys: Vec<_> = substs.as_generator().upvar_tys().collect();

Should be fine since call takes place in mir

• rustc_codegen_llvm/src/debuginfo/metadata.rs

664:            let upvar_tys: Vec<_> = substs.as_closure().upvar_tys().collect();
669:                &upvar_tys,
677:            let upvar_tys: Vec<_> = substs
682:            prepare_enum_metadata(cx, t, def_id, unique_type_id, usage_site_span, upvar_tys)

Should be fine since call takes place in codegen

• rustc_traits/src/dropck_outlives.rs

220:                    &format!("upvar_tys for closure not found. Expected capture information for closure {}", ty,),
226:                for ty in substs.as_closure().upvar_tys() {
262:                    &format!("upvar_tys for generator not found. Expected capture information for generator {}", ty,),
270:                    .upvar_tys()

Not an issue since we call is_valid on the generator and closure first.

• rustc_ty/src/needs_drop.rs

99:                            for upvar_ty in substs.as_closure().upvar_tys() {
109:                            for upvar_ty in substs.upvar_tys() {

Not an issue since we call is_valid on the generator and closure first.

• rustc_mir/src/shim.rs

316:            builder.tuple_like_shim(dest, src, substs.as_closure().upvar_tys())

Should be fine since call takes place in mir

[arora-aman]

• rustc_middle/src/ty/layout.rs

612:                let tys = substs.as_closure().upvar_tys();

This is bad, no check for ty::Infer

https://github.com/rust-lang/rust/blob/7cf57288753b0fa35c3c0e7cad90c4737a4cd754/compiler/rustc_middle/src/ty/layout.rs#L612-L617

It already throws a bug if the type there is Infer and the way it's right now it would be somewhat better-explained error message "Using upvar_tys before capture analysis" or something along those lines)
https://github.com/rust-lang/rust/blob/7cf57288753b0fa35c3c0e7cad90c4737a4cd754/compiler/rustc_middle/src/ty/layout.rs#L1244-L1246

and redirecting to tuple might not be what we want since it will use MaybeSized
https://github.com/rust-lang/rust/blob/7cf57288753b0fa35c3c0e7cad90c4737a4cd754/compiler/rustc_middle/src/ty/layout.rs#L620-L630

[nikomatsakis]

Let me know when this is ready for review (latest commits seem good)

[arora-aman]

@nikomatsakis Right sorry forgot to ping you here.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[nikomatsakis]

I think this could be queue_type(substs.as_closure().tupled_upvars_ty()), no?

[nikomatsakis]

(same with generators)

[roxelo]

done

[roxelo]

@nikomatsakis Ready for review

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[bors]

☔ The latest upstream changes (presumably #78863) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[roxelo]

@nikomatsakis Merge conflict has been resolved

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[nikomatsakis]

@bors r+

[bors]

📌 Commit e35e46c has been approved by nikomatsakis

[bors]

⌛ Testing commit e35e46c with merge c4926d0...

[bors]

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing c4926d0 to master...

[rylev]

@arora-aman @nikomatsakis this change caused a moderate regression in the const eval stress test. Perhaps an investigation is warranted into why that is.