Mir tycheck aggregate rvalues #13

Nashenas88 · 2017-11-11T20:33:32Z

@nikomatsakis I still have a couple TODO comments left in there. What should I do for those error cases? Should I just place span_mirbug macro calls there like the other functions do?

…ot passing) test

nikomatsakis · 2017-11-11T20:34:33Z

src/librustc_mir/transform/type_check.rs

+        }
+    }
+
+    #[allow(dead_code)]


why dead_code?

I was writing check_rvalue before I was using it and was trying to check the types. This is cleaned up in the latest commit.

nikomatsakis · 2017-11-11T20:35:02Z

src/librustc_mir/transform/type_check.rs

+        let tcx = self.tcx();
+        match rv {
+            Rvalue::Aggregate(ref ak, ref ops) => {
+                match **ak {


Nit: you no longer need match **ak, thanks to #[feature(match_default_bindings)]

In the latest commit I still need to because this is a &Box rather than &&.

nikomatsakis · 2017-11-11T20:35:12Z

src/librustc_mir/transform/type_check.rs

+    fn check_rvalue(&mut self, mir: &Mir<'tcx>, rv: &Rvalue<'tcx>, location: Location) {
+        let tcx = self.tcx();
+        match rv {
+            Rvalue::Aggregate(ref ak, ref ops) => {


Nit: ref ought to be the default; we're matching on a reference

nikomatsakis · 2017-11-11T20:35:37Z

src/librustc_mir/transform/type_check.rs

+                                field_ty
+                            } else {
+                                // TODO(nashenas88) log span_mirbug terr??
+                                continue;


Yes, I think span_mirbug is appropriate here.

nikomatsakis · 2017-11-11T20:35:48Z

src/librustc_mir/transform/type_check.rs

+                                Operand::Constant(c) => c.ty,
+                            };
+                            if let Err(_terr) = self.sub_types(op_ty, field_ty, location.at_successor_within_block()) {
+                                // TODO(nashenas88) log span_mirbug terr??


Also here, span_mirbug seems good.

nikomatsakis · 2017-11-11T20:36:31Z

src/test/compile-fail/aggregate-rvalues-typeck.rs

+    let mut x = 22;
+    let wrapper = Wrap { w: &mut x };
+    //~^ ERROR cannot assign to `x` because it is borrowed (Mir) [E0506]
+    //~^^ ERROR cannot use `x` because it was mutably borrowed (Mir) [E0503]


Huh ,do we get errors here too? These seem bogus...

These are actually ignored by the test. I accidentally left them in from the run before I added in revisions. It seems with revisions only comments beginning with a revision tag are looked at. These are gone in the latest commit.

nikomatsakis · 2017-11-11T20:36:49Z

src/test/compile-fail/aggregate-rvalues-typeck.rs

+    x += 1; //[ast]~ ERROR cannot assign to `x` because it is borrowed [E0506]
+    //[mir]~^ ERROR cannot assign to `x` because it is borrowed (Ast) [E0506]
+    //[mir]~^^ ERROR cannot assign to `x` because it is borrowed (Mir) [E0506]
+    //[mir]~^^^ ERROR cannot use `x` because it was mutably borrowed (Mir) [E0503]


But these seem correct.

nikomatsakis · 2017-11-11T20:37:44Z

src/test/compile-fail/aggregate-rvalues-typeck.rs

+
+#![allow(unused_assignments)]
+
+struct Wrap<'a> { w: &'a mut u32 }


Let's rename this test to compile-fail/nll/reference-carried-through-struct-field.rs. Not sure what the best naming scheme is here, but at least in the nll directory would be good.

nikomatsakis

Left some requests for changes.

…span_mirbugs

Nashenas88 · 2017-11-11T21:17:55Z

src/librustc_mir/transform/type_check.rs

+                                Operand::Consume(lv) => lv.ty(mir, tcx).to_ty(tcx),
+                                Operand::Constant(c) => c.ty,
+                            };
+                            if let Err(terr) = self.sub_types(op_ty, field_ty, location.at_successor_within_block()) {


This line causes the follow error to occur during compilation:

Compiling core v0.0.0 (file:///home/paulf/rust2/src/libcore) error: internal compiler error: broken MIR in NodeId(258966) (str::pattern::CharSearcher::{{constructor}}(_1,)): <str::pattern::CharEqPattern<char> as str::pattern::Pattern>::Searcher is not a subtype of str::pattern::CharEqSearcher<'_, _>: Sorts(ExpectedFound { expected: str::pattern::CharEqSearcher<'_, _>, found: <str::pattern::CharEqPattern<char> as str::pattern::Pattern>::Searcher }) --> src/libcore/str/pattern.rs:418:1 | 418 | pub struct CharSearcher<'a>(<CharEqPattern<char> as Pattern<'a>>::Searcher); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: the compiler unexpectedly panicked. this is a bug. note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

Do I need to do additional work to get the types to match or is this a legitimate bug?

Nashenas88 · 2017-11-11T21:20:15Z

This is a fix for rust-lang#45889.

nikomatsakis · 2017-11-12T09:23:11Z

src/librustc_mir/transform/type_check.rs

                            };
                            let op_ty = match op {
-                                Operand::Consume(lv) => lv.ty(mir, tcx).to_ty(tcx),
+                                Operand::Consume(lv) => {
+                                    self.normalize(&lv.ty(mir, tcx), location).to_ty(tcx)


Very interesting. It's not obvious to me why this is necessary, though it doesn't seem bad per se.

OK, I see, it's needed only in the "shim MIR" that we construct -- e.g., the MIR that we build for the constructor of a type like struct Foo(X). In that code (librustc_mir/shim.rs), we are not presently normalizing associated types as I would expect.

I think we'll leave it like this but open a FIXME.

nikomatsakis · 2017-11-12T11:03:30Z

I limited normalizing of operand types to when we are within an ADT constructor. Interestingly, when running the tests, I found a problem in the type-checking of unions. I am now rebuilding so I can't readily reproduce it yet.

nikomatsakis · 2017-11-12T11:12:34Z

lunch-box. rustc --stage1 union-packed.rs  -Zdump-mir=all
error: internal compiler error: broken MIR in NodeId(61) (U { a: _1 }): [u8; 3] is not a subtype of u16: Sorts(ExpectedFound { expected: u16, found: [u8; 3] })
  --> union-packed.rs:39:15
   |
39 | const CU: U = U { b: [0, 0, 0] };
   |               ^^^^^^^^^^^^^^^^^^

nikomatsakis · 2017-11-12T11:13:28Z

Problem seems to be that for unions we don't remember the correct field number. If you look at the MIR, you'll see it assigns to field 0 of the union (a) but in fact we are initializing with field 1. This seems bogus to me.

nikomatsakis · 2017-11-12T11:16:11Z

We do it very explicitly on purpose though.

cc @petrochenkov -- can you shed any light on why we are mapping all union field indices to 0? It seems like we're losing important information from the MIR (what interpretation the user assigned to).

nikomatsakis · 2017-11-12T11:21:13Z

I'm referring to these lines here:

https://github.com/rust-lang/rust/blob/b087dedf3f85c4c90e27cb6e119b4da2712be5c9/src/librustc_mir/build/expr/as_rvalue.rs#L220-L221

nikomatsakis · 2017-11-12T11:22:36Z

Ah, I see now -- there is the active_field_index which stores the field we wanted.

nikomatsakis · 2017-11-12T13:05:44Z

Pushed a fix for unions. Running tests now, but it's looking good. I expect to merge shortly, then I think I need to rebase the nll-master branch.

nikomatsakis · 2017-11-12T13:36:56Z

Merged!

sync with rust-lang/rust

Nashenas88 added 2 commits November 11, 2017 13:37

Check rvalue aggregates during check_stmt in tycheck, add initial, (n…

b30ec6c

…ot passing) test

Fix failing test

31ef286

Nashenas88 changed the title ~~Mir tycheck aggregate rvalues for nll master~~ Mir tycheck aggregate rvalues Nov 11, 2017

nikomatsakis reviewed Nov 11, 2017

View reviewed changes

nikomatsakis requested changes Nov 11, 2017

View reviewed changes

Remove attributes and test comments accidentally left behind, add in …

ba0d264

…span_mirbugs

Nashenas88 commented Nov 11, 2017

View reviewed changes

Nashenas88 mentioned this pull request Nov 11, 2017

[NLL] extend MIR type checker to check aggregate rvalues rust-lang/rust#45889

Closed

Normalize LvalueTy for ops and format code to satisfy tidy check

1041211

nikomatsakis reviewed Nov 12, 2017

View reviewed changes

nikomatsakis approved these changes Nov 12, 2017

View reviewed changes

only normalize operand types when in an ADT constructor

658d4e3

nikomatsakis added 2 commits November 12, 2017 06:25

avoid early return

00444fc

handle the active field index in unions

ad44d7e

nikomatsakis merged commit ad44d7e into nikomatsakis:nll-master Nov 12, 2017

Nashenas88 deleted the mir-tycheck-aggregate-rvalues-for-nll-master branch November 12, 2017 14:20

nikomatsakis mentioned this pull request Nov 13, 2017

[NLL] check other sorts of rvalues beyond aggregates rust-lang/rust#45959

Closed

17 tasks

nikomatsakis pushed a commit that referenced this pull request Sep 12, 2019

Merge pull request #13 from rust-lang/master

d5ef9df

sync with rust-lang/rust

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mir tycheck aggregate rvalues #13

Mir tycheck aggregate rvalues #13

Nashenas88 commented Nov 11, 2017

nikomatsakis Nov 11, 2017

Nashenas88 Nov 11, 2017

nikomatsakis Nov 11, 2017

Nashenas88 Nov 11, 2017

nikomatsakis Nov 11, 2017

nikomatsakis Nov 11, 2017

nikomatsakis Nov 11, 2017

nikomatsakis Nov 11, 2017

Nashenas88 Nov 11, 2017

nikomatsakis Nov 11, 2017

nikomatsakis Nov 11, 2017

nikomatsakis left a comment

Nashenas88 Nov 11, 2017

Nashenas88 commented Nov 11, 2017

nikomatsakis Nov 12, 2017

nikomatsakis Nov 12, 2017

nikomatsakis Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017


		#![allow(unused_assignments)]

		struct Wrap<'a> { w: &'a mut u32 }

Mir tycheck aggregate rvalues #13

Mir tycheck aggregate rvalues #13

Conversation

Nashenas88 commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nashenas88 commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017