make MIR type checker handle a number of other cases #46582

nikomatsakis · 2017-12-08T10:03:57Z

The existing type checker was primarily used to verify types, but was skipping over a number of details. For example, it was not checking that the predicates on functions were satisfied and so forth. This meant that the NLL region checker was not getting a lot of the constraints it needed. This PR closes those gaps. It also includes a bit of refactoring for the way that we store region values, encapsulating the bit matrix over into its own module and improving the data structures in use.

This is mostly work by @spastorino being ported over from nll-master.

r? @arielb1 or @pnkfelix

bors · 2017-12-11T10:54:43Z

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

arielb1

see comments. Nice PR otherwise.

arielb1 · 2017-12-11T21:33:45Z

src/librustc/ty/context.rs

@@ -1746,6 +1746,27 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
        }))
    }

+    /// Create an unsafe fn ty based on a safe fn ty.


bad comment (talks about "unsafe fn ty" but takes a closure signature).

arielb1 · 2017-12-11T21:37:03Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

-        result.push_str("}");
-
-        result
+        inferred_values.region_value_str(r)
    }

    /// Indicates that the region variable `v` is live at the point `point`.


I would prefer more documentation about the sense of the boolean - e.g. "returns true if the region had changed"

arielb1 · 2017-12-11T21:41:04Z

src/librustc_mir/borrow_check/nll/universal_regions.rs

