[NLL] Clean up handling of type annotations #57714

matthewjasper · 2019-01-17T21:41:36Z

Renames (Canonical)?UserTypeAnnotation -> (Canonical)?UserType so that the name CanonicalUserTypeAnnotation is free.
Keep the inferred type associated to user type annotations in the MIR, so that it can be compared against the annotated type, even when the annotated expression gets removed from the MIR. (NLL doesn't check that user type annotations are well-formed in unreachable code #54943)
Use the inferred type to allow infallible handling of user type projections (ICE when type checking partially annotated patterns #57531)
Uses revisions for the tests in region tests incorrectly passing with NLL enabled #56993
Check the types of Unevaluated constants with no annotations (figure out how to integrate constants and the MIR type checker #46702)
Some drive-by cleanup

Closes #46702
Closes #54943
Closes #57531
Closes #57731
cc #56993 leaving this open to track the underlying issue: we are not running tests with full NLL enabled on CI at the moment

r? @nikomatsakis

We want the name `CanonicalUserTypeAnnotation` for our own use.

We equate the type in the annotation with the inferred type first so that we have a fully inferred type to perform the well-formedness check on.

pnkfelix · 2019-01-25T12:09:08Z

src/librustc/traits/query/type_op/ascribe_user_type.rs

    ) -> Self {
-        Self { mir_ty, variance, def_id, user_substs, projs }
+        Self { mir_ty,  def_id, user_substs }


hmm interesting that you have found a way to get rid of carrying the projections around as part of the AscribeUserType.

pnkfelix · 2019-01-25T12:27:46Z

src/librustc_mir/hair/pattern/mod.rs

@@ -60,26 +60,29 @@ pub struct Pattern<'tcx> {

 #[derive(Clone, Debug)]
 pub struct PatternTypeProjection<'tcx> {


Should this be similarly renamed to just PatternType now that you've removed the projections?

pnkfelix · 2019-01-25T12:30:58Z

This looks really good to me. I'm sort of flabbergasted that I spent all that time figuring out how to factor out the projections and apply them later on in the compiler's control flow ... when I guess I should have been trying to make a change more like this...?

pnkfelix · 2019-01-25T12:33:00Z

we are not running tests with full NLL enabled on CI at the moment

I had forgotten about this detail.

If nothing else, that should be an argument for us to prioritize turning on NLL for all editions. Because right now we are leaving open a pretty big gap in our CI's testing.

pnkfelix · 2019-01-25T12:33:23Z

@bors r+

bors · 2019-01-25T12:33:24Z

📌 Commit 1593ac9 has been approved by pnkfelix

petrochenkov · 2019-01-25T13:20:32Z

@matthewjasper
Could you add a test for #57866 (from #57887)?

pnkfelix · 2019-01-25T14:12:48Z

we are not running tests with full NLL enabled on CI at the moment

I had forgotten about this detail.

If nothing else, that should be an argument for us to prioritize turning on NLL for all editions. Because right now we are leaving open a pretty big gap in our CI's testing.

Oh wait, I think I misunderstood this.

I thought that this was referring to the change to turn off compiletest's compare-mode by default (see #56391); but that didn't turn off NLL testing completely; it just restricted it to one builder.

After re-reading the comment, I think that it is in fact referring to the change introduced by PR #55134.

Which does still represent a gap in our testing.
But its not one that will be addressed by turning on NLL for all editions, at least not directly.
I suspect the way to go here might be to:
- Turn on borrowck=migrate by default for 2015 edition (as described in NLL: turn on borrowck=migrate by default on 2015 edition #57804)
- Once that is done, we can repurpose the compare mode testing to go back to testing -Z borrowck=mir -Z two-phase-borrows. (i.e. undo PR NLL: change compare-mode=nll to use borrowck=migrate #55134). That will hopefully ensure that we uncover any issues similar to region tests incorrectly passing with NLL enabled #56993 that are solely exposed when you go to full-blown NLL (and not just migrate mode).

pnkfelix · 2019-01-25T14:14:17Z

@petrochenkov I'll add that test now. (Maybe I'll change it to a // compile-pass test in drive by.)

pnkfelix · 2019-01-25T14:20:05Z

@bors r+

bors · 2019-01-25T14:20:06Z

📌 Commit 620a03f has been approved by pnkfelix

bors · 2019-01-25T14:25:45Z

⌛ Testing commit 620a03f with merge 0b1669d...

@nikomatsakis

[NLL] Clean up handling of type annotations * Renames (Canonical)?UserTypeAnnotation -> (Canonical)?UserType so that the name CanonicalUserTypeAnnotation is free. * Keep the inferred type associated to user type annotations in the MIR, so that it can be compared against the annotated type, even when the annotated expression gets removed from the MIR. (#54943) * Use the inferred type to allow infallible handling of user type projections (#57531) * Uses revisions for the tests in #56993 * Check the types of `Unevaluated` constants with no annotations (#46702) * Some drive-by cleanup Closes #46702 Closes #54943 Closes #57531 Closes #57731 cc #56993 leaving this open to track the underlying issue: we are not running tests with full NLL enabled on CI at the moment r? @nikomatsakis

bors · 2019-01-25T17:14:11Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: pnkfelix
Pushing 0b1669d to master...

rust-highfive assigned nikomatsakis Jan 17, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2019

matthewjasper added 5 commits January 19, 2019 15:15

Rename UserTypeAnnotation -> UserType

ab7bc3a

Rename CanonicalUserTypeAnnotation -> CanonicalUserType

5ca6bd5

We want the name `CanonicalUserTypeAnnotation` for our own use.

Use a struct for user type annotations

ed871cb

Handle lifetime annotations in unreachable code

65fe251

We equate the type in the annotation with the inferred type first so that we have a fully inferred type to perform the well-formedness check on.

Type check unnanotated constant items with NLL

c76e557

matthewjasper force-pushed the wellformed-unreachable branch from 040daa5 to 2cee682 Compare January 19, 2019 16:31

This comment has been minimized.

Sign in to view

Don't ignore _ in type casts and ascriptions

1593ac9

matthewjasper force-pushed the wellformed-unreachable branch from 2cee682 to 1593ac9 Compare January 19, 2019 19:33

pnkfelix mentioned this pull request Jan 21, 2019

NLL: turn on borrowck=migrate by default on 2015 edition #57804

Closed

pnkfelix reviewed Jan 25, 2019

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 Jan 25, 2019

matthewjasper mentioned this pull request Jan 25, 2019

Ignore Infer sty when relating MIR and user Ty #57887

Closed

Unit test from rust-lang#57866.

620a03f

bors merged commit 620a03f into rust-lang:master Jan 25, 2019

matthewjasper mentioned this pull request Jan 25, 2019

ICE with nested match with "Type Alias Enum Variant" feature #57866

Closed

matthewjasper deleted the wellformed-unreachable branch February 3, 2019 08:41

pnkfelix mentioned this pull request Mar 4, 2019

Compiler panic with associated constant #58435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Clean up handling of type annotations #57714

[NLL] Clean up handling of type annotations #57714

matthewjasper commented Jan 17, 2019 •

edited

Loading

This comment has been minimized.

pnkfelix Jan 25, 2019

pnkfelix Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

bors commented Jan 25, 2019

petrochenkov commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

bors commented Jan 25, 2019

bors commented Jan 25, 2019

bors commented Jan 25, 2019

		@@ -60,26 +60,29 @@ pub struct Pattern<'tcx> {

		#[derive(Clone, Debug)]
		pub struct PatternTypeProjection<'tcx> {

[NLL] Clean up handling of type annotations #57714

[NLL] Clean up handling of type annotations #57714

Conversation

matthewjasper commented Jan 17, 2019 • edited Loading

This comment has been minimized.

pnkfelix Jan 25, 2019

Choose a reason for hiding this comment

pnkfelix Jan 25, 2019

Choose a reason for hiding this comment

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

bors commented Jan 25, 2019

petrochenkov commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

pnkfelix commented Jan 25, 2019

bors commented Jan 25, 2019

bors commented Jan 25, 2019

bors commented Jan 25, 2019

matthewjasper commented Jan 17, 2019 •

edited

Loading