From fc4762513d984b3594fdaee137de15f4078b4d89 Mon Sep 17 00:00:00 2001 From: Mark McCulloh Date: Thu, 5 Oct 2023 22:09:07 -0400 Subject: [PATCH] feat(vscode): completions for `bring` and hiding more irrelevant completions (#4433) #### Stops completions on the left side of variable declarations ##### Fixes #3991 image #### Stops completions in the `in` part of `for in` (prevent the common annoying `inflight` completions) ##### Fixes #3997 image #### Adds completions for `bring` (and prevent irrelevant completions) ##### Fixes #3975 image _Didn't add documentation for them yet, wasn't sure how to go about it without some duplicated hardcoding._ --- Misc: - `bring` followed by a single character previously caused a panic (e.g. `bring c`). Most people probably didn't notice cause they were typing `bring cloud` so fast - No completions in comments or strings - Tried to centralize many of the cases where it's obvious there will be no completions available. `completions.rs` is still begging for a rewrite. *By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*. --- examples/tests/invalid/bring.test.w | 6 +- libs/tree-sitter-wing/grammar.js | 2 +- libs/wingc/src/lsp/completions.rs | 170 +++++++++++++++++- .../snapshots/completions/bring_alias.snap | 5 + .../completions/bring_suggestions.snap | 30 ++++ .../bring_suggestions_partial.snap | 30 ++++ .../lsp/snapshots/completions/comment.snap | 5 + .../completions/definition_identifier.snap | 5 + .../definition_identifier_partial.snap | 5 + .../snapshots/completions/for_in_inner.snap | 5 + .../snapshots/completions/string_inner.snap | 5 + libs/wingc/src/parser.rs | 12 +- tools/hangar/__snapshots__/invalid.ts.snap | 16 +- 13 files changed, 282 insertions(+), 14 deletions(-) create mode 100644 libs/wingc/src/lsp/snapshots/completions/bring_alias.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/bring_suggestions.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/bring_suggestions_partial.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/comment.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/definition_identifier.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/definition_identifier_partial.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/for_in_inner.snap create mode 100644 libs/wingc/src/lsp/snapshots/completions/string_inner.snap diff --git a/examples/tests/invalid/bring.test.w b/examples/tests/invalid/bring.test.w index d736cde9f61..b8612d9cfe1 100644 --- a/examples/tests/invalid/bring.test.w +++ b/examples/tests/invalid/bring.test.w @@ -4,4 +4,8 @@ bring cloud; bring cloud; // ^^^^^ "cloud" is already defined bring fs; -// ^^^^^ "fs" is not a built-in module \ No newline at end of file +// ^^^^^ "fs" is not a built-in module +bring ; +//^^^^^ Expected module specification (see https://www.winglang.io/docs/libraries) +bring c; +//^^^^^^ "c" is not a built-in module \ No newline at end of file diff --git a/libs/tree-sitter-wing/grammar.js b/libs/tree-sitter-wing/grammar.js index 04f65e7eeb8..d6ded615e22 100644 --- a/libs/tree-sitter-wing/grammar.js +++ b/libs/tree-sitter-wing/grammar.js @@ -124,7 +124,7 @@ module.exports = grammar({ import_statement: ($) => seq( "bring", - field("module_name", choice($.identifier, $.string)), + optional(field("module_name", choice($.identifier, $.string))), optional(seq("as", field("alias", $.identifier))), $._semicolon ), diff --git a/libs/wingc/src/lsp/completions.rs b/libs/wingc/src/lsp/completions.rs index 5e94484896d..3bc4cff239b 100644 --- a/libs/wingc/src/lsp/completions.rs +++ b/libs/wingc/src/lsp/completions.rs @@ -20,7 +20,7 @@ use crate::type_check::{ }; use crate::visit::{visit_expr, visit_type_annotation, Visit}; use crate::wasm_util::{ptr_to_string, string_to_combined_ptr, WASM_RETURN_ERROR}; -use crate::{UTIL_CLASS_NAME, WINGSDK_ASSEMBLY_NAME, WINGSDK_STD_MODULE}; +use crate::{UTIL_CLASS_NAME, WINGSDK_ASSEMBLY_NAME, WINGSDK_BRINGABLE_MODULES, WINGSDK_STD_MODULE}; use super::sync::check_utf8; @@ -38,6 +38,72 @@ pub unsafe extern "C" fn wingc_on_completion(ptr: u32, len: u32) -> u64 { } } +/// Using tree-sitter data only, check if there should be no valid completions at this position. +/// This allows for quick short-circuits to avoid visiting the AST for unambiguous cases. +/// Returns true if there should be no completions, false otherwise +fn check_ts_to_completions(interesting_node: &Node) -> bool { + let interesting_node_kind = interesting_node.kind(); + + let mut no_completions = match interesting_node_kind { + "comment" => true, + + // Loose backslash? Get outta here + "\\" => true, + + // Strings are just strings + // TODO Remove this for https://github.com/winglang/wing/issues/4420 + "string" | "\"" => true, + + // After "for", we are expecting a variable name + "for" => true, + + // we are at the end of an import statement and/or expecting an alias + "import_statement" | "as" => true, + + // Starting a definition + "let" | "reassignable" => true, + + // Following the keyword of a block-style declaration + "class" | "struct" | "interface" | "test" => true, + + // Starting an inflight closure + "inflight_specifier" => true, + + // No completions are valid immediately following a closing brace + ")" | "}" | "]" => true, + + // There will always be options following a colon + ":" => return false, + + _ => false, + }; + + if no_completions { + return no_completions; + } + + if let Some(parent) = interesting_node.parent() { + let parent_kind = parent.kind(); + + no_completions = match parent_kind { + "ERROR" => { + if let Some(first_child) = parent.child(0) { + match first_child.kind() { + "for" => matches!(interesting_node_kind, "in" | "identifier"), + "let" => !matches!(interesting_node_kind, "="), + _ => false, + } + } else { + false + } + } + _ => false, + }; + } + + no_completions +} + pub fn on_completion(params: lsp_types::CompletionParams) -> CompletionResponse { WING_TYPES.with(|types| { let mut types = types.borrow_mut(); @@ -86,6 +152,30 @@ pub fn on_completion(params: lsp_types::CompletionParams) -> CompletionResponse ); let node_to_complete_kind = node_to_complete.kind(); + if check_ts_to_completions(&node_to_complete) { + return vec![]; + } + + if matches!(node_to_complete.parent(), Some(p) if p.kind() == "import_statement") + && matches!(node_to_complete_kind, "identifier" | "bring") + { + let mut modules = WINGSDK_BRINGABLE_MODULES + .map(|module| CompletionItem { + label: module.to_string(), + kind: Some(CompletionItemKind::MODULE), + ..Default::default() + }) + .to_vec(); + modules.push(CompletionItem { + label: "\"module\"".to_string(), + insert_text: Some("\"$1\" as $2".to_string()), + kind: Some(CompletionItemKind::SNIPPET), + insert_text_format: Some(InsertTextFormat::SNIPPET), + ..Default::default() + }); + return modules; + } + let mut scope_visitor = ScopeVisitor::new( node_to_complete.parent().map(|parent| WingSpan { start: parent.start_position().into(), @@ -395,9 +485,6 @@ fn get_current_scope_completions( let mut in_type = preceding_text.ends_with(':'); match node_to_complete.kind() { - // there are no possible completions that could follow ")" - ")" => return vec![], - "set_literal" | "struct_literal" => { in_type = false; } @@ -439,11 +526,6 @@ fn get_current_scope_completions( }); } - // in block-style statements, there are no possible completions that can follow the keyword - "interface" | "struct" | "class" | "test" => { - return vec![]; - } - _ => {} } @@ -1485,4 +1567,74 @@ let x: Outer = { bThing: { } } //^ "# ); + + test_completion_list!( + bring_suggestions, + r#" +bring + //^ +"# + ); + + test_completion_list!( + bring_suggestions_partial, + r#" +bring c + //^ +"# + ); + + test_completion_list!( + bring_alias, + r#" +bring "" as + //^ +"#, + assert!(bring_alias.is_empty()) + ); + + test_completion_list!( + definition_identifier_partial, + r#" +let a + //^ +"#, + assert!(definition_identifier_partial.is_empty()) + ); + + test_completion_list!( + definition_identifier, + r#" +let + //^ +"#, + assert!(definition_identifier.is_empty()) + ); + + test_completion_list!( + string_inner, + r#" +let x = "" + //^ +"#, + assert!(string_inner.is_empty()) + ); + + test_completion_list!( + for_in_inner, + r#" +for x i + //^ +"#, + assert!(for_in_inner.is_empty()) + ); + + test_completion_list!( + comment, + r#" +let x = // hi + //^ +"#, + assert!(comment.is_empty()) + ); } diff --git a/libs/wingc/src/lsp/snapshots/completions/bring_alias.snap b/libs/wingc/src/lsp/snapshots/completions/bring_alias.snap new file mode 100644 index 00000000000..2382299ee10 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/bring_alias.snap @@ -0,0 +1,5 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +[] + diff --git a/libs/wingc/src/lsp/snapshots/completions/bring_suggestions.snap b/libs/wingc/src/lsp/snapshots/completions/bring_suggestions.snap new file mode 100644 index 00000000000..55328512d60 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/bring_suggestions.snap @@ -0,0 +1,30 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +- label: aws + kind: 9 + sortText: kk|aws +- label: cloud + kind: 9 + sortText: kk|cloud +- label: ex + kind: 9 + sortText: kk|ex +- label: http + kind: 9 + sortText: kk|http +- label: math + kind: 9 + sortText: kk|math +- label: regex + kind: 9 + sortText: kk|regex +- label: util + kind: 9 + sortText: kk|util +- label: "\"module\"" + kind: 15 + sortText: "ll|\"module\"" + insertText: "\"$1\" as $2" + insertTextFormat: 2 + diff --git a/libs/wingc/src/lsp/snapshots/completions/bring_suggestions_partial.snap b/libs/wingc/src/lsp/snapshots/completions/bring_suggestions_partial.snap new file mode 100644 index 00000000000..55328512d60 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/bring_suggestions_partial.snap @@ -0,0 +1,30 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +- label: aws + kind: 9 + sortText: kk|aws +- label: cloud + kind: 9 + sortText: kk|cloud +- label: ex + kind: 9 + sortText: kk|ex +- label: http + kind: 9 + sortText: kk|http +- label: math + kind: 9 + sortText: kk|math +- label: regex + kind: 9 + sortText: kk|regex +- label: util + kind: 9 + sortText: kk|util +- label: "\"module\"" + kind: 15 + sortText: "ll|\"module\"" + insertText: "\"$1\" as $2" + insertTextFormat: 2 + diff --git a/libs/wingc/src/lsp/snapshots/completions/comment.snap b/libs/wingc/src/lsp/snapshots/completions/comment.snap new file mode 100644 index 00000000000..2382299ee10 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/comment.snap @@ -0,0 +1,5 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +[] + diff --git a/libs/wingc/src/lsp/snapshots/completions/definition_identifier.snap b/libs/wingc/src/lsp/snapshots/completions/definition_identifier.snap new file mode 100644 index 00000000000..2382299ee10 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/definition_identifier.snap @@ -0,0 +1,5 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +[] + diff --git a/libs/wingc/src/lsp/snapshots/completions/definition_identifier_partial.snap b/libs/wingc/src/lsp/snapshots/completions/definition_identifier_partial.snap new file mode 100644 index 00000000000..2382299ee10 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/definition_identifier_partial.snap @@ -0,0 +1,5 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +[] + diff --git a/libs/wingc/src/lsp/snapshots/completions/for_in_inner.snap b/libs/wingc/src/lsp/snapshots/completions/for_in_inner.snap new file mode 100644 index 00000000000..2382299ee10 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/for_in_inner.snap @@ -0,0 +1,5 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +[] + diff --git a/libs/wingc/src/lsp/snapshots/completions/string_inner.snap b/libs/wingc/src/lsp/snapshots/completions/string_inner.snap new file mode 100644 index 00000000000..2382299ee10 --- /dev/null +++ b/libs/wingc/src/lsp/snapshots/completions/string_inner.snap @@ -0,0 +1,5 @@ +--- +source: libs/wingc/src/lsp/completions.rs +--- +[] + diff --git a/libs/wingc/src/parser.rs b/libs/wingc/src/parser.rs index a1cbba3ba5a..cf90fc8b368 100644 --- a/libs/wingc/src/parser.rs +++ b/libs/wingc/src/parser.rs @@ -791,7 +791,10 @@ impl<'s> Parser<'s> { } fn build_bring_statement(&self, statement_node: &Node) -> DiagnosticResult { - let module_name_node = self.get_child_field(&statement_node, "module_name")?; + let Some(module_name_node) = statement_node.child_by_field_name("module_name") else { + return self.with_error("Expected module specification (see https://www.winglang.io/docs/libraries)", &statement_node.child(statement_node.child_count() - 1).unwrap_or(*statement_node)); + }; + let module_name = self.node_symbol(&module_name_node)?; let alias = if let Some(identifier) = statement_node.child_by_field_name("alias") { Some(self.check_reserved_symbol(&identifier)?) @@ -799,7 +802,12 @@ impl<'s> Parser<'s> { None }; - let module_path = Utf8Path::new(&module_name.name[1..module_name.name.len() - 1]); + let module_path = if module_name.name.len() > 1 { + Utf8Path::new(&module_name.name[1..module_name.name.len() - 1]) + } else { + Utf8Path::new(&module_name.name) + }; + if is_absolute_path(&module_path) { return self.with_error( format!("Cannot bring \"{}\" since it is not a relative path", module_path), diff --git a/tools/hangar/__snapshots__/invalid.ts.snap b/tools/hangar/__snapshots__/invalid.ts.snap index d6918913fe7..32707182f8b 100644 --- a/tools/hangar/__snapshots__/invalid.ts.snap +++ b/tools/hangar/__snapshots__/invalid.ts.snap @@ -273,7 +273,14 @@ Duration " `; exports[`bring.test.w 1`] = ` -"error: Redundant bring of \\"std\\" +"error: Expected module specification (see https://www.winglang.io/docs/libraries) + --> ../../../examples/tests/invalid/bring.test.w:8:7 + | +8 | bring ; + | ^ Expected module specification (see https://www.winglang.io/docs/libraries) + + +error: Redundant bring of \\"std\\" --> ../../../examples/tests/invalid/bring.test.w:1:1 | 1 | bring std; @@ -294,6 +301,13 @@ error: \\"fs\\" is not a built-in module | ^^^^^^^^^ \\"fs\\" is not a built-in module +error: \\"c\\" is not a built-in module + --> ../../../examples/tests/invalid/bring.test.w:10:1 + | +10 | bring c; + | ^^^^^^^^ \\"c\\" is not a built-in module + + Tests 1 failed (1)