Support multiple crate types and versions (and multiple passed directly in-memory) #106

Xanewok · 2017-10-23T11:21:58Z

Fixes #93. (also includes and is based on #103, can rebase if needed)
Blocked by rust-lang/rust#45468 (emitting crate disambiguators in save-analysis in rustc) and rust-dev-tools/rls-data#11 (data definitions, this pulls my crate-source branch now).

Pushing this as a single squashed commit, since I did walk in circles a bit and had a lot of back and forth commits. This turned out to be bigger than intended, hope that's okay.

Most important change is that it tries to change the model from mapping Option<PathBuf> -> PerCrateAnalysis to CrateId -> PerCrateAnalysis.
To support passing multiple in-process analysis data, this effectively disconnects paths from loaded data. CrateId contains crate name and disambiguator, meaning it'll be unique for each crate target/version.

I tried to simplify some code and comment as I go, hope that's also okay.

Some quirks still to solve:

Changing dependencies for crates changes its crate id; This means that when working on a crate and reloading the new data directly, the old data will be orphaned. One idea is to forcibly reload the ones that are passed directly, e.g. every crate with a given name will be removed from database and new data (with new crate id) will be loaded
This possibly introduces small performance regressions, since timestamps are checked now after loading the data from files, not before (the blacklist is still respected before) here. This can be easily fixed by adding an additional PathBuf -> SystemTime cache (however I didn't want to change the architecture too much now).

To make it more consistent and to make external AnalysisLoader implementations more MT-safe, moved Mutex from inside the CargoAnalysisLoader to loader member itself. This works, but I'm not sure the MT code is safe and consistent in general, so it'd be good to take a look at that later, maybe.

r? @nrc

nrc

Overall approach looks great. A bunch of comments inline.

nrc · 2017-10-27T03:30:39Z

src/analysis.rs

@@ -36,7 +37,7 @@ pub struct PerCrateAnalysis {
    pub globs: HashMap<Span, Glob>,
    pub impls: HashMap<Id, Vec<Span>>,

-    pub name: String,
+    pub crate_id: CrateId,


I assume we always have a PerCrateAnalysis from Analysis, so do we need the CrateId here, when we'll always be looking up the Analysis using a CrateId? (Seems like duplication of the key).

Yup, you're right - was used as a convenience in def_roots but it'd be good not to duplicate it

nrc · 2017-10-27T03:31:18Z

src/analysis.rs

        self.per_crate
            .iter()
-            .filter_map(|(s, pc)| s.as_ref().map(|s| (s.clone(), pc.timestamp)))
+            .map(|(id,c)| (id.clone(), c.timestamp.clone()))


nit: id, c

nrc · 2017-10-27T03:55:47Z

src/lib.rs

-        let raw_analysis = read_analysis_incremental(&self.loader, timestamps, blacklist);
+        let timestamps = self.analysis.lock()?.as_ref().unwrap().timestamps();
+        let loader = self.loader.lock()?;
+        let raw_analysis = read_analysis_from_files(&*loader, timestamps, blacklist);


Could we scope the lock on loader to just this call? (Probably me being paranoid, but I'd rather drop the lock as early as possible).

Done! However, to be honest, right now it feels a bit too wild west wrt mutexes everywhere, so I think it'd be nice to make an assumption who can call what and come up with a more sane MT-safe interface

nrc · 2017-10-27T04:01:20Z

src/loader.rs

-    where
-        F: Fn(&Path) -> Vec<T>;
+    /// Returns every directory in which paths are to be considered.
+    /// TODO: Can it also return files?


nrc · 2017-10-27T04:02:10Z

src/loader.rs

-        F: Fn(&Path) -> Vec<T>;
+    /// Returns every directory in which paths are to be considered.
+    /// TODO: Can it also return files?
+    fn paths(&self) -> Vec<PathBuf>;


Could paths be renamed search_directories or something a bit more descriptive?

Sure, I wasn't sure before if it's designed to only return directories or can it also return paths, so that's the most neutral name I could come up with

nrc · 2017-10-27T04:05:17Z

src/lowering.rs

@@ -6,11 +6,12 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-// For processing the raw save-analysis data from rustc into rustw's in-memory representation.
+//! For processing the raw save-analysis data from rustc into rustw's


s/rustw/the rls

nrc · 2017-10-27T04:07:58Z

src/raw.rs

+                // TODO: Bring back path-based timestamps?
+                // This would speed up a bit reloading time, since non-blacklisted
+                // can be older than the ones loaded, so don't waste time
+                // reading the file and deserializing


Yeah, it would be nice if we could avoid reading the file - reading and deserialising can be a significant overhead for large analysis files.

@nrc Can I add this after this PR lands?

Sure, that's fine.

Xanewok · 2017-10-27T08:09:21Z

Rebased and pushed commit with addressed feedback, now this waits for rust-dev-tools/rls-data#11 to land

Xanewok · 2017-10-29T19:52:49Z

Updated test data with changes to save-analysis from rust-lang/rust#45468

Xanewok · 2017-10-30T18:33:05Z

Separated and rebased on top of #114, so it can land first for Rust CI

Xanewok · 2017-11-03T11:09:31Z

As of today nightly rustc emits the new analysis data, so I think we can proceed with this update.
Bumping version to 0.8, since it changes the public api, not to break other users, e.g. the new rustdoc.

nrc · 2017-11-03T21:09:24Z

Thanks!

Xanewok · 2017-11-03T21:15:09Z

Hopefully workspace support is more or less complete now! Now onto improvements 😄

Xanewok force-pushed the multi-crates branch from 6b2c9e6 to 802006f Compare October 23, 2017 11:27

nrc reviewed Oct 27, 2017

View reviewed changes

Xanewok force-pushed the multi-crates branch from 802006f to 0dd939c Compare October 27, 2017 08:07

nrc mentioned this pull request Oct 30, 2017

Tracking issue: multiple targets/workspaces/packages, etc. rust-lang/rls#132

Closed

This was referenced Oct 30, 2017

Emit crate disambiguators in save-analysis data rust-lang/rust#45468

Merged

Prepare to use rls-data 0.12 #114

Merged

Xanewok force-pushed the multi-crates branch from d87417e to 4a857c0 Compare October 30, 2017 18:32

Xanewok force-pushed the multi-crates branch 3 times, most recently from 52b42fb to 1d6bf96 Compare November 1, 2017 00:08

Xanewok added 2 commits November 3, 2017 12:05

Support multiple crate types/versions

2f2ec27

Bump version to 0.8

08759d4

Xanewok force-pushed the multi-crates branch from 1d6bf96 to 08759d4 Compare November 3, 2017 11:06

nrc merged commit 1e640e9 into rust-dev-tools:master Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple crate types and versions (and multiple passed directly in-memory) #106

Support multiple crate types and versions (and multiple passed directly in-memory) #106

Xanewok commented Oct 23, 2017 •

edited

Loading

nrc left a comment

nrc Oct 27, 2017

Xanewok Oct 27, 2017

nrc Oct 27, 2017

nrc Oct 27, 2017

Xanewok Oct 27, 2017

nrc Oct 27, 2017

nrc Oct 27, 2017

Xanewok Oct 27, 2017

nrc Oct 27, 2017

nrc Oct 27, 2017

Xanewok Oct 27, 2017

nrc Oct 27, 2017

Xanewok commented Oct 27, 2017

Xanewok commented Oct 29, 2017

Xanewok commented Oct 30, 2017

Xanewok commented Nov 3, 2017

nrc commented Nov 3, 2017

Xanewok commented Nov 3, 2017

Support multiple crate types and versions (and multiple passed directly in-memory) #106

Support multiple crate types and versions (and multiple passed directly in-memory) #106

Conversation

Xanewok commented Oct 23, 2017 • edited Loading

nrc 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

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

Xanewok commented Oct 27, 2017

Xanewok commented Oct 29, 2017

Xanewok commented Oct 30, 2017

Xanewok commented Nov 3, 2017

nrc commented Nov 3, 2017

Xanewok commented Nov 3, 2017

Xanewok commented Oct 23, 2017 •

edited

Loading