More precise completions and namespace member completions #1947

minestarks · 2024-10-03T23:42:40Z

Feature overview

This PR enables:

Much more precise keyword completions where we only show the relevant syntax at a particular cursor location. For example:

within { } |

At the cursor location indicated by |, we should only get the keyword apply .

More precise completions for names where we take the syntactic context into account. For example:

operation Foo(x : | )

Here, we should only get type names.

Namespace member completions. For example:

import Std.Diagnostics.|

The completions here should return only items that exist under Std.Diagnostics.

Code changes

Parser completions module

Introduce the completion module in the parser.

possible_words_at_offset_in_source - a new API to return completions at a particular offset . Uses the parser directly as the best source of knowledge about what tokens to expect.
Introduce the WordKinds flags to define the different kinds of word tokens that the parser can expect during parsing. (See compiler/qsc_parse/src/completion/word_kinds.rs for longer description)
See compiler/qsc_parse/src/completion/collector.rs for a description of how this all works.
Sprinkle expect()s at all possible locations in the parser where we expect a word.

Error recovery - AST changes

Introduce PathKind::Ok and PathKind::Err .
- This new approach allows for a valid AST node to exist even when path parsing fails for a partial path. This is good for syntax such as "open Std." where we need:
  - The identifier nodes preceding . to be valid, so we can use them to look up possible completions.
  - The open statement to also be a valid node, so we can tell that the context requires a namespace (and not, say, a type name).
- Most existing code will continue to only process valid paths.
Remove Idents struct and replace it with a trait implemented by various types such as Path and Ident slices. This should make the convenience methods easier to use without cloning in/out of the different types we need.

Error recovery - parser changes

Recovering struct parsing, so that e.g. new Foo. is still parsed as a valid struct expression node, allowing us to access the Foo ident from the AST
Recovering open statement parsing so that open Foo. can be recognized as an open item in the AST.

Language service

In language_service is essentially a complete rewrite of completions.rs, so I suggest reviewing the new code only and ditching the diff.

Other parser changes

Move implicit namespace parsing a bit so that it works better with the completion mechanism.
- This also fixes the spurious "expected item, found eof" error when document only contains one token
Delete the dead path path_field_op
Add some missing keywords to item parsing recovery (export, import)

Other notes

Locals completions exclude imports from locals (they conflict with globals already added to completion) (fix in resolve.rs)
language-configuration.json : and removed wonky word pattern in playground and VS Code because we don't need it anymore now that we have proper . completions! Also update trigger characters.
Only include the samples snippets in empty documents, because now with the more concise completion list, it gets quite intrusive for them to show up at every word when typing code.
Update integration test to account for the fact that completions are more accurate now.

Not in this PR (coming next)

Struct and UDT field access completions.

github-actions · 2024-10-07T16:40:12Z

Change in memory usage detected by benchmark.

Memory Report for `6fbbec5`

Test	This Branch	On Main	Difference
compile core + standard lib	17940700 bytes	17932132 bytes	8568 bytes

swernli · 2024-10-08T21:22:21Z

compiler/qsc_ast/src/ast.rs

    }
 }

 impl ImportOrExportItem {
-    /// Returns the span of the export item. This includes the path and , if any exists, the alias.
+    /// Returns the alias ident, if any, or the name from the path if no alias is present.
+    /// Returns `None` if this the path has an error.


nitpick: extra word

Suggested change

/// Returns `None` if this the path has an error.

/// Returns `None` if the path has an error.

minestarks · 2024-10-08T21:23:08Z

Change in memory usage detected by benchmark.

Memory Report for 6fbbec5

Test This Branch On Main Difference
compile core + standard lib 17940700 bytes 17932132 bytes 8568 bytes

I ran some tests to narrow it down. This increase is attributable entirely to the changes in the AST. A bunch of Path or Idents fields have been replaced with PathKind, a Span field has been added to ImportOrExportItem. It adds up to a net increase in memory.

I can't think of a way to shrink the AST any further, we are storing more information in there after all. So I think the 0.04% increase in memory usage is a worthwhile tradeoff for the feature.

language_service/src/completion.rs

language_service/src/completion/global_items.rs

idavis · 2024-10-11T15:45:13Z

language_service/src/tests.rs

+    expect![[r#"
+        334
+    "#]]
+    .assert_debug_eq(
+        &ls.get_completions(
            "foo.qs",
            Position {
                line: 0,
-                column: 76
-            }
+                column: 92,
+            },
        )
        .items
        .len(),
-        13
    );


Would it aid debugging test failures to compare the lists instead of the number of items in the lists?

idavis · 2024-10-11T15:46:18Z

playground/src/main.tsx

@@ -310,7 +304,7 @@ function registerMonacoLanguageServiceProviders(
        }),
      };
    },
