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 simple names of builtin declarations collide #678

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
1 change: 1 addition & 0 deletions src/language/builtins/packageNames.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const BUILTINS_ROOT_PACKAGE = 'safeds';
3 changes: 3 additions & 0 deletions src/language/lsp/safe-ds-node-kind-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
SdsPipeline,
SdsPlaceholder,
SdsResult,
SdsSchema,
SdsSegment,
SdsTypeParameter,
} from '../generated/ast.js';
Expand Down Expand Up @@ -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 */
Expand Down
6 changes: 6 additions & 0 deletions src/language/lsp/safe-ds-semantic-token-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
isSdsPipeline,
isSdsPlaceholder,
isSdsReference,
isSdsSchema,
isSdsSegment,
isSdsTypeArgument,
isSdsTypeParameter,
Expand Down Expand Up @@ -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,
Expand Down
53 changes: 35 additions & 18 deletions src/language/validation/names.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import {
isSdsQualifiedImport,
SdsAnnotation,
SdsAttribute,
SdsBlockLambda,
SdsBlockLambdaResult,
SdsCallableType,
SdsClass,
SdsDeclaration,
Expand All @@ -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,
Expand All @@ -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';
Expand Down Expand Up @@ -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)) {
Expand All @@ -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 */
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand Down
5 changes: 3 additions & 2 deletions src/language/validation/other/modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down
14 changes: 14 additions & 0 deletions tests/language/lsp/safe-ds-document-symbol-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: `
Expand Down
5 changes: 5 additions & 0 deletions tests/language/lsp/safe-ds-semantic-token-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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|>() {}',
Expand Down
Original file line number Diff line number Diff line change
@@ -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«() {}
Original file line number Diff line number Diff line change
@@ -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«() {}
Original file line number Diff line number Diff line change
@@ -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«() {}

This file was deleted.

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