feat: visibility modifiers for any module member (#1104)

Closes #1083 ### Summary of Changes All module members can now have a visibility. This is useful if, say, a class is only used internally for typing. Pipelines must always be private, which is also the default if no visibility modifier is provided. They can never be referenced anyway, not even in the same file. --------- Co-authored-by: megalinter-bot <[email protected]>
Safe-DS · Apr 25, 2024 · 3d43d38 · 3d43d38
1 parent 3f1ab6f
commit 3d43d38
Show file tree

Hide file tree

Showing 191 changed files with 2,469 additions and 583 deletions.
diff --git a/packages/safe-ds-lang/src/language/grammar/safe-ds.langium b/packages/safe-ds-lang/src/language/grammar/safe-ds.langium
@@ -111,7 +111,13 @@ QualifiedName returns string:
 // Declarations
 // -----------------------------------------------------------------------------
 
-interface SdsModuleMember extends SdsDeclaration {}
+interface SdsModuleMember extends SdsDeclaration {
+    visibility?: string
+}
+
+SdsVisibility returns string:
+    'internal' | 'private'
+;
 
 SdsAnnotatedModuleMember returns SdsModuleMember:
     {SdsAnnotationCallList}
@@ -121,12 +127,15 @@ SdsAnnotatedModuleMember returns SdsModuleMember:
         SdsAnnotationFragment
 
       | {SdsClass.annotationCallList=current}
+        visibility=SdsVisibility?
         SdsClassFragment
 
       | {SdsEnum.annotationCallList=current}
+        visibility=SdsVisibility?
         SdsEnumFragment
 
       | {SdsFunction.annotationCallList=current}
+        visibility=SdsVisibility?
         SdsFunctionFragment
 
       | {SdsSchema.annotationCallList=current}
@@ -145,12 +154,15 @@ SdsUnannotatedModuleMember returns SdsModuleMember:
       SdsAnnotationFragment
 
     | {SdsClass}
+      visibility=SdsVisibility?
       SdsClassFragment
 
     | {SdsEnum}
+      visibility=SdsVisibility?
       SdsEnumFragment
 
     | {SdsFunction}
+      visibility=SdsVisibility?
       SdsFunctionFragment
 
     | {SdsSchema}
@@ -168,6 +180,7 @@ interface SdsAnnotation extends SdsCallable, SdsModuleMember {
 }
 
 fragment SdsAnnotationFragment:
+    visibility=SdsVisibility?
     'annotation' name=ID
     parameterList=SdsParameterList?
     constraintList=SdsConstraintList?
@@ -312,20 +325,20 @@ interface SdsPipeline extends SdsModuleMember {
 }
 
 fragment SdsPipelineFragment:
+    visibility=SdsVisibility?
     'pipeline'
     name=ID
     body=SdsBlock
 ;
 
 interface SdsSegment extends SdsCallable, SdsModuleMember {
-    visibility?: string
     resultList?: SdsResultList
     constraintList?: SdsConstraintList
     body: SdsBlock
 }
 
 fragment SdsSegmentFragment:
-    visibility=('internal' | 'private')?
+    visibility=SdsVisibility?
     'segment'
     name=ID
     parameterList=SdsParameterList
@@ -1050,6 +1063,7 @@ interface SdsSchema extends SdsModuleMember {
 }
 
 fragment SdsSchemaFragment:
+    visibility=SdsVisibility?
     'schema'
     name=ID
     columnList=SdsColumnList

diff --git a/packages/safe-ds-lang/src/language/helpers/nodeProperties.ts b/packages/safe-ds-lang/src/language/helpers/nodeProperties.ts
@@ -86,11 +86,11 @@ export const hasAnnotationCallOf = (
 };
 
 export const isInternal = (node: SdsDeclaration | undefined): boolean => {
-    return isSdsSegment(node) && node.visibility === 'internal';
+    return isSdsModuleMember(node) && node.visibility === 'internal';
 };
 
 export const isPrivate = (node: SdsDeclaration | undefined): boolean => {
-    return isSdsSegment(node) && node.visibility === 'private';
+    return isSdsModuleMember(node) && node.visibility === 'private';
 };
 
 export namespace Argument {

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
@@ -74,7 +74,7 @@ export class SafeDsCompletionProvider extends DefaultCompletionProvider {
     }
 
     private illegalKeywordsInPipelineFile = new Set(['annotation', 'class', 'enum', 'fun']);
-    private illegalKeywordsInStubFile = new Set(['pipeline', 'internal', 'private', 'segment']);
+    private illegalKeywordsInStubFile = new Set(['pipeline', 'segment']);
 
     protected override filterKeyword(context: CompletionContext, keyword: Keyword): boolean {
         // Filter out keywords that do not contain any word character

diff --git a/packages/safe-ds-lang/src/language/lsp/safe-ds-formatter.ts b/packages/safe-ds-lang/src/language/lsp/safe-ds-formatter.ts
@@ -2,7 +2,7 @@ import { AstNode, CstNode, CstUtils, isAstNode } from 'langium';
 import { last } from '../../helpers/collections.js';
 import * as ast from '../generated/ast.js';
 import { getAnnotationCalls, getLiterals, getTypeArguments } from '../helpers/nodeProperties.js';
-import { AbstractFormatter, Formatting, FormattingAction, FormattingActionOptions } from 'langium/lsp';
+import { AbstractFormatter, Formatting, FormattingAction, FormattingActionOptions, NodeFormatter } from 'langium/lsp';
 import indent = Formatting.indent;
 import newLine = Formatting.newLine;
 import newLines = Formatting.newLines;
@@ -309,10 +309,7 @@ export class SafeDsFormatter extends AbstractFormatter {
     private formatSdsAnnotation(node: ast.SdsAnnotation): void {
         const formatter = this.getNodeFormatter(node);
 
-        if (getAnnotationCalls(node).length > 0) {
-            formatter.keyword('annotation').prepend(newLine());
-        }
-
+        this.formatVisibilityAndKeyword(formatter, node, 'annotation');
         formatter.property('name').prepend(oneSpace());
         formatter.property('parameterList').prepend(noSpace());
         formatter.property('constraintList').prepend(oneSpace());
@@ -337,10 +334,7 @@ export class SafeDsFormatter extends AbstractFormatter {
     private formatSdsClass(node: ast.SdsClass): void {
         const formatter = this.getNodeFormatter(node);
 
-        if (getAnnotationCalls(node).length > 0) {
-            formatter.keyword('class').prepend(newLine());
-        }
-
+        this.formatVisibilityAndKeyword(formatter, node, 'class');
         formatter.property('name').prepend(oneSpace());
         formatter.property('typeParameterList').prepend(noSpace());
         formatter.property('parameterList').prepend(noSpace());
@@ -383,10 +377,7 @@ export class SafeDsFormatter extends AbstractFormatter {
     private formatSdsEnum(node: ast.SdsEnum): void {
         const formatter = this.getNodeFormatter(node);
 
-        if (getAnnotationCalls(node).length > 0) {
-            formatter.keyword('enum').prepend(newLine());
-        }
-
+        this.formatVisibilityAndKeyword(formatter, node, 'enum');
         formatter.property('name').prepend(oneSpace());
         formatter.property('body').prepend(oneSpace());
     }
@@ -428,15 +419,15 @@ export class SafeDsFormatter extends AbstractFormatter {
     formatSdsFunction(node: ast.SdsFunction): void {
         const formatter = this.getNodeFormatter(node);
 
-        if (getAnnotationCalls(node).length > 0) {
-            if (node.isStatic) {
+        if (node.isStatic) {
+            // A static function cannot have a visibility modifier
+            if (getAnnotationCalls(node).length > 0) {
                 formatter.keyword('static').prepend(newLine());
-            } else {
-                formatter.keyword('fun').prepend(newLine());
             }
+            formatter.keyword('static').append(oneSpace());
+        } else {
+            this.formatVisibilityAndKeyword(formatter, node, 'fun');
         }
-
-        formatter.keyword('static').append(oneSpace());
         formatter.property('name').prepend(oneSpace());
         formatter.property('typeParameterList').prepend(noSpace());
         formatter.property('parameterList').prepend(noSpace());
@@ -447,37 +438,39 @@ export class SafeDsFormatter extends AbstractFormatter {
     private formatSdsPipeline(node: ast.SdsPipeline): void {
         const formatter = this.getNodeFormatter(node);
 
-        formatter.property('annotationCallList').prepend(noSpace());
-
-        if (getAnnotationCalls(node).length > 0) {
-            formatter.keyword('pipeline').prepend(newLine());
-        }
-
+        this.formatVisibilityAndKeyword(formatter, node, 'pipeline');
         formatter.property('name').prepend(oneSpace());
         formatter.node(node.body).prepend(oneSpace());
     }
 
     private formatSdsSegment(node: ast.SdsSegment): void {
         const formatter = this.getNodeFormatter(node);
 
+        this.formatVisibilityAndKeyword(formatter, node, 'segment');
+        formatter.property('name').prepend(oneSpace());
+        formatter.property('parameterList').prepend(noSpace());
+        formatter.property('resultList').prepend(oneSpace());
+        formatter.property('constraintList').prepend(oneSpace());
+        formatter.property('body').prepend(oneSpace());
+    }
+
+    private formatVisibilityAndKeyword(
+        formatter: NodeFormatter<ast.SdsModuleMember>,
+        node: ast.SdsModuleMember,
+        keyword: string,
+    ): void {
         if (getAnnotationCalls(node).length === 0) {
             if (node.visibility) {
-                formatter.keyword('segment').prepend(oneSpace());
+                formatter.keyword(keyword).prepend(oneSpace());
             }
         } else {
             if (node.visibility) {
                 formatter.property('visibility').prepend(newLine());
-                formatter.keyword('segment').prepend(oneSpace());
+                formatter.keyword(keyword).prepend(oneSpace());
             } else {
-                formatter.keyword('segment').prepend(newLine());
+                formatter.keyword(keyword).prepend(newLine());
             }
         }
-
-        formatter.property('name').prepend(oneSpace());
-        formatter.property('parameterList').prepend(noSpace());
-        formatter.property('resultList').prepend(oneSpace());
-        formatter.property('constraintList').prepend(oneSpace());
-        formatter.property('body').prepend(oneSpace());
     }
 
     // -----------------------------------------------------------------------------
@@ -963,10 +956,7 @@ export class SafeDsFormatter extends AbstractFormatter {
     private formatSdsSchema(node: ast.SdsSchema) {
         const formatter = this.getNodeFormatter(node);
 
-        if (getAnnotationCalls(node).length > 0) {
-            formatter.keyword('schema').prepend(newLine());
-        }
-
+        this.formatVisibilityAndKeyword(formatter, node, 'schema');
         formatter.property('name').prepend(oneSpace());
         formatter.property('columnList').prepend(oneSpace());
     }

diff --git a/packages/safe-ds-lang/src/language/scoping/safe-ds-scope-computation.ts b/packages/safe-ds-lang/src/language/scoping/safe-ds-scope-computation.ts
@@ -26,11 +26,12 @@ import {
     SdsParameter,
     SdsTypeParameter,
 } from '../generated/ast.js';
+import { isPrivate } from '../helpers/nodeProperties.js';
 
 export class SafeDsScopeComputation extends DefaultScopeComputation {
     protected override exportNode(node: AstNode, exports: AstNodeDescription[], document: LangiumDocument): void {
-        // Modules, pipelines, and private segments cannot be referenced from other documents
-        if (isSdsModule(node) || isSdsPipeline(node) || (isSdsSegment(node) && node.visibility === 'private')) {
+        // Modules, pipelines, and private declarations cannot be referenced from other documents
+        if (isSdsModule(node) || isSdsPipeline(node) || (isSdsDeclaration(node) && isPrivate(node))) {
             return;
         }
 

diff --git a/packages/safe-ds-lang/src/language/validation/other/declarations/moduleMembers.ts b/packages/safe-ds-lang/src/language/validation/other/declarations/moduleMembers.ts
@@ -0,0 +1,27 @@
+import { SafeDsServices } from '../../../safe-ds-module.js';
+import { isSdsPipeline, SdsModuleMember } from '../../../generated/ast.js';
+import { ValidationAcceptor } from 'langium';
+import { DiagnosticTag } from 'vscode-languageserver';
+
+export const CODE_MODULE_MEMBER_UNUSED = 'module-member/unused';
+
+export const moduleMemberShouldBeUsed = (services: SafeDsServices) => {
+    const referenceProvider = services.references.References;
+
+    return (node: SdsModuleMember, accept: ValidationAcceptor) => {
+        // Don't show this warning for pipelines or public declarations
+        if (isSdsPipeline(node) || node.visibility === undefined) {
+            return;
+        }
+
+        const references = referenceProvider.findReferences(node, {});
+        if (references.isEmpty()) {
+            accept('warning', 'This declaration is unused and can be removed.', {
+                node,
+                property: 'name',
+                code: CODE_MODULE_MEMBER_UNUSED,
+                tags: [DiagnosticTag.Unnecessary],
+            });
+        }
+    };
+};
diff --git a/packages/safe-ds-lang/src/language/validation/other/declarations/pipelines.ts b/packages/safe-ds-lang/src/language/validation/other/declarations/pipelines.ts
@@ -0,0 +1,21 @@
+import { ValidationAcceptor } from 'langium';
+import { SdsPipeline } from '../../../generated/ast.js';
+import { isInternal, isPrivate } from '../../../helpers/nodeProperties.js';
+
+export const CODE_PIPELINE_VISIBILITY = 'pipeline/visibility';
+
+export const pipelinesMustBePrivate = (node: SdsPipeline, accept: ValidationAcceptor) => {
+    if (isInternal(node)) {
+        accept('error', 'Pipelines are always private and cannot be declared as internal.', {
+            node,
+            property: 'visibility',
+            code: CODE_PIPELINE_VISIBILITY,
+        });
+    } else if (isPrivate(node)) {
+        accept('info', 'Pipelines are always private and need no explicit visibility modifier.', {
+            node,
+            property: 'visibility',
+            code: CODE_PIPELINE_VISIBILITY,
+        });
+    }
+};
diff --git a/packages/safe-ds-lang/src/language/validation/other/declarations/segments.ts b/packages/safe-ds-lang/src/language/validation/other/declarations/segments.ts
@@ -6,7 +6,6 @@ import { DiagnosticTag } from 'vscode-languageserver';
 
 export const CODE_SEGMENT_DUPLICATE_YIELD = 'segment/duplicate-yield';
 export const CODE_SEGMENT_UNASSIGNED_RESULT = 'segment/unassigned-result';
-export const CODE_SEGMENT_UNUSED = 'segment/unused';
 export const CODE_SEGMENT_UNUSED_PARAMETER = 'segment/unused-parameter';
 
 export const segmentResultMustBeAssignedExactlyOnce = (services: SafeDsServices) => {
@@ -37,27 +36,6 @@ export const segmentResultMustBeAssignedExactlyOnce = (services: SafeDsServices)
     };
 };
 
-export const segmentShouldBeUsed = (services: SafeDsServices) => {
-    const referenceProvider = services.references.References;
-
-    return (node: SdsSegment, accept: ValidationAcceptor) => {
-        // Don't show this warning for public segments
-        if (node.visibility === undefined) {
-            return;
-        }
-
-        const references = referenceProvider.findReferences(node, {});
-        if (references.isEmpty()) {
-            accept('warning', 'This segment is unused and can be removed.', {
-                node,
-                property: 'name',
-                code: CODE_SEGMENT_UNUSED,
-                tags: [DiagnosticTag.Unnecessary],
-            });
-        }
-    };
-};
-
 export const segmentParameterShouldBeUsed = (services: SafeDsServices) => {
     const nodeMapper = services.helpers.NodeMapper;
 

diff --git a/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts b/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts
@@ -76,11 +76,7 @@ import {
     constantParameterMustHaveTypeThatCanBeEvaluatedToConstant,
 } from './other/declarations/parameters.js';
 import { placeholderShouldBeUsed, placeholdersMustNotBeAnAlias } from './other/declarations/placeholders.js';
-import {
-    segmentParameterShouldBeUsed,
-    segmentResultMustBeAssignedExactlyOnce,
-    segmentShouldBeUsed,
-} from './other/declarations/segments.js';
+import { segmentParameterShouldBeUsed, segmentResultMustBeAssignedExactlyOnce } from './other/declarations/segments.js';
 import {
     typeParameterMustBeUsedInCorrectPosition,
     typeParameterMustHaveSufficientContext,
@@ -190,6 +186,8 @@ import {
 } from './other/declarations/parameterBounds.js';
 import { unknownMustOnlyBeUsedAsDefaultValueOfStub } from './other/expressions/literals.js';
 import { tagsShouldNotHaveDuplicateEntries } from './builtins/tags.js';
+import { moduleMemberShouldBeUsed } from './other/declarations/moduleMembers.js';
+import { pipelinesMustBePrivate } from './other/declarations/pipelines.js';
 
 /**
  * Register custom validation checks.
@@ -325,6 +323,7 @@ export const registerValidationChecks = function (services: SafeDsServices) {
             pipelineFileMustNotBeInBuiltinPackage,
             pythonModuleShouldDifferFromSafeDsPackage(services),
         ],
+        SdsModuleMember: [moduleMemberShouldBeUsed(services)],
         SdsNamedType: [
             namedTypeDeclarationShouldNotBeDeprecated(services),
             namedTypeDeclarationShouldNotBeExperimental(services),
@@ -350,7 +349,7 @@ export const registerValidationChecks = function (services: SafeDsServices) {
             parameterBoundRightOperandMustEvaluateToFloatConstantOrIntConstant(services),
         ],
         SdsParameterList: [parameterListMustNotHaveRequiredParametersAfterOptionalParameters],
-        SdsPipeline: [pipelineMustContainUniqueNames],
+        SdsPipeline: [pipelinesMustBePrivate, pipelineMustContainUniqueNames],
         SdsPlaceholder: [placeholdersMustNotBeAnAlias, placeholderShouldBeUsed(services)],
         SdsPrefixOperation: [prefixOperationOperandMustHaveCorrectType(services)],
         SdsReference: [
@@ -367,7 +366,6 @@ export const registerValidationChecks = function (services: SafeDsServices) {
             segmentParameterShouldBeUsed(services),
             segmentResultMustBeAssignedExactlyOnce(services),
             segmentResultListShouldNotBeEmpty(services),
-            segmentShouldBeUsed(services),
         ],
         SdsStatement: [statementMustDoSomething(services)],
         SdsTemplateString: [templateStringMustHaveExpressionBetweenTwoStringParts],