Add structs for borrow and ownership information #71

Nashenas88 · 2017-06-18T14:30:55Z

Note: This can't be merged until the latest changes to rls-data are pushed to crates.io and this PR's Cargo.toml is updated.

Nashenas88 · 2017-06-18T14:31:26Z

Cargo.toml

@@ -10,6 +10,6 @@ categories = ["development-tools"]
 [dependencies]
 rustc-serialize = "0.3"
 log = "0.3"
-rls-data = "= 0.4.1"
+rls-data = { path = "../rls-data", default-features = true, features = ["borrows"] }


This needs to be updated once rls-data is updated in crates.io.

v0.7 is now published, but I think we need to upgrade rustc to use that version before we can make rls-analysis use that version.

Is it necessary to specify default-features = true?

I misread the docs the first time. I thought it was necessary when overriding the features list, but it seems to not be the case. And yes, this is dependent on both rustc and rls-data. Did I mention that incorrectly in the rust-lang/rust PR?

Nashenas88 · 2017-06-18T14:33:19Z

src/lib.rs

+                for (_, b) in c.per_fn_borrows.iter() {
+                    // If we find a `BorrowData` where there's a matching scope, then filter out
+                    // out matching items.
+                    if b.scopes.iter().any(|a| a.ref_id == id) {


I built BorrowData around the idea that I could quickly figure out the fn item wrapping a span. I couldn't figure out how to do that just yet. Any ideas? If it's not something that's going to be supported, I'll change per_fn_borrows to be a Vec rather than a HashMap.

Hmm, that is not something that is easy to do (at the moment), I think. We could save a 'map' of spans to function ids, then do some kind of sub-span check to get the function id. I think that should be relatively easy if the 'map' is the correct data structure (maybe just an ordered Vec is fine, but building it would be easier if it were some kind of tree) and I think the memory overhead wouldn't be too high.

I think I have a working implementation with BTreeMap. I'll clean it up and update the PR for review after work (~10 hours from now). The implementation is ... interesting? I'll add a lot of comments to explain what's going on.

Nashenas88 · 2017-06-18T14:33:49Z

src/lib.rs

@@ -610,6 +635,39 @@ pub struct Glob {
    pub value: String,
 }

+#[derive(Debug, Clone)]
+pub struct BorrowData {


Should I toggle all of this behind a feature flag like I did in rls-data?

no, I don't think it is necessary

Nashenas88 · 2017-06-18T14:34:55Z

src/lowering.rs

+                }
+            };
+
+            let scopes = b.scopes.into_iter().filter_map(|a|


This section seems a bit repetitive. Is there a simple way to clean this up, or do you think we need to worry about logging here?

Also, I was using into_iter here to avoid cloneing everything. Is that a bad idea?

Also, I was using into_iter here to avoid cloneing everything. Is that a bad idea?

No, that is a good thing to do

An easy refactoring would be to factor out lower functions to map rls-data structs to rls-analysis ones. A better, but trickier one would be to make a Lower trait and then use macros 1.1 to derive that for each of the data types.

Nashenas88 · 2017-06-22T02:32:47Z

src/lib.rs

@@ -569,6 +613,8 @@ pub struct PerCrateAnalysis {
    ref_spans: HashMap<Id, Vec<Span>>,
    globs: HashMap<Span, Glob>,
    impls: HashMap<Id, Vec<Span>>,
+    per_fn_borrows: HashMap<Id, BorrowData>,
+    fn_span_lookup_tree: BTreeMap<SearchSpan, Id>,


I had an alternative design where rather than storing SearchSpan here, I stored a type called InTreeSearchSpan, which was just a wrapper around SearchSpan(some_span, SpanKind::InTree). Though in that case, I had to implement Borrow<SearchSpan> as well as all the Eq and Ord traits. It's safer type-wise since one wouldn't be able to accidentally store the SpanKind's Start and End values like you can now. However, I wasn't sure if it's worth the extra maintenance cost since the usage is so simple and easy to verify for correctness. Let me know if you'd like me to add it back in (assuming this SearchSpan and BTreeMap combination is something you're ok with).

It's safer type-wise since one wouldn't be able to accidentally store the SpanKind's Start and End values like you can now

Is this invariant enforced dynamically in the current system?

It's not really enforced at all. The only reason it's working is because I only insert SearchSpans of kind InTreeinto the BTreeMap. I'll add it back in since that will prevent any errors in the future when someone else tries to modify the code.

nrc · 2017-06-22T20:24:10Z

src/lib.rs

@@ -26,12 +26,17 @@ mod test;

 pub use self::raw::{Target, name_space_for_def_kind, read_analyis_incremental};

+mod search_span;
+use search_span::*;


could you use explicit imports instead of a glob please

nrc · 2017-06-22T20:24:46Z

src/search_span.rs

+use std::ops::Range;
+use span;
+
+pub type Span = span::Span<span::ZeroIndexed>;


I would put this at the top level

nrc · 2017-06-22T20:25:27Z

src/search_span.rs

+
+pub type Span = span::Span<span::ZeroIndexed>;
+
+#[derive(Debug)]


Could you add a module comment explaining how to use this module?

nrc · 2017-06-22T20:26:18Z

src/search_span.rs

+            | (&SpanKind::End, &SpanKind::Start) => self.0 == other.0,
+            _ => false,
+        }
+


nit: blank line

nrc · 2017-06-22T20:27:07Z

src/search_span.rs

+
+    }
+}
+impl Eq for SearchSpan { }


Could derive this?

No because that would also require deriving PartialEq and Eq for SpanKind, and I think that would imply the SpanKind value would be used to implement the derived Eq? See my response to the Ord derive.

nrc · 2017-06-22T20:27:57Z

src/search_span.rs

+        }
+    }
+}
+impl PartialOrd<SearchSpan> for SearchSpan {


I think you could move the Ord implementation here, and then derive Ord

This requires implementing PartialOrd and Ord on SpanKind, which I don't think makes sense. This would mean the SpanKind would be used to break up the ordering when the spans are equal, but I don't want that to happen.

nrc · 2017-06-22T20:31:00Z

src/lib.rs

@@ -569,6 +613,8 @@ pub struct PerCrateAnalysis {
    ref_spans: HashMap<Id, Vec<Span>>,
    globs: HashMap<Span, Glob>,
    impls: HashMap<Id, Vec<Span>>,
+    per_fn_borrows: HashMap<Id, BorrowData>,
+    fn_span_lookup_tree: BTreeMap<SearchSpan, Id>,


It's safer type-wise since one wouldn't be able to accidentally store the SpanKind's Start and End values like you can now

Is this invariant enforced dynamically in the current system?

nrc · 2017-06-22T20:32:18Z

src/lib.rs

+
+                    if borrow_data.scopes.iter().any(|a| a.ref_id == id) {
+                        // If we find a `BorrowData` where there's a matching scope, then filter out
+                        // out matching items.


typo - "out\n out"

…up nits

Nashenas88 · 2017-06-24T18:45:24Z

Argh, I forgot rebasing messes up the comment history. I'm sorry about that. Note that the last few commits are now dependent on rust-dev-tools/rls-data#6. I believe I addressed all of your current concerns either with the commits or in the comments.

nrc · 2017-07-04T06:03:02Z

Thanks for the fixes and updates! I think this is ready to land now right? (I merged the rls-data change, and it looks like the Cargo files take that into account). Could you confirm and also rebase please?

(Sorry for the slow review here, I was travelling last week).

Nashenas88 · 2017-07-04T13:48:11Z

I commented on rust#42733 that I don't think using Id's for the Scopes, Moves and Loans is going to work. I was originally testing with a crate that had no external dependencies and all types were defined in the same file. Once I was able to test on more complex examples, I noticed a serious flaw where all of those ref_ids get transformed into a NULL Id (the local variables don't have a def_id) when the type was outside of the local crate. I'm a little confused as to how it was tracking the loans and moves uniquely within the file.

Given that this would require changes to rls-data and the PR linked above, would you still want to push this through? Wouldn't it prevent other work from moving forward?

nrc · 2017-07-05T00:14:35Z

Ah, I hadn't seen that comment. Given the changes, lets close this for now.

Nashenas88 commented Jun 18, 2017

View reviewed changes

Nashenas88 mentioned this pull request Jun 18, 2017

Support borrow, scope and move visualization through RLS rust-lang/rust#42733

Closed

Nashenas88 commented Jun 22, 2017

View reviewed changes

nrc reviewed Jun 22, 2017

View reviewed changes

Nashenas88 added 3 commits June 24, 2017 14:42

Add structs for borrow and ownership information

de7e1a0

Add capability to quickly find id for closest matching fn-like span

21bafc4

Add module comment in search_span, use InTreeSpan in BTreeMap, clean …

7c8f808

…up nits

Nashenas88 force-pushed the master branch from 3bf3f2e to 7c8f808 Compare June 24, 2017 18:43

Fix bug in SearchSpan

0296c74

Nashenas88 mentioned this pull request Jun 30, 2017

Support visualization of scopes, borrows and moves rust-lang/rls#387

Closed

nrc closed this Jul 5, 2017


		pub type Span = span::Span<span::ZeroIndexed>;

		#[derive(Debug)]

Add structs for borrow and ownership information #71

Add structs for borrow and ownership information #71

Uh oh!

Conversation

Nashenas88 commented Jun 18, 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!

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 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrc commented Jul 4, 2017

Uh oh!

Nashenas88 commented Jul 4, 2017

Uh oh!

nrc commented Jul 5, 2017

Uh oh!

Uh oh!

Nashenas88 commented Jun 24, 2017 •

edited

Loading