Adding E0623 for structs #43700

gaurikholkar-zz · 2017-08-06T17:15:32Z

This is a fix to #43275

The error message is

+error[E0623]: lifetime mismatch
+  --> $DIR/ex3-both-anon-regions-both-are-structs.rs:15:12
+   |
+14 | fn foo(mut x: Vec<Ref>, y: Ref) {
+   |                   ---      --- these structs are declared with different lifetimes...
+15 |     x.push(y);
+   |            ^ ...but data from `y` flows into `x` here
+
+error: aborting due to previous error

r? @nikomatsakis

kennytm · 2017-08-06T17:52:54Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+            return false;
+        };
+
+        if let Some(error_label) = self.process_anon_anon_conflict(first_is_struct, second_is_struct) {


This line has >100 chars.

GuillaumeGomez · 2017-08-06T18:02:19Z

~~You'll have a conflict with #43699. Change E0623 to E0624 please.~~

EDIT: woups, nevermind. I'm the one who's in the wrong here.

estebank · 2017-08-06T19:00:48Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+                                  is_arg2_struct: bool)
+                                  -> Option<(String)> {
+        let arg1_label = {
+            if is_arg1_struct && is_arg2_struct {


Nitpick: This is a case where using match might match can make the code cleaner and let you be assured that you're not missing a case (the let x = if already emits an error for missing returns, but match without matching on _ makes it clear that you didn't forget a case):

let arg1_label = match (is_arg1_struct, is_arg2_struct) { (true, true) => "these structs".to_string(), (true, false) => "the struct and reference".to_string(), (false, true) => "the reference and the struct".to_string(), (false, false) => "these references".to_string(), }

estebank · 2017-08-06T19:01:31Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+            } else if is_arg1_struct && !is_arg2_struct {
+                format!("the struct and reference")
+            } else if !is_arg1_struct && is_arg2_struct {
+                format!("the reference and the struct")


different wording vs line 120

estebank · 2017-08-06T19:02:34Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+                format!("these references")
+            }
+        };
+        Some(arg1_label)


There's no case where this method doesn't return Some, so you can change the method's signature to return String, instead of Option<String>.

estebank · 2017-08-06T19:03:56Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+                if subvisitor.found_it {
+                    self.is_struct = true;
+                    self.found_type = Some(arg);
+                } else {


Empty else clause, simply remove it.

estebank · 2017-08-06T19:04:16Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

            _ => {}
        }
-        // walk the embedded contents: e.g., if we are visiting `Vec<&Foo>`,
-        // go on to visit `&Foo`


Why removing comments?

estebank · 2017-08-06T19:07:03Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-3.stderr

@@ -1,10 +0,0 @@
-error[E0623]: lifetime mismatch


For other reviewers: ex3-both-anon-regions-3.rs was moved to ex3-both-anon-regions-one-is-struct-2.rs

gaurikholkar-zz · 2017-08-07T21:49:58Z

error[E0623]: lifetime mismatch
  --> $DIR/ex3-both-anon-regions-both-are-structs.rs:15:12
   |
