Skip to content

Commit

Permalink
fix: prevent overwriting core declarations (#761)
Browse files Browse the repository at this point in the history
### Summary of Changes

Update the scope provider, so core declarations can never be overwritten
by own declarations.
  • Loading branch information
lars-reimann authored Nov 11, 2023
1 parent 23b340e commit 36663ca
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 17 deletions.
14 changes: 14 additions & 0 deletions packages/safe-ds-lang/src/language/helpers/nodeProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,20 @@ export const getParentTypes = (node: SdsClass | undefined): SdsType[] => {
return node?.parentTypeList?.parentTypes ?? [];
};

export const getQualifiedName = (node: SdsDeclaration | undefined): string | undefined => {
const segments = [];

let current: SdsDeclaration | undefined = node;
while (current) {
if (current.name) {
segments.unshift(current.name);
}
current = getContainerOfType(current.$container, isSdsDeclaration);
}

return segments.join('.');
};

export const streamPlaceholders = (node: SdsBlock | undefined): Stream<SdsPlaceholder> => {
return stream(getStatements(node)).filter(isSdsAssignment).flatMap(getAssignees).filter(isSdsPlaceholder);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
getDocument,
ReferenceInfo,
Scope,
WorkspaceCache,
} from 'langium';
import {
isSdsAbstractCall,
Expand Down Expand Up @@ -36,6 +37,7 @@ import {
isSdsTypeArgument,
isSdsWildcardImport,
isSdsYield,
SdsAnnotation,
SdsArgument,
type SdsCallable,
SdsDeclaration,
Expand Down Expand Up @@ -82,6 +84,8 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
private readonly packageManager: SafeDsPackageManager;
private readonly typeComputer: SafeDsTypeComputer;

private readonly coreDeclarationCache: WorkspaceCache<string, AstNodeDescription[]>;

constructor(services: SafeDsServices) {
super(services);

Expand All @@ -90,26 +94,30 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
this.nodeMapper = services.helpers.NodeMapper;
this.packageManager = services.workspace.PackageManager;
this.typeComputer = services.types.TypeComputer;

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

override getScope(context: ReferenceInfo): Scope {
const node = context.container;

if (isSdsArgument(node) && context.property === 'parameter') {
if (isSdsAnnotationCall(node) && context.property === 'annotation') {
return this.getScopeForAnnotationCallAnnotation(context);
} else if (isSdsArgument(node) && context.property === 'parameter') {
return this.getScopeForArgumentParameter(node);
} else if (isSdsImportedDeclaration(node) && context.property === 'declaration') {
return this.getScopeForImportedDeclarationDeclaration(node);
} else if (isSdsNamedType(node) && context.property === 'declaration') {
if (isSdsMemberType(node.$container) && node.$containerProperty === 'member') {
return this.getScopeForMemberTypeMember(node.$container);
} else {
return super.getScope(context);
return this.getScopeForNamedTypeDeclaration(context);
}
} else if (isSdsReference(node) && context.property === 'target') {
if (isSdsMemberAccess(node.$container) && node.$containerProperty === 'member') {
return this.getScopeForMemberAccessMember(node.$container);
} else {
return this.getScopeForDirectReferenceTarget(node, context);
return this.getScopeForReferenceTarget(node, context);
}
} else if (isSdsTypeArgument(node) && context.property === 'typeParameter') {
return this.getScopeForTypeArgumentTypeParameter(node);
Expand All @@ -120,6 +128,10 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
}
}

private getScopeForAnnotationCallAnnotation(context: ReferenceInfo) {
return this.coreDeclarations(SdsAnnotation, super.getScope(context));
}

private getScopeForArgumentParameter(node: SdsArgument): Scope {
const containingAbstractCall = getContainerOfType(node, isSdsAbstractCall);
const callable = this.nodeMapper.callToCallable(containingAbstractCall);
Expand All @@ -141,7 +153,7 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
}

const declarationsInPackage = this.packageManager.getDeclarationsInPackage(containingQualifiedImport.package, {
nodeType: 'SdsDeclaration',
nodeType: SdsDeclaration,
hideInternal: containingQualifiedImport.package !== ownPackageName,
});
return this.createScope(declarationsInPackage);
Expand Down Expand Up @@ -181,6 +193,10 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
}
}

private getScopeForNamedTypeDeclaration(context: ReferenceInfo): Scope {
return this.coreDeclarations(SdsNamedTypeDeclaration, super.getScope(context));
}

private getScopeForMemberAccessMember(node: SdsMemberAccess): Scope {
// Static access
const declaration = this.getUniqueReferencedDeclarationForExpression(node.receiver);
Expand Down Expand Up @@ -249,9 +265,9 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
}
}

private getScopeForDirectReferenceTarget(node: SdsReference, context: ReferenceInfo): Scope {
private getScopeForReferenceTarget(node: SdsReference, context: ReferenceInfo): Scope {
// Declarations in other files
let currentScope = this.getGlobalScope('SdsDeclaration', context);
let currentScope = this.getGlobalScope(SdsDeclaration, context);

// Declarations in this file
currentScope = this.globalDeclarationsInSameFile(node, currentScope);
Expand All @@ -260,7 +276,10 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {
currentScope = this.containingDeclarations(node, currentScope);

// Declarations in containing blocks
return this.localDeclarations(node, currentScope);
currentScope = this.localDeclarations(node, currentScope);

// Core declarations
return this.coreDeclarations(SdsDeclaration, currentScope);
}

private containingDeclarations(node: AstNode, outerScope: Scope): Scope {
Expand Down Expand Up @@ -465,4 +484,14 @@ export class SafeDsScopeProvider extends DefaultScopeProvider {

return result;
}

private coreDeclarations(referenceType: string, outerScope: Scope): Scope {
const descriptions = this.coreDeclarationCache.get(referenceType, () =>
this.packageManager.getDeclarationsInPackage('safeds.lang', {
nodeType: referenceType,
hideInternal: true,
}),
);
return this.createScope(descriptions, outerScope);
}
}
16 changes: 13 additions & 3 deletions packages/safe-ds-lang/src/language/validation/inheritance.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expandToStringWithNL, ValidationAcceptor } from 'langium';
import { expandToStringWithNL, getContainerOfType, ValidationAcceptor } from 'langium';
import { isEmpty } from '../../helpers/collectionUtils.js';
import { SdsClass, type SdsClassMember } from '../generated/ast.js';
import { getParentTypes } from '../helpers/nodeProperties.js';
import { isSdsClass, SdsClass, type SdsClassMember } from '../generated/ast.js';
import { getParentTypes, getQualifiedName } from '../helpers/nodeProperties.js';
import { SafeDsServices } from '../safe-ds-module.js';
import { ClassType, UnknownType } from '../typing/model.js';

Expand Down Expand Up @@ -40,6 +40,11 @@ export const classMemberMustMatchOverriddenMemberAndShouldBeNeeded = (services:
},
);
} else if (typeChecker.isAssignableTo(overriddenMemberType, ownMemberType)) {
// Prevents the info from showing when editing the builtin files
if (isInSafedsLangAnyClass(node)) {
return;
}

accept('info', 'Overriding member is identical to overridden member and can be removed.', {
node,
property: 'name',
Expand All @@ -49,6 +54,11 @@ export const classMemberMustMatchOverriddenMemberAndShouldBeNeeded = (services:
};
};

const isInSafedsLangAnyClass = (node: SdsClassMember): boolean => {
const containingClass = getContainerOfType(node, isSdsClass);
return isSdsClass(containingClass) && getQualifiedName(containingClass) === 'safeds.lang.Any';
};

export const classMustOnlyInheritASingleClass = (services: SafeDsServices) => {
const typeComputer = services.types.TypeComputer;
const computeType = typeComputer.computeType.bind(typeComputer);
Expand Down
72 changes: 65 additions & 7 deletions packages/safe-ds-lang/tests/language/scoping/scoping.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AssertionError } from 'assert';
import { DocumentValidator, LangiumDocument, Reference, URI } from 'langium';
import { AstNode, DocumentValidator, getDocument, LangiumDocument, Reference, URI } from 'langium';
import { NodeFileSystem } from 'langium/node';
import { clearDocuments, isRangeEqual, validationHelper } from 'langium/test';
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
Expand All @@ -8,8 +8,13 @@ import { createSafeDsServices } from '../../../src/language/index.js';
import { isLocationEqual, locationToString } from '../../helpers/location.js';
import { loadDocuments } from '../../helpers/testResources.js';
import { createScopingTests, ExpectedReference } from './creator.js';
import { getNodeOfType } from '../../helpers/nodeFinder.js';
import { isSdsAnnotationCall, isSdsNamedType, isSdsReference } from '../../../src/language/generated/ast.js';

const services = createSafeDsServices(NodeFileSystem).SafeDs;
const builtinAnnotations = services.builtins.Annotations;
const builtinEnums = services.builtins.Enums;
const builtinClasses = services.builtins.Classes;

describe('scoping', async () => {
beforeEach(async () => {
Expand Down Expand Up @@ -69,15 +74,45 @@ describe('scoping', async () => {
}
});

it('should not replace core declarations (annotation call)', async () => {
const code = `
annotation PythonName(name: String)
@PythonName(name: String)
segment mySegment() {}
`;
const annotationCall = await getNodeOfType(services, code, isSdsAnnotationCall);
expectSameDocument(annotationCall.annotation?.ref, builtinAnnotations.PythonName);
});

it('should not replace core declarations (named type)', async () => {
const code = `
class Any
segment mySegment(p: Any) {}
`;
const namedType = await getNodeOfType(services, code, isSdsNamedType);
expectSameDocument(namedType.declaration?.ref, builtinClasses.Any);
});

it('should not replace core declarations (reference)', async () => {
const code = `
enum AnnotationTarget
@Target([AnnotationTarget.Annotation])
annotation MyAnnotation
`;
const reference = await getNodeOfType(services, code, isSdsReference);
expectSameDocument(reference.target?.ref, builtinEnums.AnnotationTarget);
});

it('should resolve members on literals', async () => {
const code = `
pipeline myPipeline {
1.toString();
}
`;
const { diagnostics } = await validationHelper(services)(code);
const linkingError = diagnostics.filter((d) => d.data?.code === DocumentValidator.LinkingError);
expect(linkingError).toStrictEqual([]);
await expectNoLinkingErrors(code);
});

it('should resolve members on literal types', async () => {
Expand All @@ -86,9 +121,7 @@ describe('scoping', async () => {
p.toString();
}
`;
const { diagnostics } = await validationHelper(services)(code);
const linkingError = diagnostics.filter((d) => d.data?.code === DocumentValidator.LinkingError);
expect(linkingError).toStrictEqual([]);
await expectNoLinkingErrors(code);
});
});

Expand Down Expand Up @@ -146,3 +179,28 @@ const findActualReference = (document: LangiumDocument, expectedReference: Expec
}
return actualReference;
};

/**
* Both nodes should be defined and in the same document or an `AssertionError` is thrown.
*/
const expectSameDocument = (node1: AstNode | undefined, node2: AstNode | undefined): void => {
if (!node1) {
throw new AssertionError({ message: `node1 is undefined.` });
} else if (!node2) {
throw new AssertionError({ message: `node2 is undefined.` });
}

const document1 = getDocument(node1);
const document2 = getDocument(node2);

expect(document1.uri.toString()).toStrictEqual(document2.uri.toString());
};

/**
* The given code should have no linking errors or an `AssertionError` is thrown.
*/
const expectNoLinkingErrors = async (code: string) => {
const { diagnostics } = await validationHelper(services)(code);
const linkingError = diagnostics.filter((d) => d.data?.code === DocumentValidator.LinkingError);
expect(linkingError).toStrictEqual([]);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package safeds.lang

class Any {
// $TEST$ no info "Overriding member is identical to overridden member and can be removed."
fun »toString«() -> s: String
}

0 comments on commit 36663ca

Please sign in to comment.