add notes to report_conflicting_borrow MIR borrowck #44882

mikhail-m1 · 2017-09-27T07:24:49Z

rust-highfive · 2017-09-27T07:24:55Z

(rust_highfive has picked a reviewer for you, use r? to override)

arielb1 · 2017-09-27T11:06:26Z

src/librustc_mir/borrow_check.rs

+
+        let issued_span = self.retrieve_borrow_span(issued_borrow);
+
+        let issued_end_loan_span = if let Lvalue::Local(local) = issued_borrow.lvalue {


That's not how you do it. You should look up the span of the StatementKind::EndRegion for the borrow region, which should be easily available by looking at the MIR.

I've spent some time trying to do this, but StatementKind::EndRegion contains region::Scope, and I cannot get span from it.

impl Scope { pub fn span(&self, tcx: TyCtxt, scope_tree: &ScopeTree)

Where should I get ScopeTree? I found MIR build already do this and resided to use it result.

@mikhail-m1 I think the idea is that the EndRegion itself is a Statement, and every Statement carries a SourceInfo which in turn carries a Span.

arielb1 · 2017-09-27T11:09:05Z

src/librustc_borrowck/borrowck/check_loans.rs

+                        new_loan.span, &nl, &new_loan_msg, old_loan.span, &old_loan_msg,
+                        previous_end_span, Origin::Ast),
+                (ty::UniqueImmBorrow, ty::UniqueImmBorrow) =>
+                    self.bccx.cannot_uniquely_borrow_by_two_closures(


I'm not sure how "unique" became related to closures. I mean, unique implies closure but not vice-versa (closures can also borrow immutably). But that's pre-existing.

arielb1

generally LGTM, but you should get the region-end span the right way - I think you should create a Region -> span-of-endregion map by indexing the MIR, and look the region up in that.

arielb1 · 2017-09-27T11:14:45Z

src/librustc_mir/borrow_check.rs

        // FIXME: obviously falsifiable. Generalize for non-eq lvalues later.
-        assert_eq!(loan1_lvalue, loan2_lvalue);
+        assert_eq!(lvalue, &issued_borrow.lvalue);


Could you add a message to this assertions, e.g.:

assert_eq!(lvalue, &issued_borrow.lvalue, "complex conflicts are not supported");

pnkfelix · 2017-10-02T11:52:57Z

src/librustc_mir/dataflow/impls/borrows.rs

+                               statement: &mir::Statement<'tcx>,
+                               location: Location) {
+                if let mir::StatementKind::EndRegion(region_scope) = statement.kind {
+                    error!("statement {:?} {:?}", statement, statement.source_info.span);


Is this supposed to be in here? Or was this instrumentation you put in but was supposed to be removed before merging?

pnkfelix · 2017-10-02T11:55:27Z

src/librustc_mir/dataflow/impls/borrows.rs

+                               location: Location) {
+                if let mir::StatementKind::EndRegion(region_scope) = statement.kind {
+                    error!("statement {:?} {:?}", statement, statement.source_info.span);
+                    if let Some(span) = self.region_span_map.insert(ReScope(region_scope), statement.source_info.span) {


I think the tidy pass is complaining about this line being too long. (We have a hard 100-character limit on line length.)

pnkfelix · 2017-10-03T12:18:25Z

@bors r+

bors · 2017-10-03T12:18:26Z

📌 Commit a9eb8e7 has been approved by pnkfelix

pnkfelix · 2017-10-03T12:27:03Z

src/librustc_mir/borrow_check.rs

@@ -396,26 +396,31 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx>
                            ReadKind::Copy =>
                                this.report_use_while_mutably_borrowed(
                                    context, lvalue_span, borrow),
-                            ReadKind::Borrow(bk) =>
+                            ReadKind::Borrow(bk) =>{


nit: coding style in project is to put a single space between the => and the { in a match arm

pnkfelix · 2017-10-03T12:27:47Z

src/librustc_mir/borrow_check.rs

-                                    context, lvalue_span,
-                                    (lvalue_span.0, bk), (&borrow.lvalue, borrow.kind)),
+                                    context, lvalue_span, bk, &borrow, end_issued_loan_span)
+                            },


nit: coding style in project is to leave out the trailing comma when a match arm's code is delimited by curly braces

pnkfelix · 2017-10-03T12:28:28Z

src/librustc_mir/borrow_check.rs

                                this.report_conflicting_borrow(
-                                    context, lvalue_span,
-                                    (lvalue_span.0, bk), (&borrow.lvalue, borrow.kind)),
+                                    context, lvalue_span, bk, &borrow, end_issued_loan_span)},


nit: coding style in project is to both omit the trailing comma here (see earlier comment) and also put a line break before the ending }

pnkfelix · 2017-10-03T12:37:19Z

@bors r+

bors · 2017-10-03T12:37:20Z

📌 Commit c0ca0f9 has been approved by pnkfelix

bors · 2017-10-03T14:44:16Z

⌛ Testing commit c0ca0f906116b46cbeb341b8097174d60ca175da with merge 8c59b0f38daa077d520d78ff37629e103c37e9ea...

bors · 2017-10-03T16:42:24Z

💔 Test failed - status-travis

mikhail-m1 · 2017-10-03T19:03:25Z

looks like some bug with powerpc64 build
ERROR: Could not find a valid gem 'aws-sdk-resources' (= 2.10.57) in any repository

bors · 2017-10-04T05:46:07Z

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

pnkfelix · 2017-10-04T09:27:32Z

@bors r+

bors · 2017-10-04T09:27:33Z

📌 Commit c68b10f has been approved by pnkfelix

bors · 2017-10-04T11:26:19Z

⌛ Testing commit c68b10f with merge fd8099f...

add notes to report_conflicting_borrow MIR borrowck part of #44596

bors · 2017-10-04T13:53:53Z

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

rust-highfive assigned arielb1 Sep 27, 2017

arielb1 reviewed Sep 27, 2017

View reviewed changes

arielb1 suggested changes Sep 27, 2017

View reviewed changes

zilbuz mentioned this pull request Sep 27, 2017

MIR-borrowck: Adding notes to E0506 #44811

Merged

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 28, 2017

pnkfelix reviewed Oct 2, 2017

View reviewed changes

mikhail-m1 force-pushed the master branch from 5bcd84b to a9eb8e7 Compare October 3, 2017 08:45

pnkfelix reviewed Oct 3, 2017

View reviewed changes

pnkfelix mentioned this pull request Oct 3, 2017

Overlapping borrows can point to different lvalues. #44999

Merged

mikhail-m1 force-pushed the master branch 2 times, most recently from e052df4 to 24489e3 Compare October 4, 2017 07:17

add notes to report_conflicting_borrow MIR borrowck

c68b10f

mikhail-m1 force-pushed the master branch from 24489e3 to c68b10f Compare October 4, 2017 07:36

pnkfelix added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 4, 2017

pnkfelix removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 4, 2017

bors added a commit that referenced this pull request Oct 4, 2017

Auto merge of #44882 - mikhail-m1:master, r=pnkfelix

fd8099f

add notes to report_conflicting_borrow MIR borrowck part of #44596

bors merged commit c68b10f into rust-lang:master Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add notes to report_conflicting_borrow MIR borrowck #44882

add notes to report_conflicting_borrow MIR borrowck #44882

mikhail-m1 commented Sep 27, 2017 •

edited

Loading

rust-highfive commented Sep 27, 2017

arielb1 Sep 27, 2017 •

edited

Loading

mikhail-m1 Sep 27, 2017 •

edited

Loading

pnkfelix Sep 27, 2017

arielb1 Sep 27, 2017

arielb1 left a comment

arielb1 Sep 27, 2017

pnkfelix Oct 2, 2017

pnkfelix Oct 2, 2017

pnkfelix commented Oct 3, 2017

bors commented Oct 3, 2017

pnkfelix Oct 3, 2017

pnkfelix Oct 3, 2017

pnkfelix Oct 3, 2017

pnkfelix commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 3, 2017

mikhail-m1 commented Oct 3, 2017 •

edited

Loading

bors commented Oct 4, 2017

pnkfelix commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 4, 2017


		let issued_span = self.retrieve_borrow_span(issued_borrow);

		let issued_end_loan_span = if let Lvalue::Local(local) = issued_borrow.lvalue {

add notes to report_conflicting_borrow MIR borrowck #44882

add notes to report_conflicting_borrow MIR borrowck #44882

Conversation

mikhail-m1 commented Sep 27, 2017 • edited Loading

rust-highfive commented Sep 27, 2017

arielb1 Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

mikhail-m1 Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 left a 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

pnkfelix commented Oct 3, 2017

bors commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 3, 2017

mikhail-m1 commented Oct 3, 2017 • edited Loading

bors commented Oct 4, 2017

pnkfelix commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 4, 2017

mikhail-m1 commented Sep 27, 2017 •

edited

Loading

arielb1 Sep 27, 2017 •

edited

Loading

mikhail-m1 Sep 27, 2017 •

edited

Loading

mikhail-m1 commented Oct 3, 2017 •

edited

Loading