From fa487cfcce3fb5ec2996b82320b365fe9ceea18c Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Tue, 30 Jan 2024 14:01:01 -0600 Subject: [PATCH 1/3] Bump lsp deps --- Cargo.lock | 8 ++++---- Cargo.toml | 4 ++-- lsp/lsp-harness/src/jsonrpc.rs | 3 ++- lsp/nls/src/diagnostic.rs | 10 +++++----- lsp/nls/src/field_walker.rs | 6 +++--- lsp/nls/src/requests/completion.rs | 4 ++-- lsp/nls/src/requests/symbols.rs | 2 +- lsp/nls/src/server.rs | 2 +- 8 files changed, 20 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a1e0199218..7dc78a1516 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1418,9 +1418,9 @@ dependencies = [ [[package]] name = "lsp-server" -version = "0.6.0" +version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f70570c1c29cf6654029b8fe201a5507c153f0d85be6f234d471d756bc36775a" +checksum = "248f65b78f6db5d8e1b1604b4098a28b43d21a8eb1deeca22b1c421b276c7095" dependencies = [ "crossbeam-channel", "log", @@ -1430,9 +1430,9 @@ dependencies = [ [[package]] name = "lsp-types" -version = "0.88.0" +version = "0.95.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d8e8e042772e4e10b3785822f63c82399d0dd233825de44d2596f7fa86e023e0" +checksum = "158c1911354ef73e8fe42da6b10c0484cb65c7f1007f28022e847706c1ab6984" dependencies = [ "bitflags 1.3.2", "serde", diff --git a/Cargo.toml b/Cargo.toml index 1595e852a3..222e83d639 100644 --- a/Cargo.toml +++ b/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" diff --git a/lsp/lsp-harness/src/jsonrpc.rs b/lsp/lsp-harness/src/jsonrpc.rs index 7b200268d8..a2a68d06db 100644 --- a/lsp/lsp-harness/src/jsonrpc.rs +++ b/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::(InitializedParams {}) } diff --git a/lsp/nls/src/diagnostic.rs b/lsp/nls/src/diagnostic.rs index ac863b89eb..6895bfb6f8 100644 --- a/lsp/nls/src/diagnostic.rs +++ b/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, files: &mut Files) -> Vec { 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 diff --git a/lsp/nls/src/field_walker.rs b/lsp/nls/src/field_walker.rs index 5f90b15f7a..46af775397 100644 --- a/lsp/nls/src/field_walker.rs +++ b/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() } diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index 970f429443..fea5aeb5d2 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/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 diff --git a/lsp/nls/src/requests/symbols.rs b/lsp/nls/src/requests/symbols.rs index b8b8e4c606..5c4d22be7d 100644 --- a/lsp/nls/src/requests/symbols.rs +++ b/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, diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index 2bf9304148..de2e78617f 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -56,7 +56,7 @@ impl Server { text_document_sync: Some(TextDocumentSyncCapability::Options( TextDocumentSyncOptions { open_close: Some(true), - change: Some(TextDocumentSyncKind::Full), + change: Some(TextDocumentSyncKind::FULL), ..TextDocumentSyncOptions::default() }, )), From d342445d7daf3922ace775bde3d11f46bb30461d Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Tue, 30 Jan 2024 14:16:49 -0600 Subject: [PATCH 2/3] Support NICKEL_IMPORT_PATH in nls --- lsp/nls/src/server.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index de2e78617f..67ed4777f8 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -86,6 +86,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(); From 7022fca295cf448762a32fd1f410562b285b5cf7 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Tue, 30 Jan 2024 15:44:02 -0600 Subject: [PATCH 3/3] Improve cache invalidation for failed imports. We were already tracking import dependencies for cache invalidation, but only for imports that exist. Here, we add support for imports that fail because of I/O errors: we remember the filename of the failed import, and whenever nls becomes aware of a new file with that name, it invalidates the importers. --- lsp/nls/src/files.rs | 40 ++++++++++++++++++++++++++++++++++++++-- lsp/nls/src/server.rs | 14 +++++++++++++- lsp/nls/tests/main.rs | 25 +++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/lsp/nls/src/files.rs b/lsp/nls/src/files.rs index 64b8ee2ad9..c868c613e7 100644 --- a/lsp/nls/src/files.rs +++ b/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(¶ms.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!(), }) diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index 67ed4777f8..07fef4dab6 100644 --- a/lsp/nls/src/server.rs +++ b/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>, } impl Server { @@ -101,6 +112,7 @@ impl Server { analysis: AnalysisRegistry::default(), initial_ctxt, initial_term_env: crate::usage::Environment::new(), + failed_imports: HashMap::new(), } } diff --git a/lsp/nls/tests/main.rs b/lsp/nls/tests/main.rs index c2b6424a7c..3620f22665 100644 --- a/lsp/nls/tests/main.rs +++ b/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()); +}