Skip to content

Commit

Permalink
feat: various checks related to inheritance (#633)
Browse files Browse the repository at this point in the history
Closes partially #543 

### Summary of Changes

Show an error if a class
* inherits from multiple types,
* inherits from something that is not a class, or
* inherits from itself.

---------

Co-authored-by: megalinter-bot <129584137+megalinter-bot@users.noreply.github.com>
  • Loading branch information
lars-reimann and megalinter-bot committed Oct 12, 2023
1 parent b72768c commit 7ec746a
Show file tree
Hide file tree
Showing 15 changed files with 322 additions and 14 deletions.
7 changes: 6 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@
"scopeName": "source.safe-ds",
"path": "./syntaxes/safe-ds.tmLanguage.json"
}
]
],
"configurationDefaults": {
"[safe-ds]": {
"editor.wordSeparators": "`~!@#$%^&*()-=+[]{}\\|;:'\",.<>/?»«"
}
}
},
"type": "module",
"main": "out/extension/main.cjs",
Expand Down
5 changes: 5 additions & 0 deletions src/language/helpers/nodeProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
SdsResult,
SdsResultList,
SdsStatement,
SdsType,
SdsTypeArgument,
SdsTypeArgumentList,
SdsTypeParameter,
Expand Down Expand Up @@ -165,6 +166,10 @@ export const parametersOrEmpty = (node: SdsCallable | undefined): SdsParameter[]
return node?.parameterList?.parameters ?? [];
};

export const parentTypesOrEmpty = (node: SdsClass | undefined): SdsType[] => {
return node?.parentTypeList?.parentTypes ?? [];
};

