[move-ide] Added virtualized source file reader #16213
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
awelc
force-pushed
the
aw/virtual-source-files
branch
from
d12b11c
to
71ade14
Compare
| build_plan.compile_with_driver(Some(Box::new(vfs)), &mut std::io::sink(), |compiler| { | ||
| // extract expansion AST |
There was a problem hiding this comment.
Couldn't you just set the vfs inside here with set_source_file_reader?
There was a problem hiding this comment.
That is what I had here initially, but since I have to use it before/after compiler_with_driver call to retrieve file sources for the symbolicator to use, I could not figure out how to avoid copying/cloning of vfs. In particular, I can't make the closure into the move one as we have things that we need to copy out of it. It's likely a gap in my Rust skills, so suggestions welcome!
There was a problem hiding this comment.
Not sure that I follow. I don't see it being used after compile_with_driver. And before shouldn't matter ?
There was a problem hiding this comment.
I tried to respond why it seems to matter in #16213 (comment) but I may be misunderstanding what Rust is trying to tell me...
| @@ -54,6 +54,8 @@ pub struct Compiler<'a> { | |||
| known_warning_filters: Vec<(/* Prefix */ Option<Symbol>, Vec<WarningFilter>)>, | |||
| package_configs: BTreeMap<Symbol, PackageConfig>, | |||
| default_config: Option<PackageConfig>, | |||
| /// Abstracted source file reader | |||
| source_file_reader: Box<dyn SourceFileReader>, | |||
There was a problem hiding this comment.
Nit, make this a Option, and assert it is None when being set so that it is not set more than once
There was a problem hiding this comment.
I did it in a similar style to how other Options in the compiler are handled, but I am not sure if it's the most elegant (though at least consistent). Another alternative perhaps would be to make Option<Box<dyn SourceFileReader>> an argument to from_package_paths.
There was a problem hiding this comment.
I think just making this an Option here would be fine, and would match the other ones like interface_files_dir_opt, pre_compiled_lib, or package_configs
There was a problem hiding this comment.
This is indeed the solution I went for.
| // Source file reader | ||
| //************************************************************************************************** | ||
|
|
||
| pub trait SourceFileReader { |
There was a problem hiding this comment.
Nit on the name, why not just FileReader?
| @@ -278,14 +286,20 @@ impl<'a> Compiler<'a> { | |||
| known_warning_filters, | |||
| package_configs, | |||
| default_config, | |||
| source_file_reader, | |||
| } = self; | |||
| generate_interface_files_for_deps( | |||
There was a problem hiding this comment.
It makes me a bit uneasy that we don't use the vfs for interface files. That is, I don't particularly like the split of some reads being directly from Path::is_file and others being through the vfs
Also not necessary now, but worth considering writes too
There was a problem hiding this comment.
Folks asked long ago that we don't stick these interface files in /tmp since it makes running the compiler difficult in some environments
| @@ -54,6 +54,8 @@ pub struct Compiler<'a> { | |||
| known_warning_filters: Vec<(/* Prefix */ Option<Symbol>, Vec<WarningFilter>)>, | |||
| package_configs: BTreeMap<Symbol, PackageConfig>, | |||
| default_config: Option<PackageConfig>, | |||
| /// Abstracted source file reader | |||
| source_file_reader: Box<dyn SourceFileReader>, | |||
There was a problem hiding this comment.
I think just making this an Option here would be fine, and would match the other ones like interface_files_dir_opt, pre_compiled_lib, or package_configs
| build_plan.compile_with_driver(Some(Box::new(vfs)), &mut std::io::sink(), |compiler| { | ||
| // extract expansion AST |
There was a problem hiding this comment.
Not sure that I follow. I don't see it being used after compile_with_driver. And before shouldn't matter ?
| @@ -986,7 +1003,7 @@ pub fn get_symbols( | |||
| let mut parsed_ast = None; | |||
| let mut typed_ast = None; | |||
| let mut diagnostics = None; | |||
| build_plan.compile_with_driver(&mut std::io::sink(), |compiler| { | |||
| build_plan.compile_with_driver(Some(Box::new(vfs)), &mut std::io::sink(), |compiler| { | |||
There was a problem hiding this comment.
Concretely what I'm suggesting
| build_plan.compile_with_driver(Some(Box::new(vfs)), &mut std::io::sink(), |compiler| { | |
| build_plan.compile_with_driver(&mut std::io::sink(), |compiler| { | |
| let compiler = compiler.set_file_reader(Box::new(vfs)); |
There was a problem hiding this comment.
I understood what the suggestion was, particularly that I tried this myself. The problem here (at least as I understand it) is that Rust needs to copy vfs into the closure, which would involve making a copy of all source files read from the file system before compile_with_driver is invoked. I am trying to avoid it by passing vfs by-value to the function itself.
| @@ -28,7 +28,7 @@ use move_compiler::{ | |||
| compiled_unit::{AnnotatedCompiledUnit, CompiledUnit, NamedCompiledModule}, | |||
| diagnostics::FilesSourceText, | |||
| editions::Flavor, | |||
| shared::{NamedAddressMap, NumericalAddress, PackageConfig, PackagePaths}, | |||
| shared::{NamedAddressMap, NumericalAddress, PackageConfig, PackagePaths, SourceFileReader}, | |||
There was a problem hiding this comment.
I'm also wondering if we should add this to the BuildConfig or anything?
Like sure the source files can be virtual, but the toml file isn't.
I'm
A) not sure we want/need this
B) not sure how difficult it would be
mostly flagging as a concern since the compiler would now have one view of IO and the package system another
awelc
force-pushed
the
aw/virtual-source-files
branch
from
a8f5d2d
to
58bd8b6
Compare
|
Superseded by #16298 |
Description
This PR adds support for virtualized source file reader, allowing the compiler to build a package from files kept by the IDE in memory.
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.