feat(language server): integrated new vfs #294

baszalmstra · 2021-01-11T20:38:36Z

This quite a large PR that drops the use ra_vfs and instead implements a custom solution based on the current solution in rust-analyzer but a tailored towards Mun.

This also adds a new crate mun_paths that provides convenience structs for dealing with absolute and relative paths. SourceRoots have also been changed a bit. They now contain the relative paths to the files they contain instead of that being stored in the database. Since a single file might be contained in multiple source roots this makes more sense. I also introduced the PackageSet to the database which holds all different packages that the database should know of.

A lot of boilerplate around the language server is now consolidated which now also enables multiple mun packages in a single workspace.

I also hacked a bit around the use of async functions. Im planning to remove all async code and using a simple taskpool. I think async works really well for IO operations but for our computationally heavy usecase its easier to use threads. (did this in #295)

This PR also paves the way for much better testing of the language server because its now much easier to reason about the virtual filesystem and its progress (which was missing in the previous implementation). (similar to #293 )

codecov · 2021-01-11T20:57:07Z

Codecov Report

Merging #294 (1a06e0f) into master (388cee1) will decrease coverage by 0.77%.
The diff coverage is 45.26%.

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
- Coverage   79.54%   78.76%   -0.78%     
==========================================
  Files         228      236       +8     
  Lines       13480    13866     +386     
==========================================
+ Hits        10722    10922     +200     
- Misses       2758     2944     +186

Impacted Files	Coverage Δ
crates/mun_compiler/src/lib.rs	`71.05% <ø> (ø)`
crates/mun_compiler_daemon/src/lib.rs	`0.00% <0.00%> (ø)`
crates/mun_hir/src/ids.rs	`82.35% <ø> (ø)`
...tes/mun_hir/src/name_resolution/path_resolution.rs	`75.00% <ø> (ø)`
crates/mun_hir/src/package_defs.rs	`100.00% <ø> (ø)`
crates/mun_hir/src/package_defs/collector.rs	`84.78% <ø> (ø)`
crates/mun_language_server/src/analysis.rs	`23.52% <0.00%> (-1.48%)`	⬇️
crates/mun_language_server/src/conversion.rs	`0.00% <0.00%> (ø)`
crates/mun_language_server/src/lib.rs	`0.00% <0.00%> (ø)`
crates/mun_language_server/src/project_manifest.rs	`0.00% <0.00%> (ø)`
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 388cee1...1a06e0f. Read the comment docs.

Wodann

Reviewed the first half of this change. In general it looks good, but I'm not sure why we need to move away from async? Was that just easier to keep in line with RA?

crates/mun_language_server/src/lib.rs

crates/mun_paths/Cargo.toml

crates/mun_vfs/Cargo.toml

baszalmstra · 2021-01-12T07:40:09Z

but I'm not sure why we need to move away from async? Was that just easier to keep in line with RA?

Its not about performance but more about the general simplicity of the code. It also makes it easier to use some of the tools out there. A lot is not async (salsa being a big example). You can see currently there are a lot of block_ons in there for that reason.

Wodann

Still 2 large files left to review

crates/mun_language_server/src/project_manifest.rs

crates/mun_vfs/src/lib.rs

Wodann

Once you've incorporated these changes, feel free to merge.

Wodann · 2021-01-15T01:51:35Z

crates/mun_vfs/src/monitor.rs

+    /// Returns true if, according to this instance, the file at the given `path` is contained in
+    /// this set.


Suggested change

/// Returns true if, according to this instance, the file at the given `path` is contained in

/// this set.

/// Returns whether - according to this instance - the file at the given `path` is contained in

/// this set.

I actually think returns true if is clearer. Do you want me to replace the commas with dashes?

The intend was to replace the "true if" as whether is the linguistic form of representing that. I always find it weird when people refer to a logical (or programmer) perspective of true in sentences, such as in the documentation.

Wodann · 2021-01-15T01:52:01Z

crates/mun_vfs/src/monitor.rs

+    /// Returns true if, according to this instance, the directory at the given `path` is contained
+    /// in this set.


Suggested change

/// Returns true if, according to this instance, the directory at the given `path` is contained

/// in this set.

/// Returns whether - according to this instance - the directory at the given `path` is contained

/// in this set.

Wodann · 2021-01-15T01:52:14Z

crates/mun_vfs/src/monitor.rs

+        self.includes_path(path)
+    }
+
+    /// Returns true if the given path is considered part of this set.


Suggested change

/// Returns true if the given path is considered part of this set.

/// Returns whether the given path is considered part of this set.

Wodann · 2021-01-15T01:54:22Z

crates/mun_vfs/src/monitor.rs

+    /// Returns true if, according to this instance, the file at the given `path` is contained in
+    /// this entry.


Suggested change

/// Returns true if, according to this instance, the file at the given `path` is contained in

/// this entry.

/// Returns whether - according to this instance - the file at the given `path` is contained in

/// this entry.

Wodann · 2021-01-15T01:54:44Z

crates/mun_vfs/src/monitor.rs

+    /// Returns true if, according to this instance, the directory at the given `path` is contained
+    /// in this set.


Suggested change

/// Returns true if, according to this instance, the directory at the given `path` is contained

/// in this set.

/// Returns whether - according to this instance - the directory at the given `path` is contained

/// in this set.

crates/mun_vfs/src/monitor/notify_monitor.rs

baszalmstra requested a review from Wodann January 11, 2021 20:38

baszalmstra self-assigned this Jan 11, 2021

baszalmstra changed the title ~~feat(vfs): integrated new vfs~~ feat(language server): integrated new vfs Jan 11, 2021

baszalmstra added pri: intermediate An issue resulting in non-critical functionality loss and no significant effect on usability type: refactor Refactor existing code labels Jan 11, 2021

This was referenced Jan 11, 2021

feat(language server): removed async code and switched to lsp_server #295

Merged

feat: adds lsp document symbol provider #293

Closed

Wodann reviewed Jan 12, 2021

View reviewed changes

baszalmstra force-pushed the feature/new_vfs branch from 8ef71f4 to 8a6e9b4 Compare January 12, 2021 16:48

Wodann reviewed Jan 13, 2021

View reviewed changes

baszalmstra force-pushed the feature/new_vfs branch from 8a6e9b4 to d5592d6 Compare January 14, 2021 17:17

Wodann requested changes Jan 15, 2021

View reviewed changes

feat(vfs): integrated new vfs

1a06e0f

baszalmstra force-pushed the feature/new_vfs branch from d5592d6 to 1a06e0f Compare January 15, 2021 08:23

Wodann approved these changes Jan 15, 2021

View reviewed changes

baszalmstra merged commit 044ba56 into mun-lang:master Jan 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(language server): integrated new vfs #294

feat(language server): integrated new vfs #294

baszalmstra commented Jan 11, 2021 •

edited

Loading

codecov bot commented Jan 11, 2021 •

edited

Loading

Wodann left a comment

baszalmstra commented Jan 12, 2021 •

edited

Loading

Wodann left a comment

Wodann left a comment

Wodann Jan 15, 2021

baszalmstra Jan 15, 2021

Wodann Jan 15, 2021

Wodann Jan 15, 2021

Wodann Jan 15, 2021

Wodann Jan 15, 2021

Wodann Jan 15, 2021

		/// Returns true if, according to this instance, the file at the given `path` is contained in
		/// this set.

		/// Returns true if, according to this instance, the directory at the given `path` is contained
		/// in this set.

	/// Returns true if the given path is considered part of this set.
	/// Returns whether the given path is considered part of this set.

		/// Returns true if, according to this instance, the file at the given `path` is contained in
		/// this entry.

feat(language server): integrated new vfs #294

feat(language server): integrated new vfs #294

Conversation

baszalmstra commented Jan 11, 2021 • edited Loading

codecov bot commented Jan 11, 2021 • edited Loading

Codecov Report

Wodann left a comment

Choose a reason for hiding this comment

baszalmstra commented Jan 12, 2021 • edited Loading

Wodann left a comment

Choose a reason for hiding this comment

Wodann left a comment

Choose a reason for hiding this comment

Wodann Jan 15, 2021

Choose a reason for hiding this comment

baszalmstra Jan 15, 2021

Choose a reason for hiding this comment

Wodann Jan 15, 2021

Choose a reason for hiding this comment

Wodann Jan 15, 2021

Choose a reason for hiding this comment

Wodann Jan 15, 2021

Choose a reason for hiding this comment

Wodann Jan 15, 2021

Choose a reason for hiding this comment

Wodann Jan 15, 2021

Choose a reason for hiding this comment

baszalmstra commented Jan 11, 2021 •

edited

Loading

codecov bot commented Jan 11, 2021 •

edited

Loading

baszalmstra commented Jan 12, 2021 •

edited

Loading