[move-compiler] Added support for virtual file system #16330

awelc · 2024-02-21T23:23:19Z

Description

This PR adds support for virtual file system, with the intention of allowing the compiler to build a package from files kept by the IDE in memory (this feature is upcoming).

It also removes a certain awkwardness from the compiler implementation where interface files had to be built in a temporary directory (now they are built in an in-memory file system).

It also fixes an existing bug (move-language/move#1082) where source files used in the symbolicator (obtained from resolution graph) and source files used by the compiler could be modified between the two uses resulting in different file hashes which can ultimately lead to crash when translating diagnostics (generated by the compiler and using "compiler file hashes") using symbolicator source files (and "symbolicator file hashes")

Test Plan

All existing tests must pass. Also, manually tested the IDE no longer needs file saves to reflect changes in the file (e.g., compiler diagnostics appear as the code is being typed).

vercel · 2024-02-21T23:23:28Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:52pm
multisig-toolkit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:52pm
mysten-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:52pm
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:52pm
sui-kiosk	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:52pm
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:52pm

cgswords · 2024-02-23T20:39:43Z

external-crates/move/crates/move-cli/tests/build_tests/build_with_dep_warnings/args.exp

@@ -15,7 +15,7 @@ B0:
 Command `build -p dep`:
 BUILDING SomeDep
 warning[W09002]: unused variable
-  ┌─ ./sources/has_warning.move:2:20
+  ┌─ sources/has_warning.move:2:20


Can we get the ./ back easily?

cgswords · 2024-02-26T18:04:48Z

external-crates/move/crates/move-compiler/src/command_line/compiler.rs

-        let (id, interface_contents) =
-            interface_generator::write_file_to_string(module_to_named_address, &path)?;
+        let (id, interface_contents) = interface_generator::write_file_to_string(
+            vfs.clone(),


Is clone the right thing here? An in-memory VFS might not do the right thing.

Virtual file system in the compiler is represented by VfsPath from https://docs.rs/vfs/0.10.0/vfs/ which is this:

pub struct VfsPath { path: String, fs: Arc<VFS>, }

As I understand it, cloning should be OK for this piece of data (particularly for the fs part which internally represents a given file system) as it simply increments the reference count.

I feel like we should be able to borrow these paths? Which shoudl remove the clone?

cgswords · 2024-02-26T18:15:56Z

external-crates/move/crates/move-analyzer/src/bin/move-analyzer.rs

@@ -125,7 +124,8 @@ fn main() {
                .unwrap_or(false);
        }

-        symbolicator_runner = symbols::SymbolicatorRunner::new(symbols.clone(), diag_sender, lint);
+        symbolicator_runner =
+            symbols::SymbolicatorRunner::new(ide_files.clone(), symbols.clone(), diag_sender, lint);


Is this clone (plus subsequent ones) safe for in-memory VFS?

Same answer as here, but it's totally possible that I misunderstand Rust's behavior in this case.

cgswords · 2024-02-26T18:28:54Z

external-crates/move/crates/move-compiler/src/command_line/compiler.rs

-        generate_interface_files_for_deps(
+        let vfs = match vfs {
+            Some(vfs) => vfs,
+            None => VfsPath::new(PhysicalFS::new("/")),


Is this... a physical filesystem from root?

Yes. Some paths are provided as absolute paths right off the bat so I think this is the only way...

SHouldn't we just map each path into the VfsPath? i.e. taking all of the input paths here and mapping VfsPath::new(PhysicalFS::new

tnowacki

Biggest thing sticking out to me is all the weird handling of the paths in the compiler. I feel like we should always/immediately map into a VFSPath, instead of this weird game of holding onto a path root and name and generating it just before parsing

tnowacki · 2024-02-26T21:20:45Z

external-crates/move/crates/move-compiler/src/command_line/compiler.rs

 }

 pub fn generate_interface_files(
+    vfs: VfsPath,


I'm surprised this has to be owned. Does &VfsPath not work?

tnowacki · 2024-02-26T21:21:23Z

external-crates/move/crates/move-compiler/src/command_line/compiler.rs

-        let (id, interface_contents) =
-            interface_generator::write_file_to_string(module_to_named_address, &path)?;
+        let (id, interface_contents) = interface_generator::write_file_to_string(
+            vfs.clone(),


I feel like we should be able to borrow these paths? Which shoudl remove the clone?

tnowacki · 2024-02-26T21:23:05Z

external-crates/move/crates/move-compiler/src/parser/mod.rs

+/// Contains the same data as IndexedPackagePath but also information about which file system this
+/// path is located in.
+pub struct InterfaceFilePath {
+    idx_pkg_path: IndexedPackagePath,


What is this path? The path to the mv file?

tnowacki · 2024-02-26T21:23:32Z

external-crates/move/crates/move-compiler/src/parser/mod.rs

+/// Contains the same data as IndexedPackagePath but also information about which file system this
+/// path is located in.


Is this used for anything other than interface files?
Could you describe the relationship between these paths a bit more?

tnowacki · 2024-02-26T21:31:50Z

external-crates/move/crates/move-compiler/src/parser/mod.rs

@@ -152,8 +179,7 @@ fn parse_file(
    MatchedFileCommentMap,
    FileHash,
 )> {
-    let mut f = File::open(fname.as_str())
-        .map_err(|err| std::io::Error::new(err.kind(), format!("{}: {}", err, fname)))?;
+    let mut f = vfs.join(canonicalize(&fname))?.open_file()?;


Why do we need both the fname and the path at this point? Can't we have just the path?

tnowacki · 2024-02-26T21:33:16Z

external-crates/move/crates/move-compiler/src/command_line/compiler.rs

-        generate_interface_files_for_deps(
+        let vfs = match vfs {
+            Some(vfs) => vfs,
+            None => VfsPath::new(PhysicalFS::new("/")),


SHouldn't we just map each path into the VfsPath? i.e. taking all of the input paths here and mapping VfsPath::new(PhysicalFS::new

tnowacki · 2024-02-26T21:33:46Z

external-crates/move/crates/move-compiler/src/parser/mod.rs

@@ -143,6 +169,7 @@ fn ensure_targets_deps_dont_intersect(
 }

 fn parse_file(
+    vfs: &VfsPath,


nit, this name is really weird to me

Suggested change

vfs: &VfsPath,

path: &VfsPath,

It's not a virtual file system, just a path

tzakian

Overall changes look good to me, and no immediate concerns from a package standpoint (although the path handling in the package system/documentation generation is so complex it will need to be something we see when we get there, but I don't spot anything now -- if anything the VFS ability to root the filesystem will be very helpful!).

tnowacki

Thanks for working at this!

## Description This PR adds support for virtual file system, with the intention of allowing the compiler to build a package from files kept by the IDE in memory (this feature is upcoming). It also removes a certain awkwardness from the compiler implementation where interface files had to be built in a temporary directory (now they are built in an in-memory file system). It also fixes an existing bug (move-language/move#1082) where source files used in the symbolicator (obtained from resolution graph) and source files used by the compiler could be modified between the two uses resulting in different file hashes which can ultimately lead to crash when translating diagnostics (generated by the compiler and using "compiler file hashes") using symbolicator source files (and "symbolicator file hashes") ## Test Plan All existing tests must pass. Also, manually tested the IDE no longer needs file saves to reflect changes in the file (e.g., compiler diagnostics appear as the code is being typed).

awelc self-assigned this Feb 21, 2024

vercel bot deployed to Preview – sui-core February 21, 2024 23:24 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 21, 2024 23:25 View deployment

vercel bot deployed to Preview – sui-core February 21, 2024 23:26 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 00:00 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 00:01 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 00:17 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 00:20 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 00:40 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 00:40 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 00:46 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 00:47 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 00:59 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 01:00 View deployment

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 01:21 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 01:21 View deployment

awelc marked this pull request as ready for review February 23, 2024 01:46

awelc requested review from cgswords, rvantonder, tnowacki and dariorussi February 23, 2024 01:47

cgswords reviewed Feb 23, 2024

View reviewed changes

vercel bot deployed to Preview – sui-typescript-docs February 23, 2024 21:40 View deployment

vercel bot deployed to Preview – sui-core February 23, 2024 21:42 View deployment

awelc requested a review from cgswords February 23, 2024 22:21

cgswords reviewed Feb 26, 2024

View reviewed changes

tnowacki reviewed Feb 26, 2024

View reviewed changes

awelc added 11 commits February 29, 2024 10:35

Made clippy happy

c774a95

Prevented path canonicalization from affecting output

63252dc

Addressed review feedback

b7a61e7

Removed unnecessary canonicalization function

2c5b098

Fixed test output

886eec9

Made clippy happy

234aac9

Use standard canonicalization

593f076

Canonicalize virtual paths

92fe940

Trying old-style file finding

968636e

Reverted recent fix attempts and finally fixed Windows build

f62479b

Made clippy happy

e745c64

awelc force-pushed the aw/off-the-shelf-vfs branch from bfe4ed1 to e745c64 Compare February 29, 2024 18:36

vercel bot deployed to Preview – multisig-toolkit February 29, 2024 18:37 View deployment

vercel bot deployed to Preview – sui-kiosk February 29, 2024 18:37 View deployment

Fixed test output after merge

78bf002

vercel bot deployed to Preview – mysten-ui February 29, 2024 18:38 View deployment

vercel bot deployed to Preview – explorer February 29, 2024 18:38 View deployment

vercel bot deployed to Preview – sui-core February 29, 2024 18:39 View deployment

awelc added 2 commits February 29, 2024 10:49

Addressed review comments

441e958

Addressed remaining review comments

f62f5ab

vercel bot deployed to Preview – sui-core February 29, 2024 18:55 View deployment

awelc requested a review from tnowacki February 29, 2024 19:27

Addressed a review comment

3074217

vercel bot deployed to Preview – sui-core March 1, 2024 18:52 View deployment

tzakian reviewed Mar 1, 2024

View reviewed changes

tnowacki approved these changes Mar 1, 2024

View reviewed changes

awelc merged commit 9050365 into main Mar 1, 2024
41 checks passed

awelc deleted the aw/off-the-shelf-vfs branch March 1, 2024 20:18

awelc mentioned this pull request Mar 1, 2024

Added virtual file system to the compiler and IDE support #16298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler] Added support for virtual file system #16330

[move-compiler] Added support for virtual file system #16330

awelc commented Feb 21, 2024 •

edited

Loading

vercel bot commented Feb 21, 2024 •

edited

Loading

cgswords Feb 23, 2024

awelc Feb 23, 2024

cgswords Feb 26, 2024

awelc Feb 26, 2024

tnowacki Feb 26, 2024

cgswords Feb 26, 2024 •

edited

Loading

awelc Feb 26, 2024

cgswords Feb 26, 2024

awelc Feb 26, 2024

tnowacki Feb 26, 2024

tnowacki left a comment

tnowacki Feb 26, 2024

tnowacki Feb 26, 2024

tnowacki Feb 26, 2024

tnowacki Feb 26, 2024

tnowacki Feb 26, 2024

tnowacki Feb 26, 2024

tnowacki Feb 26, 2024

tzakian left a comment

tnowacki left a comment

		/// Contains the same data as IndexedPackagePath but also information about which file system this
		/// path is located in.

[move-compiler] Added support for virtual file system #16330

[move-compiler] Added support for virtual file system #16330

Conversation

awelc commented Feb 21, 2024 • edited Loading

Description

Test Plan

vercel bot commented Feb 21, 2024 • edited Loading

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

cgswords Feb 26, 2024 • edited Loading

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

tnowacki 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

tzakian left a comment

Choose a reason for hiding this comment

tnowacki left a comment

Choose a reason for hiding this comment

awelc commented Feb 21, 2024 •

edited

Loading

vercel bot commented Feb 21, 2024 •

edited

Loading

cgswords Feb 26, 2024 •

edited

Loading