Repository: tweag/nickel
Pull request #1947 (2024-06-09 00:12:00)

flake.lock: Update
Automated changes by the [update-flake-lock](https://github.com/DeterminateSystems/update-flake-lock) GitHub Action.

```
Flake lock file updates:

• Updated input 'crane':
    'github:ipetkov/crane/480dff0be03dac0e51a8dfc26e882b0d123a450e?narHash=sha256-dIubLa56W9sNNz0e8jGxrX3CAkPXsq7snuFA/Ie6dn8%3D' (2024-05-29)
  → 'github:ipetkov/crane/55e7754ec31dac78980c8be45f8a28e80e370946?narHash=sha256-1hZ/txnbd/RmiBPNUs7i8UQw2N89uAK3UzrGAWdnFfU%3D' (2024-06-04)
• Updated input 'nix-input':
    'github:nixos/nix/802b4e403bb5ad34b26e36fbbe89dab91126d215?narHash=sha256-plqdBU6ySFf0whOcLlMramll1T5r38vC9dwF9RlEO0Y%3D' (2024-05-31)
  → 'github:nixos/nix/0ab9369572f64b1ab70a8db29f79ae730ff31ab6?narHash=sha256-DHM/6HZsuJO%2Bk32vBfPzzhEwgMVpYdHiMD5r6qoCk9k%3D' (2024-06-08)
• Updated input 'nix-input/nixpkgs':
    'github:NixOS/nixpkgs/b550fe4b4776908ac2a861124307045f8e717c8e?narHash=sha256-7kkJQd4rZ%2BvFrzWu8sTRtta5D1kBG0LSRYAfhtmMlSo%3D' (2024-02-28)
  → 'github:NixOS/nixpkgs/88269ab3044128b7c2f4c7d68448b2fb50456870?narHash=sha256-%2Bf9c4/ZX5MWDOuB1rKoWj%2BlBNm0z0rs4CK47HBLxy1o%3D' (2024-06-03)
• Added input 'nix-input/nixpkgs-23-11':
    'github:NixOS/nixpkgs/a62e6edd6d5e1fa0329b8653c801147986f8d446?narHash=sha256-oamiKNfr2MS6yH64rUn99mIZjc45nGJlj9eGth/3Xuw%3D' (2024-05-31)
• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/ad57eef4ef0659193044870c731987a6df5cf56b?narHash=sha256-SzDKxseEcHR5KzPXLwsemyTR/kaM9whxeiJohbL04rs%3D' (2024-05-29)
  → 'github:NixOS/nixpkgs/051f920625ab5aabe37c920346e3e69d7d34400e?narHash=sha256-4q0s6m0GUcN7q%2BY2DqD27iLvbcd1G50T2lv08kKxkSI%3D' (2024-06-07)
• Updated input 'pre-commit-hooks':
    'github:cachix/pre-commit-hooks.nix/0e8fcc54b842ad8428c9e705cb5994eaf05c26a0?narHash=sha256-xrsYFST8ij4QWaV6HEokCUNIZLjjLP1bYC60K8XiBVA%3D' (2024-05-20)
  → 'github:cachix/pre-commit-hooks.nix/cc4d466cb1254af050ff7bdf47f6d404a7c646d1?narHash=sha256-7XfBuLULizXjXfBYy/VV%2BSpYMHreNRHk9nKMsm1bgb4%3D' (2024-06-06)
• Updated input 'rust-overlay':
    'github:oxalica/rust-overlay/ab69b67fac9a96709fbef0b899db308ca714a120?narHash=sha256-4gVhbC%2BNjSQ4c6cJvJGNCI1oTcD%2B8jRRNAnOF9faGCE%3D' (2024-06-01)
  → 'github:oxalica/rust-overlay/6dc3e45fe4aee36efeed24d64fc68b1f989d5465?narHash=sha256-wqbRwq3i7g5EHIui0bIi84mdqZ/It1AXBSLJ5tafD28%3D' (2024-06-08)
```

### Running GitHub Actions on this PR

GitHub Actions will not run workflows on pull requests which are opened by a GitHub Action.

To run GitHub Actions workflows on this PR, run:

```sh
git branch -D update_flake_lock_action
git fetch origin
git checkout update_flake_lock_action
git commit --amend --no-edit
git push origin update_flake_lock_action --force
```


Patch:
diff --git a/core/src/cache.rs b/core/src/cache.rs
index 5c535d42..b406127d 100644
--- a/core/src/cache.rs
+++ b/core/src/cache.rs
@@ -1013,6 +1013,38 @@ impl Cache {
             .map(|TermEntry { state, .. }| std::mem::replace(state, new))
     }
 
+    /// Remove the cached term associated with this id, and any cached terms
+    /// that import it.
+    ///
+    /// The file contents associated with this id remain, and they will be
+    /// re-parsed if necessary.
+    ///
+    /// This invalidation scheme is probably too aggressive; there are
+    /// situations where a change in one file doesn't require invalidation
+    /// of other files that import it. For example, if the parse status (i.e.
+    /// success/failure) of a file doesn't change, files that import it don't
+    /// need to re-resolve their imports. If the checked type of a file doesn't
+    /// change, files that import it don't need to be re-typechecked.
+    ///
+    /// Returns all the additional (i.e. not including the passed one) file ids
+    /// whose caches were invalidated.
+    pub fn invalidate_cache(&mut self, file_id: FileId) -> Vec<FileId> {
+        fn invalidate_rec(slf: &mut Cache, acc: &mut Vec<FileId>, file_id: FileId) {
+            slf.terms.remove(&file_id);
+            slf.imports.remove(&file_id);
+            let rev_deps = slf.rev_imports.remove(&file_id).unwrap_or_default();
+
+            acc.extend(rev_deps.iter().copied());
+            for f in &rev_deps {
+                invalidate_rec(slf, acc, *f);
+            }
+        }
+
+        let mut ret = vec![];
+        invalidate_rec(self, &mut ret, file_id);
+        ret
+    }
+
     /// Retrieve the state of an entry. Return `None` if the entry is not in the term cache,
     /// meaning that the content of the source has been loaded but has not been parsed yet.
     pub fn entry_state(&self, file_id: FileId) -> Option<EntryState> {
diff --git a/flake.lock b/flake.lock
index d81a8162..7fc1265e 100644
--- a/flake.lock
+++ b/flake.lock
@@ -23,11 +23,11 @@
         ]
       },
       "locked": {
-        "lastModified": 1717025063,
-        "narHash": "sha256-dIubLa56W9sNNz0e8jGxrX3CAkPXsq7snuFA/Ie6dn8=",
+        "lastModified": 1717535930,
+        "narHash": "sha256-1hZ/txnbd/RmiBPNUs7i8UQw2N89uAK3UzrGAWdnFfU=",
         "owner": "ipetkov",
         "repo": "crane",
-        "rev": "480dff0be03dac0e51a8dfc26e882b0d123a450e",
+        "rev": "55e7754ec31dac78980c8be45f8a28e80e370946",
         "type": "github"
       },
       "original": {
@@ -237,15 +237,16 @@
         "flake-parts": "flake-parts",
         "libgit2": "libgit2",
         "nixpkgs": "nixpkgs",
+        "nixpkgs-23-11": "nixpkgs-23-11",
         "nixpkgs-regression": "nixpkgs-regression",
         "pre-commit-hooks": "pre-commit-hooks"
       },
       "locked": {
-        "lastModified": 1717155024,
-        "narHash": "sha256-plqdBU6ySFf0whOcLlMramll1T5r38vC9dwF9RlEO0Y=",
+        "lastModified": 1717862855,
+        "narHash": "sha256-DHM/6HZsuJO+k32vBfPzzhEwgMVpYdHiMD5r6qoCk9k=",
         "owner": "nixos",
         "repo": "nix",
-        "rev": "802b4e403bb5ad34b26e36fbbe89dab91126d215",
+        "rev": "0ab9369572f64b1ab70a8db29f79ae730ff31ab6",
         "type": "github"
       },
       "original": {
@@ -256,20 +257,36 @@
     },
     "nixpkgs": {
       "locked": {
-        "lastModified": 1709083642,
-        "narHash": "sha256-7kkJQd4rZ+vFrzWu8sTRtta5D1kBG0LSRYAfhtmMlSo=",
+        "lastModified": 1717432640,
+        "narHash": "sha256-+f9c4/ZX5MWDOuB1rKoWj+lBNm0z0rs4CK47HBLxy1o=",
         "owner": "NixOS",
         "repo": "nixpkgs",
-        "rev": "b550fe4b4776908ac2a861124307045f8e717c8e",
+        "rev": "88269ab3044128b7c2f4c7d68448b2fb50456870",
         "type": "github"
       },
       "original": {
         "owner": "NixOS",
-        "ref": "release-23.11",
+        "ref": "release-24.05",
         "repo": "nixpkgs",
         "type": "github"
       }
     },
