From c4221730bf156247160d46ca418adc2cfb9f9b97 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Fri, 20 Oct 2023 17:14:10 +0200 Subject: [PATCH 01/10] Define 'ExtraData' trait for errors This should allow passing some extra information to LSP diagnostic in order to provide quickfix actions, such as auto-imports. --- crates/aiken-lang/src/error/mod.rs | 3 ++ crates/aiken-lang/src/lib.rs | 1 + crates/aiken-lang/src/tipo/error.rs | 82 +++++++++++++++++++++++++++++ crates/aiken-project/src/error.rs | 39 ++++++++++++++ 4 files changed, 125 insertions(+) create mode 100644 crates/aiken-lang/src/error/mod.rs diff --git a/crates/aiken-lang/src/error/mod.rs b/crates/aiken-lang/src/error/mod.rs new file mode 100644 index 00000000..30b902c3 --- /dev/null +++ b/crates/aiken-lang/src/error/mod.rs @@ -0,0 +1,3 @@ +pub trait ExtraData { + fn extra_data(&self) -> Option; +} diff --git a/crates/aiken-lang/src/lib.rs b/crates/aiken-lang/src/lib.rs index 3b4eb8de..3e7dc7cc 100644 --- a/crates/aiken-lang/src/lib.rs +++ b/crates/aiken-lang/src/lib.rs @@ -5,6 +5,7 @@ use std::sync::{ pub mod ast; pub mod builtins; +pub mod error; pub mod expr; pub mod format; pub mod gen_uplc; diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 396da272..5c695d20 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -1,4 +1,5 @@ use super::Type; +use crate::error::ExtraData; use crate::{ ast::{Annotation, BinOp, CallArg, LogicalOpChainKind, Span, UntypedPattern}, expr::{self, UntypedExpr}, @@ -922,6 +923,63 @@ The best thing to do from here is to remove it."#))] }, } +impl ExtraData for Error { + fn extra_data(&self) -> Option { + match self { + Error::CastDataNoAnn { .. } + | Error::CouldNotUnify { .. } + | Error::CyclicTypeDefinitions { .. } + | Error::DuplicateArgument { .. } + | Error::DuplicateConstName { .. } + | Error::DuplicateField { .. } + | Error::DuplicateImport { .. } + | Error::DuplicateName { .. } + | Error::DuplicateTypeName { .. } + | Error::DuplicateVarInPattern { .. } + | Error::ExtraVarInAlternativePattern { .. } + | Error::FunctionTypeInData { .. } + | Error::ImplicitlyDiscardedExpression { .. } + | Error::IncorrectFieldsArity { .. } + | Error::IncorrectFunctionCallArity { .. } + | Error::IncorrectPatternArity { .. } + | Error::IncorrectTupleArity { .. } + | Error::IncorrectTypeArity { .. } + | Error::IncorrectValidatorArity { .. } + | Error::KeywordInModuleName { .. } + | Error::LastExpressionIsAssignment { .. } + | Error::LogicalOpChainMissingExpr { .. } + | Error::MissingVarInAlternativePattern { .. } + | Error::MultiValidatorEqualArgs { .. } + | Error::NonLocalClauseGuardVariable { .. } + | Error::NotATuple { .. } + | Error::NotExhaustivePatternMatch { .. } + | Error::NotFn { .. } + | Error::PositionalArgumentAfterLabeled { .. } + | Error::PrivateTypeLeak { .. } + | Error::RecordAccessUnknownType { .. } + | Error::RecordUpdateInvalidConstructor { .. } + | Error::RecursiveType { .. } + | Error::RedundantMatchClause { .. } + | Error::TupleIndexOutOfBound { .. } + | Error::UnexpectedLabeledArg { .. } + | Error::UnexpectedLabeledArgInPattern { .. } + | Error::UnknownLabels { .. } + | Error::UnknownModule { .. } + | Error::UnknownModuleField { .. } + | Error::UnknownModuleType { .. } + | Error::UnknownModuleValue { .. } + | Error::UnknownRecordField { .. } + | Error::UnknownType { .. } + | Error::UnknownTypeConstructor { .. } + | Error::UnnecessarySpreadOperator { .. } + | Error::UpdateMultiConstructorType { .. } + | Error::ValidatorImported { .. } + | Error::ValidatorMustReturnBool { .. } => None, + Error::UnknownVariable { name, .. } => Some(name.clone()), + } + } +} + impl Error { pub fn call_situation(mut self) -> Self { if let Error::UnknownRecordField { @@ -1522,6 +1580,30 @@ pub enum Warning { }, } +impl ExtraData for Warning { + fn extra_data(&self) -> Option { + match self { + Warning::AllFieldsRecordUpdate { .. } + | Warning::ImplicitlyDiscardedResult { .. } + | Warning::NoFieldsRecordUpdate { .. } + | Warning::PubInValidatorModule { .. } + | Warning::SingleConstructorExpect { .. } + | Warning::SingleWhenClause { .. } + | Warning::Todo { .. } + | Warning::UnexpectedTypeHole { .. } + | Warning::UnusedConstructor { .. } + | Warning::UnusedImportedModule { .. } + | Warning::UnusedImportedValue { .. } + | Warning::UnusedPrivateFunction { .. } + | Warning::UnusedPrivateModuleConstant { .. } + | Warning::UnusedType { .. } + | Warning::UnusedVariable { .. } + | Warning::Utf8ByteArrayIsValidHexString { .. } + | Warning::ValidatorInLibraryModule { .. } => None, + } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum UnifyErrorSituation { /// Clauses in a case expression were found to return different types. diff --git a/crates/aiken-project/src/error.rs b/crates/aiken-project/src/error.rs index f657c83b..0dcd2653 100644 --- a/crates/aiken-project/src/error.rs +++ b/crates/aiken-project/src/error.rs @@ -4,6 +4,7 @@ use crate::{ }; use aiken_lang::{ ast::{self, BinOp, Span}, + error::ExtraData, parser::error::ParseError, tipo, }; @@ -172,6 +173,34 @@ impl From for Vec { } } +impl ExtraData for Error { + fn extra_data(&self) -> Option { + match self { + Error::DuplicateModule { .. } => None, + Error::FileIo { .. } => None, + Error::Format { .. } => None, + Error::StandardIo { .. } => None, + Error::Blueprint { .. } => None, + Error::MissingManifest { .. } => None, + Error::TomlLoading { .. } => None, + Error::ImportCycle { .. } => None, + Error::Parse { .. } => None, + Error::Type { error, .. } => error.extra_data(), + Error::TestFailure { .. } => None, + Error::Http { .. } => None, + Error::ZipExtract { .. } => None, + Error::JoinError { .. } => None, + Error::UnknownPackageVersion { .. } => None, + Error::UnableToResolvePackage { .. } => None, + Error::Json { .. } => None, + Error::MalformedStakeAddress { .. } => None, + Error::NoValidatorNotFound { .. } => None, + Error::MoreThanOneValidatorFound { .. } => None, + Error::Module { .. } => None, + } + } +} + pub trait GetSource { fn path(&self) -> Option; fn src(&self) -> Option; @@ -481,6 +510,16 @@ pub enum Warning { DependencyAlreadyExists { name: PackageName }, } +impl ExtraData for Warning { + fn extra_data(&self) -> Option { + match self { + Warning::NoValidators { .. } => None, + Warning::DependencyAlreadyExists { .. } => None, + Warning::Type { warning, .. } => warning.extra_data(), + } + } +} + impl GetSource for Warning { fn path(&self) -> Option { match self { From 66ade8e3e30931676585bf3b3d13710102ff4541 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Fri, 20 Oct 2023 17:59:18 +0200 Subject: [PATCH 02/10] Implement simple code action quickfix for unknown variable. --- crates/aiken-lang/src/ast.rs | 12 +++ crates/aiken-lsp/src/lib.rs | 1 + crates/aiken-lsp/src/server.rs | 102 ++++++++++++++++++++- crates/aiken-lsp/src/server/lsp_project.rs | 4 +- 4 files changed, 113 insertions(+), 6 deletions(-) diff --git a/crates/aiken-lang/src/ast.rs b/crates/aiken-lang/src/ast.rs index 61f93e6a..ace86680 100644 --- a/crates/aiken-lang/src/ast.rs +++ b/crates/aiken-lang/src/ast.rs @@ -78,6 +78,18 @@ impl TypedModule { .find_map(|definition| definition.find_node(byte_index)) } + pub fn has_definition(&self, name: &str) -> bool { + self.definitions.iter().any(|def| match def { + Definition::Fn(f) => f.public && f.name == name, + Definition::TypeAlias(_) => false, + Definition::DataType(_) => false, + Definition::Use(_) => false, + Definition::ModuleConstant(_) => false, + Definition::Test(_) => false, + Definition::Validator(_) => false, + }) + } + pub fn validate_module_name(&self) -> Result<(), Error> { if self.name == "aiken" || self.name == "aiken/builtin" { return Err(Error::ReservedModuleName { diff --git a/crates/aiken-lsp/src/lib.rs b/crates/aiken-lsp/src/lib.rs index 310fe37e..0c0fbc45 100644 --- a/crates/aiken-lsp/src/lib.rs +++ b/crates/aiken-lsp/src/lib.rs @@ -60,6 +60,7 @@ fn capabilities() -> lsp_types::ServerCapabilities { // work_done_progress: None, // }, // }), + code_action_provider: Some(lsp_types::CodeActionProviderCapability::Simple(true)), document_formatting_provider: Some(lsp_types::OneOf::Left(true)), definition_provider: Some(lsp_types::OneOf::Left(true)), hover_provider: Some(lsp_types::HoverProviderCapability::Simple(true)), diff --git a/crates/aiken-lsp/src/server.rs b/crates/aiken-lsp/src/server.rs index ff42e881..3249b954 100644 --- a/crates/aiken-lsp/src/server.rs +++ b/crates/aiken-lsp/src/server.rs @@ -6,6 +6,7 @@ use std::{ use aiken_lang::{ ast::{Definition, Located, ModuleKind, Span, Use}, + error::ExtraData, parser, tipo::pretty::Printer, }; @@ -23,7 +24,8 @@ use lsp_types::{ Notification, Progress, PublishDiagnostics, ShowMessage, }, request::{ - Completion, Formatting, GotoDefinition, HoverRequest, Request, WorkDoneProgressCreate, + CodeActionRequest, Completion, Formatting, GotoDefinition, HoverRequest, Request, + WorkDoneProgressCreate, }, DocumentFormattingParams, InitializeParams, TextEdit, }; @@ -44,6 +46,8 @@ use self::lsp_project::LspProject; mod lsp_project; pub mod telemetry; +const UNKNOWN_VARIABLE: &str = "aiken::check::unknown::variable"; + #[allow(dead_code)] pub struct Server { // Project root directory @@ -288,6 +292,7 @@ impl Server { } } } + HoverRequest::METHOD => { let params = cast_request::(request)?; @@ -324,6 +329,48 @@ impl Server { }) } + CodeActionRequest::METHOD => { + let params = + cast_request::(request).expect("cast code action request"); + + // Identify any diagnostic which refers to an unknown variable. In which case, we + // might want to provide some imports suggestion. + let unknown_variables = params + .context + .diagnostics + .into_iter() + .filter(|diagnostic| { + let is_error = + diagnostic.severity == Some(lsp_types::DiagnosticSeverity::ERROR); + let is_unknown_variable = diagnostic.code + == Some(lsp_types::NumberOrString::String( + UNKNOWN_VARIABLE.to_string(), + )); + + is_error && is_unknown_variable + }) + .collect::>(); + + match unknown_variables.first() { + Some(diagnostic) => Ok(lsp_server::Response { + id, + error: None, + result: Some(serde_json::to_value( + self.quickfix_unknown_variable( + params.text_document, + diagnostic.to_owned(), + ) + .unwrap_or(vec![]), + )?), + }), + None => Ok(lsp_server::Response { + id, + error: None, + result: Some(serde_json::Value::Null), + }), + } + } + unsupported => Err(ServerError::UnsupportedLspRequest { request: unsupported.to_string(), }), @@ -355,6 +402,54 @@ impl Server { } } + fn quickfix_unknown_variable( + &self, + text_document: lsp_types::TextDocumentIdentifier, + diagnostic: lsp_types::Diagnostic, + ) -> Option> { + let compiler = self.compiler.as_ref()?; + + let mut actions = Vec::new(); + + if let Some(serde_json::Value::String(ref var_name)) = diagnostic.data { + for module in compiler.project.modules() { + let mut changes = HashMap::new(); + + if module.ast.has_definition(var_name) { + let import_line = format!("use {}.{{{}}}", module.name, var_name); + + changes.insert( + text_document.uri.clone(), + vec![lsp_types::TextEdit { + range: lsp_types::Range { + start: lsp_types::Position::new(0, 0), + end: lsp_types::Position::new(0, 0), + }, + new_text: format!("{import_line}\n"), + }], + ); + + actions.push(lsp_types::CodeAction { + title: import_line, + kind: Some(lsp_types::CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + is_preferred: Some(true), + disabled: None, + data: None, + command: None, + edit: Some(lsp_types::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + }); + } + } + } + + Some(actions) + } + fn completion_for_import(&self) -> Option> { let compiler = self.compiler.as_ref()?; @@ -445,7 +540,6 @@ impl Server { fn module_for_uri(&self, uri: &url::Url) -> Option<&CheckedModule> { self.compiler.as_ref().and_then(|compiler| { let module_name = uri_to_module_name(uri, &self.root).expect("uri to module name"); - compiler.modules.get(&module_name) }) } @@ -591,7 +685,7 @@ impl Server { /// the `showMessage` notification instead. fn process_diagnostic(&mut self, error: E) -> Result<(), ServerError> where - E: Diagnostic + GetSource, + E: Diagnostic + GetSource + ExtraData, { let (severity, typ) = match error.severity() { Some(severity) => match severity { @@ -642,7 +736,7 @@ impl Server { message, related_information: None, tags: None, - data: None, + data: error.extra_data().map(serde_json::Value::String), }; #[cfg(not(target_os = "windows"))] diff --git a/crates/aiken-lsp/src/server/lsp_project.rs b/crates/aiken-lsp/src/server/lsp_project.rs index e581f86b..4c58a79d 100644 --- a/crates/aiken-lsp/src/server/lsp_project.rs +++ b/crates/aiken-lsp/src/server/lsp_project.rs @@ -38,8 +38,6 @@ impl LspProject { self.project.restore(checkpoint); - result?; - let modules = self.project.modules(); for mut module in modules.into_iter() { @@ -61,6 +59,8 @@ impl LspProject { self.modules.insert(module.name.to_string(), module); } + result?; + Ok(()) } } From 699d0a537c77abcd633eb4c816ba92c26ff921f7 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 10:42:59 +0200 Subject: [PATCH 03/10] Use (untyped) AST to find the right insert location for imports. This removes the need to rely on the formatter to clear things up after insert a new import. While this is not so useful for imports, I wanted to experiment with the approach for future similar edits (for example, when suggesting an inline rewrite). --- crates/aiken-lang/src/ast.rs | 89 ++++++++++++++++++++++++++++++++++ crates/aiken-lsp/src/server.rs | 88 +++++++++++++++++++++++---------- 2 files changed, 152 insertions(+), 25 deletions(-) diff --git a/crates/aiken-lang/src/ast.rs b/crates/aiken-lang/src/ast.rs index ace86680..a68f3464 100644 --- a/crates/aiken-lang/src/ast.rs +++ b/crates/aiken-lang/src/ast.rs @@ -69,6 +69,95 @@ impl UntypedModule { }) .collect() } + + /// Find a suitable location (Span) in the import list. The boolean in the answer indicates + /// whether the import is a newline or not. It is set to 'false' when adding a qualified import + /// to an existing list. + pub fn edit_import(&self, module: &[&str], unqualified: Option<&str>) -> Option { + let mut last_import = None; + + for def in self.definitions() { + match def { + Definition::Use(Use { + location, + module: existing_module, + unqualified: unqualified_list, + .. + }) => { + last_import = Some(*location); + + if module != existing_module.as_slice() { + continue; + } + + match unqualified { + // There's already a matching qualified import, so we have nothing to do. + None => return None, + Some(unqualified) => { + // Insert lexicographically, assuming unqualified imports are already + // ordered. If they are not, it doesn't really matter where we insert + // anyway. + for existing_unqualified in unqualified_list { + let existing_name = existing_unqualified + .as_name + .as_ref() + .unwrap_or(&existing_unqualified.name); + + // The unqualified import already exist, nothing to do. + if unqualified == existing_name { + return None; + // Current import is lexicographically smaller, we can insert after. + } else if existing_name.as_str() < unqualified { + return Some(EditImport { + location: Span { + start: existing_unqualified.location.end, + end: existing_unqualified.location.end, + }, + is_new_line: false, + is_first_unqualified: false, + }); + } else { + continue; + } + } + + // Only happens if 'unqualified_list' is empty, in which case, we + // simply create a new unqualified list of import. + return Some(EditImport { + location: Span { + start: location.end, + end: location.end, + }, + is_new_line: false, + is_first_unqualified: true, + }); + } + } + } + _ => continue, + } + } + + // If the search above didn't lead to anything, we simply insert the import either: + // + // (a) After the last import statement if any; + // (b) As the first statement in the module. + Some(EditImport { + location: match last_import { + None => Span { start: 0, end: 0 }, + Some(Span { end, .. }) => Span { start: end, end }, + }, + is_new_line: true, + is_first_unqualified: false, + }) + } +} + +#[derive(Debug, Clone, PartialEq)] +pub struct EditImport { + pub location: Span, + pub is_new_line: bool, + pub is_first_unqualified: bool, } impl TypedModule { diff --git a/crates/aiken-lsp/src/server.rs b/crates/aiken-lsp/src/server.rs index 3249b954..eb0bb0ce 100644 --- a/crates/aiken-lsp/src/server.rs +++ b/crates/aiken-lsp/src/server.rs @@ -409,40 +409,78 @@ impl Server { ) -> Option> { let compiler = self.compiler.as_ref()?; + let file_path = text_document + .uri + .to_file_path() + .expect("invalid text document uri?"); + + let source_code = fs::read_to_string(&file_path).ok()?; + + let line_numbers = LineNumbers::new(&source_code); + + // NOTE: The 'ModuleKind' second argument doesn't matter. This is just added to the final + // object but has no influence on the parsing. + let (untyped_module, _) = aiken_lang::parser::module(&source_code, ModuleKind::Lib).ok()?; + let mut actions = Vec::new(); if let Some(serde_json::Value::String(ref var_name)) = diagnostic.data { for module in compiler.project.modules() { let mut changes = HashMap::new(); + let module_path = module.name.split('/').collect_vec(); if module.ast.has_definition(var_name) { - let import_line = format!("use {}.{{{}}}", module.name, var_name); + if let Some(edit) = + untyped_module.edit_import(module_path.as_slice(), Some(var_name)) + { + let (title, new_text) = if edit.is_new_line { + ( + format!( + "Add new import line: use {}.{{{}}}", + module.name, var_name + ), + format!("\nuse {}.{{{}}}", module.name, var_name), + ) + } else if edit.is_first_unqualified { + ( + format!( + "Add new unqualified import '{}' to {}", + var_name, module.name + ), + format!(".{{{}}}", var_name), + ) + } else { + ( + format!( + "Add new unqualified import '{}' to {}", + var_name, module.name + ), + format!(", {}", var_name), + ) + }; - changes.insert( - text_document.uri.clone(), - vec![lsp_types::TextEdit { - range: lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 0), - }, - new_text: format!("{import_line}\n"), - }], - ); + let range = span_to_lsp_range(edit.location, &line_numbers); - actions.push(lsp_types::CodeAction { - title: import_line, - kind: Some(lsp_types::CodeActionKind::QUICKFIX), - diagnostics: Some(vec![diagnostic.clone()]), - is_preferred: Some(true), - disabled: None, - data: None, - command: None, - edit: Some(lsp_types::WorkspaceEdit { - changes: Some(changes), - document_changes: None, - change_annotations: None, - }), - }); + changes.insert( + text_document.uri.clone(), + vec![lsp_types::TextEdit { range, new_text }], + ); + + actions.push(lsp_types::CodeAction { + title, + kind: Some(lsp_types::CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + is_preferred: Some(true), + disabled: None, + data: None, + command: None, + edit: Some(lsp_types::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + }); + } } } } From 763516eb9655333c9e855769ab55be67f35b064b Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 11:31:01 +0200 Subject: [PATCH 04/10] Refactor and relocate document edits function for imports. It's a bit 'off-topic' to keep these in aiken-lang as those functions are really just about lsp. Plus, it removes a bit some of the boilerplate and make the entire edition more readable and re-usable. Now we can tackle other similar errors with the same quickfix. --- crates/aiken-lang/src/ast.rs | 89 ---------------- crates/aiken-lsp/src/edits.rs | 180 +++++++++++++++++++++++++++++++++ crates/aiken-lsp/src/lib.rs | 1 + crates/aiken-lsp/src/server.rs | 53 +--------- 4 files changed, 185 insertions(+), 138 deletions(-) create mode 100644 crates/aiken-lsp/src/edits.rs diff --git a/crates/aiken-lang/src/ast.rs b/crates/aiken-lang/src/ast.rs index a68f3464..ace86680 100644 --- a/crates/aiken-lang/src/ast.rs +++ b/crates/aiken-lang/src/ast.rs @@ -69,95 +69,6 @@ impl UntypedModule { }) .collect() } - - /// Find a suitable location (Span) in the import list. The boolean in the answer indicates - /// whether the import is a newline or not. It is set to 'false' when adding a qualified import - /// to an existing list. - pub fn edit_import(&self, module: &[&str], unqualified: Option<&str>) -> Option { - let mut last_import = None; - - for def in self.definitions() { - match def { - Definition::Use(Use { - location, - module: existing_module, - unqualified: unqualified_list, - .. - }) => { - last_import = Some(*location); - - if module != existing_module.as_slice() { - continue; - } - - match unqualified { - // There's already a matching qualified import, so we have nothing to do. - None => return None, - Some(unqualified) => { - // Insert lexicographically, assuming unqualified imports are already - // ordered. If they are not, it doesn't really matter where we insert - // anyway. - for existing_unqualified in unqualified_list { - let existing_name = existing_unqualified - .as_name - .as_ref() - .unwrap_or(&existing_unqualified.name); - - // The unqualified import already exist, nothing to do. - if unqualified == existing_name { - return None; - // Current import is lexicographically smaller, we can insert after. - } else if existing_name.as_str() < unqualified { - return Some(EditImport { - location: Span { - start: existing_unqualified.location.end, - end: existing_unqualified.location.end, - }, - is_new_line: false, - is_first_unqualified: false, - }); - } else { - continue; - } - } - - // Only happens if 'unqualified_list' is empty, in which case, we - // simply create a new unqualified list of import. - return Some(EditImport { - location: Span { - start: location.end, - end: location.end, - }, - is_new_line: false, - is_first_unqualified: true, - }); - } - } - } - _ => continue, - } - } - - // If the search above didn't lead to anything, we simply insert the import either: - // - // (a) After the last import statement if any; - // (b) As the first statement in the module. - Some(EditImport { - location: match last_import { - None => Span { start: 0, end: 0 }, - Some(Span { end, .. }) => Span { start: end, end }, - }, - is_new_line: true, - is_first_unqualified: false, - }) - } -} - -#[derive(Debug, Clone, PartialEq)] -pub struct EditImport { - pub location: Span, - pub is_new_line: bool, - pub is_first_unqualified: bool, } impl TypedModule { diff --git a/crates/aiken-lsp/src/edits.rs b/crates/aiken-lsp/src/edits.rs new file mode 100644 index 00000000..280ec4f7 --- /dev/null +++ b/crates/aiken-lsp/src/edits.rs @@ -0,0 +1,180 @@ +use crate::{line_numbers::LineNumbers, utils::span_to_lsp_range}; +use aiken_lang::ast::{Definition, ModuleKind, Span, UntypedDefinition, Use}; +use aiken_project::module::CheckedModule; +use itertools::Itertools; +use std::fs; + +/// A freshly parsed module alongside its line numbers. +pub struct ParsedDocument { + definitions: Vec, + line_numbers: LineNumbers, +} + +/// Parse the target document as an 'UntypedModule' alongside its line numbers. This is useful in +/// case we need to manipulate the AST for a quickfix. +pub fn parse_document(document: &lsp_types::TextDocumentIdentifier) -> Option { + let file_path = document + .uri + .to_file_path() + .expect("invalid text document uri?"); + + let source_code = fs::read_to_string(file_path).ok()?; + + let line_numbers = LineNumbers::new(&source_code); + + // NOTE: The 'ModuleKind' second argument doesn't matter. This is just added to the final + // object but has no influence on the parsing. + let (untyped_module, _) = aiken_lang::parser::module(&source_code, ModuleKind::Lib).ok()?; + + Some(ParsedDocument { + definitions: untyped_module.definitions, + line_numbers, + }) +} + +/// Insert some text at the given location. +fn insert_text(at: usize, line_numbers: &LineNumbers, new_text: String) -> lsp_types::TextEdit { + let range = span_to_lsp_range(Span { start: at, end: at }, line_numbers); + lsp_types::TextEdit { range, new_text } +} + +/// Find a suitable location (Span) in the import list. The boolean in the answer indicates +/// whether the import is a newline or not. It is set to 'false' when adding a qualified import +/// to an existing list. +impl ParsedDocument { + pub fn import( + &self, + import: &CheckedModule, + unqualified: Option<&str>, + ) -> Option<(String, lsp_types::TextEdit)> { + let import_path = import.name.split('/').collect_vec(); + + let mut last_import = None; + + for def in self.definitions.iter() { + match def { + Definition::Use(Use { + location, + module: existing_module, + unqualified: unqualified_list, + .. + }) => { + last_import = Some(*location); + + if import_path != existing_module.as_slice() { + continue; + } + + match unqualified { + // There's already a matching qualified import, so we have nothing to do. + None => return None, + Some(unqualified) => { + // Insert lexicographically, assuming unqualified imports are already + // ordered. If they are not, it doesn't really matter where we insert + // anyway. + for existing_unqualified in unqualified_list { + let existing_name = existing_unqualified + .as_name + .as_ref() + .unwrap_or(&existing_unqualified.name); + + // The unqualified import already exist, nothing to do. + if unqualified == existing_name { + return None; + // Current import is lexicographically smaller, we can insert after. + } else if existing_name.as_str() < unqualified { + return Some(self.insert_into_qualified( + import, + unqualified, + existing_unqualified.location, + )); + } else { + continue; + } + } + + // Only happens if 'unqualified_list' is empty, in which case, we + // simply create a new unqualified list of import. + return Some(self.add_new_qualified(import, unqualified, *location)); + } + } + } + _ => continue, + } + } + + // If the search above didn't lead to anything, we simply insert the import either: + // + // (a) After the last import statement if any; + // (b) As the first statement in the module. + Some(self.add_new_import_line(import, unqualified, last_import)) + } + + fn insert_into_qualified( + &self, + import: &CheckedModule, + unqualified: &str, + location: Span, + ) -> (String, lsp_types::TextEdit) { + let title = format!( + "Insert new unqualified import '{}' to {}", + unqualified, import.name + ); + ( + title, + insert_text( + location.end, + &self.line_numbers, + format!(", {}", unqualified), + ), + ) + } + + fn add_new_qualified( + &self, + import: &CheckedModule, + unqualified: &str, + location: Span, + ) -> (String, lsp_types::TextEdit) { + let title = format!( + "Add new unqualified import '{}' to {}", + unqualified, import.name + ); + ( + title, + insert_text( + location.end, + &self.line_numbers, + format!(".{{{}}}", unqualified), + ), + ) + } + + fn add_new_import_line( + &self, + import: &CheckedModule, + unqualified: Option<&str>, + location: Option, + ) -> (String, lsp_types::TextEdit) { + let import_line = format!( + "use {}{}", + import.name, + match unqualified { + Some(unqualified) => format!(".{{{}}}", unqualified), + None => String::new(), + } + ); + + let title = format!("Add new import line: {import_line}"); + + ( + title, + match location { + None => insert_text(0, &self.line_numbers, format!("{import_line}\n")), + Some(Span { end, .. }) => { + insert_text(end, &self.line_numbers, format!("\n{import_line}")) + } + }, + ) + } +} diff --git a/crates/aiken-lsp/src/lib.rs b/crates/aiken-lsp/src/lib.rs index 0c0fbc45..a8f6fc95 100644 --- a/crates/aiken-lsp/src/lib.rs +++ b/crates/aiken-lsp/src/lib.rs @@ -5,6 +5,7 @@ use lsp_server::Connection; use std::env; mod cast; +mod edits; pub mod error; mod line_numbers; pub mod server; diff --git a/crates/aiken-lsp/src/server.rs b/crates/aiken-lsp/src/server.rs index eb0bb0ce..62b8ef54 100644 --- a/crates/aiken-lsp/src/server.rs +++ b/crates/aiken-lsp/src/server.rs @@ -33,6 +33,7 @@ use miette::Diagnostic; use crate::{ cast::{cast_notification, cast_request}, + edits, error::Error as ServerError, line_numbers::LineNumbers, utils::{ @@ -409,63 +410,17 @@ impl Server { ) -> Option> { let compiler = self.compiler.as_ref()?; - let file_path = text_document - .uri - .to_file_path() - .expect("invalid text document uri?"); - - let source_code = fs::read_to_string(&file_path).ok()?; - - let line_numbers = LineNumbers::new(&source_code); - - // NOTE: The 'ModuleKind' second argument doesn't matter. This is just added to the final - // object but has no influence on the parsing. - let (untyped_module, _) = aiken_lang::parser::module(&source_code, ModuleKind::Lib).ok()?; + let parsed_document = edits::parse_document(&text_document)?; let mut actions = Vec::new(); if let Some(serde_json::Value::String(ref var_name)) = diagnostic.data { for module in compiler.project.modules() { let mut changes = HashMap::new(); - let module_path = module.name.split('/').collect_vec(); if module.ast.has_definition(var_name) { - if let Some(edit) = - untyped_module.edit_import(module_path.as_slice(), Some(var_name)) - { - let (title, new_text) = if edit.is_new_line { - ( - format!( - "Add new import line: use {}.{{{}}}", - module.name, var_name - ), - format!("\nuse {}.{{{}}}", module.name, var_name), - ) - } else if edit.is_first_unqualified { - ( - format!( - "Add new unqualified import '{}' to {}", - var_name, module.name - ), - format!(".{{{}}}", var_name), - ) - } else { - ( - format!( - "Add new unqualified import '{}' to {}", - var_name, module.name - ), - format!(", {}", var_name), - ) - }; - - let range = span_to_lsp_range(edit.location, &line_numbers); - - changes.insert( - text_document.uri.clone(), - vec![lsp_types::TextEdit { range, new_text }], - ); - + if let Some((title, edit)) = parsed_document.import(&module, Some(var_name)) { + changes.insert(text_document.uri.clone(), vec![edit]); actions.push(lsp_types::CodeAction { title, kind: Some(lsp_types::CodeActionKind::QUICKFIX), From e48ac6b592744e422a9ec2b634a1416db4ff3d45 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 12:00:58 +0200 Subject: [PATCH 05/10] Relocate and refactor quickfix code into its own module We're going to have more quickfixes, to it's best not to overload the 'server' module. Plus, there's a lot of boilerplate around the quickfixes so we might want to factor it out. --- crates/aiken-lsp/src/lib.rs | 1 + crates/aiken-lsp/src/quickfix.rs | 66 +++++++++++++++++++ crates/aiken-lsp/src/server.rs | 105 ++++++++----------------------- 3 files changed, 92 insertions(+), 80 deletions(-) create mode 100644 crates/aiken-lsp/src/quickfix.rs diff --git a/crates/aiken-lsp/src/lib.rs b/crates/aiken-lsp/src/lib.rs index a8f6fc95..b627081f 100644 --- a/crates/aiken-lsp/src/lib.rs +++ b/crates/aiken-lsp/src/lib.rs @@ -8,6 +8,7 @@ mod cast; mod edits; pub mod error; mod line_numbers; +mod quickfix; pub mod server; mod utils; diff --git a/crates/aiken-lsp/src/quickfix.rs b/crates/aiken-lsp/src/quickfix.rs new file mode 100644 index 00000000..3e5f57c4 --- /dev/null +++ b/crates/aiken-lsp/src/quickfix.rs @@ -0,0 +1,66 @@ +use crate::{edits, server::lsp_project::LspProject}; +use std::collections::HashMap; + +const UNKNOWN_VARIABLE: &str = "aiken::check::unknown::variable"; + +const UNKNOWN_MODULE: &str = "aiken::check::unknown::module"; + +/// Errors for which we can provide quickfixes +pub enum Quickfix { + UnknownVariable, +} + +fn match_code(diagnostic: &lsp_types::Diagnostic, expected: &str) -> bool { + diagnostic.code == Some(lsp_types::NumberOrString::String(expected.to_string())) +} + +pub fn assert(diagnostic: &lsp_types::Diagnostic) -> Option { + let is_error = diagnostic.severity == Some(lsp_types::DiagnosticSeverity::ERROR); + + if is_error && match_code(diagnostic, UNKNOWN_VARIABLE) { + return Some(Quickfix::UnknownVariable); + } + + if is_error && match_code(diagnostic, UNKNOWN_MODULE) { + todo!() + } + + None +} + +pub fn unknown_variable( + compiler: &LspProject, + text_document: &lsp_types::TextDocumentIdentifier, + diagnostic: &lsp_types::Diagnostic, +) -> Vec { + let mut actions = Vec::new(); + + if let Some(parsed_document) = edits::parse_document(text_document) { + if let Some(serde_json::Value::String(ref var_name)) = diagnostic.data { + for module in compiler.project.modules() { + let mut changes = HashMap::new(); + if module.ast.has_definition(var_name) { + if let Some((title, edit)) = parsed_document.import(&module, Some(var_name)) { + changes.insert(text_document.uri.clone(), vec![edit]); + actions.push(lsp_types::CodeAction { + title, + kind: Some(lsp_types::CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + is_preferred: Some(true), + disabled: None, + data: None, + command: None, + edit: Some(lsp_types::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + }); + } + } + } + } + } + + actions +} diff --git a/crates/aiken-lsp/src/server.rs b/crates/aiken-lsp/src/server.rs index 62b8ef54..56f7f5b5 100644 --- a/crates/aiken-lsp/src/server.rs +++ b/crates/aiken-lsp/src/server.rs @@ -33,9 +33,9 @@ use miette::Diagnostic; use crate::{ cast::{cast_notification, cast_request}, - edits, error::Error as ServerError, line_numbers::LineNumbers, + quickfix::{self, Quickfix}, utils::{ path_to_uri, span_to_lsp_range, text_edit_replace, uri_to_module_name, COMPILING_PROGRESS_TOKEN, CREATE_COMPILING_PROGRESS_TOKEN, @@ -44,11 +44,9 @@ use crate::{ use self::lsp_project::LspProject; -mod lsp_project; +pub mod lsp_project; pub mod telemetry; -const UNKNOWN_VARIABLE: &str = "aiken::check::unknown::variable"; - #[allow(dead_code)] pub struct Server { // Project root directory @@ -331,45 +329,32 @@ impl Server { } CodeActionRequest::METHOD => { - let params = - cast_request::(request).expect("cast code action request"); + let mut actions = Vec::new(); - // Identify any diagnostic which refers to an unknown variable. In which case, we - // might want to provide some imports suggestion. - let unknown_variables = params - .context - .diagnostics - .into_iter() - .filter(|diagnostic| { - let is_error = - diagnostic.severity == Some(lsp_types::DiagnosticSeverity::ERROR); - let is_unknown_variable = diagnostic.code - == Some(lsp_types::NumberOrString::String( - UNKNOWN_VARIABLE.to_string(), - )); + if let Some(ref compiler) = self.compiler { + let params = cast_request::(request) + .expect("cast code action request"); - is_error && is_unknown_variable - }) - .collect::>(); - - match unknown_variables.first() { - Some(diagnostic) => Ok(lsp_server::Response { - id, - error: None, - result: Some(serde_json::to_value( - self.quickfix_unknown_variable( - params.text_document, - diagnostic.to_owned(), - ) - .unwrap_or(vec![]), - )?), - }), - None => Ok(lsp_server::Response { - id, - error: None, - result: Some(serde_json::Value::Null), - }), + for diagnostic in params.context.diagnostics.iter() { + match quickfix::assert(diagnostic) { + Some(Quickfix::UnknownVariable) => { + let quickfixes = quickfix::unknown_variable( + compiler, + ¶ms.text_document, + diagnostic, + ); + actions.extend(quickfixes); + } + None => (), + } + } } + + Ok(lsp_server::Response { + id, + error: None, + result: Some(serde_json::to_value(actions)?), + }) } unsupported => Err(ServerError::UnsupportedLspRequest { @@ -403,46 +388,6 @@ impl Server { } } - fn quickfix_unknown_variable( - &self, - text_document: lsp_types::TextDocumentIdentifier, - diagnostic: lsp_types::Diagnostic, - ) -> Option> { - let compiler = self.compiler.as_ref()?; - - let parsed_document = edits::parse_document(&text_document)?; - - let mut actions = Vec::new(); - - if let Some(serde_json::Value::String(ref var_name)) = diagnostic.data { - for module in compiler.project.modules() { - let mut changes = HashMap::new(); - - if module.ast.has_definition(var_name) { - if let Some((title, edit)) = parsed_document.import(&module, Some(var_name)) { - changes.insert(text_document.uri.clone(), vec![edit]); - actions.push(lsp_types::CodeAction { - title, - kind: Some(lsp_types::CodeActionKind::QUICKFIX), - diagnostics: Some(vec![diagnostic.clone()]), - is_preferred: Some(true), - disabled: None, - data: None, - command: None, - edit: Some(lsp_types::WorkspaceEdit { - changes: Some(changes), - document_changes: None, - change_annotations: None, - }), - }); - } - } - } - } - - Some(actions) - } - fn completion_for_import(&self) -> Option> { let compiler = self.compiler.as_ref()?; From c550b4766d082a11640f55c8f6e3a49d1eac9693 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 12:59:48 +0200 Subject: [PATCH 06/10] Implement quickfix for 'UnknownModule'. --- crates/aiken-lang/src/tipo/error.rs | 5 +- crates/aiken-lsp/src/edits.rs | 10 +-- crates/aiken-lsp/src/quickfix.rs | 98 +++++++++++++++++++++-------- crates/aiken-lsp/src/server.rs | 20 +++--- 4 files changed, 89 insertions(+), 44 deletions(-) diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 5c695d20..3fcdb18b 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -964,7 +964,6 @@ impl ExtraData for Error { | Error::UnexpectedLabeledArg { .. } | Error::UnexpectedLabeledArgInPattern { .. } | Error::UnknownLabels { .. } - | Error::UnknownModule { .. } | Error::UnknownModuleField { .. } | Error::UnknownModuleType { .. } | Error::UnknownModuleValue { .. } @@ -975,7 +974,9 @@ impl ExtraData for Error { | Error::UpdateMultiConstructorType { .. } | Error::ValidatorImported { .. } | Error::ValidatorMustReturnBool { .. } => None, - Error::UnknownVariable { name, .. } => Some(name.clone()), + Error::UnknownVariable { name, .. } | Error::UnknownModule { name, .. } => { + Some(name.clone()) + } } } } diff --git a/crates/aiken-lsp/src/edits.rs b/crates/aiken-lsp/src/edits.rs index 280ec4f7..77317640 100644 --- a/crates/aiken-lsp/src/edits.rs +++ b/crates/aiken-lsp/src/edits.rs @@ -10,6 +10,8 @@ pub struct ParsedDocument { line_numbers: LineNumbers, } +pub type AnnotatedEdit = (String, lsp_types::TextEdit); + /// Parse the target document as an 'UntypedModule' alongside its line numbers. This is useful in /// case we need to manipulate the AST for a quickfix. pub fn parse_document(document: &lsp_types::TextDocumentIdentifier) -> Option { @@ -46,7 +48,7 @@ impl ParsedDocument { &self, import: &CheckedModule, unqualified: Option<&str>, - ) -> Option<(String, lsp_types::TextEdit)> { + ) -> Option { let import_path = import.name.split('/').collect_vec(); let mut last_import = None; @@ -115,7 +117,7 @@ impl ParsedDocument { import: &CheckedModule, unqualified: &str, location: Span, - ) -> (String, lsp_types::TextEdit) { + ) -> AnnotatedEdit { let title = format!( "Insert new unqualified import '{}' to {}", unqualified, import.name @@ -135,7 +137,7 @@ impl ParsedDocument { import: &CheckedModule, unqualified: &str, location: Span, - ) -> (String, lsp_types::TextEdit) { + ) -> AnnotatedEdit { let title = format!( "Add new unqualified import '{}' to {}", unqualified, import.name @@ -155,7 +157,7 @@ impl ParsedDocument { import: &CheckedModule, unqualified: Option<&str>, location: Option, - ) -> (String, lsp_types::TextEdit) { + ) -> AnnotatedEdit { let import_line = format!( "use {}{}", import.name, diff --git a/crates/aiken-lsp/src/quickfix.rs b/crates/aiken-lsp/src/quickfix.rs index 3e5f57c4..ca745991 100644 --- a/crates/aiken-lsp/src/quickfix.rs +++ b/crates/aiken-lsp/src/quickfix.rs @@ -1,4 +1,7 @@ -use crate::{edits, server::lsp_project::LspProject}; +use crate::{ + edits::{self, AnnotatedEdit, ParsedDocument}, + server::lsp_project::LspProject, +}; use std::collections::HashMap; const UNKNOWN_VARIABLE: &str = "aiken::check::unknown::variable"; @@ -7,60 +10,101 @@ const UNKNOWN_MODULE: &str = "aiken::check::unknown::module"; /// Errors for which we can provide quickfixes pub enum Quickfix { - UnknownVariable, + UnknownIdentifier, + UnknownModule, } fn match_code(diagnostic: &lsp_types::Diagnostic, expected: &str) -> bool { diagnostic.code == Some(lsp_types::NumberOrString::String(expected.to_string())) } +/// Assert whether a diagnostic can be automatically fixed. Note that diagnostics often comes in +/// two severities, an error and hint; so we must be careful only addressing errors. pub fn assert(diagnostic: &lsp_types::Diagnostic) -> Option { let is_error = diagnostic.severity == Some(lsp_types::DiagnosticSeverity::ERROR); if is_error && match_code(diagnostic, UNKNOWN_VARIABLE) { - return Some(Quickfix::UnknownVariable); + return Some(Quickfix::UnknownIdentifier); } if is_error && match_code(diagnostic, UNKNOWN_MODULE) { - todo!() + return Some(Quickfix::UnknownModule); } None } -pub fn unknown_variable( +pub fn quickfix( compiler: &LspProject, text_document: &lsp_types::TextDocumentIdentifier, diagnostic: &lsp_types::Diagnostic, + quickfix: &Quickfix, ) -> Vec { let mut actions = Vec::new(); - if let Some(parsed_document) = edits::parse_document(text_document) { - if let Some(serde_json::Value::String(ref var_name)) = diagnostic.data { - for module in compiler.project.modules() { + if let Some(ref parsed_document) = edits::parse_document(text_document) { + if let Some(serde_json::Value::String(ref data)) = diagnostic.data { + let edits = match quickfix { + Quickfix::UnknownIdentifier => unknown_identifier(compiler, parsed_document, data), + Quickfix::UnknownModule => unknown_module(compiler, parsed_document, data), + }; + + for (title, edit) in edits.into_iter() { let mut changes = HashMap::new(); - if module.ast.has_definition(var_name) { - if let Some((title, edit)) = parsed_document.import(&module, Some(var_name)) { - changes.insert(text_document.uri.clone(), vec![edit]); - actions.push(lsp_types::CodeAction { - title, - kind: Some(lsp_types::CodeActionKind::QUICKFIX), - diagnostics: Some(vec![diagnostic.clone()]), - is_preferred: Some(true), - disabled: None, - data: None, - command: None, - edit: Some(lsp_types::WorkspaceEdit { - changes: Some(changes), - document_changes: None, - change_annotations: None, - }), - }); - } - } + changes.insert(text_document.uri.clone(), vec![edit]); + actions.push(lsp_types::CodeAction { + title, + kind: Some(lsp_types::CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + is_preferred: Some(true), + disabled: None, + data: None, + command: None, + edit: Some(lsp_types::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + }); } } } actions } + +fn unknown_identifier( + compiler: &LspProject, + parsed_document: &ParsedDocument, + var_name: &str, +) -> Vec { + let mut edits = Vec::new(); + + for module in compiler.project.modules() { + if module.ast.has_definition(var_name) { + if let Some(edit) = parsed_document.import(&module, Some(var_name)) { + edits.push(edit) + } + } + } + + edits +} + +fn unknown_module( + compiler: &LspProject, + parsed_document: &ParsedDocument, + module_name: &str, +) -> Vec { + let mut edits = Vec::new(); + + for module in compiler.project.modules() { + if module.name.ends_with(module_name) { + if let Some(edit) = parsed_document.import(&module, None) { + edits.push(edit); + } + } + } + + edits +} diff --git a/crates/aiken-lsp/src/server.rs b/crates/aiken-lsp/src/server.rs index 56f7f5b5..19e9942a 100644 --- a/crates/aiken-lsp/src/server.rs +++ b/crates/aiken-lsp/src/server.rs @@ -35,7 +35,7 @@ use crate::{ cast::{cast_notification, cast_request}, error::Error as ServerError, line_numbers::LineNumbers, - quickfix::{self, Quickfix}, + quickfix, utils::{ path_to_uri, span_to_lsp_range, text_edit_replace, uri_to_module_name, COMPILING_PROGRESS_TOKEN, CREATE_COMPILING_PROGRESS_TOKEN, @@ -336,16 +336,14 @@ impl Server { .expect("cast code action request"); for diagnostic in params.context.diagnostics.iter() { - match quickfix::assert(diagnostic) { - Some(Quickfix::UnknownVariable) => { - let quickfixes = quickfix::unknown_variable( - compiler, - ¶ms.text_document, - diagnostic, - ); - actions.extend(quickfixes); - } - None => (), + if let Some(strategy) = quickfix::assert(diagnostic) { + let quickfixes = quickfix::quickfix( + compiler, + ¶ms.text_document, + diagnostic, + &strategy, + ); + actions.extend(quickfixes); } } } From d965467a5381942f4ba107e5bf419e9c49882447 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 13:56:15 +0200 Subject: [PATCH 07/10] Fix insertion of unqualified import when first I previously missed a case and it causes qualified imports to be added at the end if they are lexicographically smaller than ALL other qualified imports. No big deal, but this is now fixed. --- crates/aiken-lsp/src/edits.rs | 47 +++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/crates/aiken-lsp/src/edits.rs b/crates/aiken-lsp/src/edits.rs index 77317640..8aa9093b 100644 --- a/crates/aiken-lsp/src/edits.rs +++ b/crates/aiken-lsp/src/edits.rs @@ -71,10 +71,14 @@ impl ParsedDocument { // There's already a matching qualified import, so we have nothing to do. None => return None, Some(unqualified) => { + let mut last_unqualified = None; + // Insert lexicographically, assuming unqualified imports are already // ordered. If they are not, it doesn't really matter where we insert // anyway. for existing_unqualified in unqualified_list { + last_unqualified = Some(existing_unqualified.location); + let existing_name = existing_unqualified .as_name .as_ref() @@ -83,9 +87,9 @@ impl ParsedDocument { // The unqualified import already exist, nothing to do. if unqualified == existing_name { return None; - // Current import is lexicographically smaller, we can insert after. - } else if existing_name.as_str() < unqualified { - return Some(self.insert_into_qualified( + // Current import is lexicographically greater, we can insert before + } else if unqualified < existing_name.as_str() { + return Some(self.insert_qualified_before( import, unqualified, existing_unqualified.location, @@ -95,9 +99,18 @@ impl ParsedDocument { } } - // Only happens if 'unqualified_list' is empty, in which case, we - // simply create a new unqualified list of import. - return Some(self.add_new_qualified(import, unqualified, *location)); + return match last_unqualified { + // Only happens if 'unqualified_list' is empty, in which case, we + // simply create a new unqualified list of import. + None => { + Some(self.add_new_qualified(import, unqualified, *location)) + } + // Happens if the new qualified import is lexicographically after + // all existing ones. + Some(location) => { + Some(self.insert_qualified_after(import, unqualified, location)) + } + }; } } } @@ -112,7 +125,27 @@ impl ParsedDocument { Some(self.add_new_import_line(import, unqualified, last_import)) } - fn insert_into_qualified( + fn insert_qualified_before( + &self, + import: &CheckedModule, + unqualified: &str, + location: Span, + ) -> AnnotatedEdit { + let title = format!( + "Insert new unqualified import '{}' to {}", + unqualified, import.name + ); + ( + title, + insert_text( + location.start, + &self.line_numbers, + format!("{}, ", unqualified), + ), + ) + } + + fn insert_qualified_after( &self, import: &CheckedModule, unqualified: &str, From 5986163ba7a3de365e0a0f4036e7340ba428e21a Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 13:57:06 +0200 Subject: [PATCH 08/10] Add quickfix for unknown alias & data types. --- crates/aiken-lang/src/ast.rs | 6 +++--- crates/aiken-lang/src/tipo/error.rs | 10 +++++----- crates/aiken-lsp/src/quickfix.rs | 14 +++++++++++--- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/crates/aiken-lang/src/ast.rs b/crates/aiken-lang/src/ast.rs index ace86680..b9447de8 100644 --- a/crates/aiken-lang/src/ast.rs +++ b/crates/aiken-lang/src/ast.rs @@ -81,10 +81,10 @@ impl TypedModule { pub fn has_definition(&self, name: &str) -> bool { self.definitions.iter().any(|def| match def { Definition::Fn(f) => f.public && f.name == name, - Definition::TypeAlias(_) => false, - Definition::DataType(_) => false, + Definition::TypeAlias(alias) => alias.public && alias.alias == name, + Definition::ModuleConstant(cst) => cst.public && cst.name == name, + Definition::DataType(t) => t.public && t.name == name, Definition::Use(_) => false, - Definition::ModuleConstant(_) => false, Definition::Test(_) => false, Definition::Validator(_) => false, }) diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 3fcdb18b..93706a8c 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -968,15 +968,15 @@ impl ExtraData for Error { | Error::UnknownModuleType { .. } | Error::UnknownModuleValue { .. } | Error::UnknownRecordField { .. } - | Error::UnknownType { .. } - | Error::UnknownTypeConstructor { .. } | Error::UnnecessarySpreadOperator { .. } | Error::UpdateMultiConstructorType { .. } | Error::ValidatorImported { .. } | Error::ValidatorMustReturnBool { .. } => None, - Error::UnknownVariable { name, .. } | Error::UnknownModule { name, .. } => { - Some(name.clone()) - } + + Error::UnknownType { name, .. } + | Error::UnknownTypeConstructor { name, .. } + | Error::UnknownVariable { name, .. } + | Error::UnknownModule { name, .. } => Some(name.clone()), } } } diff --git a/crates/aiken-lsp/src/quickfix.rs b/crates/aiken-lsp/src/quickfix.rs index ca745991..f21eb48b 100644 --- a/crates/aiken-lsp/src/quickfix.rs +++ b/crates/aiken-lsp/src/quickfix.rs @@ -5,8 +5,8 @@ use crate::{ use std::collections::HashMap; const UNKNOWN_VARIABLE: &str = "aiken::check::unknown::variable"; - const UNKNOWN_MODULE: &str = "aiken::check::unknown::module"; +const UNKNOWN_TYPE: &str = "aiken::check::unknown::type"; /// Errors for which we can provide quickfixes pub enum Quickfix { @@ -23,11 +23,19 @@ fn match_code(diagnostic: &lsp_types::Diagnostic, expected: &str) -> bool { pub fn assert(diagnostic: &lsp_types::Diagnostic) -> Option { let is_error = diagnostic.severity == Some(lsp_types::DiagnosticSeverity::ERROR); - if is_error && match_code(diagnostic, UNKNOWN_VARIABLE) { + if !is_error { + return None; + } + + if match_code(diagnostic, UNKNOWN_VARIABLE) { return Some(Quickfix::UnknownIdentifier); } - if is_error && match_code(diagnostic, UNKNOWN_MODULE) { + if match_code(diagnostic, UNKNOWN_TYPE) { + return Some(Quickfix::UnknownIdentifier); + } + + if match_code(diagnostic, UNKNOWN_MODULE) { return Some(Quickfix::UnknownModule); } From f6eff7ec589caaa399ef57139ba4d1f126e76a22 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 14:09:07 +0200 Subject: [PATCH 09/10] Fix incoherent 'UnknownVariable' being returned in type-check I initially removed the 'UnkownTypeConstructor' since it wasn't used anywhere and was in fact dead-code. On second thoughts however, it is nicer to provide a slightly better error message when a constructor is missing as well as some valid suggestion. Prior to that commit, we would simply return a 'UnknownVariable' and the hint might suggest lowercase identifiers; which is wrong. --- crates/aiken-lang/src/tipo/environment.rs | 21 ++++++++++++++++----- crates/aiken-lang/src/tipo/error.rs | 2 +- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/crates/aiken-lang/src/tipo/environment.rs b/crates/aiken-lang/src/tipo/environment.rs index b95a5a0b..954ebfdc 100644 --- a/crates/aiken-lang/src/tipo/environment.rs +++ b/crates/aiken-lang/src/tipo/environment.rs @@ -309,11 +309,14 @@ impl<'a> Environment<'a> { location: Span, ) -> Result<&ValueConstructor, Error> { match module { - None => self.scope.get(name).ok_or_else(|| Error::UnknownVariable { - location, - name: name.to_string(), - variables: self.local_value_names(), - }), + None => self + .scope + .get(name) + .ok_or_else(|| Error::UnknownTypeConstructor { + location, + name: name.to_string(), + constructors: self.local_constructor_names(), + }), Some(m) => { let (_, module) = @@ -577,6 +580,14 @@ impl<'a> Environment<'a> { .collect() } + pub fn local_constructor_names(&self) -> Vec { + self.scope + .keys() + .filter(|&t| t.chars().next().unwrap_or_default().is_uppercase()) + .map(|t| t.to_string()) + .collect() + } + fn make_type_vars( &mut self, args: &[String], diff --git a/crates/aiken-lang/src/tipo/error.rs b/crates/aiken-lang/src/tipo/error.rs index 93706a8c..2ce8b768 100644 --- a/crates/aiken-lang/src/tipo/error.rs +++ b/crates/aiken-lang/src/tipo/error.rs @@ -799,7 +799,7 @@ Perhaps, try the following: suggest_neighbor(name, constructors.iter(), "Did you forget to import it?") ))] UnknownTypeConstructor { - #[label] + #[label("unknown constructor")] location: Span, name: String, constructors: Vec, From c0513da032b2840db01b39732368094e20809dfe Mon Sep 17 00:00:00 2001 From: KtorZ Date: Sat, 21 Oct 2023 14:24:47 +0200 Subject: [PATCH 10/10] Add quickfix for unknown constructors. --- crates/aiken-lang/src/ast.rs | 16 ++++++++++++++++ crates/aiken-lsp/src/edits.rs | 15 +++------------ crates/aiken-lsp/src/quickfix.rs | 30 +++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/crates/aiken-lang/src/ast.rs b/crates/aiken-lang/src/ast.rs index b9447de8..e820de51 100644 --- a/crates/aiken-lang/src/ast.rs +++ b/crates/aiken-lang/src/ast.rs @@ -90,6 +90,22 @@ impl TypedModule { }) } + pub fn has_constructor(&self, name: &str) -> bool { + self.definitions.iter().any(|def| match def { + Definition::DataType(t) if t.public && !t.opaque => t + .constructors + .iter() + .any(|constructor| constructor.name == name), + Definition::DataType(_) => false, + Definition::Fn(_) => false, + Definition::TypeAlias(_) => false, + Definition::ModuleConstant(_) => false, + Definition::Use(_) => false, + Definition::Test(_) => false, + Definition::Validator(_) => false, + }) + } + pub fn validate_module_name(&self) -> Result<(), Error> { if self.name == "aiken" || self.name == "aiken/builtin" { return Err(Error::ReservedModuleName { diff --git a/crates/aiken-lsp/src/edits.rs b/crates/aiken-lsp/src/edits.rs index 8aa9093b..1ccb4a0b 100644 --- a/crates/aiken-lsp/src/edits.rs +++ b/crates/aiken-lsp/src/edits.rs @@ -131,10 +131,7 @@ impl ParsedDocument { unqualified: &str, location: Span, ) -> AnnotatedEdit { - let title = format!( - "Insert new unqualified import '{}' to {}", - unqualified, import.name - ); + let title = format!("Use '{}' from {}", unqualified, import.name); ( title, insert_text( @@ -151,10 +148,7 @@ impl ParsedDocument { unqualified: &str, location: Span, ) -> AnnotatedEdit { - let title = format!( - "Insert new unqualified import '{}' to {}", - unqualified, import.name - ); + let title = format!("Use '{}' from {}", unqualified, import.name); ( title, insert_text( @@ -171,10 +165,7 @@ impl ParsedDocument { unqualified: &str, location: Span, ) -> AnnotatedEdit { - let title = format!( - "Add new unqualified import '{}' to {}", - unqualified, import.name - ); + let title = format!("Use '{}' from {}", unqualified, import.name); ( title, insert_text( diff --git a/crates/aiken-lsp/src/quickfix.rs b/crates/aiken-lsp/src/quickfix.rs index f21eb48b..a5c6184d 100644 --- a/crates/aiken-lsp/src/quickfix.rs +++ b/crates/aiken-lsp/src/quickfix.rs @@ -5,13 +5,16 @@ use crate::{ use std::collections::HashMap; const UNKNOWN_VARIABLE: &str = "aiken::check::unknown::variable"; -const UNKNOWN_MODULE: &str = "aiken::check::unknown::module"; const UNKNOWN_TYPE: &str = "aiken::check::unknown::type"; +const UNKNOWN_CONSTRUCTOR: &str = "aiken::check::unknown::type_constructor"; +const UNKNOWN_MODULE: &str = "aiken::check::unknown::module"; /// Errors for which we can provide quickfixes +#[allow(clippy::enum_variant_names)] pub enum Quickfix { UnknownIdentifier, UnknownModule, + UnknownConstructor, } fn match_code(diagnostic: &lsp_types::Diagnostic, expected: &str) -> bool { @@ -35,6 +38,10 @@ pub fn assert(diagnostic: &lsp_types::Diagnostic) -> Option { return Some(Quickfix::UnknownIdentifier); } + if match_code(diagnostic, UNKNOWN_CONSTRUCTOR) { + return Some(Quickfix::UnknownConstructor); + } + if match_code(diagnostic, UNKNOWN_MODULE) { return Some(Quickfix::UnknownModule); } @@ -55,6 +62,9 @@ pub fn quickfix( let edits = match quickfix { Quickfix::UnknownIdentifier => unknown_identifier(compiler, parsed_document, data), Quickfix::UnknownModule => unknown_module(compiler, parsed_document, data), + Quickfix::UnknownConstructor => { + unknown_constructor(compiler, parsed_document, data) + } }; for (title, edit) in edits.into_iter() { @@ -99,6 +109,24 @@ fn unknown_identifier( edits } +fn unknown_constructor( + compiler: &LspProject, + parsed_document: &ParsedDocument, + constructor_name: &str, +) -> Vec { + let mut edits = Vec::new(); + + for module in compiler.project.modules() { + if module.ast.has_constructor(constructor_name) { + if let Some(edit) = parsed_document.import(&module, Some(constructor_name)) { + edits.push(edit) + } + } + } + + edits +} + fn unknown_module( compiler: &LspProject, parsed_document: &ParsedDocument,