-    triggerCharacters: ["@"], // for attribute completions
+    triggerCharacters: ["@", "."],


Should we add a note that we need to keep this list in sync with the one in extension.ts?

idavis · 2024-10-11T15:47:58Z

vscode/src/completion.ts

-    return results.concat(this.samples);
+
+    // Include the samples list for empty documents
+    return document.lineCount === 0 ? results.concat(this.samples) : results;


Would it make sense to make the expected case the first, with fallback after?

return document.lineCount > 0 ? results : results.concat(this.samples);

idavis · 2024-10-11T15:52:04Z

language_service/src/test_utils.rs

    target::Profile,
    LanguageFeatures, PackageStore, PackageType, SourceMap, Span,
 };
 use qsc_project::{PackageGraphSources, PackageInfo};
 use rustc_hash::FxHashMap;

+const FAKE_STDLIB_CONTENTS: &str = r#"


Having std lib in the name is a little consfusing to me. Is this just some lib, or is it supposed to be an alternative std lib?

Yes, it's supposed to replace the stdlib in most language service tests, since it's more minimal (faster to build) and I can easily add any edge case I want to cover.

idavis · 2024-10-11T16:32:09Z

compiler/qsc_doc_gen/src/display.rs

+        if let ast::PathKind::Ok(path) = self.path {
+            write!(f, "{}", path.full_name())
+        } else {
+            write!(f, "?")


Should we delegate this Display impl to IncompletePath which can render the segments with a ?

I don't think this would work in this case. The AST structs' Display impls all print in that "AST-ish" format, like:

Err IncompletePath [157-166]: Ident 13 [157-160] "Foo" Ident 14 [161-164] "Bar"

like what you see in the "AST" tab in the playground. These are not meant to be used when pretty printing to the user. (This line actually had a bug where it was exposing the AST-ish format to the user, which I fixed.

We've had to differentiate these impls from the user visible "pretty printing" impls, which live here. That's why there are a bunch of structs in this file that wrap the AST/HIR structs only to implement Display.

idavis · 2024-10-11T16:47:51Z

language_service/src/references/tests.rs

+                            line: 2,
+                            column: 18,
                        },
                        end: Position {
-                            line: 1,
-                            column: 30,
+                            line: 2,
+                            column: 22,


Why are the lines and columns changing in these tests?

Because of my changes in test_utils.rs - I think I changed some whitespace in the "Fake stdlib" while moving strings around.

idavis · 2024-10-11T17:08:50Z

language_service/src/completion/path_context.rs

+    fn visit_path_kind(&mut self, path: &'a qsc::ast::PathKind) {
+        let span = match path {
+            qsc::ast::PathKind::Ok(path) => &path.span,
+            qsc::ast::PathKind::Err(Some(incomplete_path)) => &incomplete_path.span,
+            qsc::ast::PathKind::Err(None) => return,
+        };


nit: clean up with use to match rest of file

idavis · 2024-10-11T17:09:21Z

language_service/src/completion/path_context.rs

+        self.qualifier = match path {
+            qsc::ast::PathKind::Ok(path) => path.segments.as_ref().map(AsRef::as_ref),
+            qsc::ast::PathKind::Err(Some(incomplete_path)) => Some(&incomplete_path.segments),
+            qsc::ast::PathKind::Err(None) => None,
+        };


nit: clean up with use to match rest of file

Sad story: there are two different structs named PathKind in this file, and the qualifiers here are to disambiguate. Maybe I'll use an alias.

idavis · 2024-10-11T17:41:54Z

compiler/qsc_parse/src/completion/collector.rs

+    pub fn did_advance(&mut self, next_token: &mut Token, scanner_offset: u32) {
+        match self.state {


Should this be advance? Being did_advance I was looking for what bool it was going to return but was ().

This is semantically an "event handler", as in, it's informing the callee that advancing did occur.

I got used to this naming convention in LSP / VS Code APIS (e.g. didDeleteFile ) but I guess it's not coming across :D

Any other suggestions? on_advance()? on_advanced()? advanced() ?

idavis · 2024-10-11T17:48:05Z

language_service/src/completion.rs

+    let source_name_relative = compilation
        .user_unit()
        .sources
-        .find_by_offset(package_offset)
-        .expect("source should exist in the user source map");
-    let source_offset = (package_offset - source.offset)
-        .try_into()
-        .expect("offset can't be converted to uszie");
-    let before_offset = &source.contents[..source_offset];
-    let mut indent = match before_offset.rfind(|c| c == '{' || c == '\n') {
-        Some(begin) => {
-            let indent = &before_offset[begin..];
-            indent.strip_prefix('{').unwrap_or(indent)
-        }
-        None => before_offset,
-    }
-    .to_string();
-    if !indent.starts_with('\n') {
-        indent.insert(0, '\n');
+        .relative_sources()
+        .find(|s| s.offset == source.offset)
+        .expect("source should exist in the user source map")
+        .name;


Would it make sense to expose something like as_relative_source(source: Source) rather than iterating and cloning all of the sources in the map?

let source_name_relative = compilation .user_unit() .sources .as_relative_source(source).name

Yes, I like that idea.

github-actions · 2024-10-11T18:04:39Z

Change in memory usage detected by benchmark.

Memory Report for `c119d44`

Test	This Branch	On Main	Difference
compile core + standard lib	17945380 bytes	17936812 bytes	8568 bytes

github-actions · 2024-10-11T18:45:13Z

Change in memory usage detected by benchmark.

Memory Report for `a9e7e76`

Test	This Branch	On Main	Difference
compile core + standard lib	17945380 bytes	17936812 bytes	8568 bytes

ScottCarda-MS

partially reviewed, will continue to review

compiler/qsc_ast/src/ast.rs

ScottCarda-MS · 2024-10-07T23:06:06Z

compiler/qsc_ast/src/mut_visit.rs

 pub fn walk_ident(vis: &mut impl MutVisitor, ident: &mut Ident) {
    vis.visit_span(&mut ident.span);
 }

-pub fn walk_idents(vis: &mut impl MutVisitor, ident: &mut crate::ast::Idents) {
-    for ref mut ident in &mut *ident.0 {
+pub fn walk_idents(vis: &mut impl MutVisitor, idents: &mut [Ident]) {


Should we drop this visitor handle if idents are a proper node type anymore?

Great question, I went back and forth on this for a while. Ultimately, I decided to keep visit_idents to avoid updating the current usages and bloating this PR even further.

This came up in a conversation with Ian as well, and my response was basically:

In the AST, many of choices came down to "how do I avoid touching a billion lines of code", and this was one of them

I think I'll sleep on it and see on Monday if I want to open a separate refactoring PR, or push even more changes to this one...

ScottCarda-MS · 2024-10-07T23:06:44Z

compiler/qsc_ast/src/visit.rs

+    }
+}
+
+pub fn walk_idents<'a>(vis: &mut impl Visitor<'a>, idents: &'a [Ident]) {


ScottCarda-MS · 2024-10-07T23:46:42Z

compiler/qsc_frontend/src/typeck/tests.rs

@@ -3080,15 +3080,15 @@ fn local_open() {
            namespace B { function Bar() : () {} }
        "},
        "",
-        &expect![[r#"
+        &expect![[r##"


nit: sometimes the macro will erroneously put extra # characters in the expects. Please remove them.

OMG, I took so many passes over this PR trying to revert the sneaky hash changes. I agree with you, but in this file, the warning is suppressed (with #![allow(clippy::needless_raw_string_hashes)]) , and the existing test cases are full of ##. So I thought I should stick to that convention. Now what?! 😆

ScottCarda-MS · 2024-10-10T16:56:22Z

compiler/qsc_parse/src/completion/tests.rs

+        "namespace |",
+        &expect![[r"
+            WordKinds(
+                0x0,


Maybe a comment here saying that 0x0 means no valid completion kinds.

playground/src/main.tsx

minestarks · 2024-10-11T19:29:25Z

@idavis found a bug:

Trying to complete

import QIR.in;

causes a panic since in is a keyword and that's a parse tree I did not expect to have to handle....

Working on the fix.

minestarks · 2024-10-11T22:03:38Z

@idavis found a bug:

Trying to complete
import QIR.in;
causes a panic since in is a keyword and that's a parse tree I did not expect to have to handle....

Working on the fix.

Fixed with my latest commit.

github-actions · 2024-10-11T22:04:34Z

Change in memory usage detected by benchmark.

Memory Report for `96002f0`

Test	This Branch	On Main	Difference
compile core + standard lib	17945380 bytes	17936812 bytes	8568 bytes

minestarks added 4 commits October 3, 2024 22:36

member completions

9cf51c8

rename to PathKind

74a3930

rename function

42fcb76

add a nested syntax test case

38a9916

minestarks changed the title ~~[DRAFT] Context-aware completions, namespace member completions, and other stories~~ More precise completions and namespace member completions Oct 7, 2024

bit more cleanup and more test cases

93c3360

minestarks marked this pull request as ready for review October 7, 2024 16:38

minestarks requested review from idavis, swernli, billti, sezna, cesarzc, ScottCarda-MS and DmitryVasilevsky as code owners October 7, 2024 16:38

swernli reviewed Oct 8, 2024

View reviewed changes

minestarks mentioned this pull request Oct 9, 2024

UDT field completions #1954

Open

swernli reviewed Oct 9, 2024

View reviewed changes

language_service/src/completion.rs Show resolved Hide resolved

minestarks commented Oct 10, 2024

View reviewed changes

language_service/src/completion/global_items.rs Outdated Show resolved Hide resolved

idavis reviewed Oct 11, 2024

View reviewed changes

add a few missed keywords, WordKinds is now u128

3c5b548

UPDATE_EXPECT=1

7c89f2b

ScottCarda-MS reviewed Oct 11, 2024

View reviewed changes

Handle when path ends with a keyword, and multi-character op tokens

96d6dce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More precise completions and namespace member completions #1947

More precise completions and namespace member completions #1947

minestarks commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 7, 2024

swernli Oct 8, 2024

minestarks commented Oct 8, 2024 •

edited

Loading

Memory Report for 6fbbec5

idavis Oct 11, 2024

idavis Oct 11, 2024

idavis Oct 11, 2024

idavis Oct 11, 2024

minestarks Oct 11, 2024

idavis Oct 11, 2024

minestarks Oct 12, 2024

idavis Oct 11, 2024

minestarks Oct 11, 2024

idavis Oct 11, 2024

idavis Oct 11, 2024

minestarks Oct 11, 2024

idavis Oct 11, 2024

minestarks Oct 11, 2024 •

edited

Loading

idavis Oct 11, 2024

minestarks Oct 11, 2024

github-actions bot commented Oct 11, 2024

github-actions bot commented Oct 11, 2024

ScottCarda-MS left a comment

ScottCarda-MS Oct 7, 2024

minestarks Oct 11, 2024

ScottCarda-MS Oct 7, 2024

ScottCarda-MS Oct 7, 2024

minestarks Oct 11, 2024

ScottCarda-MS Oct 10, 2024

minestarks commented Oct 11, 2024

minestarks commented Oct 11, 2024

github-actions bot commented Oct 11, 2024

	/// Returns `None` if this the path has an error.
	/// Returns `None` if the path has an error.

		pub fn did_advance(&mut self, next_token: &mut Token, scanner_offset: u32) {
		match self.state {

More precise completions and namespace member completions #1947

Are you sure you want to change the base?

More precise completions and namespace member completions #1947

Conversation

minestarks commented Oct 3, 2024 • edited Loading

Feature overview

Code changes

Parser completions module

Error recovery - AST changes

Error recovery - parser changes

Language service

Other parser changes

Other notes

Not in this PR (coming next)

github-actions bot commented Oct 7, 2024

Memory Report for 6fbbec5

Choose a reason for hiding this comment

minestarks commented Oct 8, 2024 • edited Loading

Memory Report for 6fbbec5

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2024

Memory Report for c119d44

github-actions bot commented Oct 11, 2024

Memory Report for a9e7e76

ScottCarda-MS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks commented Oct 11, 2024

minestarks commented Oct 11, 2024

github-actions bot commented Oct 11, 2024

Memory Report for 96002f0

minestarks commented Oct 3, 2024 •

edited

Loading

Memory Report for `6fbbec5`

minestarks commented Oct 8, 2024 •

edited

Loading

minestarks Oct 11, 2024 •

edited

Loading

Memory Report for `c119d44`

Memory Report for `a9e7e76`

Memory Report for `96002f0`