+    "nixpkgs-23-11": {
+      "locked": {
+        "lastModified": 1717159533,
+        "narHash": "sha256-oamiKNfr2MS6yH64rUn99mIZjc45nGJlj9eGth/3Xuw=",
+        "owner": "NixOS",
+        "repo": "nixpkgs",
+        "rev": "a62e6edd6d5e1fa0329b8653c801147986f8d446",
+        "type": "github"
+      },
+      "original": {
+        "owner": "NixOS",
+        "repo": "nixpkgs",
+        "rev": "a62e6edd6d5e1fa0329b8653c801147986f8d446",
+        "type": "github"
+      }
+    },
     "nixpkgs-regression": {
       "locked": {
         "lastModified": 1643052045,
@@ -304,11 +321,11 @@
     },
     "nixpkgs_2": {
       "locked": {
-        "lastModified": 1716948383,
-        "narHash": "sha256-SzDKxseEcHR5KzPXLwsemyTR/kaM9whxeiJohbL04rs=",
+        "lastModified": 1717786204,
+        "narHash": "sha256-4q0s6m0GUcN7q+Y2DqD27iLvbcd1G50T2lv08kKxkSI=",
         "owner": "NixOS",
         "repo": "nixpkgs",
-        "rev": "ad57eef4ef0659193044870c731987a6df5cf56b",
+        "rev": "051f920625ab5aabe37c920346e3e69d7d34400e",
         "type": "github"
       },
       "original": {
@@ -375,11 +392,11 @@
         "nixpkgs-stable": "nixpkgs-stable"
       },
       "locked": {
-        "lastModified": 1716213921,
-        "narHash": "sha256-xrsYFST8ij4QWaV6HEokCUNIZLjjLP1bYC60K8XiBVA=",
+        "lastModified": 1717664902,
+        "narHash": "sha256-7XfBuLULizXjXfBYy/VV+SpYMHreNRHk9nKMsm1bgb4=",
         "owner": "cachix",
         "repo": "pre-commit-hooks.nix",
-        "rev": "0e8fcc54b842ad8428c9e705cb5994eaf05c26a0",
+        "rev": "cc4d466cb1254af050ff7bdf47f6d404a7c646d1",
         "type": "github"
       },
       "original": {
@@ -409,11 +426,11 @@
         ]
       },
       "locked": {
-        "lastModified": 1717208326,
-        "narHash": "sha256-4gVhbC+NjSQ4c6cJvJGNCI1oTcD+8jRRNAnOF9faGCE=",
+        "lastModified": 1717813066,
+        "narHash": "sha256-wqbRwq3i7g5EHIui0bIi84mdqZ/It1AXBSLJ5tafD28=",
         "owner": "oxalica",
         "repo": "rust-overlay",
-        "rev": "ab69b67fac9a96709fbef0b899db308ca714a120",
+        "rev": "6dc3e45fe4aee36efeed24d64fc68b1f989d5465",
         "type": "github"
       },
       "original": {
diff --git a/lsp/nls/src/files.rs b/lsp/nls/src/files.rs
index 8a216cc6..94e2e3cc 100644
--- a/lsp/nls/src/files.rs
+++ b/lsp/nls/src/files.rs
@@ -23,7 +23,8 @@ pub(crate) fn uri_to_path(uri: &Url) -> std::result::Result<PathBuf, Error> {
     }
 }
 
-pub fn handle_open(server: &mut Server, params: DidOpenTextDocumentParams) -> Result<()> {
+/// Returns a list of open files that were potentially invalidated by the changes.
+pub fn handle_open(server: &mut Server, params: DidOpenTextDocumentParams) -> Result<Vec<Url>> {
     let id: RequestId = format!(
         "{}#{}",
         params.text_document.uri, params.text_document.version
@@ -49,10 +50,11 @@ pub fn handle_open(server: &mut Server, params: DidOpenTextDocumentParams) -> Re
         server.issue_diagnostics(*rev_dep, diags);
     }
     Trace::reply(id);
-    Ok(())
+    Ok(server.world.uris(invalid).cloned().collect())
 }
 
-pub fn handle_save(server: &mut Server, params: DidChangeTextDocumentParams) -> Result<()> {
+/// Returns a list of open files that were potentially invalidated by the changes.
+pub fn handle_save(server: &mut Server, params: DidChangeTextDocumentParams) -> Result<Vec<Url>> {
     let id: RequestId = format!(
         "{}#{}",
         params.text_document.uri, params.text_document.version
@@ -76,9 +78,9 @@ pub fn handle_save(server: &mut Server, params: DidChangeTextDocumentParams) ->
     server.issue_diagnostics(file_id, diags);
 
     for f in &invalid {
-        let errors = server.world.typecheck(*f).err().unwrap_or_default();
+        let errors = server.world.parse_and_typecheck(*f);
         server.issue_diagnostics(*f, errors);
     }
     Trace::reply(id);
-    Ok(())
+    Ok(server.world.uris(invalid).cloned().collect())
 }
diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs
index 65208802..e67347ad 100644
--- a/lsp/nls/src/server.rs
+++ b/lsp/nls/src/server.rs
@@ -183,10 +183,13 @@ impl Server {
                     serde_json::from_value::<DidOpenTextDocumentParams>(notification.params)?;
                 let uri = params.text_document.uri.clone();
                 let contents = params.text_document.text.clone();
-                crate::files::handle_open(self, params)?;
+                let invalid = crate::files::handle_open(self, params)?;
                 self.background_jobs
                     .update_file(uri.clone(), contents, &self.world);
                 self.background_jobs.eval_file(uri);
+                for uri in invalid {
+                    self.background_jobs.eval_file(uri);
+                }
                 Ok(())
             }
             DidChangeTextDocument::METHOD => {
@@ -195,10 +198,13 @@ impl Server {
                     serde_json::from_value::<DidChangeTextDocumentParams>(notification.params)?;
                 let uri = params.text_document.uri.clone();
                 let contents = params.content_changes[0].text.clone();
-                crate::files::handle_save(self, params)?;
+                let invalid = crate::files::handle_save(self, params)?;
                 self.background_jobs
                     .update_file(uri.clone(), contents, &self.world);
                 self.background_jobs.eval_file(uri);
+                for uri in invalid {
+                    self.background_jobs.eval_file(uri);
+                }
                 Ok(())
             }
             _ => Ok(()),
diff --git a/lsp/nls/src/world.rs b/lsp/nls/src/world.rs
index 9b5e9a83..ec50ec00 100644
--- a/lsp/nls/src/world.rs
+++ b/lsp/nls/src/world.rs
@@ -5,6 +5,7 @@ use std::{
 };
 
 use codespan::FileId;
+use log::warn;
 use lsp_server::{ErrorCode, ResponseError};
 use lsp_types::Url;
 use nickel_lang_core::{
@@ -84,7 +85,7 @@ impl World {
 
         // Invalidate the cache of every file that tried, but failed, to import a file
         // with a name like this.
-        let mut invalid = path
+        let failed_to_import = path
             .file_name()
             .and_then(|name| self.failed_imports.remove(name))
             .unwrap_or_default();
@@ -93,16 +94,17 @@ impl World {
         // cache if it was imported by an already-open file.
         let file_id = self.cache.replace_string(SourcePath::Path(path), contents);
 
-        // Invalidate any cached inputs that imported the newly-opened file, so that any
-        // cross-file references are updated.
-        invalid.extend(self.cache.get_rev_imports_transitive(file_id));
+        // The cache automatically invalidates reverse-dependencies; we also need
+        // to track them, so that we can clear our own analysis.
+        let mut invalid = failed_to_import.clone();
+        invalid.extend(self.cache.invalidate_cache(file_id));
+
+        for f in failed_to_import {
+            invalid.extend(self.cache.invalidate_cache(f));
+        }
 
         for rev_dep in &invalid {
             self.analysis.remove(*rev_dep);
-            // Reset the cached state (Parsed is the earliest one) so that it will
-            // re-resolve its imports.
-            self.cache
-                .update_state(*rev_dep, nickel_lang_core::cache::EntryState::Parsed);
         }
 
         self.file_uris.insert(file_id, uri);
@@ -117,11 +119,11 @@ impl World {
         &mut self,
         uri: Url,
         contents: String,
-    ) -> anyhow::Result<(FileId, HashSet<FileId>)> {
+    ) -> anyhow::Result<(FileId, Vec<FileId>)> {
         let path = uri_to_path(&uri)?;
         let file_id = self.cache.replace_string(SourcePath::Path(path), contents);
 
-        let invalid = self.cache.get_rev_imports_transitive(file_id);
+        let invalid = self.cache.invalidate_cache(file_id);
         for f in &invalid {
             self.analysis.remove(*f);
         }
@@ -351,4 +353,13 @@ impl World {
         }
         inner(self, span).unwrap_or_default()
     }
+
+    pub fn uris(&self, ids: impl IntoIterator<Item = FileId>) -> impl Iterator<Item = &Url> {
+        ids.into_iter().filter_map(|id| {
+            self.file_uris.get(&id).or_else(|| {
+                warn!("no uri for {id:?}");
+                None
+            })
+        })
+    }
 }


Test patch:
diff --git a/lsp/nls/tests/main.rs b/lsp/nls/tests/main.rs
index 37d21da9..442245f7 100644
--- a/lsp/nls/tests/main.rs
+++ b/lsp/nls/tests/main.rs
@@ -63,3 +63,51 @@ fn refresh_missing_imports() {
         }
     }
 }
+
+// Regression test for #1943, in which the cache was not properly reset
+// for imports that changed state from parseable to not-parseable.
+#[test]
+fn reload_broken_imports() {
+    let _ = env_logger::try_init();
+    let mut harness = TestHarness::new();
+
+    let url = |s: &str| lsp_types::Url::from_file_path(s).unwrap();
+    harness.send_file(url("/dep.ncl"), "{ x }");
+
+    harness.send_file(url("/test.ncl"), "import \"dep.ncl\"");
+    let diags = harness.wait_for_diagnostics();
+
+    assert_eq!("/dep.ncl", diags.uri.path());
+    assert!(diags.diagnostics.is_empty());
+
+    let diags = harness.wait_for_diagnostics();
+    assert_eq!("/test.ncl", diags.uri.path());
+    assert!(diags.diagnostics.is_empty());
+
+    // We expect two more diagnostics coming from background eval.
+    let _diags = harness.wait_for_diagnostics();
+    let _diags = harness.wait_for_diagnostics();
+
+    // Introduce an error in the import.
+    harness.send_file(url("/dep.ncl"), "{ `x = 1 }");
+
+    // Check that we get back clean diagnostics for both files.
+    // (LSP doesn't define the order, but we happen to know it)
+    // Loop because we can get back the diagnostics twice from each
+    // file (once from synchronous typechecking, once from eval in the background).
+    loop {
+        let diags = harness.wait_for_diagnostics();
+        if diags.uri.path() == "/dep.ncl" {
+            assert_eq!(diags.diagnostics[0].message, "unexpected token");
+            break;
+        }
+    }
+
+    loop {
+        let diags = harness.wait_for_diagnostics();
+        if diags.uri.path() == "/test.ncl" {
+            assert_eq!(diags.diagnostics[0].message, "unexpected token");
+            break;
+        }
+    }
+}