export const placeholdersOrEmpty = (node: SdsBlock | undefined): SdsPlaceholder[] => {
return stream(statementsOrEmpty(node))
.filter(isSdsAssignment)
Expand Down
3 changes: 3 additions & 0 deletions src/language/safe-ds-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { SafeDsClasses } from './builtins/safe-ds-classes.js';
import { SafeDsPackageManager } from './workspace/safe-ds-package-manager.js';
import { SafeDsNodeMapper } from './helpers/safe-ds-node-mapper.js';
import { SafeDsAnnotations } from './builtins/safe-ds-annotations.js';
import { SafeDsClassHierarchy } from './typing/safe-ds-class-hierarchy.js';

/**
* Declaration of custom services - add your own service classes here.
Expand All @@ -34,6 +35,7 @@ export type SafeDsAddedServices = {
NodeMapper: SafeDsNodeMapper;
};
types: {
ClassHierarchy: SafeDsClassHierarchy;
TypeComputer: SafeDsTypeComputer;
};
workspace: {
Expand Down Expand Up @@ -71,6 +73,7 @@ export const SafeDsModule: Module<SafeDsServices, PartialLangiumServices & SafeD
ScopeProvider: (services) => new SafeDsScopeProvider(services),
},
types: {
ClassHierarchy: (services) => new SafeDsClassHierarchy(services),
TypeComputer: (services) => new SafeDsTypeComputer(services),
},
workspace: {
Expand Down
60 changes: 60 additions & 0 deletions src/language/typing/safe-ds-class-hierarchy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { SafeDsServices } from '../safe-ds-module.js';
import { SafeDsClasses } from '../builtins/safe-ds-classes.js';
import { SdsClass } from '../generated/ast.js';
import { stream, Stream } from 'langium';
import { parentTypesOrEmpty } from '../helpers/nodeProperties.js';
import { SafeDsTypeComputer } from './safe-ds-type-computer.js';
import { ClassType } from './model.js';

export class SafeDsClassHierarchy {
private readonly builtinClasses: SafeDsClasses;
private readonly typeComputer: SafeDsTypeComputer;

constructor(services: SafeDsServices) {
this.builtinClasses = services.builtins.Classes;
this.typeComputer = services.types.TypeComputer;
}

/**
* Returns a stream of all superclasses of the given class. The class itself is not included in the stream unless
* there is a cycle in the inheritance hierarchy. Direct ancestors are returned first, followed by their ancestors
* and so on.
*/
streamSuperclasses(node: SdsClass | undefined): Stream<SdsClass> {
if (!node) {
return stream();
}

const capturedThis = this;
const generator = function* () {
const visited = new Set<SdsClass>();
let current = capturedThis.parentClassOrUndefined(node);
while (current && !visited.has(current)) {
yield current;
visited.add(current);
current = capturedThis.parentClassOrUndefined(current);
}

const anyClass = capturedThis.builtinClasses.Any;
if (anyClass && node !== anyClass && !visited.has(anyClass)) {
yield anyClass;
}
};

return stream(generator());
}

/**
* Returns the parent class of the given class, or undefined if there is no parent class. Only the first parent
* type is considered, i.e. multiple inheritance is not supported.
*/
private parentClassOrUndefined(node: SdsClass | undefined): SdsClass | undefined {
const [firstParentType] = parentTypesOrEmpty(node);
const computedType = this.typeComputer.computeType(firstParentType);
if (computedType instanceof ClassType) {
return computedType.sdsClass;
}

return undefined;
}
}
23 changes: 13 additions & 10 deletions src/language/typing/safe-ds-type-computer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,19 +85,22 @@ import {

export class SafeDsTypeComputer {
private readonly astNodeLocator: AstNodeLocator;
private readonly coreClasses: SafeDsClasses;
private readonly builtinClasses: SafeDsClasses;
private readonly nodeMapper: SafeDsNodeMapper;

private readonly typeCache: WorkspaceCache<string, Type>;

constructor(services: SafeDsServices) {
this.astNodeLocator = services.workspace.AstNodeLocator;
this.coreClasses = services.builtins.Classes;
this.builtinClasses = services.builtins.Classes;
this.nodeMapper = services.helpers.NodeMapper;

this.typeCache = new WorkspaceCache(services.shared);
}

/**
* Computes the type of the given node.
*/
computeType(node: AstNode | undefined): Type {
if (!node) {
return UnknownType;
Expand Down Expand Up @@ -507,35 +510,35 @@ export class SafeDsTypeComputer {
// -----------------------------------------------------------------------------------------------------------------

private AnyOrNull(): Type {
return this.createCoreType(this.coreClasses.Any, true);
return this.createCoreType(this.builtinClasses.Any, true);
}

private Boolean(): Type {
return this.createCoreType(this.coreClasses.Boolean);
return this.createCoreType(this.builtinClasses.Boolean);
}

private Float(): Type {
return this.createCoreType(this.coreClasses.Float);
return this.createCoreType(this.builtinClasses.Float);
}

private Int(): Type {
return this.createCoreType(this.coreClasses.Int);
return this.createCoreType(this.builtinClasses.Int);
}

private List(): Type {
return this.createCoreType(this.coreClasses.List);
return this.createCoreType(this.builtinClasses.List);
}

private Map(): Type {
return this.createCoreType(this.coreClasses.Map);
return this.createCoreType(this.builtinClasses.Map);
}

private NothingOrNull(): Type {
return this.createCoreType(this.coreClasses.Nothing, true);
return this.createCoreType(this.builtinClasses.Nothing, true);
}

private String(): Type {
return this.createCoreType(this.coreClasses.String);
return this.createCoreType(this.builtinClasses.String);
}

private createCoreType(coreClass: SdsClass | undefined, isNullable: boolean = false): Type {
Expand Down
55 changes: 55 additions & 0 deletions src/language/validation/inheritance.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { ValidationAcceptor } from 'langium';
import { SdsClass } from '../generated/ast.js';
import { parentTypesOrEmpty } from '../helpers/nodeProperties.js';
import { isEmpty } from 'radash';
import { SafeDsServices } from '../safe-ds-module.js';
import { ClassType, UnknownType } from '../typing/model.js';

export const CODE_INHERITANCE_CYCLE = 'inheritance/cycle';
export const CODE_INHERITANCE_MULTIPLE_INHERITANCE = 'inheritance/multiple-inheritance';
export const CODE_INHERITANCE_NOT_A_CLASS = 'inheritance/not-a-class';

export const classMustOnlyInheritASingleClass = (services: SafeDsServices) => {
const typeComputer = services.types.TypeComputer;
const computeType = typeComputer.computeType.bind(typeComputer);

return (node: SdsClass, accept: ValidationAcceptor): void => {
const parentTypes = parentTypesOrEmpty(node);
if (isEmpty(parentTypes)) {
return;
}

const [firstParentType, ...otherParentTypes] = parentTypes;

// First parent type must be a class
const computedType = computeType(firstParentType);
if (computedType !== UnknownType && !(computedType instanceof ClassType)) {
accept('error', 'A class must only inherit classes.', {
node: firstParentType,
code: CODE_INHERITANCE_NOT_A_CLASS,
});
}

// Must have only one parent type
for (const parentType of otherParentTypes) {
accept('error', 'Multiple inheritance is not supported. Only the first parent type will be considered.', {
node: parentType,
code: CODE_INHERITANCE_MULTIPLE_INHERITANCE,
});
}
};
};

export const classMustNotInheritItself = (services: SafeDsServices) => {
const classHierarchy = services.types.ClassHierarchy;

return (node: SdsClass, accept: ValidationAcceptor): void => {
const superClasses = classHierarchy.streamSuperclasses(node);
if (superClasses.includes(node)) {
accept('error', 'A class must not directly or indirectly be a subtype of itself.', {
node: parentTypesOrEmpty(node)[0],
code: CODE_INHERITANCE_CYCLE,
});
}
};
};
9 changes: 7 additions & 2 deletions src/language/validation/safe-ds-validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import {
import { placeholderShouldBeUsed, placeholdersMustNotBeAnAlias } from './other/declarations/placeholders.js';
import { segmentParameterShouldBeUsed, segmentResultMustBeAssignedExactlyOnce } from './other/declarations/segments.js';
import { lambdaParameterMustNotHaveConstModifier } from './other/expressions/lambdas.js';
import { indexedAccessesShouldBeUsedWithCaution } from './experimentalLanguageFeature.js';
import { indexedAccessesShouldBeUsedWithCaution } from './experimentalLanguageFeatures.js';
import { requiredParameterMustNotBeExpert } from './builtins/expert.js';
import {
callableTypeParametersMustNotBeAnnotated,
Expand All @@ -86,6 +86,7 @@ import {
namedTypeMustNotSetTypeParameterMultipleTimes,
namedTypeTypeArgumentListMustNotHavePositionalArgumentsAfterNamedArguments,
} from './other/types/namedTypes.js';
import { classMustNotInheritItself, classMustOnlyInheritASingleClass } from './inheritance.js';

/**
* Register custom validation checks.
Expand Down Expand Up @@ -124,7 +125,11 @@ export const registerValidationChecks = function (services: SafeDsServices) {
callableTypeParameterMustNotHaveConstModifier,
callableTypeResultsMustNotBeAnnotated,
],
SdsClass: [classMustContainUniqueNames],
SdsClass: [
classMustContainUniqueNames,
classMustOnlyInheritASingleClass(services),
classMustNotInheritItself(services),
],
SdsClassBody: [classBodyShouldNotBeEmpty],
SdsConstraintList: [constraintListShouldNotBeEmpty],
SdsDeclaration: [nameMustNotStartWithBlockLambdaPrefix, nameShouldHaveCorrectCasing],
Expand Down
2 changes: 1 addition & 1 deletion tests/language/helpers/stringUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('pluralize', () => {
singular: 'apple',
expected: 'apples',
},
])('should return the singular or plural form based on the count', ({ count, singular, plural, expected }) => {
])('should return the singular or plural form based on the count (%#)', ({ count, singular, plural, expected }) => {
expect(pluralize(count, singular, plural)).toBe(expected);
});
});
88 changes: 88 additions & 0 deletions tests/language/typing/safe-ds-class-hierarchy.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
import { createSafeDsServices } from '../../../src/language/safe-ds-module.js';
import { NodeFileSystem } from 'langium/node';
import { clearDocuments } from 'langium/test';
import { isSdsClass, SdsClass } from '../../../src/language/generated/ast.js';
import { getNodeOfType } from '../../helpers/nodeFinder.js';

const services = createSafeDsServices(NodeFileSystem).SafeDs;
const classHierarchy = services.types.ClassHierarchy;

describe('SafeDsClassHierarchy', async () => {
beforeEach(async () => {
// Load the builtin library
await services.shared.workspace.WorkspaceManager.initializeWorkspace([]);
});

afterEach(async () => {
await clearDocuments(services);
});

describe('streamSuperclasses', () => {
const superclassNames = (node: SdsClass | undefined) =>
classHierarchy
.streamSuperclasses(node)
.map((clazz) => clazz.name)
.toArray();

it('should return an empty stream if passed undefined', () => {
expect(superclassNames(undefined)).toStrictEqual([]);
});

const testCases = [
{
testName: 'should return "Any" if the class has no parent types',
code: `
class A
`,
expected: ['Any'],
},
{
testName: 'should return "Any" if the first parent type is not a class',
code: `
class A sub E
enum E
`,
expected: ['Any'],
},
{
testName: 'should return the superclasses of a class (no cycle, implicit any)',
code: `
class A sub B
class B
`,
expected: ['B', 'Any'],
},
{
testName: 'should return the superclasses of a class (no cycle, explicit any)',
code: `
class A sub Any
`,
expected: ['Any'],
},
{
testName: 'should return the superclasses of a class (cycle)',
code: `
class A sub B
class B sub C
class C sub A
`,
expected: ['B', 'C', 'A', 'Any'],
},
{
testName: 'should only consider the first parent type',
code: `
class A sub B, C
class B
class C
`,
expected: ['B', 'Any'],
},
];

it.each(testCases)('$testName', async ({ code, expected }) => {
const firstClass = await getNodeOfType(services, code, isSdsClass);
expect(superclassNames(firstClass)).toStrictEqual(expected);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package tests.validation.inheritance.mustBeAcyclic

// $TEST$ error "A class must not directly or indirectly be a subtype of itself."
class MyClass1 sub »MyClass3«
// $TEST$ error "A class must not directly or indirectly be a subtype of itself."
class MyClass2 sub »MyClass1«
// $TEST$ error "A class must not directly or indirectly be a subtype of itself."
class MyClass3 sub »MyClass2«

class MyClass4
// $TEST$ no error "A class must not directly or indirectly be a subtype of itself."
class MyClass5 sub »MyClass4«

// $TEST$ no error "A class must not directly or indirectly be a subtype of itself."
class MyClass6 sub »MyClass7«
// $TEST$ no error "A class must not directly or indirectly be a subtype of itself."
class MyClass7 sub Any, »MyClass6«

// $TEST$ no error "A class must not directly or indirectly be a subtype of itself."
class MyClass8 sub »Unresolved«
// $TEST$ no error "A class must not directly or indirectly be a subtype of itself."
class MyClass9 sub »MyClass8«
Loading

0 comments on commit 7ec746a

Please sign in to comment.