Nls import improvements

Cargo.toml

@@ -59,8 +59,8 @@ lalrpop-util = "0.19.9"
 lazy_static = "1"
 log = "0.4"
 logos = "0.12"
-lsp-server = "0.6"
-lsp-types = "0.88"
+lsp-server = "0.7"
+lsp-types = "0.95"
 malachite = "0.4"
 malachite-q = "0.4"
 md-5 = "0.10.5"

lsp/lsp-harness/src/jsonrpc.rs

@@ -14,7 +14,7 @@ use lsp_types::{
     ClientCapabilities, DidChangeTextDocumentParams, DidOpenTextDocumentParams,
     GotoDefinitionParams, GotoDefinitionResponse, InitializeParams, InitializedParams, Position,
     TextDocumentContentChangeEvent, TextDocumentIdentifier, TextDocumentPositionParams, Url,
-    VersionedTextDocumentIdentifier,
+    VersionedTextDocumentIdentifier, WorkDoneProgressParams,
 };
 use std::{
     io::{BufRead, BufReader, Read, Write},
@@ -166,6 +166,7 @@ impl Server {
             workspace_folders: None,
             client_info: None,
             locale: None,
+            work_done_progress_params: WorkDoneProgressParams::default(),
         })?;
         self.send_notification::<Initialized>(InitializedParams {})
     }

lsp/nls/src/diagnostic.rs