@@ -493,7 +493,27 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> {
                substs.substs


A comment describing what this function and defining_ty do would be welcome

Also, for the comment for TyClosure: weirdly enough, closure substs don't contain any extra regions, so identity_substs.regions().count() should be the same as substs.substs.regions().count().

arielb1 · 2017-12-11T21:51:03Z

src/librustc_mir/borrow_check/nll/universal_regions.rs

+            // types.  What is the best way to handle this? Should we
+            // be checking something other than the type of the def-id
+            // to figure out what to do (e.g. the def-key?).
+            ty::TyUint(..) => {


How does this code work with constants that don't have type uint? Doesn't it get called for them? Associated constants can have region parameters.

We might want to add the free region information to the MIR or to a query on the def id (computed by typeck::collect).

But sure, constants should have no inputs and 1 output.

It ICEs. I have later patches that do better.

arielb1 · 2017-12-11T22:08:27Z

src/librustc_mir/transform/type_check.rs

@@ -171,7 +174,42 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> {
        );

        let expected_ty = match constant.literal {
-            Literal::Value { value } => value.ty,
+            Literal::Value { value } => {
+                if let ConstVal::Function(def_id, ..) = value.val {


Yeah this is really the wrong way to go around doing things - we should be having the region relations from the unevaluated constant type, especially if it is an associated constant. value.val is already "too cooked".

arielb1 · 2017-12-12T14:14:15Z

src/librustc_mir/transform/type_check.rs

+
+                    let ty_fn_ptr_from = tcx.mk_fn_ptr(fn_sig);
+
+                    if let Err(terr) = self.eq_types(ty_fn_ptr_from, ty, location.at_self()) {


I think typeck allows some sort of (e.g. higher-ranked) subtyping here, so you'll need to allow it too.

e.g.

// this sig contains a binder fn foo<'a>(_x: &'a ()) {} fn bar<'b>() { foo as fn(&'b ()); // ... but this doesn't }

Hmm, that code type-checks. Perhaps because I haven't addressed previous comment yet.

arielb1 · 2017-12-12T14:19:39Z

src/test/compile-fail/mir_check_cast_reify.rs

+    // During NLL region analysis, this will get renumbered to `typeof(foo::<'?0>)`
+    // where `'?0` is a new region variable.
+    //
+    // (Note that if `'a` on `foo` were early-bound, the type would be


... but 'a is early-bound. Did you mean late-bound?

I did mean late-bound -- if it were late-bound, it wouldn't be part of the type of foo

nikomatsakis · 2017-12-13T00:49:05Z

OK @arielb1 pushed some comments to address those.

Otherwise, `run-pass/typeck-fn-to-unsafe-fn-ptr.rs` fails the MIR type checker.

nikomatsakis · 2017-12-13T11:23:24Z

rebased, fixed a few minor things relating to changes in the underlying borrowck leading to slightly different errors, but I kept the separate commits in place

arielb1 · 2017-12-13T17:16:22Z

Error message needs fixing:

[00:59:35] 
[00:59:35] ---- [compile-fail] compile-fail/nll/where_clauses_in_repeat_rvalue.rs stdout ----
[00:59:35] 	
[00:59:35] error: /checkout/src/test/compile-fail/nll/where_clauses_in_repeat_rvalue.rs:30: unexpected error: '30:17: 30:19: `x` does not live long enough [E0597]'
[00:59:35] 
[00:59:35] error: /checkout/src/test/compile-fail/nll/where_clauses_in_repeat_rvalue.rs:38: expected error not found: borrowed value does not live long enough [E0597]
[00:59:35] 
[00:59:35] error: 1 unexpected errors found, 1 expected errors not found
[00:59:35] status: exit code: 101
[00:59:35] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/nll/where_clauses_in_repeat_rvalue.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/nll/where_clauses_in_repeat_rvalue.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "borrowck=mir" "-Z" "nll" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/nll/where_clauses_in_repeat_rvalue.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
[00:59:35] unexpected errors (from JSON output): [
[00:59:35]     Error {
[00:59:35]         line_num: 30,
[00:59:35]         kind: Some(
[00:59:35]             Error
[00:59:35]         ),
[00:59:35]         msg: "30:17: 30:19: `x` does not live long enough [E0597]"
[00:59:35]     }
[00:59:35] ]
[00:59:35] 
[00:59:35] not found errors (from test file): [
[00:59:35]     Error {
[00:59:35]         line_num: 38,
[00:59:35]         kind: Some(
[00:59:35]             Error
[00:59:35]         ),
[00:59:35]         msg: "borrowed value does not live long enough [E0597]"
[00:59:35]     }
[00:59:35] ]
[00:59:35] 
[00:59:35] thread '[compile-fail] compile-fail/nll/where_clauses_in_repeat_rvalue.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:1163:12
[00:59:35] 
[00:59:35] 
[00:59:35] failures:
[00:59:35]     [compile-fail] compile-fail/nll/where_clauses_in_repeat_rvalue.rs
[00:59:35] 
[00:59:35] test result: �[31mFAILED�(B�[m. 2698 passed; 1 failed; 15 ignored; 0 measured; 0 filtered out
[00:59:35]

These tests had FIXMEs for errors that were not previously being reported.

Converting a `RegionElementIndex` to a `Location` is O(n) though could trivially be O(log n), but we don't do it that much anyhow -- just on error and debugging.

Plus an extra assertion.

nikomatsakis · 2017-12-13T17:25:49Z

@arielb1 pushed an update to that error msg...let's see what travis thinks

arielb1 · 2017-12-13T17:38:16Z

@bors r+

bors · 2017-12-13T17:38:16Z

📌 Commit 237dd41 has been approved by arielb1

bors · 2017-12-13T17:38:17Z

🌲 The tree is currently closed for pull requests below priority 2, this pull request will be tested once the tree is reopened

bors · 2017-12-14T04:32:22Z

⌛ Testing commit 237dd41 with merge f03e067...

@spastorino

…ielb1 make MIR type checker handle a number of other cases The existing type checker was primarily used to verify types, but was skipping over a number of details. For example, it was not checking that the predicates on functions were satisfied and so forth. This meant that the NLL region checker was not getting a lot of the constraints it needed. This PR closes those gaps. It also includes a bit of refactoring for the way that we store region values, encapsulating the bit matrix over into its own module and improving the data structures in use. This is mostly work by @spastorino being ported over from nll-master. r? @arielb1 or @pnkfelix

bors · 2017-12-14T07:05:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing f03e067 to master...

rust-highfive assigned arielb1 Dec 8, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2017

nikomatsakis force-pushed the nll-master-to-rust-master-4 branch from 2a0d380 to 3cf58dc Compare December 11, 2017 15:34

arielb1 suggested changes Dec 12, 2017

View reviewed changes

spastorino and others added 14 commits December 13, 2017 06:03

Check Repeat Rvalue

d6772cb

fix universal regions to handle constant expressions like [T; 22]

7f20b91

Check NullaryOp Rvalue

c915926

Check Aggregate predicates

5010496

Check functions predicates

688ab5a

Add more debug logs

4449240

Restructure a bit check_aggregate_rvalue code

8635548

Mir typeck Cast for ReifyFnPtr value

7d56131

Mir typeck Cast for UnsafeFnPtr value

900d4d5

Extract coerce_closure_fn_ty function

ae035cb

Mir typeck Cast for ClosureFnPtr value

14700e5

Mir typeck Cast for Unsize value

0c26d8f

normalize fn sig as part of reification

d5cff07

when reifying a safe fn as an unsafe fn ptr, insert two casts

decbbb3

Otherwise, `run-pass/typeck-fn-to-unsafe-fn-ptr.rs` fails the MIR type checker.

nikomatsakis force-pushed the nll-master-to-rust-master-4 branch 2 times, most recently from b192e0a to 2d853aa Compare December 13, 2017 11:22

nikomatsakis added 6 commits December 13, 2017 12:20

fix closure tests now that MIR typeck works properly

a30e225

These tests had FIXMEs for errors that were not previously being reported.

refactor region value bitmatrix

77663a6

change to use an O(1) data structure for looking up point indices

7a20a3f

Converting a `RegionElementIndex` to a `Location` is O(n) though could trivially be O(log n), but we don't do it that much anyhow -- just on error and debugging.

improve comments on safe_to_unsafe_fn_ty and coerce_closure_fn_ty

f6723a9

document return value of add_live_point

75ac071

comments for defining_ty and compute_indices

abd6d0d

Plus an extra assertion.

nikomatsakis added 2 commits December 13, 2017 12:20

add FIXME related to constant well-formedness

51847a1

correct comment in test

237dd41

nikomatsakis force-pushed the nll-master-to-rust-master-4 branch from 2d853aa to 237dd41 Compare December 13, 2017 17:25

arielb1 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 Dec 13, 2017

bors merged commit 237dd41 into rust-lang:master Dec 14, 2017

Deewiant mentioned this pull request Dec 22, 2017

ICE on pattern match against const &`static [u8] #46920

Closed

alexcrichton mentioned this pull request Jan 8, 2018

Significantly slower --emit=metadata #47267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make MIR type checker handle a number of other cases #46582

make MIR type checker handle a number of other cases #46582

nikomatsakis commented Dec 8, 2017

bors commented Dec 11, 2017

arielb1 left a comment •

edited

Loading

arielb1 Dec 11, 2017

arielb1 Dec 11, 2017

arielb1 Dec 11, 2017

arielb1 Dec 11, 2017

arielb1 Dec 11, 2017

nikomatsakis Dec 13, 2017

arielb1 Dec 11, 2017

arielb1 Dec 12, 2017

nikomatsakis Dec 13, 2017

arielb1 Dec 12, 2017

nikomatsakis Dec 13, 2017

nikomatsakis commented Dec 13, 2017

nikomatsakis commented Dec 13, 2017

arielb1 commented Dec 13, 2017

nikomatsakis commented Dec 13, 2017

arielb1 commented Dec 13, 2017

bors commented Dec 13, 2017

bors commented Dec 13, 2017

bors commented Dec 14, 2017

bors commented Dec 14, 2017

		@@ -493,7 +493,27 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> {
		substs.substs


		let ty_fn_ptr_from = tcx.mk_fn_ptr(fn_sig);

		if let Err(terr) = self.eq_types(ty_fn_ptr_from, ty, location.at_self()) {

make MIR type checker handle a number of other cases #46582

make MIR type checker handle a number of other cases #46582

Conversation

nikomatsakis commented Dec 8, 2017

bors commented Dec 11, 2017

arielb1 left a comment • edited Loading

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

Choose a reason for hiding this comment

nikomatsakis commented Dec 13, 2017

nikomatsakis commented Dec 13, 2017

arielb1 commented Dec 13, 2017

nikomatsakis commented Dec 13, 2017

arielb1 commented Dec 13, 2017

bors commented Dec 13, 2017

bors commented Dec 13, 2017

bors commented Dec 14, 2017

bors commented Dec 14, 2017

arielb1 left a comment •

edited

Loading