-
Notifications
You must be signed in to change notification settings - Fork 11.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[move-compiler] Added support for virtual file system #16330
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the ./
back easily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is clone
the right thing here? An in-memory VFS might not do the right thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should be able to borrow these paths? Which shoudl remove the clone?
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this clone (plus subsequent ones) safe for in-memory VFS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as here, but it's totally possible that I misunderstand Rust's behavior in this case.
generate_interface_files_for_deps( | ||
let vfs = match vfs { | ||
Some(vfs) => vfs, | ||
None => VfsPath::new(PhysicalFS::new("/")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this... a physical filesystem from root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Some paths are provided as absolute paths right off the bat so I think this is the only way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
|
||
pub fn generate_interface_files( | ||
vfs: VfsPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this has to be owned. Does &VfsPath
not work?
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should be able to borrow these paths? Which shoudl remove the clone?
/// 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this path? The path to the mv file?
/// Contains the same data as IndexedPackagePath but also information about which file system this | ||
/// path is located in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used for anything other than interface files?
Could you describe the relationship between these paths a bit more?
@@ -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()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need both the fname and the path at this point? Can't we have just the path?
generate_interface_files_for_deps( | ||
let vfs = match vfs { | ||
Some(vfs) => vfs, | ||
None => VfsPath::new(PhysicalFS::new("/")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -143,6 +169,7 @@ fn ensure_targets_deps_dont_intersect( | |||
} | |||
|
|||
fn parse_file( | |||
vfs: &VfsPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, this name is really weird to me
vfs: &VfsPath, | |
path: &VfsPath, |
It's not a virtual file system, just a path
bfe4ed1
to
e745c64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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).