@@ -57,11 +57,11 @@ impl LocationCompat for lsp_types::Location {
 impl DiagnosticCompat for lsp_types::Diagnostic {
     fn from_codespan(diagnostic: Diagnostic<FileId>, files: &mut Files<String>) -> Vec<Self> {
         let severity = Some(match diagnostic.severity {
-            diagnostic::Severity::Bug => lsp_types::DiagnosticSeverity::Warning,
-            diagnostic::Severity::Error => lsp_types::DiagnosticSeverity::Error,
-            diagnostic::Severity::Warning => lsp_types::DiagnosticSeverity::Warning,
-            diagnostic::Severity::Note => lsp_types::DiagnosticSeverity::Information,
-            diagnostic::Severity::Help => lsp_types::DiagnosticSeverity::Hint,
+            diagnostic::Severity::Bug => lsp_types::DiagnosticSeverity::WARNING,
+            diagnostic::Severity::Error => lsp_types::DiagnosticSeverity::ERROR,
+            diagnostic::Severity::Warning => lsp_types::DiagnosticSeverity::WARNING,
+            diagnostic::Severity::Note => lsp_types::DiagnosticSeverity::INFORMATION,
+            diagnostic::Severity::Help => lsp_types::DiagnosticSeverity::HINT,
         });
 
         diagnostic

lsp/nls/src/field_walker.rs

@@ -48,7 +48,7 @@ impl Record {
                 .map(|(id, val)| CompletionItem {
                     label: ident_quoted(id),
                     detail: metadata_detail(&val.metadata),
-                    kind: Some(CompletionItemKind::Property),
+                    kind: Some(CompletionItemKind::PROPERTY),
                     documentation: metadata_doc(&val.metadata),
                     ident: Some((*id).into()),
                 })
@@ -60,7 +60,7 @@ impl Record {
                     RecordRowsIteratorItem::TailVar(_) => None,
                     RecordRowsIteratorItem::Row(r) => Some(CompletionItem {
                         label: ident_quoted(&r.id),
-                        kind: Some(CompletionItemKind::Property),
+                        kind: Some(CompletionItemKind::PROPERTY),
                         detail: Some(r.typ.to_string()),
                         ..Default::default()
                     }),
@@ -236,7 +236,7 @@ impl Def {
         CompletionItem {
             label: ident_quoted(&self.ident().into()),
             detail: self.metadata().and_then(metadata_detail),
-            kind: Some(CompletionItemKind::Property),
+            kind: Some(CompletionItemKind::PROPERTY),
             documentation: self.metadata().and_then(metadata_doc),
             ..Default::default()
         }

lsp/nls/src/files.rs

@@ -11,7 +11,7 @@ use lsp_types::{
 };
 use nickel_lang_core::{
     cache::{CacheError, CacheOp, SourcePath},
-    error::IntoDiagnostics,
+    error::{ImportError, IntoDiagnostics},
 };
 
 use crate::{
@@ -46,12 +46,32 @@ pub fn handle_open(server: &mut Server, params: DidOpenTextDocumentParams) -> Re
         },
     );
     let path = uri_to_path(&params.text_document.uri)?;
+
+    // Invalidate the cache of every file that tried, but failed, to import a file
+    // with a name like this.
+    let invalid = path
+        .file_name()
+        .and_then(|name| server.failed_imports.remove(name))
+        .unwrap_or_default();
+    for rev_dep in &invalid {
+        server.analysis.remove(*rev_dep);
+        // Reset the cached state (Parsed is the earliest one) so that it will
+        // re-resolve its imports.
+        server
+            .cache
+            .update_state(*rev_dep, nickel_lang_core::cache::EntryState::Parsed);
+    }
+
     let file_id = server
         .cache
         .add_string(SourcePath::Path(path), params.text_document.text);
     server.file_uris.insert(file_id, params.text_document.uri);
 
     parse_and_typecheck(server, file_id)?;
+
+    for rev_dep in &invalid {
+        parse_and_typecheck(server, *rev_dep)?;
+    }
     Trace::reply(id);
     Ok(())
 }
@@ -97,6 +117,19 @@ pub fn handle_save(server: &mut Server, params: DidChangeTextDocumentParams) ->
     Ok(())
 }
 
+// Make a record of I/O errors in imports so that we can retry them when appropriate.
+fn associate_failed_import(server: &mut Server, err: &nickel_lang_core::error::Error) {
+    if let nickel_lang_core::error::Error::ImportError(ImportError::IOError(name, _, pos)) = &err {
+        if let Some((filename, pos)) = PathBuf::from(name).file_name().zip(pos.into_opt()) {
+            server
+                .failed_imports
+                .entry(filename.to_owned())
+                .or_default()
+                .insert(pos.src_id);
+        }
+    }
+}
+
 pub(crate) fn typecheck(
     server: &mut Server,
     file_id: FileId,
@@ -112,7 +145,10 @@ pub(crate) fn typecheck(
         .map_err(|error| match error {
             CacheError::Error(tc_error) => tc_error
                 .into_iter()
-                .flat_map(|err| err.into_diagnostics(server.cache.files_mut(), None))
+                .flat_map(|err| {
+                    associate_failed_import(server, &err);
+                    err.into_diagnostics(server.cache.files_mut(), None)
+                })
                 .collect(),
             CacheError::NotParsed => unreachable!(),
         })

lsp/nls/src/requests/completion.rs

@@ -254,9 +254,9 @@ fn handle_import_completion(
         })
         .map(|entry| {
             let kind = if entry.file {
-                CompletionItemKind::File
+                CompletionItemKind::FILE
             } else {
-                CompletionItemKind::Folder
+                CompletionItemKind::FOLDER
             };
             lsp_types::CompletionItem {
                 label: entry

lsp/nls/src/requests/symbols.rs

@@ -33,7 +33,7 @@ pub fn handle_document_symbols(
             Some(DocumentSymbol {
                 name: ident.ident.to_string(),
                 detail: ty.map(Type::to_string),
-                kind: SymbolKind::Variable,
+                kind: SymbolKind::VARIABLE,
                 tags: None,
                 range,
                 selection_range: range,

lsp/nls/src/server.rs

@@ -1,4 +1,7 @@
-use std::collections::HashMap;
+use std::{
+    collections::{HashMap, HashSet},
+    ffi::OsString,
+};
 
 use anyhow::Result;
 use codespan::FileId;
@@ -48,6 +51,14 @@ pub struct Server {
     pub analysis: AnalysisRegistry,
     pub initial_ctxt: Context,
     pub initial_term_env: crate::usage::Environment,
+
+    /// A map associating imported files with failed imports. This allows us to
+    /// invalidate the cached version of a file when one of its imports becomes available.
+    ///
+    /// The keys in this map are the filenames (just the basename; no directory) of the
+    /// files that failed to import, and the values in this map are the file ids that tried
+    /// to import it.
+    pub failed_imports: HashMap<OsString, HashSet<FileId>>,
 }
 
 impl Server {
@@ -56,7 +67,7 @@ impl Server {
             text_document_sync: Some(TextDocumentSyncCapability::Options(
                 TextDocumentSyncOptions {
                     open_close: Some(true),
-                    change: Some(TextDocumentSyncKind::Full),
+                    change: Some(TextDocumentSyncKind::FULL),
                     ..TextDocumentSyncOptions::default()
                 },
             )),
@@ -86,6 +97,11 @@ impl Server {
 
     pub fn new(connection: Connection) -> Server {
         let mut cache = Cache::new(ErrorTolerance::Tolerant);
+
+        if let Ok(nickel_path) = std::env::var("NICKEL_IMPORT_PATH") {
+            cache.add_import_paths(nickel_path.split(':'));
+        }
+
         // We don't recover from failing to load the stdlib for now.
         cache.load_stdlib().unwrap();
         let initial_ctxt = cache.mk_type_ctxt().unwrap();
@@ -96,6 +112,7 @@ impl Server {
             analysis: AnalysisRegistry::default(),
             initial_ctxt,
             initial_term_env: crate::usage::Environment::new(),
+            failed_imports: HashMap::new(),
         }
     }
 

lsp/nls/tests/main.rs

@@ -22,3 +22,28 @@ fn check_snapshots(path: &str) {
 
     insta::assert_snapshot!(path, output);
 }
+
+#[test]
+fn refresh_missing_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("/test.ncl"), "import \"dep.ncl\"");
+    let diags = harness.wait_for_diagnostics().diagnostics;
+    assert_eq!(1, diags.len());
+    assert!(diags[0].message.contains("import of dep.ncl failed"));
+
+    // Now provide the import.
+    harness.send_file(url("/dep.ncl"), "42");
+
+    // Check that we get back clean diagnostics for both files.
+    // (LSP doesn't define the order, but we happen to know it)
+    let diags = harness.wait_for_diagnostics();
+    assert_eq!(diags.uri.path(), "/dep.ncl");
+    assert!(diags.diagnostics.is_empty());
+
+    let diags = harness.wait_for_diagnostics();
+    assert_eq!(diags.uri.path(), "/test.ncl");
+    assert!(diags.diagnostics.is_empty());
+}