14 | fn foo(mut x: Vec<Ref>, y: Ref) {
   |                   ---      --- these two types are declared with different lifetimes...
15 |     x.push(y);
   |            ^ ...but data from `y` flows into `x` here

error: aborting due to previous error

Generalized the error message after a discussion with @nikomatsakis. @estebank updated PR.

arielb1 · 2017-08-08T08:30:54Z

Maybe it would scale better to attach spans to anonymous lifetimes in resolve_lifetime?

gaurikholkar-zz · 2017-08-09T04:41:54Z

Fixed tidy checks

gaurikholkar-zz · 2017-08-11T05:52:41Z

@nikomatsakis @estebank @arielb1 @kennytm Needed help with writing examples for the cases where both the regions are anonymous and the anonymous region is in

1. self
2. return type
3. case of trait impls

I have made the code changes for this but the ui tests I have added don't trigger the error the way I thought it would.
My examples self and
return-type

As per the logs I generated, for the above to examples, they seem to return a false value at
this line it returns a false here i.e. for the function find_anon_type().
I also need to add NodeImplItem(it) and NodeTraitItem(it) checks here and I doubt that is causing a problem here. Also, for the case of trait impls, I have the following example which gives the same error as above(I'm hoping adding a NodeImplItem will fix that though)

trait Foo {
    fn foo<'a>(x: &mut Vec<&'a u8>, y: &'a u8);
}
impl Foo for () {

    fn foo(x: &mut Vec<&u8>, y: &u8) {
       x.push(y);
   }
}

EDIT - Turns out the report_anon_anon_conflict method was incomplete and didn't have checks for NodeImplItems and NodeTraitItems. Working now :)

estebank · 2017-08-11T17:20:30Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-self-is-anon.rs

@@ -0,0 +1,22 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:34Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-return-type-is-anon.rs

@@ -0,0 +1,22 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:38Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct.rs

@@ -0,0 +1,20 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:41Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-4.rs

@@ -0,0 +1,17 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:45Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-3.rs

@@ -0,0 +1,17 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:50Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-2.rs

@@ -0,0 +1,17 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:55Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-4.rs

@@ -0,0 +1,19 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:20:59Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-3.rs

@@ -0,0 +1,19 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T17:21:03Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-2.rs

@@ -0,0 +1,19 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update copyright year

estebank · 2017-08-11T19:00:37Z

src/librustc/infer/error_reporting/mod.rs

@@ -963,4 +963,4 @@ impl<'tcx> ObligationCause<'tcx> {
            _ => "types are compatible",
        }
    }
-}
+}


Revert trivial change to this file.

estebank · 2017-08-11T20:53:44Z

https://travis-ci.org/rust-lang/rust/jobs/263641387#L1240

