Skip to content

Commit

Permalink
feat: error if simple names of builtin declarations collide (#678)
Browse files Browse the repository at this point in the history
Closes #672

### Summary of Changes

<!-- Please provide a summary of changes in this pull request, ensuring
all changes are explained. -->
  • Loading branch information
lars-reimann authored Oct 22, 2023
1 parent e846b59 commit 275ad5e
Show file tree
Hide file tree
Showing 12 changed files with 150 additions and 46 deletions.
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«() {}

0 comments on commit 275ad5e

Please sign in to comment.