Stream: t-compiler/wg-rls-2.0

Topic: Creating Files from an Assist


theduke (Jul 07 2020 at 09:38, on Zulip):

I'm trying to implement an assist that extracts inline modules to a a file. ( https://github.com/rust-analyzer/rust-analyzer/issues/5054 )

@matklad referenced two starting points, but I'm bumping up against some limitations because assists currently don't have the machinery for adding new files.

I have an idea how to implement it, but I thought I'd ask if this sounds reasonable first:

For LSP this would be expanded to 2 separate document changes in snippet_workspace_edit() ( lsp_types::CreateFile + a edit with the initial content )

matklad (Jul 07 2020 at 09:40, on Zulip):

Yup, these all sounds resonnable

matklad (Jul 07 2020 at 09:41, on Zulip):

The only bit I am unusre about is the last one, about create_file:

matklad (Jul 07 2020 at 09:41, on Zulip):

but I don't have any strictly better proposals of hand

matklad (Jul 07 2020 at 09:42, on Zulip):

@theduke I guess one thing though is that let's first refactor assist to hold SourceChange and send that as a PR without functional changes.

matklad (Jul 07 2020 at 09:42, on Zulip):

Than it would be easier to review file addition API

theduke (Jul 07 2020 at 09:45, on Zulip):

Can do, but I just did that and it's a tiny diff

diff --git a/crates/ra_assists/src/assist_context.rs b/crates/ra_assists/src/assist_context.rs
index 3640bb4d2..f34c6dfde 100644
--- a/crates/ra_assists/src/assist_context.rs
+++ b/crates/ra_assists/src/assist_context.rs
@@ -7,7 +7,7 @@ use hir::Semantics;
 use ra_db::{FileId, FileRange};
 use ra_fmt::{leading_indent, reindent};
 use ra_ide_db::{
-    source_change::{SourceChange, SourceFileEdit},
+    source_change::{SourceChange, SourceFileEdit, FileSystemEdit},
     RootDatabase,
 };
 use ra_syntax::{
@@ -176,7 +176,7 @@ pub(crate) struct AssistBuilder {
     edit: TextEditBuilder,
     file_id: FileId,
     is_snippet: bool,
-    edits: Vec<SourceFileEdit>,
+    change: SourceChange,
 }

 impl AssistBuilder {
@@ -185,7 +185,7 @@ impl AssistBuilder {
             edit: TextEditBuilder::default(),
             file_id,
             is_snippet: false,
-            edits: Vec::new(),
+            change: SourceChange::default(),
         }
     }

@@ -197,8 +197,8 @@ impl AssistBuilder {
         let edit = mem::take(&mut self.edit).finish();
         if !edit.is_empty() {
             let new_edit = SourceFileEdit { file_id: self.file_id, edit };
-            assert!(!self.edits.iter().any(|it| it.file_id == new_edit.file_id));
-            self.edits.push(new_edit);
+            assert!(!self.change.source_file_edits.iter().any(|it| it.file_id == new_edit.file_id));
+            self.change.source_file_edits.push(new_edit);
         }
     }

@@ -265,10 +265,10 @@ impl AssistBuilder {

     fn finish(mut self) -> SourceChange {
         self.commit();
-        let mut res: SourceChange = mem::take(&mut self.edits).into();
+        let mut change = mem::take(&mut self.change);
         if self.is_snippet {
-            res.is_snippet = true;
+            change.is_snippet = true;
         }
-        res
+        change
     }
 }
matklad (Jul 07 2020 at 09:46, on Zulip):

Can do, but I just did that and it's a tiny diff

The tinyer the diff, the merrier I am :-)

theduke (Jul 07 2020 at 09:46, on Zulip):

hehe, alright I'll open a separate PR then

theduke (Jul 07 2020 at 10:21, on Zulip):

Should I also do two separate PRs for the create file changes and the actual new assist?

matklad (Jul 07 2020 at 10:25, on Zulip):

Either way is fine

theduke (Jul 07 2020 at 13:07, on Zulip):

How would I get the current file name in an assist?
As in, somehow turn a FileId into a path

I've been jumping around the code for a while and can't really find a way

I need it to determine the target path

eg for mod x{} :

theduke (Jul 07 2020 at 13:32, on Zulip):

Seems like that would require a quite a lot of new plumbing since it needs a new method on:

Unless I'm missing some existing helper method somewhere (which I probably am)

matklad (Jul 07 2020 at 13:40, on Zulip):

So, the general answer that you don't :)

matklad (Jul 07 2020 at 13:40, on Zulip):

We deliberatelly don't expose full file paths to internals

matklad (Jul 07 2020 at 13:41, on Zulip):

so that we don't accidently rely on files being on disk or something like that

matklad (Jul 07 2020 at 13:42, on Zulip):

We have is_mod_rs on Module for this, and you can look at the Module's name. I think that should be enough

theduke (Jul 07 2020 at 14:44, on Zulip):

Makes sense. Yeah is_mod_rs is all I need.

matklad (Jul 07 2020 at 14:52, on Zulip):

so that we don't accidently rely on files being on disk

To clarify, this is not about "files phisicaly being stored on a hard drive", but about "path represents an inode". I can imagine some remote analysis scenarios, where files are purely virtual, and don't correspond to an existing FS at all. This might be overenginering though, but maybe not :)

theduke (Jul 07 2020 at 14:59, on Zulip):

No it makes sense to have an abstraction from complete files
also when considering more complicated refactorings

theduke (Jul 07 2020 at 15:00, on Zulip):

I'm thinking about how to test the new assist

matklad (Jul 07 2020 at 15:00, on Zulip):

Good question!

theduke (Jul 07 2020 at 15:00, on Zulip):

right now, all the tests simply create a mock file, manually get the source code, apply the assist and that's it

matklad (Jul 07 2020 at 15:01, on Zulip):

You can create mulitfile fixtures

matklad (Jul 07 2020 at 15:01, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_assists/src/handlers/fix_visibility.rs#L510-L524

matklad (Jul 07 2020 at 15:01, on Zulip):

But I think you can assert only a single file

theduke (Jul 07 2020 at 15:03, on Zulip):

yeah, and the test code only applies a single edit manually

theduke (Jul 07 2020 at 15:04, on Zulip):

I guess rename would also touch and test multiple files, but that doesn't seem to be in ra_assists::handlers ?

theduke (Jul 07 2020 at 15:05, on Zulip):

ah, ra_ide::references::rename

theduke (Jul 07 2020 at 15:08, on Zulip):

The rename check does more or less the same thing

theduke (Jul 07 2020 at 15:10, on Zulip):

but it would be really nice to have something like SourceChange::apply(db) -> RootDatabase

theduke (Jul 07 2020 at 15:10, on Zulip):

or whatever the right target would be

matklad (Jul 07 2020 at 15:12, on Zulip):

I think that should be a testing utility, which returns a set of modified files (so that you don't need to compare files which are not changed)

matklad (Jul 07 2020 at 15:12, on Zulip):

And, in tests, I think you should have access to file paths

Last update: Sep 27 2020 at 13:45UTC