[00:19:55] error: variable does not need to be mutable
[00:19:55]    --> /checkout/src/librustc/infer/error_reporting/anon_anon_conflict.rs:261:21
[00:19:55]     |
[00:19:55] 261 |                 let mut subvisitor = &mut TyPathVisitor {
[00:19:55]     |                     ^^^^^^^^^^^^^^
[00:19:55]     |
[00:19:55] note: lint level defined here
[00:19:55]    --> /checkout/src/librustc/lib.rs:23:9
[00:19:55]     |
[00:19:55] 23  | #![deny(warnings)]
[00:19:55]     |         ^^^^^^^^
[00:19:55]     = note: #[deny(unused_mut)] implied by #[deny(warnings)]
[00:19:55] 
[00:19:56] error: aborting due to previous error
[00:19:56] 
[00:19:56] error: Could not compile `rustc`.
[00:19:56]

kennytm · 2017-08-12T16:55:31Z

@gaurikholkar: the CI failure that @estebank mentioned still exists in 1ed03e7...

gaurikholkar-zz · 2017-08-13T11:06:42Z

@kennytm I changed it to let subvisitor = &mut TyPathVisitor { but it is still giving the same error. Also the variable is being used here - if subvisitor.found_it {. Doesn't it have to be mutable?

gaurikholkar-zz · 2017-08-13T11:08:48Z

@nikomatsakis Also, the E0623 is enabled for trait impls too. With the code changes, E0611 which was disabled for impls is enabled again, hence the ui output changed here Fixed this :D

kennytm · 2017-08-13T11:11:30Z

@gaurikholkar I don't see any error from Travis about 591b8f6 yet (it is still running), so no conclusion. Perhaps the error is pointing to other places.

You don't need to mark subvisitor as mut since you are not mutating subvisitor itself. Reading subvisitor.found_it is not a mutation.

kennytm · 2017-08-13T13:38:09Z

src/librustc/infer/error_reporting/named_anon_conflict.rs

-                 self.is_suitable_anonymous_region(sup).unwrap())
-            } else if sup.is_named_region() && self.is_suitable_anonymous_region(sub).is_some() {
+                 self.is_suitable_anonymous_region(sup, false).unwrap())
+            } else if sup.is_named_region() && self.is_suitable_anonymous_region(sub, false).is_some() {


This line is too long.

[00:03:12] tidy error: /checkout/src/librustc/infer/error_reporting/named_anon_conflict.rs:38: line longer than 100 chars

arielb1 · 2017-08-21T19:01:46Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-4.rs

@@ -0,0 +1,19 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT


duplicate test?

arielb1 · 2017-08-21T19:04:12Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-3.rs

@@ -0,0 +1,17 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT


duplicate test with previous (except x and y were replaced?)

arielb1 · 2017-08-21T19:05:25Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct.rs

@@ -0,0 +1,20 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT


duplicate test?

arielb1 · 2017-08-21T19:08:03Z

src/librustc/infer/error_reporting/util.rs

@@ -79,22 +106,26 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {

    // This method returns whether the given Region is Anonymous
    // and returns the DefId and the BoundRegion corresponding to the given region.
+    // The is_anon_anon is set true when we are dealing with cases where


Could you please store is_impl_item in FreeRegionInfo and branch on it instead of passing is_anon_anon as a parameter? It being a parameter is confusing.

arielb1 · 2017-08-21T19:10:05Z

src/librustc/infer/error_reporting/util.rs

-                        }
-                        Some(hir_map::NodeImplItem(..)) => {
+            if let ty::BrAnon(..) = free_region.bound_region {
+                let anonymous_region_binding_scope = free_region.scope;


Actually, this bunch of code only detects whether we are in an impl item. Could you extract it to an is_bound_region_in_impl_item function and call it from whenever its needed?

arielb1 · 2017-08-21T19:12:32Z

src/librustc/infer/error_reporting/util.rs

+    // corresponds to self and if yes, we display E0312.
+    // FIXME(#42700) - Need to format self properly to
+    // enable E0621 for it.
+    pub fn is_self_anon(&self, is_first: bool, scope_def_id: DefId) -> bool {


{ is_first && self.tcx .opt_associated_item(scope_def_id) .map(|i| i.method_has_self_argument) == Some(true) }

arielb1 · 2017-08-21T19:18:00Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+                                                                anon_reg_sub.boundregion);
+                if let (Some(anonarg_sup), Some(anonarg_sub)) =
+                    (self.find_anon_type(sup, &br_sup), self.find_anon_type(sub, &br_sub)) {
+                    (anonarg_sup, anonarg_sub, def_id_sup, def_id_sub, br_sup, br_sub)


style: you can return anon_reg_sup & anon_reg_sub.

@arielb1 Are you saying use anon_reg_sup instead of Some(anon_reg_sup). Can you elaborate on this please?

arielb1 · 2017-08-21T19:22:07Z

Ok by me modulo nits - I'll fix the binder trouble myself

gaurikholkar-zz · 2017-08-23T13:18:44Z

@arielb1 updated

nikomatsakis · 2017-08-23T19:57:55Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

+
+        let ty_sub = or_false!(self.find_anon_type(sub, &bregion_sub));
+
+        let (main_label, label1, label2) = if let (Some(sup_arg), Some(sub_arg)) =


maybe flatten these if let constraints with or_else! too? I hate to drag out this PR anymore though, so happy to do it as follow-up

I'll open up a separate PR for that

nikomatsakis · 2017-08-23T20:01:34Z

We decided to do more "flattening" as a separate PR.

nikomatsakis · 2017-08-23T20:01:43Z

@bors r+

bors · 2017-08-23T20:01:44Z

📌 Commit cb563a9 has been approved by nikomatsakis

kennytm · 2017-08-23T21:17:48Z

@nikomatsakis Needs re-r+ after the CI actually passes.

nikomatsakis · 2017-08-25T11:14:31Z

@bors r+

bors · 2017-08-25T11:14:32Z

📌 Commit 2cd1318 has been approved by nikomatsakis

bors · 2017-08-25T12:59:11Z

⌛ Testing commit 2cd1318 with merge a4d1149...

@nikomatsakis

Adding E0623 for structs This is a fix to #43275 The error message is ``` +error[E0623]: lifetime mismatch + --> $DIR/ex3-both-anon-regions-both-are-structs.rs:15:12 + | +14 | fn foo(mut x: Vec<Ref>, y: Ref) { + | --- --- these structs are declared with different lifetimes... +15 | x.push(y); + | ^ ...but data from `y` flows into `x` here + +error: aborting due to previous error ``` r? @nikomatsakis

bors · 2017-08-25T15:34:33Z

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

rust-highfive assigned nikomatsakis Aug 6, 2017

kennytm reviewed Aug 6, 2017

View reviewed changes

gaurikholkar-zz force-pushed the struct_lifetimes branch 2 times, most recently from 98189ad to 5cf4f2a Compare August 6, 2017 18:36

estebank requested changes Aug 6, 2017

View reviewed changes

gaurikholkar-zz force-pushed the struct_lifetimes branch from a96ac00 to 529baac Compare August 7, 2017 22:57

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

gaurikholkar-zz closed this Aug 9, 2017

gaurikholkar-zz reopened this Aug 9, 2017

gaurikholkar-zz force-pushed the struct_lifetimes branch from 5ef8486 to 6b08259 Compare August 9, 2017 12:48

gaurikholkar-zz force-pushed the struct_lifetimes branch 3 times, most recently from 11ddeb4 to 8da0aed Compare August 11, 2017 18:45

estebank reviewed Aug 11, 2017

View reviewed changes

gaurikholkar-zz force-pushed the struct_lifetimes branch from 8da0aed to 11ddeb4 Compare August 11, 2017 19:55

gaurikholkar-zz force-pushed the struct_lifetimes branch from f636e43 to 1ed03e7 Compare August 12, 2017 15:54

gaurikholkar-zz force-pushed the struct_lifetimes branch 2 times, most recently from 0239364 to 591b8f6 Compare August 13, 2017 11:05

gaurikholkar-zz force-pushed the struct_lifetimes branch from 591b8f6 to 58cfc83 Compare August 13, 2017 11:16

kennytm reviewed Aug 13, 2017

View reviewed changes

arielb1 reviewed Aug 21, 2017

View reviewed changes

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

code review fixes

90ab9d9

minor fix

b569094

gaurikholkar-zz force-pushed the struct_lifetimes branch from 151b5bb to 0f58e18 Compare August 23, 2017 19:55

nikomatsakis reviewed Aug 23, 2017

View reviewed changes

adding macro, cleaning up code

cb563a9

gaurikholkar-zz force-pushed the struct_lifetimes branch from 0f58e18 to cb563a9 Compare August 23, 2017 20:00

build fixes

2cd1318

nikomatsakis approved these changes Aug 23, 2017

View reviewed changes

bors merged commit 2cd1318 into rust-lang:master Aug 25, 2017

bors mentioned this pull request Aug 25, 2017

Extend E0623 for LateBound and EarlyBound Regions #44079

Merged

		@@ -0,0 +1,22 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

		@@ -0,0 +1,20 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

		@@ -0,0 +1,17 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

		@@ -0,0 +1,19 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

		@@ -0,0 +1,19 @@
		// Copyright 2017 The Rust Project Developers. See the COPYRIGHT


		let ty_sub = or_false!(self.find_anon_type(sub, &bregion_sub));

		let (main_label, label1, label2) = if let (Some(sup_arg), Some(sub_arg)) =

Adding E0623 for structs #43700

Adding E0623 for structs #43700

Conversation

gaurikholkar-zz commented Aug 6, 2017

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 6, 2017 • 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

gaurikholkar-zz commented Aug 7, 2017 • edited Loading

arielb1 commented Aug 8, 2017

gaurikholkar-zz commented Aug 9, 2017 • edited Loading

gaurikholkar-zz commented Aug 11, 2017 • 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

estebank commented Aug 11, 2017

kennytm commented Aug 12, 2017

gaurikholkar-zz commented Aug 13, 2017

gaurikholkar-zz commented Aug 13, 2017 • edited Loading

kennytm commented Aug 13, 2017 • edited Loading

kennytm Aug 13, 2017 • 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

gaurikholkar-zz Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 commented Aug 21, 2017

gaurikholkar-zz commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 23, 2017

nikomatsakis commented Aug 23, 2017

bors commented Aug 23, 2017

kennytm commented Aug 23, 2017

nikomatsakis commented Aug 25, 2017

bors commented Aug 25, 2017

bors commented Aug 25, 2017

bors commented Aug 25, 2017

GuillaumeGomez commented Aug 6, 2017 •

edited

Loading

gaurikholkar-zz commented Aug 7, 2017 •

edited

Loading

gaurikholkar-zz commented Aug 9, 2017 •

edited

Loading

gaurikholkar-zz commented Aug 11, 2017 •

edited

Loading

gaurikholkar-zz commented Aug 13, 2017 •

edited

Loading

kennytm commented Aug 13, 2017 •

edited

Loading

kennytm Aug 13, 2017 •

edited

Loading

gaurikholkar-zz Aug 23, 2017 •

edited

Loading