From 275ad5e62f3180673be564c92c40d4012f4322cd Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Sun, 22 Oct 2023 22:48:05 +0200 Subject: [PATCH] feat: error if simple names of builtin declarations collide (#678) Closes #672 ### Summary of Changes --- src/language/builtins/packageNames.ts | 1 + .../lsp/safe-ds-node-kind-provider.ts | 3 ++ .../lsp/safe-ds-semantic-token-provider.ts | 6 +++ src/language/validation/names.ts | 53 ++++++++++++------- src/language/validation/other/modules.ts | 5 +- .../safe-ds-document-symbol-provider.test.ts | 14 +++++ .../safe-ds-semantic-token-provider.test.ts | 5 ++ .../across files/other package.sdstest | 27 ++++++---- .../duplicates/across files/safeds 1.sdstest | 27 ++++++++++ .../duplicates/across files/safeds 2.sdstest | 20 +++++++ .../duplicates/across files/safeds.sdstest | 8 --- .../across files/same package.sdstest | 27 ++++++---- 12 files changed, 150 insertions(+), 46 deletions(-) create mode 100644 src/language/builtins/packageNames.ts create mode 100644 tests/resources/validation/names/duplicates/across files/safeds 1.sdstest create mode 100644 tests/resources/validation/names/duplicates/across files/safeds 2.sdstest delete mode 100644 tests/resources/validation/names/duplicates/across files/safeds.sdstest diff --git a/src/language/builtins/packageNames.ts b/src/language/builtins/packageNames.ts new file mode 100644 index 000000000..b6abe37d9 --- /dev/null +++ b/src/language/builtins/packageNames.ts @@ -0,0 +1 @@ +export const BUILTINS_ROOT_PACKAGE = 'safeds'; diff --git a/src/language/lsp/safe-ds-node-kind-provider.ts b/src/language/lsp/safe-ds-node-kind-provider.ts index e2fc1f86f..4de7e87bb 100644 --- a/src/language/lsp/safe-ds-node-kind-provider.ts +++ b/src/language/lsp/safe-ds-node-kind-provider.ts @@ -15,6 +15,7 @@ import { SdsPipeline, SdsPlaceholder, SdsResult, + SdsSchema, SdsSegment, SdsTypeParameter, } from '../generated/ast.js'; @@ -57,6 +58,8 @@ export class SafeDsNodeKindProvider implements NodeKindProvider { /* c8 ignore next 2 */ case SdsResult: return SymbolKind.Variable; + case SdsSchema: + return SymbolKind.Struct; case SdsSegment: return SymbolKind.Function; /* c8 ignore next 2 */ diff --git a/src/language/lsp/safe-ds-semantic-token-provider.ts b/src/language/lsp/safe-ds-semantic-token-provider.ts index ec729662a..a2be26037 100644 --- a/src/language/lsp/safe-ds-semantic-token-provider.ts +++ b/src/language/lsp/safe-ds-semantic-token-provider.ts @@ -24,6 +24,7 @@ import { isSdsPipeline, isSdsPlaceholder, isSdsReference, + isSdsSchema, isSdsSegment, isSdsTypeArgument, isSdsTypeParameter, @@ -206,6 +207,11 @@ export class SafeDsSemanticTokenProvider extends AbstractSemanticTokenProvider { type: SemanticTokenTypes.variable, modifier: [SemanticTokenModifiers.readonly, ...additionalModifiers], }; + } else if (isSdsSchema(node)) { + return { + type: SemanticTokenTypes.type, + modifier: additionalModifiers, + }; } else if (isSdsSegment(node)) { return { type: SemanticTokenTypes.function, diff --git a/src/language/validation/names.ts b/src/language/validation/names.ts index b2de3d9d2..1b5b98fb5 100644 --- a/src/language/validation/names.ts +++ b/src/language/validation/names.ts @@ -1,7 +1,9 @@ import { isSdsQualifiedImport, SdsAnnotation, + SdsAttribute, SdsBlockLambda, + SdsBlockLambdaResult, SdsCallableType, SdsClass, SdsDeclaration, @@ -11,11 +13,15 @@ import { SdsFunction, SdsImportedDeclaration, SdsModule, + SdsParameter, SdsPipeline, + SdsPlaceholder, + SdsResult, SdsSchema, SdsSegment, + SdsTypeParameter, } from '../generated/ast.js'; -import { getDocument, ValidationAcceptor } from 'langium'; +import { AstNodeDescription, getDocument, ValidationAcceptor } from 'langium'; import { getColumns, getEnumVariants, @@ -37,6 +43,7 @@ import { declarationIsAllowedInPipelineFile, declarationIsAllowedInStubFile } fr import { SafeDsServices } from '../safe-ds-module.js'; import { listBuiltinFiles } from '../builtins/fileFinder.js'; import { CODEGEN_PREFIX } from '../../cli/generator.js'; +import { BUILTINS_ROOT_PACKAGE } from '../builtins/packageNames.js'; export const CODE_NAME_CODEGEN_PREFIX = 'name/codegen-prefix'; export const CODE_NAME_CASING = 'name/casing'; @@ -67,21 +74,21 @@ export const nameMustNotStartWithCodegenPrefix = (node: SdsDeclaration, accept: export const nameShouldHaveCorrectCasing = (node: SdsDeclaration, accept: ValidationAcceptor): void => { switch (node.$type) { - case 'SdsAnnotation': + case SdsAnnotation: return nameShouldBeUpperCamelCase(node, 'annotations', accept); - case 'SdsAttribute': + case SdsAttribute: return nameShouldBeLowerCamelCase(node, 'attributes', accept); - case 'SdsBlockLambdaResult': + case SdsBlockLambdaResult: return nameShouldBeLowerCamelCase(node, 'block lambda results', accept); - case 'SdsClass': + case SdsClass: return nameShouldBeUpperCamelCase(node, 'classes', accept); - case 'SdsEnum': + case SdsEnum: return nameShouldBeUpperCamelCase(node, 'enums', accept); - case 'SdsEnumVariant': + case SdsEnumVariant: return nameShouldBeUpperCamelCase(node, 'enum variants', accept); - case 'SdsFunction': + case SdsFunction: return nameShouldBeLowerCamelCase(node, 'functions', accept); - case 'SdsModule': + case SdsModule: const name = node.name ?? ''; const segments = name.split('.'); if (name !== '' && segments.every((it) => it !== '') && !segments.every(isLowerCamelCase)) { @@ -92,19 +99,19 @@ export const nameShouldHaveCorrectCasing = (node: SdsDeclaration, accept: Valida }); } return; - case 'SdsParameter': + case SdsParameter: return nameShouldBeLowerCamelCase(node, 'parameters', accept); - case 'SdsPipeline': + case SdsPipeline: return nameShouldBeLowerCamelCase(node, 'pipelines', accept); - case 'SdsPlaceholder': + case SdsPlaceholder: return nameShouldBeLowerCamelCase(node, 'placeholders', accept); - case 'SdsResult': + case SdsResult: return nameShouldBeLowerCamelCase(node, 'results', accept); - case 'SdsSchema': + case SdsSchema: return nameShouldBeUpperCamelCase(node, 'schemas', accept); - case 'SdsSegment': + case SdsSegment: return nameShouldBeLowerCamelCase(node, 'segments', accept); - case 'SdsTypeParameter': + case SdsTypeParameter: return nameShouldBeUpperCamelCase(node, 'type parameters', accept); } /* c8 ignore next */ @@ -224,7 +231,17 @@ export const moduleMemberMustHaveNameThatIsUniqueInPackage = (services: SafeDsSe for (const member of getModuleMembers(node)) { const packageName = getPackageName(member) ?? ''; - const declarationsInPackage = packageManager.getDeclarationsInPackage(packageName); + + let declarationsInPackage: AstNodeDescription[]; + let kind: string; + if (packageName.startsWith(BUILTINS_ROOT_PACKAGE)) { + // For a builtin package the simple names of declarations must be unique + declarationsInPackage = packageManager.getDeclarationsInPackageOrSubpackage(BUILTINS_ROOT_PACKAGE); + kind = 'builtin declarations'; + } else { + declarationsInPackage = packageManager.getDeclarationsInPackage(packageName); + kind = 'declarations in this package'; + } if ( declarationsInPackage.some( @@ -234,7 +251,7 @@ export const moduleMemberMustHaveNameThatIsUniqueInPackage = (services: SafeDsSe !builtinUris.has(it.documentUri.toString()), ) ) { - accept('error', `Multiple declarations in this package have the name '${member.name}'.`, { + accept('error', `Multiple ${kind} have the name '${member.name}'.`, { node: member, property: 'name', code: CODE_NAME_DUPLICATE, diff --git a/src/language/validation/other/modules.ts b/src/language/validation/other/modules.ts index 6de62f1e3..343f6ce68 100644 --- a/src/language/validation/other/modules.ts +++ b/src/language/validation/other/modules.ts @@ -2,6 +2,7 @@ import { ValidationAcceptor } from 'langium'; import { isSdsDeclaration, isSdsPipeline, isSdsSegment, SdsDeclaration, SdsModule } from '../../generated/ast.js'; import { isInPipelineFile, isInStubFile } from '../../helpers/fileExtensions.js'; import { getModuleMembers } from '../../helpers/nodeProperties.js'; +import { BUILTINS_ROOT_PACKAGE } from '../../builtins/packageNames.js'; export const CODE_MODULE_FORBIDDEN_IN_PIPELINE_FILE = 'module/forbidden-in-pipeline-file'; export const CODE_MODULE_FORBIDDEN_IN_STUB_FILE = 'module/forbidden-in-stub-file'; @@ -56,8 +57,8 @@ export const moduleWithDeclarationsMustStatePackage = (node: SdsModule, accept: }; export const pipelineFileMustNotBeInSafedsPackage = (node: SdsModule, accept: ValidationAcceptor): void => { - if (isInPipelineFile(node) && node.name?.startsWith('safeds')) { - accept('error', "A pipeline file must not be in a 'safeds' package.", { + if (isInPipelineFile(node) && node.name?.startsWith(BUILTINS_ROOT_PACKAGE)) { + accept('error', `A pipeline file must not be in a '${BUILTINS_ROOT_PACKAGE}' package.`, { node, property: 'name', code: CODE_MODULE_PIPELINE_FILE_IN_SAFEDS_PACKAGE, diff --git a/tests/language/lsp/safe-ds-document-symbol-provider.test.ts b/tests/language/lsp/safe-ds-document-symbol-provider.test.ts index af461dcf1..4f1d49bd0 100644 --- a/tests/language/lsp/safe-ds-document-symbol-provider.test.ts +++ b/tests/language/lsp/safe-ds-document-symbol-provider.test.ts @@ -155,6 +155,20 @@ describe('SafeDsSemanticTokenProvider', async () => { }, ], }, + { + testName: 'schema declaration', + code: ` + schema S { + "a": Int + } + `, + expectedSymbols: [ + { + name: 'S', + kind: SymbolKind.Struct, + }, + ], + }, { testName: 'segment declaration', code: ` diff --git a/tests/language/lsp/safe-ds-semantic-token-provider.test.ts b/tests/language/lsp/safe-ds-semantic-token-provider.test.ts index b70b20159..01639db9a 100644 --- a/tests/language/lsp/safe-ds-semantic-token-provider.test.ts +++ b/tests/language/lsp/safe-ds-semantic-token-provider.test.ts @@ -100,6 +100,11 @@ describe('SafeDsSemanticTokenProvider', async () => { `, expectedTokenTypes: [SemanticTokenTypes.variable], }, + { + testName: 'schema declaration', + code: 'schema <|S|>() {}', + expectedTokenTypes: [SemanticTokenTypes.type], + }, { testName: 'segment declaration', code: 'segment <|s|>() {}', diff --git a/tests/resources/validation/names/duplicates/across files/other package.sdstest b/tests/resources/validation/names/duplicates/across files/other package.sdstest index c859e3989..84389abf4 100644 --- a/tests/resources/validation/names/duplicates/across files/other package.sdstest +++ b/tests/resources/validation/names/duplicates/across files/other package.sdstest @@ -1,11 +1,20 @@ 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() {} +// $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«() {} diff --git a/tests/resources/validation/names/duplicates/across files/safeds 1.sdstest b/tests/resources/validation/names/duplicates/across files/safeds 1.sdstest new file mode 100644 index 000000000..605fa9d5e --- /dev/null +++ b/tests/resources/validation/names/duplicates/across files/safeds 1.sdstest @@ -0,0 +1,27 @@ +package safeds.lang + +/* + * Declarations that only occur a second time in builtin files should be excluded, so we don't get errors while editing them. + */ + +// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\." +class »Any« + +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateAnnotation'." +annotation »DuplicateAnnotation« +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateClass'." +class »DuplicateClass« +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateEnum'." +enum »DuplicateEnum« +// $TEST$ error "Multiple builtin declarations have the name 'duplicateFunction'." +fun »duplicateFunction«() +// $TEST$ no error r"Multiple builtin declarations have the name '\w*'\." +pipeline »duplicatePipeline« {} +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateSchema'." +schema »DuplicateSchema« {} +// $TEST$ error "Multiple builtin declarations have the name 'duplicatePublicSegment'." +segment »duplicatePublicSegment«() {} +// $TEST$ error "Multiple builtin declarations have the name 'duplicateInternalSegment'." +internal segment »duplicateInternalSegment«() {} +// $TEST$ no error r"Multiple builtin declarations have the name '\w*'\." +private segment »duplicatePrivateSegment«() {} diff --git a/tests/resources/validation/names/duplicates/across files/safeds 2.sdstest b/tests/resources/validation/names/duplicates/across files/safeds 2.sdstest new file mode 100644 index 000000000..c0f4a34d0 --- /dev/null +++ b/tests/resources/validation/names/duplicates/across files/safeds 2.sdstest @@ -0,0 +1,20 @@ +package safeds.lang.other + +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateAnnotation'." +annotation »DuplicateAnnotation« +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateClass'." +class »DuplicateClass« +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateEnum'." +enum »DuplicateEnum« +// $TEST$ error "Multiple builtin declarations have the name 'duplicateFunction'." +fun »duplicateFunction«() +// $TEST$ no error r"Multiple builtin declarations have the name '\w*'\." +pipeline »duplicatePipeline« {} +// $TEST$ error "Multiple builtin declarations have the name 'DuplicateSchema'." +schema »DuplicateSchema« {} +// $TEST$ error "Multiple builtin declarations have the name 'duplicatePublicSegment'." +segment »duplicatePublicSegment«() {} +// $TEST$ error "Multiple builtin declarations have the name 'duplicateInternalSegment'." +internal segment »duplicateInternalSegment«() {} +// $TEST$ no error r"Multiple builtin declarations have the name '\w*'\." +private segment »duplicatePrivateSegment«() {} diff --git a/tests/resources/validation/names/duplicates/across files/safeds.sdstest b/tests/resources/validation/names/duplicates/across files/safeds.sdstest deleted file mode 100644 index e7b6779cb..000000000 --- a/tests/resources/validation/names/duplicates/across files/safeds.sdstest +++ /dev/null @@ -1,8 +0,0 @@ -package safeds.lang - -/* - * Declarations from builtin files should be excluded, so we don't get errors while editing them. - */ - -// $TEST$ no error r"Multiple declarations in this package have the name '\w*'\." - class »Any« diff --git a/tests/resources/validation/names/duplicates/across files/same package.sdstest b/tests/resources/validation/names/duplicates/across files/same package.sdstest index 04c8a4401..ea17382ab 100644 --- a/tests/resources/validation/names/duplicates/across files/same package.sdstest +++ b/tests/resources/validation/names/duplicates/across files/same package.sdstest @@ -1,11 +1,20 @@ 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() {} +// $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«() {}