Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/language/helpers/nodeProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
SdsCallable,
SdsClass,
SdsClassMember,
SdsColumn,
SdsDeclaration,
SdsEnum,
SdsEnumVariant,
Expand All @@ -42,6 +43,7 @@ import {
SdsQualifiedImport,
SdsResult,
SdsResultList,
SdsSchema,
SdsStatement,
SdsType,
SdsTypeArgument,
Expand Down Expand Up @@ -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 ?? [];
};
Expand All @@ -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) ?? [];
};
Expand Down
107 changes: 101 additions & 6 deletions src/language/validation/names.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
isSdsQualifiedImport,
SdsAnnotation,
SdsBlockLambda,
SdsCallableType,
Expand All @@ -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';
Expand Down Expand Up @@ -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),
Expand All @@ -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(
Expand All @@ -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,
});
}
}
};
14 changes: 11 additions & 3 deletions src/language/validation/other/modules.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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);
};
11 changes: 10 additions & 1 deletion src/language/validation/safe-ds-validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@ import {
enumVariantMustContainUniqueNames,
expressionLambdaMustContainUniqueNames,
functionMustContainUniqueNames,
moduleMemberMustHaveNameThatIsUniqueInPackage,
moduleMustContainUniqueNames,
nameMustNotStartWithBlockLambdaPrefix,
nameShouldHaveCorrectCasing,
pipelineMustContainUniqueNames,
schemaMustContainUniqueNames,
segmentMustContainUniqueNames,
} from './names.js';
import {
Expand Down Expand Up @@ -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),
Expand All @@ -181,6 +189,7 @@ export const registerValidationChecks = function (services: SafeDsServices) {
referenceTargetShouldNotExperimental(services),
],
SdsResult: [resultMustHaveTypeHint],
SdsSchema: [schemaMustContainUniqueNames],
SdsSegment: [
segmentMustContainUniqueNames,
segmentParameterShouldBeUsed(services),
Expand Down
3 changes: 0 additions & 3 deletions src/resources/builtins/safeds/lang/coreClasses.sdsstub
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
Original file line number Diff line number Diff line change
@@ -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«() {}
Original file line number Diff line number Diff line change
@@ -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() {}
Original file line number Diff line number Diff line change
@@ -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() {}
Loading