Remove extra call to upvar_tys #78725

arora-aman · 2020-11-04T10:00:58Z

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

Fixes #78720

r? @ghost

arora-aman · 2020-11-04T10:01:09Z

Running tests

arora-aman · 2020-11-04T10:45:41Z

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 · 2020-11-04T13:47:04Z

@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 · 2020-11-13T02:17:52Z

compiler/rustc_middle/src/ty/layout.rs


                    // Discriminant field for enums (where applicable).
                    Variants::Multiple { ref tag, .. } => {
                        assert_eq!(i, 0);
-                        return tag_layout(tag);
+                        return Err(tag_layout(tag));


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

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

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

arora-aman · 2020-11-13T02:19:44Z

compiler/rustc_middle/src/ty/layout.rs

+        cx: &C,
+        i: usize,
+        ty: C::Ty,
+    ) -> Result<C::Ty, C::TyAndLayout> {


I'd probably add an enum which is

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

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

compiler/rustc_middle/src/ty/layout.rs

compiler/rustc_target/src/abi/mod.rs

compiler/rustc_middle/src/ty/layout.rs

roxelo · 2020-11-17T00:10:31Z

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 · 2020-11-17T00:33:33Z

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 · 2020-11-17T23:19:25Z

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

arora-aman · 2020-11-18T00:42:11Z

@nikomatsakis Right sorry forgot to ping you here.

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

nikomatsakis · 2020-11-19T13:44:29Z

compiler/rustc_ty/src/needs_drop.rs

-                        for upvar_ty in substs.as_closure().upvar_tys() {
-                            queue_type(self, upvar_ty);
+                        // Ensure that the upvar tys has been inferred
+                        if substs.as_closure().is_valid() {


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

(same with generators)

roxelo · 2020-11-21T22:32:25Z

@nikomatsakis Ready for review

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

bors · 2020-11-29T11:55:47Z

☔ 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

Fixes rust-lang#78720

roxelo · 2020-11-30T22:17:17Z

@nikomatsakis Merge conflict has been resolved

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

nikomatsakis · 2020-11-30T23:19:19Z

@bors r+

bors · 2020-11-30T23:19:20Z

📌 Commit e35e46c has been approved by nikomatsakis

bors · 2020-12-01T07:00:47Z

⌛ Testing commit e35e46c with merge c4926d0...

bors · 2020-12-01T09:29:31Z

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

rylev · 2020-12-03T12:24:33Z

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

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-closures Area: Closures (`|…| { … }`) labels Nov 4, 2020

nikomatsakis approved these changes Nov 4, 2020

View reviewed changes

arora-aman force-pushed the fix-78720 branch from 8e58641 to 7cf5728 Compare November 12, 2020 07:43

arora-aman commented Nov 13, 2020

View reviewed changes

nikomatsakis reviewed Nov 13, 2020

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

compiler/rustc_target/src/abi/mod.rs Outdated Show resolved Hide resolved

nikomatsakis reviewed Nov 16, 2020

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2020

nikomatsakis requested changes Nov 19, 2020

View reviewed changes

roxelo force-pushed the fix-78720 branch 3 times, most recently from c0efdd4 to 589fa91 Compare November 21, 2020 22:31

arora-aman added 2 commits November 29, 2020 19:11

Remove extra call to upvar_tys

5da2bf1

Fixes rust-lang#78720

Be cautious of calling upvar_tys before mir

e35e46c

roxelo force-pushed the fix-78720 branch from 589fa91 to e35e46c Compare November 30, 2020 04:50

nikomatsakis approved these changes Nov 30, 2020

View reviewed changes

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 Nov 30, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2020

bors merged commit c4926d0 into rust-lang:master Dec 1, 2020

rustbot added this to the 1.50.0 milestone Dec 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove extra call to upvar_tys #78725

Remove extra call to upvar_tys #78725

arora-aman commented Nov 4, 2020 •

edited

Loading

arora-aman commented Nov 4, 2020

arora-aman commented Nov 4, 2020

nikomatsakis commented Nov 4, 2020

arora-aman Nov 13, 2020

roxelo Nov 13, 2020

roxelo Nov 13, 2020

arora-aman Nov 13, 2020 •

edited

Loading

roxelo Nov 13, 2020

roxelo commented Nov 17, 2020 •

edited

Loading

arora-aman commented Nov 17, 2020 •

edited

Loading

nikomatsakis commented Nov 17, 2020

arora-aman commented Nov 18, 2020

nikomatsakis Nov 19, 2020

nikomatsakis Nov 19, 2020

roxelo Nov 20, 2020

roxelo commented Nov 21, 2020

bors commented Nov 29, 2020

roxelo commented Nov 30, 2020

nikomatsakis commented Nov 30, 2020

bors commented Nov 30, 2020

bors commented Dec 1, 2020

bors commented Dec 1, 2020

rylev commented Dec 3, 2020

Remove extra call to upvar_tys #78725

Remove extra call to upvar_tys #78725

Conversation

arora-aman commented Nov 4, 2020 • edited Loading

arora-aman commented Nov 4, 2020

arora-aman commented Nov 4, 2020

nikomatsakis commented Nov 4, 2020

arora-aman Nov 13, 2020

Choose a reason for hiding this comment

roxelo Nov 13, 2020

Choose a reason for hiding this comment

roxelo Nov 13, 2020

Choose a reason for hiding this comment

arora-aman Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

roxelo Nov 13, 2020

Choose a reason for hiding this comment

roxelo commented Nov 17, 2020 • edited Loading

arora-aman commented Nov 17, 2020 • edited Loading

nikomatsakis commented Nov 17, 2020

arora-aman commented Nov 18, 2020

nikomatsakis Nov 19, 2020

Choose a reason for hiding this comment

nikomatsakis Nov 19, 2020

Choose a reason for hiding this comment

roxelo Nov 20, 2020

Choose a reason for hiding this comment

roxelo commented Nov 21, 2020

bors commented Nov 29, 2020

roxelo commented Nov 30, 2020

nikomatsakis commented Nov 30, 2020

bors commented Nov 30, 2020

bors commented Dec 1, 2020

bors commented Dec 1, 2020

rylev commented Dec 3, 2020

arora-aman commented Nov 4, 2020 •

edited

Loading

arora-aman Nov 13, 2020 •

edited

Loading

roxelo commented Nov 17, 2020 •

edited

Loading

arora-aman commented Nov 17, 2020 •

edited

Loading