feat: error if names are not unique (part 2) (#640)

Closes partially #543 ### Summary of Changes Show an error if * module members in the same file have duplicate names, * module members in the same package, but different files have duplicate names, * schema columns have duplicate names. --------- Co-authored-by: megalinter-bot <[email protected]>
Safe-DS · Oct 16, 2023 · 38d1181 · 38d1181
1 parent e0fa032
commit 38d1181
Show file tree

Hide file tree

Showing 15 changed files with 521 additions and 19 deletions.
diff --git a/src/language/helpers/nodeProperties.ts b/src/language/helpers/nodeProperties.ts
@@ -27,6 +27,7 @@ import {
     SdsCallable,
     SdsClass,
     SdsClassMember,
+    SdsColumn,
     SdsDeclaration,
     SdsEnum,
     SdsEnumVariant,
@@ -42,6 +43,7 @@ import {
     SdsQualifiedImport,
     SdsResult,
     SdsResultList,
+    SdsSchema,
     SdsStatement,
     SdsType,
     SdsTypeArgument,
@@ -132,20 +134,18 @@ export const blockLambdaResultsOrEmpty = (node: SdsBlockLambda | undefined): Sds
         .filter(isSdsBlockLambdaResult)
         .toArray();
 };
-export const importedDeclarationsOrEmpty = (node: SdsQualifiedImport | undefined): SdsImportedDeclaration[] => {
-    return node?.importedDeclarationList?.importedDeclarations ?? [];
-};
 
-export const literalsOrEmpty = (node: SdsLiteralType | undefined): SdsLiteral[] => {
-    return node?.literalList?.literals ?? [];
-};
 export const classMembersOrEmpty = (
     node: SdsClass | undefined,
     filterFunction: (member: SdsClassMember) => boolean = () => true,
 ): SdsClassMember[] => {
     return node?.body?.members?.filter(filterFunction) ?? [];
 };
 
+export const columnsOrEmpty = (node: SdsSchema | undefined): SdsColumn[] => {
+    return node?.columnList?.columns ?? [];
+};
+
 export const enumVariantsOrEmpty = (node: SdsEnum | undefined): SdsEnumVariant[] => {
     return node?.body?.variants ?? [];
 };
@@ -154,6 +154,14 @@ export const importsOrEmpty = (node: SdsModule | undefined): SdsImport[] => {
     return node?.imports ?? [];
 };
 
+export const importedDeclarationsOrEmpty = (node: SdsQualifiedImport | undefined): SdsImportedDeclaration[] => {
+    return node?.importedDeclarationList?.importedDeclarations ?? [];
+};
+
+export const literalsOrEmpty = (node: SdsLiteralType | undefined): SdsLiteral[] => {
+    return node?.literalList?.literals ?? [];
+};
+
 export const moduleMembersOrEmpty = (node: SdsModule | undefined): SdsModuleMember[] => {
     return node?.members?.filter(isSdsModuleMember) ?? [];
 };

diff --git a/src/language/validation/names.ts b/src/language/validation/names.ts
@@ -1,4 +1,5 @@
 import {
+    isSdsQualifiedImport,
     SdsAnnotation,
     SdsBlockLambda,
     SdsCallableType,
@@ -8,21 +9,32 @@ import {
     SdsEnumVariant,
     SdsExpressionLambda,
     SdsFunction,
+    SdsImportedDeclaration,
+    SdsModule,
     SdsPipeline,
+    SdsSchema,
     SdsSegment,
 } from '../generated/ast.js';
-import { ValidationAcceptor } from 'langium';
+import { getDocument, ValidationAcceptor } from 'langium';
 import {
     blockLambdaResultsOrEmpty,
     classMembersOrEmpty,
+    columnsOrEmpty,
     enumVariantsOrEmpty,
+    importedDeclarationsOrEmpty,
+    importsOrEmpty,
     isStatic,
+    moduleMembersOrEmpty,
+    packageNameOrUndefined,
     parametersOrEmpty,
     placeholdersOrEmpty,
     resultsOrEmpty,
     typeParametersOrEmpty,
 } from '../helpers/nodeProperties.js';
 import { duplicatesBy } from '../helpers/collectionUtils.js';
+import { isInPipelineFile, isInStubFile, isInTestFile } from '../helpers/fileExtensions.js';
+import { declarationIsAllowedInPipelineFile, declarationIsAllowedInStubFile } from './other/modules.js';
+import { SafeDsServices } from '../safe-ds-module.js';
 
 export const CODE_NAME_BLOCK_LAMBDA_PREFIX = 'name/block-lambda-prefix';
 export const CODE_NAME_CASING = 'name/casing';
@@ -211,6 +223,75 @@ export const functionMustContainUniqueNames = (node: SdsFunction, accept: Valida
     );
 };
 
+export const moduleMemberMustHaveNameThatIsUniqueInPackage = (services: SafeDsServices) => {
+    const packageManager = services.workspace.PackageManager;
+
+    return (node: SdsModule, accept: ValidationAcceptor): void => {
+        for (const member of moduleMembersOrEmpty(node)) {
+            const packageName = packageNameOrUndefined(member) ?? '';
+            const declarationsInPackage = packageManager.getDeclarationsInPackage(packageName);
+            const memberUri = getDocument(member).uri?.toString();
+
+            if (
+                declarationsInPackage.some((it) => it.name === member.name && it.documentUri.toString() !== memberUri)
+            ) {
+                accept('error', `Multiple declarations in this package have the name '${member.name}'.`, {
+                    node: member,
+                    property: 'name',
+                    code: CODE_NAME_DUPLICATE,
+                });
+            }
+        }
+    };
+};
+
+export const moduleMustContainUniqueNames = (node: SdsModule, accept: ValidationAcceptor): void => {
+    // Names of imported declarations must be unique
+    const importedDeclarations = importsOrEmpty(node).filter(isSdsQualifiedImport).flatMap(importedDeclarationsOrEmpty);
+    for (const duplicate of duplicatesBy(importedDeclarations, importedDeclarationName)) {
+        if (duplicate.alias) {
+            accept('error', `A declaration with name '${importedDeclarationName(duplicate)}' was imported already.`, {
+                node: duplicate.alias,
+                property: 'alias',
+                code: CODE_NAME_DUPLICATE,
+            });
+        } else {
+            accept('error', `A declaration with name '${importedDeclarationName(duplicate)}' was imported already.`, {
+                node: duplicate,
+                property: 'declaration',
+                code: CODE_NAME_DUPLICATE,
+            });
+        }
+    }
+
+    // Names of module members must be unique
+    if (isInPipelineFile(node)) {
+        namesMustBeUnique(
+            moduleMembersOrEmpty(node),
+            (name) => `A declaration with name '${name}' exists already in this file.`,
+            accept,
+            declarationIsAllowedInPipelineFile,
+        );
+    } else if (isInStubFile(node)) {
+        namesMustBeUnique(
+            moduleMembersOrEmpty(node),
+            (name) => `A declaration with name '${name}' exists already in this file.`,
+            accept,
+            declarationIsAllowedInStubFile,
+        );
+    } else if (isInTestFile(node)) {
+        namesMustBeUnique(
+            moduleMembersOrEmpty(node),
+            (name) => `A declaration with name '${name}' exists already in this file.`,
+            accept,
+        );
+    }
+};
+
+const importedDeclarationName = (node: SdsImportedDeclaration | undefined): string | undefined => {
+    return node?.alias?.alias ?? node?.declaration.ref?.name;
+};
+
 export const pipelineMustContainUniqueNames = (node: SdsPipeline, accept: ValidationAcceptor): void => {
     namesMustBeUnique(
         placeholdersOrEmpty(node.body),
@@ -219,6 +300,17 @@ export const pipelineMustContainUniqueNames = (node: SdsPipeline, accept: Valida
     );
 };
 
+export const schemaMustContainUniqueNames = (node: SdsSchema, accept: ValidationAcceptor): void => {
+    const duplicates = duplicatesBy(columnsOrEmpty(node), (it) => it.columnName.value);
+    for (const duplicate of duplicates) {
+        accept('error', `A column with name '${duplicate.columnName.value}' exists already.`, {
+            node: duplicate,
+            property: 'columnName',
+            code: CODE_NAME_DUPLICATE,
+        });
+    }
+};
+
 export const segmentMustContainUniqueNames = (node: SdsSegment, accept: ValidationAcceptor): void => {
     const parametersAndPlaceholder = [...parametersOrEmpty(node), ...placeholdersOrEmpty(node.body)];
     namesMustBeUnique(
@@ -238,12 +330,15 @@ const namesMustBeUnique = (
     nodes: Iterable<SdsDeclaration>,
     createMessage: (name: string) => string,
     accept: ValidationAcceptor,
+    shouldReportErrorOn: (node: SdsDeclaration) => boolean = () => true,
 ): void => {
     for (const node of duplicatesBy(nodes, (it) => it.name)) {
-        accept('error', createMessage(node.name), {
-            node,
-            property: 'name',
-            code: CODE_NAME_DUPLICATE,
-        });
+        if (shouldReportErrorOn(node)) {
+            accept('error', createMessage(node.name), {
+                node,
+                property: 'name',
+                code: CODE_NAME_DUPLICATE,
+            });
+        }
     }
 };
diff --git a/src/language/validation/other/modules.ts b/src/language/validation/other/modules.ts
@@ -1,5 +1,5 @@
 import { ValidationAcceptor } from 'langium';
-import { isSdsDeclaration, isSdsPipeline, isSdsSegment, SdsModule } from '../../generated/ast.js';
+import { isSdsDeclaration, isSdsPipeline, isSdsSegment, SdsDeclaration, SdsModule } from '../../generated/ast.js';
 import { isInPipelineFile, isInStubFile } from '../../helpers/fileExtensions.js';
 
 export const CODE_MODULE_MISSING_PACKAGE = 'module/missing-package';
@@ -24,7 +24,7 @@ export const moduleDeclarationsMustMatchFileKind = (node: SdsModule, accept: Val
 
     if (isInPipelineFile(node)) {
         for (const declaration of declarations) {
-            if (!isSdsPipeline(declaration) && !isSdsSegment(declaration)) {
+            if (!declarationIsAllowedInPipelineFile(declaration)) {
                 accept('error', 'A pipeline file must only declare pipelines and segments.', {
                     node: declaration,
                     property: 'name',
@@ -34,7 +34,7 @@ export const moduleDeclarationsMustMatchFileKind = (node: SdsModule, accept: Val
         }
     } else if (isInStubFile(node)) {
         for (const declaration of declarations) {
-            if (isSdsPipeline(declaration) || isSdsSegment(declaration)) {
+            if (!declarationIsAllowedInStubFile(declaration)) {
                 accept('error', 'A stub file must not declare pipelines or segments.', {
                     node: declaration,
                     property: 'name',
@@ -44,3 +44,11 @@ export const moduleDeclarationsMustMatchFileKind = (node: SdsModule, accept: Val
         }
     }
 };
+
+export const declarationIsAllowedInPipelineFile = (declaration: SdsDeclaration): boolean => {
+    return isSdsPipeline(declaration) || isSdsSegment(declaration);
+};
+
+export const declarationIsAllowedInStubFile = (declaration: SdsDeclaration): boolean => {
+    return !isSdsPipeline(declaration) && !isSdsSegment(declaration);
+};
diff --git a/src/language/validation/safe-ds-validator.ts b/src/language/validation/safe-ds-validator.ts
@@ -10,9 +10,12 @@ import {
     enumVariantMustContainUniqueNames,
     expressionLambdaMustContainUniqueNames,
     functionMustContainUniqueNames,
+    moduleMemberMustHaveNameThatIsUniqueInPackage,
+    moduleMustContainUniqueNames,
     nameMustNotStartWithBlockLambdaPrefix,
     nameShouldHaveCorrectCasing,
     pipelineMustContainUniqueNames,
+    schemaMustContainUniqueNames,
     segmentMustContainUniqueNames,
 } from './names.js';
 import {
@@ -156,7 +159,12 @@ export const registerValidationChecks = function (services: SafeDsServices) {
             memberAccessMustBeNullSafeIfReceiverIsNullable(services),
             memberAccessNullSafetyShouldBeNeeded(services),
         ],
-        SdsModule: [moduleDeclarationsMustMatchFileKind, moduleWithDeclarationsMustStatePackage],
+        SdsModule: [
+            moduleDeclarationsMustMatchFileKind,
+            moduleMemberMustHaveNameThatIsUniqueInPackage(services),
+            moduleMustContainUniqueNames,
+            moduleWithDeclarationsMustStatePackage,
+        ],
         SdsNamedType: [
             namedTypeDeclarationShouldNotBeDeprecated(services),
             namedTypeDeclarationShouldNotBeExperimental(services),
@@ -181,6 +189,7 @@ export const registerValidationChecks = function (services: SafeDsServices) {
             referenceTargetShouldNotExperimental(services),
         ],
         SdsResult: [resultMustHaveTypeHint],
+        SdsSchema: [schemaMustContainUniqueNames],
         SdsSegment: [
             segmentMustContainUniqueNames,
             segmentParameterShouldBeUsed(services),

diff --git a/src/resources/builtins/safeds/lang/coreClasses.sdsstub b/src/resources/builtins/safeds/lang/coreClasses.sdsstub
@@ -18,9 +18,6 @@ class Int sub Number
 @Description("A floating-point number.")
 class Float sub Number
 
-@Description("A floating-point number.")
-class Float sub Number
-
 @Description("A list of elements.")
 class List<E>
 

diff --git a/tests/resources/validation/names/duplicates/across files/main.sdstest b/tests/resources/validation/names/duplicates/across files/main.sdstest
@@ -0,0 +1,78 @@
+package tests.validation.names.acrossFiles
+
+// $TEST$ error "Multiple declarations in this package have the name 'DuplicateAnnotation'."
+annotation »DuplicateAnnotation«
+// $TEST$ error "Multiple declarations in this package have the name 'DuplicateClass'."
+class »DuplicateClass«
+// $TEST$ error "Multiple declarations in this package have the name 'DuplicateEnum'."
+enum »DuplicateEnum«
+// $TEST$ error "Multiple declarations in this package have the name 'duplicateFunction'."
+fun »duplicateFunction«()
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+pipeline »duplicatePipeline« {}
+// $TEST$ error "Multiple declarations in this package have the name 'DuplicateSchema'."
+schema »DuplicateSchema« {}
+// $TEST$ error "Multiple declarations in this package have the name 'duplicatePublicSegment'."
+segment »duplicatePublicSegment«() {}
+// $TEST$ error "Multiple declarations in this package have the name 'duplicateInternalSegment'."
+internal segment »duplicateInternalSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+private segment »duplicatePrivateSegment«() {}
+
+
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+annotation »UniqueAnnotation«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+class »UniqueClass«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+enum »UniqueEnum«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+fun »uniqueFunction«()
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+pipeline »uniquePipeline« {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+schema »UniqueSchema« {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+segment »uniquePublicSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+internal segment »uniqueInternalSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+private segment »uniquePrivateSegment«() {}
+
+
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+annotation »MyAnnotation«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+annotation »MyAnnotation«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+class »MyClass«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+class »MyClass«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+enum »MyEnum«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+enum »MyEnum«
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+fun »myFunction«()
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+fun »myFunction«()
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+pipeline »myPipeline« {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+pipeline »myPipeline« {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+schema »MySchema« {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+schema »MySchema« {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+segment »myPublicSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+segment »myPublicSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+internal segment »myInternalSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+internal segment »myInternalSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+private segment »myPrivateSegment«() {}
+// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\."
+private segment »myPrivateSegment«() {}
diff --git a/tests/resources/validation/names/duplicates/across files/other package.sdstest b/tests/resources/validation/names/duplicates/across files/other package.sdstest
@@ -0,0 +1,11 @@
+package tests.validation.names.acrossFiles.other
+
+annotation UniqueAnnotation
+class UniqueClass
+enum UniqueEnum
+fun uniqueFunction()
+pipeline uniquePipeline {}
+schema UniqueSchema {}
+segment uniquePublicSegment() {}
+internal segment uniqueInternalSegment() {}
+private segment uniquePrivateSegment() {}
diff --git a/tests/resources/validation/names/duplicates/across files/same package.sdstest b/tests/resources/validation/names/duplicates/across files/same package.sdstest
@@ -0,0 +1,11 @@
+package tests.validation.names.acrossFiles
+
+annotation DuplicateAnnotation
+class DuplicateClass
+enum DuplicateEnum
+fun duplicateFunction()
+pipeline duplicatePipeline {}
+schema DuplicateSchema {}
+segment duplicatePublicSegment() {}
+internal segment duplicateInternalSegment() {}
+private segment duplicatePrivateSegment() {}