Add Borrow and Ownership structs and fields #5

Nashenas88 · 2017-06-04T21:16:08Z

Initial changes to support borrow/ownership visualization through rls.

Nashenas88 · 2017-06-04T21:18:47Z

src/lib.rs

+}
+
+#[derive(Debug, Clone, RustcDecodable, RustcEncodable)]
+pub enum CauseData {


I'm not sure if exposing the causes for loans and moves is a good idea. Would this be too coupled to compiler internals? The idea behind exposing these is that people would be able to ask "why" a loan or move occurred.

I would not, at least at first. I would start with a very simple system, then add stuff.

nrc · 2017-06-06T02:18:43Z

src/lib.rs

@@ -42,6 +42,7 @@ pub struct Analysis {
    pub refs: Vec<Ref>,
    pub macro_refs: Vec<MacroRef>,
    pub relations: Vec<Relation>,
+    pub borrows: Vec<Borrows>,


Why a vec of Borrows? What does a single Borrows represent?

Each Borrows object represents all of the borrows in a single function/closure. The vec represents the collection of borrows for all functions/closures in a crate. I could have also taken another approach and simply moved its internals (assignments, loans, moves) up, but this would have required more processing to be done within the compiler. I took the current approach because it was simplest based on how the borrow checker structures its data and required the least transformations.

Naming things is not my strong suit, so I'm very willing to change this name to something clearer, or restructuring the structs if that makes more sense.

OK, cool. Then maybe per_fn_borrows rather than just borrows for the field name? I think I prefer BorrowData rather than Borrows.

I like those names, I'll go with that. 👍

nrc · 2017-06-06T02:19:22Z

src/lib.rs

@@ -240,3 +242,59 @@ pub struct SigElement {
    pub start: usize,
    pub end: usize,
 }
+
+#[derive(Debug, Clone, RustcDecodable, RustcEncodable)]
+pub struct Borrows {


Could you document the data structures and fields please?

Sure thing. I'll have that in the next update.

nrc · 2017-06-06T02:19:47Z

src/lib.rs

+#[derive(Debug, Clone, RustcDecodable, RustcEncodable)]
+pub struct Borrows {
+    pub ref_id: Id,
+    pub assignments: Vec<Assignment>,


Why track assignments?

I use this to help display the "scope" (Niko's terminology) of the variable. The start of the scope is the low byte of the assignment's kill span (represented by just span here), and the end is either the high byte of the assignment span, or the high byte of span of the first move for this variable. I checked with @pnkfelix on irc some time ago to make sure this was valid, and he thought it matched existing compiler behavior.

Could we explicitly store that span, rather than compute it from assignments?

This exact span isn't stored anywhere. The borrow checking code does not hold on to those values during its passes. I spent quite some time making sure because I felt uncomfortable doing the computing myself. It was only after @pnkfelix reassured me that this matched the behavior that I was ok moving forward.

Though, I could do this computation early. I just thought of a way to do it more efficiently than sorting. My original impl filtered all moves by matching ref_id, then sorted by their span's lo byte, then just used the first item. I could instead fold all moves into a hashmap with the ref ids as keys, swapping the entry if the new move has a lower lo byte. This should run in O(n) time, and I already run in O(n) when mapping. I was initially worried about the cost of doing this for all assignments in a crate. If I do early computation it would also make this more compatible with the EndRegion changes for Mir borrow checking...

Ah so, you compute the 'scope span' on the fly and that depends on some assignments and some other data for a variable (loans?). So either we compute this in the compiler and pass the span explicitly, but don't need to pass assignments and moves, or we pass a load of data and compute in the tool, which gives a faster save-analysis? Do you have an idea for how long it will take to do this in the compiler? I would prefer to do the compiler option and plan to do this on demand in the near future, rather than do this in the RLS.

The 'scope span' depends on an assignment and the related moves. Loans are ignored for this purpose, we still try to visualize even non-compiling code (preparing for an idea where this gets used to display compilation errors). Then I wouldn't need to send the full assignment, just the span with the matching ref_id. The moves are still needed because those are visualized (in non-compiling code where someone attempted to move twice).

I'm not sure how long this will take in the compiler, but the more I think about it the more I think it won't be a big burden (unless maybe we're talking about something like servo... that's where I really become unsure). I think there'd be on average (just guessing, no data) a 1:1 ratio of assignments to moves (maybe more as not all assignments have moves). I don't expect high move to assignment ratio because that would mean a lot of non-compiling code 😄 . Given that, I think it's not a lot of additional cost to do what I mentioned above (even sorting might be simple if we only have 2-3 moves in non-compiling code). I can add timing checks to see how long it takes to transform early in something like servo. If it's trivial I'll make it a permanent change.

nrc · 2017-06-06T02:20:39Z

src/lib.rs

+}
+
+#[derive(Debug, Clone, RustcDecodable, RustcEncodable)]
+pub enum CauseData {


I would not, at least at first. I would start with a very simple system, then add stuff.

nrc · 2017-06-06T02:21:15Z

src/lib.rs

+
+#[derive(Debug, Clone, RustcDecodable, RustcEncodable)]
+pub struct Loan {
+    pub ref_id: Id,


What do the ref_ids point to?

They point to the original assignment that is being borrowed from. It's like the def in goto_def, but for borrows and moves.

They may not be as necessary as I thought. I'll have to follow up on the ide extension prototypes to see if I needed to use those, or if that was just something needed for the original filtering. If we don't keep the Borrows struct, and only maintain the loans, moves, etc. in the analysis struct, then they will be necessary because that's what will be used to find the matching loans, moves, etc given the def_id for a span. Based on this, should I have named them def_id instead?

I'm not sure what "original assignment" means. Is it where the variable is declared? I think it can't be since it is values that are borrowed, not variables? Could you give an example please?

Sorry, I am responding while too tired and was also thinking about the front-end implementation too much. I had intended to mean the variable declaration since the prototypes only tracked borrows and moves from a variable declaration selected by the cursor. I hadn't attempted to show borrows of borrows yet (mostly because I hadn't figured out how to cleanly visualize that). Here the ref_ids point to the value that is being borrowed. I'm about to head to bed so I'll add examples tomorrow morning (US Eastern morning).

Nashenas88 · 2017-06-06T12:29:06Z

Looks like I'll be quite busy tonight. I'm going to get to work on the changes tomorrow evening instead.

Nashenas88 · 2017-06-07T03:56:36Z

So trying to incorporate the latest changes into the compiler, and I'm getting:

   Compiling rustc_save_analysis v0.0.0 (file:///Users/paul/programming/rust_borrow_save/src/librustc_save_analysis)
error[E0560]: struct `rls_data::Signature` has no field named `span`
   --> src/librustc_save_analysis/json_dumper.rs:490:13
    |
490 |             span: self.span,
    |             ^^^^^ `rls_data::Signature` does not have this field

error[E0560]: struct `rls_data::Signature` has no field named `ident_start`
   --> src/librustc_save_analysis/json_dumper.rs:492:13
    |
492 |             ident_start: self.ident_start,
    |             ^^^^^^^^^^^^ `rls_data::Signature` does not have this field

error[E0560]: struct `rls_data::Signature` has no field named `ident_end`
   --> src/librustc_save_analysis/json_dumper.rs:493:13
    |
493 |             ident_end: self.ident_end,
    |             ^^^^^^^^^^ `rls_data::Signature` does not have this field

Is the compiler not on the latest rls-data version?

nrc · 2017-06-07T23:02:36Z

It is not. This PR puts it on the latest version - rust-lang/rust#42471. Hopefully it will land soon.

…w 'borrows' feature flag

Nashenas88 · 2017-06-17T14:06:18Z

I believe I addressed all concerns in the comments. Switching to track scopes rather than assignments was actually not a lot of work (in the compiler). Outside of what was mentioned I also added a borrows feature flag so when this does eventually get in, no one will have to wait for the rest of the changes to go into the compiler before advancing the lib. I'll try actually opening the compiler PR soon since I have a really good feeling it will need a lot of cleanup / rework.

nrc · 2017-06-18T03:36:05Z

Cool, thanks for the changes!

Nashenas88 · 2017-06-18T13:03:38Z

Should I have bumped the crate version for this?

nrc · 2017-06-18T22:36:56Z

Should I have bumped the crate version for this?

Yes, basically any change is a breaking change here because we use the data structures with transmute

nrc · 2017-06-18T22:44:07Z

Sorry, to clarify, the crate version needs bumping to publish, you don't need to do it, I'm happy to do so (I thought you had, but I mistook cargo.lock for cargo.toml). tl;dr you can bump if you like, it doesn't matter either way :-)

And I just published v0.7 with these changes.

Add Borrow and Ownership structs and fields

737b449

Nashenas88 commented Jun 4, 2017

View reviewed changes

nrc reviewed Jun 6, 2017

View reviewed changes

Removed cause structs, documented borrow types and hid them behind ne…

6507f37

…w 'borrows' feature flag

nrc merged commit 0cf8072 into rust-dev-tools:master Jun 18, 2017

Add Borrow and Ownership structs and fields #5

Add Borrow and Ownership structs and fields #5

Uh oh!

Conversation

Nashenas88 commented Jun 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nashenas88 commented Jun 6, 2017

Uh oh!

Nashenas88 commented Jun 7, 2017

Uh oh!

nrc commented Jun 7, 2017

Uh oh!

Nashenas88 commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrc commented Jun 18, 2017

Uh oh!

Nashenas88 commented Jun 18, 2017

Uh oh!

nrc commented Jun 18, 2017

Uh oh!

nrc commented Jun 18, 2017

Uh oh!

Uh oh!

Nashenas88 commented Jun 17, 2017 •

edited

Loading