feat: filter suggestions by node type (#999)

Closes #998 ### Summary of Changes * Don't suggest references to annotations/pipelines/schemas * Don't suggest named types pointing to type parameters of containing classes * Fix an issue in the fuzzy matcher: Now the first character of the query can match the first character of the label or the first character of any word transition (e.g. the `C` in `camelCase` or the `c` in `snake_case`).
Safe-DS · Apr 6, 2024 · 8d22e67 · 8d22e67
1 parent 61e776b
commit 8d22e67
Show file tree

Hide file tree

Showing 6 changed files with 209 additions and 55 deletions.
diff --git a/packages/safe-ds-lang/src/language/lsp/safe-ds-completion-provider.ts b/packages/safe-ds-lang/src/language/lsp/safe-ds-completion-provider.ts
@@ -1,13 +1,26 @@
 import { CompletionContext, CompletionValueItem, DefaultCompletionProvider } from 'langium/lsp';
-import { AstNode, AstNodeDescription, ReferenceInfo, Stream } from 'langium';
+import { AstNode, AstNodeDescription, AstUtils, ReferenceInfo, Stream } from 'langium';
 import { SafeDsServices } from '../safe-ds-module.js';
 import { CompletionItemTag, MarkupContent } from 'vscode-languageserver';
 import { createMarkupContent } from '../documentation/safe-ds-comment-provider.js';
 import { SafeDsDocumentationProvider } from '../documentation/safe-ds-documentation-provider.js';
 import type { SafeDsAnnotations } from '../builtins/safe-ds-annotations.js';
-import { isSdsAnnotatedObject, isSdsModule, isSdsNamedType, isSdsReference } from '../generated/ast.js';
+import {
+    isSdsAnnotatedObject,
+    isSdsClass,
+    isSdsDeclaration,
+    isSdsModule,
+    isSdsNamedType,
+    isSdsReference,
+    isSdsTypeArgument,
+    isSdsTypeParameter,
+    SdsAnnotation,
+    SdsPipeline,
+    SdsSchema,
+} from '../generated/ast.js';
 import { getPackageName } from '../helpers/nodeProperties.js';
 import { isInPipelineFile, isInStubFile } from '../helpers/fileExtensions.js';
+import { classTypeParameterIsUsedInCorrectPosition } from '../validation/other/declarations/typeParameters.js';
 
 export class SafeDsCompletionProvider extends DefaultCompletionProvider {
     private readonly builtinAnnotations: SafeDsAnnotations;
@@ -29,34 +42,51 @@ export class SafeDsCompletionProvider extends DefaultCompletionProvider {
         context: CompletionContext,
     ): Stream<AstNodeDescription> {
         this.fixReferenceInfo(refInfo);
-        return super.getReferenceCandidates(refInfo, context);
+        return super
+            .getReferenceCandidates(refInfo, context)
+            .filter((description) => this.filterReferenceCandidate(refInfo, description));
     }
 
-    private fixReferenceInfo(refInfo: ReferenceInfo): void {
-        if (isSdsNamedType(refInfo.container) && refInfo.container.$containerProperty === 'declaration') {
-            const syntheticNode = refInfo.container.$container as AstNode;
-            if (isSdsNamedType(syntheticNode) && syntheticNode.$containerProperty === 'member') {
-                refInfo.container = {
-                    ...refInfo.container,
-                    $container: syntheticNode.$container,
-                    $containerProperty: 'member',
-                };
-            } else {
-                refInfo.container = {
-                    ...refInfo.container,
-                    $containerProperty: 'member',
-                };
-            }
-        } else if (isSdsReference(refInfo.container) && refInfo.container.$containerProperty === 'member') {
-            const syntheticNode = refInfo.container.$container as AstNode;
-            if (isSdsReference(syntheticNode) && syntheticNode.$containerProperty === 'member') {
-                refInfo.container = {
-                    ...refInfo.container,
-                    $container: syntheticNode.$container,
-                    $containerProperty: 'member',
-                };
+    private illegalNodeTypesForReferences = new Set([SdsAnnotation, SdsPipeline, SdsSchema]);
+
+    private filterReferenceCandidate(refInfo: ReferenceInfo, description: AstNodeDescription): boolean {
+        if (isSdsNamedType(refInfo.container)) {
+            if (isSdsTypeParameter(description.node)) {
+                const declarationWithTypeParameter = AstUtils.getContainerOfType(
+                    description.node.$container,
+                    isSdsDeclaration,
+                );
+
+                return (
+                    !isSdsClass(declarationWithTypeParameter) ||
+                    classTypeParameterIsUsedInCorrectPosition(declarationWithTypeParameter, refInfo.container)
+                );
             }
+        } else if (isSdsReference(refInfo.container)) {
+            return !this.illegalNodeTypesForReferences.has(description.type);
         }
+
+        return true;
+    }
+
+    private illegalKeywordsInPipelineFile = new Set(['annotation', 'class', 'enum', 'fun', 'schema']);
+    private illegalKeywordsInStubFile = new Set(['pipeline', 'internal', 'private', 'segment']);
+
+    protected override filterKeyword(context: CompletionContext, keyword: Keyword): boolean {
+        // Filter out keywords that do not contain any word character
+        if (!/\p{L}/u.test(keyword.value)) {
+            return false;
+        }
+
+        if ((!context.node || isSdsModule(context.node)) && !getPackageName(context.node)) {
+            return keyword.value === 'package';
+        } else if (isSdsModule(context.node) && isInPipelineFile(context.node)) {
+            return !this.illegalKeywordsInPipelineFile.has(keyword.value);
+        } else if (isSdsModule(context.node) && isInStubFile(context.node)) {
+            return !this.illegalKeywordsInStubFile.has(keyword.value);
+        }
+
+        return true;
     }
 
     protected override createReferenceCompletionItem(nodeDescription: AstNodeDescription): CompletionValueItem {
@@ -89,23 +119,39 @@ export class SafeDsCompletionProvider extends DefaultCompletionProvider {
         }
     }
 
-    private illegalKeywordsInPipelineFile = new Set(['annotation', 'class', 'enum', 'fun', 'schema']);
-    private illegalKeywordsInStubFile = new Set(['pipeline', 'internal', 'private', 'segment']);
-
-    protected override filterKeyword(context: CompletionContext, keyword: Keyword): boolean {
-        // Filter out keywords that do not contain any word character
-        if (!/\p{L}/u.test(keyword.value)) {
-            return false;
-        }
-
-        if ((!context.node || isSdsModule(context.node)) && !getPackageName(context.node)) {
-            return keyword.value === 'package';
-        } else if (isSdsModule(context.node) && isInPipelineFile(context.node)) {
-            return !this.illegalKeywordsInPipelineFile.has(keyword.value);
-        } else if (isSdsModule(context.node) && isInStubFile(context.node)) {
-            return !this.illegalKeywordsInStubFile.has(keyword.value);
-        } else {
-            return true;
+    private fixReferenceInfo(refInfo: ReferenceInfo): void {
+        if (isSdsNamedType(refInfo.container) && refInfo.container.$containerProperty === 'declaration') {
+            const syntheticNode = refInfo.container.$container as AstNode;
+            if (isSdsNamedType(syntheticNode) && syntheticNode.$containerProperty === 'member') {
+                refInfo.container = {
+                    ...refInfo.container,
+                    $container: syntheticNode.$container,
+                    $containerProperty: 'member',
+                };
+            } else {
+                refInfo.container = {
+                    ...refInfo.container,
+                    $containerProperty: 'member',
+                };
+            }
+        } else if (isSdsReference(refInfo.container) && refInfo.container.$containerProperty === 'member') {
+            const syntheticNode = refInfo.container.$container as AstNode;
+            if (isSdsReference(syntheticNode) && syntheticNode.$containerProperty === 'member') {
+                refInfo.container = {
+                    ...refInfo.container,
+                    $container: syntheticNode.$container,
+                    $containerProperty: 'member',
+                };
+            }
+        } else if (isSdsTypeArgument(refInfo.container) && refInfo.container.$containerProperty === 'typeParameter') {
+            const syntheticNode = refInfo.container.$container as AstNode;
+            if (isSdsNamedType(syntheticNode) && syntheticNode.$containerProperty === 'value') {
+                refInfo.container = {
+                    ...refInfo.container,
+                    $container: syntheticNode.$container,
+                    $containerProperty: 'typeParameter',
+                };
+            }
         }
     }
 }

diff --git a/packages/safe-ds-lang/src/language/lsp/safe-ds-fuzzy-matcher.ts b/packages/safe-ds-lang/src/language/lsp/safe-ds-fuzzy-matcher.ts
@@ -0,0 +1,33 @@
+import { DefaultFuzzyMatcher } from 'langium/lsp';
+
+export class SafeDsFuzzyMatcher extends DefaultFuzzyMatcher {
+    public override match(query: string, text: string): boolean {
+        // Fixes a bug in the default fuzzy matcher. Can be removed once the bug is fixed upstream.
+        if (query.length === 0) {
+            return true;
+        }
+
+        // eslint-disable-next-line no-param-reassign
+        let matchedFirstCharacter = false;
+        let previous: number | undefined = undefined;
+        let character = 0;
+        const len = text.length;
+        for (let i = 0; i < len; i++) {
+            const strChar = text.charCodeAt(i);
+            const testChar = query.charCodeAt(character);
+            if (strChar === testChar || this.toUpperCharCode(strChar) === this.toUpperCharCode(testChar)) {
+                matchedFirstCharacter ||=
+                    previous === undefined || // Beginning of word
+                    this.isWordTransition(previous, strChar);
+                if (matchedFirstCharacter) {
+                    character++;
+                }
+                if (character === query.length) {
+                    return true;
+                }
+            }
+            previous = strChar;
+        }
+        return false;
+    }
+}
diff --git a/packages/safe-ds-lang/src/language/safe-ds-module.ts b/packages/safe-ds-lang/src/language/safe-ds-module.ts
@@ -44,6 +44,7 @@ import { SafeDsRunner } from './runner/safe-ds-runner.js';
 import { SafeDsTypeFactory } from './typing/safe-ds-type-factory.js';
 import { SafeDsMarkdownGenerator } from './generation/safe-ds-markdown-generator.js';
 import { SafeDsCompletionProvider } from './lsp/safe-ds-completion-provider.js';
+import { SafeDsFuzzyMatcher } from './lsp/safe-ds-fuzzy-matcher.js';
 
 /**
  * Declaration of custom services - add your own service classes here.
@@ -170,6 +171,7 @@ export type SafeDsSharedServices = LangiumSharedServices;
 
 export const SafeDsSharedModule: Module<SafeDsSharedServices, DeepPartial<SafeDsSharedServices>> = {
     lsp: {
+        FuzzyMatcher: () => new SafeDsFuzzyMatcher(),
         NodeKindProvider: () => new SafeDsNodeKindProvider(),
     },
     workspace: {

diff --git a/packages/safe-ds-lang/src/language/validation/other/declarations/typeParameters.ts b/packages/safe-ds-lang/src/language/validation/other/declarations/typeParameters.ts
@@ -4,6 +4,7 @@ import {
     isSdsClass,
     isSdsClassMember,
     isSdsDeclaration,
+    isSdsNamedType,
     isSdsNamedTypeDeclaration,
     isSdsParameter,
     isSdsParameterList,
@@ -93,7 +94,7 @@ export const typeParameterMustBeUsedInCorrectPosition = (services: SafeDsService
 
         AstUtils.findLocalReferences(node).forEach((it) => {
             const reference = it.$refNode?.astNode;
-            if (!reference) {
+            if (!reference || !isSdsNamedType(reference)) {
                 /* c8 ignore next 2 */
                 return;
             }
@@ -145,7 +146,7 @@ const isInConstructor = (node: AstNode) => {
     return isSdsClass(parameterList?.$container);
 };
 
-const classTypeParameterIsUsedInCorrectPosition = (classWithTypeParameter: SdsClass, reference: AstNode) => {
+export const classTypeParameterIsUsedInCorrectPosition = (classWithTypeParameter: SdsClass, reference: AstNode) => {
     const containingClassMember = AstUtils.getContainerOfType(reference, isSdsClassMember);
 
     // Handle usage in constructor

diff --git a/packages/safe-ds-lang/tests/language/lsp/safe-ds.completion-provider.test.ts b/packages/safe-ds-lang/tests/language/lsp/safe-ds.completion-provider.test.ts
@@ -255,7 +255,9 @@ describe('SafeDsCompletionProvider', async () => {
             {
                 testName: 'type arguments (no prefix)',
                 code: `
-                    class MyClass<T> {
+                    class MyClass<T>
+
+                    class OtherClass {
                         attr a: MyClass<<|>
                 `,
                 expectedLabels: {
@@ -265,7 +267,9 @@ describe('SafeDsCompletionProvider', async () => {
             {
                 testName: 'type arguments (with prefix)',
                 code: `
-                    class MyClass<T1, U2> {
+                    class MyClass<T1, U2>
+
+                    class OtherClass {
                         attr a: MyClass<T<|>
                 `,
                 expectedLabels: {
@@ -294,6 +298,69 @@ describe('SafeDsCompletionProvider', async () => {
                     shouldNotContain: ['s2'],
                 },
             },
+
+            // Filtering by node type
+            {
+                testName: 'named type to type parameter of containing class',
+                code: `
+                    class MyClass<T1> {
+                        class MyNestedClass<T2>(p: <|>
+                    }
+                `,
+                expectedLabels: {
+                    shouldContain: ['T2'],
+                    shouldNotContain: ['T1'],
+                },
+            },
+            {
+                testName: 'reference to annotation',
+                code: `
+                    annotation MyAnnotation
+
+                    pipeline myPipeline {
+                        <|>
+                `,
+                expectedLabels: {
+                    shouldNotContain: ['MyAnnotation'],
+                },
+            },
+            {
+                testName: 'reference to pipeline',
+                code: `
+                    pipeline myPipeline {
+                        <|>
+                `,
+                expectedLabels: {
+                    shouldNotContain: ['myPipeline'],
+                },
+            },
+            {
+                testName: 'reference to schema',
+                code: `
+                    schema MySchema {}
+
+                    pipeline myPipeline {
+                        <|>
+                `,
+                expectedLabels: {
+                    shouldNotContain: ['MySchema'],
+                },
+            },
+
+            // Special cases
+            {
+                testName: 'fuzzy matching',
+                code: `
+                    annotation Annotation
+                    annotation MyAnnotation
+                    annotation OtherAnnotation
+
+                    @Anno<|>
+                `,
+                expectedLabels: {
+                    shouldContain: ['Annotation', 'MyAnnotation', 'OtherAnnotation'],
+                },
+            },
         ];
 
         it.each(testCases)('$testName', async ({ code, uri, expectedLabels }) => {

diff --git a/...validation/other/declarations/type parameters/usage of class type parameters/main.sdstest b/...validation/other/declarations/type parameters/usage of class type parameters/main.sdstest
@@ -4,41 +4,46 @@ package tests.validation.other.declarations.typeParameters.usageOfClassTypeParam
 // $TEST$ no error "This type parameter of a containing class cannot be used here."
 class MyClass<T>(p: »T«) sub »T« {
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
-    attr a: »T«
+    attr a1: »T«
 
     // $TEST$ error "This type parameter of a containing class cannot be used here."
-    static attr a: »T«
+    static attr a2: »T«
 
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
-    fun f<S>(p1: »T«, p2: »S«) -> (r1: »T«, r2: »S«)
+    @Pure fun f1<S>(p1: »T«, p2: »S«) -> (r1: »T«, r2: »S«)
 
     // $TEST$ error "This type parameter of a containing class cannot be used here."
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
     // $TEST$ error "This type parameter of a containing class cannot be used here."
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
-    static fun f<S>(p1: »T«, p2: »S«) -> (r1: »T«, r2: »S«)
+    @Pure static fun f2<S>(p1: »T«, p2: »S«) -> (r1: »T«, r2: »S«)
 
     // $TEST$ error "This type parameter of a containing class cannot be used here."
     // $TEST$ no error "This type parameter of a containing class cannot be used here."
     class MyInnerClass<S>(p1: »T«, p2: »S«) {
         // $TEST$ error "This type parameter of a containing class cannot be used here."
-        attr a: »T«
+        attr a1: »T«
 
         // $TEST$ error "This type parameter of a containing class cannot be used here."
-        static attr a: »T«
+        static attr a2: »T«
 
         // $TEST$ error "This type parameter of a containing class cannot be used here."
-        fun f(p: »T«)
+        @Pure fun f1(p: »T«)
 
         // $TEST$ error "This type parameter of a containing class cannot be used here."
-        static fun f(p: »T«)
+        @Pure static fun f2(p: »T«)
     }
 
     enum MyInnerEnum {
         // $TEST$ error "This type parameter of a containing class cannot be used here."
         MyEnumVariant(p1: »T«)
     }
 }
+
+class MyOtherClass {
+    // $TEST$ no error "This type parameter of a containing class cannot be used here."
+    attr a: MyClass<»T« = Int>
+}