Fix critical GC <> LSP bug and enable dynamic GC configuration. (#5813)

.github/workflows/ci.yml

@@ -499,7 +499,7 @@ jobs:
           toolchain: ${{ env.RUST_VERSION }}
       - uses: Swatinem/rust-cache@v2
       - name: Run sway-lsp tests sequentially
-        run: cargo test --locked --release -p sway-lsp -- --test-threads=1
+        run: cargo test --locked --release -p sway-lsp -- --nocapture --test-threads=1
   cargo-test-workspace:
     runs-on: ubuntu-latest
     steps:

sway-core/src/build_config.rs

@@ -1,5 +1,5 @@
 use serde::{Deserialize, Deserializer, Serialize};
-use std::{path::PathBuf, sync::Arc};
+use std::{collections::BTreeMap, path::PathBuf, sync::Arc};
 use strum::{Display, EnumString};
 
 #[derive(
@@ -217,6 +217,9 @@ pub struct LspConfig {
     //
     // This is set to false if compilation was triggered by a didSave or didOpen LSP event.
     pub optimized_build: bool,
+    // The value of the `version` field in the `DidChangeTextDocumentParams` struct.
+    // This is used to determine if the file has been modified since the last compilation.
+    pub file_versions: BTreeMap<PathBuf, Option<u64>>,
 }
 
 #[cfg(test)]

sway-core/src/lib.rs

@@ -110,6 +110,7 @@ pub fn parse(
             config.build_target,
             config.include_tests,
             config.experimental,
+            config.lsp_mode.as_ref(),
         )
         .map(
             |ParsedModuleTree {
@@ -254,6 +255,7 @@ fn parse_submodules(
     build_target: BuildTarget,
     include_tests: bool,
     experimental: ExperimentalFlags,
+    lsp_mode: Option<&LspConfig>,
 ) -> Submodules {
     // Assume the happy path, so there'll be as many submodules as dependencies, but no more.
     let mut submods = Vec::with_capacity(module.submodules().count());
@@ -287,6 +289,7 @@ fn parse_submodules(
             build_target,
             include_tests,
             experimental,
+            lsp_mode,
         ) {
             if !matches!(kind, parsed::TreeType::Library) {
                 let source_id = engines.se().get_source_id(submod_path.as_ref());
@@ -340,6 +343,7 @@ fn parse_module_tree(
     build_target: BuildTarget,
     include_tests: bool,
     experimental: ExperimentalFlags,
+    lsp_mode: Option<&LspConfig>,
 ) -> Result<ParsedModuleTree, ErrorEmitted> {
     let query_engine = engines.qe();
 
@@ -359,6 +363,7 @@ fn parse_module_tree(
         build_target,
         include_tests,
         experimental,
+        lsp_mode,
     );
 
     // Convert from the raw parsed module to the `ParseTree` ready for type-check.
@@ -403,57 +408,73 @@ fn parse_module_tree(
         .ok()
         .and_then(|m| m.modified().ok());
     let dependencies = submodules.into_iter().map(|s| s.path).collect::<Vec<_>>();
-    let parsed_module_tree = ParsedModuleTree {
-        tree_type: kind,
-        lexed_module: lexed,
-        parse_module: parsed,
-    };
+    let version = lsp_mode
+        .and_then(|lsp| lsp.file_versions.get(path.as_ref()).copied())
+        .unwrap_or(None);
     let cache_entry = ModuleCacheEntry {
         path,
         modified_time,
         hash,
         dependencies,
         include_tests,
+        version,
     };
     query_engine.insert_parse_module_cache_entry(cache_entry);
 
-    Ok(parsed_module_tree)
+    Ok(ParsedModuleTree {
+        tree_type: kind,
+        lexed_module: lexed,
+        parse_module: parsed,
+    })
 }
 
 fn is_parse_module_cache_up_to_date(
     engines: &Engines,
     path: &Arc<PathBuf>,
     include_tests: bool,
+    build_config: Option<&BuildConfig>,
 ) -> bool {
     let query_engine = engines.qe();
     let key = ModuleCacheKey::new(path.clone(), include_tests);
     let entry = query_engine.get_parse_module_cache_entry(&key);
     match entry {
         Some(entry) => {
-            let modified_time = std::fs::metadata(path.as_path())
-                .ok()
-                .and_then(|m| m.modified().ok());
-
             // Let's check if we can re-use the dependency information
-            // we got from the cache, which is only true if the file hasn't been
-            // modified since or if its hash is the same.
-            let cache_up_to_date = entry.modified_time == modified_time || {
-                let src = std::fs::read_to_string(path.as_path()).unwrap();
-
-                let mut hasher = DefaultHasher::new();
-                src.hash(&mut hasher);
-                let hash = hasher.finish();
-
-                hash == entry.hash
-            };
+            // we got from the cache.
+            let cache_up_to_date = build_config
+                .as_ref()
+                .and_then(|x| x.lsp_mode.as_ref())
+                .and_then(|lsp| {
+                    // First try to get the file version from lsp if it exists
+                    lsp.file_versions.get(path.as_ref())
+                })
+                .map_or_else(
+                    || {
+                        // Otherwise we can safely read the file from disk here, as the LSP has not modified it, or we are not in LSP mode.
+                        // Check if the file has been modified or if its hash is the same as the last compilation
+                        let modified_time = std::fs::metadata(path.as_path())
+                            .ok()
+                            .and_then(|m| m.modified().ok());
+                        entry.modified_time == modified_time || {
+                            let src = std::fs::read_to_string(path.as_path()).unwrap();
+                            let mut hasher = DefaultHasher::new();
+                            src.hash(&mut hasher);
+                            let hash = hasher.finish();
+                            hash == entry.hash
+                        }
+                    },
+                    |version| {
+                        // The cache is invalid if the lsp version is greater than the last compilation
+                        !version.map_or(false, |v| v > entry.version.unwrap_or(0))
+                    },
+                );
 
             // Look at the dependencies recursively to make sure they have not been
             // modified either.
             if cache_up_to_date {
-                entry
-                    .dependencies
-                    .iter()
-                    .all(|path| is_parse_module_cache_up_to_date(engines, path, include_tests))
+                entry.dependencies.iter().all(|path| {
+                    is_parse_module_cache_up_to_date(engines, path, include_tests, build_config)
+                })
             } else {
                 false
             }
@@ -528,7 +549,6 @@ pub fn parsed_to_ast(
         package_name,
         build_config,
     );
-
     check_should_abort(handler, retrigger_compilation.clone())?;
 
     // Only clear the parsed AST nodes if we are running a regular compilation pipeline.
@@ -681,16 +701,14 @@ pub fn compile_to_ast(
     retrigger_compilation: Option<Arc<AtomicBool>>,
 ) -> Result<Programs, ErrorEmitted> {
     check_should_abort(handler, retrigger_compilation.clone())?;
-
     let query_engine = engines.qe();
     let mut metrics = PerformanceData::default();
-
     if let Some(config) = build_config {
         let path = config.canonical_root_module();
         let include_tests = config.include_tests;
 
         // Check if we can re-use the data in the cache.
-        if is_parse_module_cache_up_to_date(engines, &path, include_tests) {
+        if is_parse_module_cache_up_to_date(engines, &path, include_tests, build_config) {
             let mut entry = query_engine.get_programs_cache_entry(&path).unwrap();
             entry.programs.metrics.reused_modules += 1;
 

sway-core/src/query_engine/mod.rs

@@ -32,6 +32,7 @@ pub struct ModuleCacheEntry {
     pub hash: u64,
     pub dependencies: Vec<ModulePath>,
     pub include_tests: bool,
+    pub version: Option<u64>,
 }
 
 pub type ModuleCacheMap = HashMap<ModuleCacheKey, ModuleCacheEntry>;
@@ -45,19 +46,11 @@ pub struct ProgramsCacheEntry {
 
 pub type ProgramsCacheMap = HashMap<ModulePath, ProgramsCacheEntry>;
 
-#[derive(Debug, Default)]
+#[derive(Debug, Default, Clone)]
 pub struct QueryEngine {
-    parse_module_cache: RwLock<ModuleCacheMap>,
-    programs_cache: RwLock<ProgramsCacheMap>,
-}
-
-impl Clone for QueryEngine {
-    fn clone(&self) -> Self {
-        Self {
-            parse_module_cache: RwLock::new(self.parse_module_cache.read().unwrap().clone()),
-            programs_cache: RwLock::new(self.programs_cache.read().unwrap().clone()),
-        }
-    }
+    // We want the below types wrapped in Arcs to optimize cloning from LSP.
+    parse_module_cache: Arc<RwLock<ModuleCacheMap>>,
+    programs_cache: Arc<RwLock<ProgramsCacheMap>>,
 }
 
 impl QueryEngine {

sway-core/src/semantic_analysis/module.rs

@@ -303,6 +303,7 @@ impl ty::TyModule {
         )?;
 
         let mut all_nodes = Self::type_check_nodes(handler, ctx.by_ref(), ordered_nodes)?;
+
         let submodules = submodules_res?;
 
         let fallback_fn = collect_fallback_fn(&all_nodes, engines, handler)?;

sway-lsp/benches/lsp_benchmarks/compile.rs

@@ -15,6 +15,7 @@ fn benchmarks(c: &mut Criterion) {
     let uri = Url::from_file_path(super::benchmark_dir().join("src/main.sw")).unwrap();
     let mut lsp_mode = Some(sway_core::LspConfig {
         optimized_build: false,
+        file_versions: Default::default(),
     });
     c.bench_function("compile", |b| {
         b.iter(|| {

sway-lsp/benches/lsp_benchmarks/mod.rs

@@ -14,6 +14,7 @@ pub async fn compile_test_project() -> (Url, Arc<Session>) {
     let session = Arc::new(Session::new());
     let lsp_mode = Some(sway_core::LspConfig {
         optimized_build: false,
+        file_versions: Default::default(),
     });
     // Load the test project
     let uri = Url::from_file_path(benchmark_dir().join("src/main.sw")).unwrap();

sway-lsp/src/config.rs

@@ -16,6 +16,8 @@ pub struct Config {
     pub on_enter: OnEnterConfig,
     #[serde(default, skip_serializing)]
     trace: TraceConfig,
+    #[serde(default)]
+    pub garbage_collection: GarbageCollectionConfig,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Default)]
@@ -53,7 +55,26 @@ impl Default for DiagnosticConfig {
     }
 }
 
-// Options for confguring server logging.
+// Options for configuring garbage collection.
+#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct GarbageCollectionConfig {
+    pub gc_enabled: bool,
+    pub gc_frequency: i32,
+}
+
+impl Default for GarbageCollectionConfig {
+    fn default() -> Self {
+        Self {
+            gc_enabled: true,
+            // Garbage collection is fairly expsensive so we default to only clear on every 3rd keystroke.
+            // Waiting too long to clear can cause a stack overflow to occur.
+            gc_frequency: 3,
+        }
+    }
+}
+
+// Options for configuring server logging.
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 pub struct LoggingConfig {
     #[serde(with = "LevelFilterDef")]

sway-lsp/src/core/session.rs

@@ -259,6 +259,7 @@ impl Session {
                 path: uri.path().to_string(),
                 err: err.to_string(),
             })?;
+
         Ok(())
     }
 
@@ -372,7 +373,7 @@ type CompileResults = (Vec<CompileError>, Vec<CompileWarning>);
 
 pub fn traverse(
     results: Vec<(Option<Programs>, Handler)>,
-    engines: &Engines,
+    engines_clone: &Engines,
     session: Arc<Session>,
 ) -> Result<Option<CompileResults>, LanguageServerError> {
     session.token_map.clear();
@@ -399,6 +400,18 @@ pub fn traverse(
             session.metrics.insert(source_id, metrics.clone());
         }
 
+        let engines_ref = session.engines.read();
+        // Check if the cached AST was returned by the compiler for the users workspace.
+        // If it was, then we need to use the original engines for traversal.
+        //
+        // This is due to the garbage collector removing types from the engines_clone
+        // and they have not been re-added due to compilation being skipped.
+        let engines = if i == results_len - 1 && metrics.reused_modules > 0 {
+            &*engines_ref
+        } else {
+            engines_clone
+        };
+
         // Get a reference to the typed program AST.
         let typed_program = typed
             .as_ref()