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.

Sign up for GitHub

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

Jump to bottom

Support discarding the CST to preserve memory #1733

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Support discarding the CST to preserve memory #1733

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ec3f63
Support discarding the CST to preserve memory
msujew Oct 29, 2024
5104b23
Support comments in closed files
msujew Nov 6, 2024
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
36 changes: 20 additions & 16 deletions examples/requirements/test/validator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ import { NodeFileSystem } from 'langium/node';
describe('A requirement identifier and a test identifier shall contain a number.', () => {
test('T001_good_case', async () => {
const services = createRequirementsAndTestsLangServices(NodeFileSystem);
const [mainDoc,allDocs] = await extractDocuments(
const [mainDoc, allDocs] = await extractDocuments(
path.join(__dirname, 'files', 'good', 'requirements.req'),
services.requirements
);
expect((mainDoc.diagnostics ?? [])).toEqual([]);
expect(allDocs.length).toEqual(3);
allDocs.forEach(doc=>{
allDocs.forEach(doc => {
expect((doc.diagnostics ?? [])).toEqual([]);
});
});
Expand All @@ -35,7 +35,7 @@ describe('A requirement identifier shall contain a number.', () => {
expect(mainDoc.diagnostics ?? []).toEqual(expect.arrayContaining([
expect.objectContaining({
message: expect.stringMatching('Requirement name ReqIdABC_reqID should container a number'),
range: expect.objectContaining({start:expect.objectContaining({line: 2})}) // zero based
range: expect.objectContaining({ start: expect.objectContaining({ line: 2 }) }) // zero based
})
]));

Expand All @@ -45,17 +45,17 @@ describe('A requirement identifier shall contain a number.', () => {
describe('A test identifier shall contain a number.', () => {
test('T003_badTstId: bad case', async () => {
const services = createRequirementsAndTestsLangServices(NodeFileSystem);
const [,allDocs] = await extractDocuments(
const [, allDocs] = await extractDocuments(
path.join(__dirname, 'files', 'bad1', 'requirements.req'),
services.requirements
);
const doc = allDocs.find(doc=>/tests_part1.tst/.test(doc.uri.fsPath));
const doc = allDocs.find(doc => /tests_part1.tst/.test(doc.uri.fsPath));
expect(doc).toBeDefined();
if (!doc) throw new Error('impossible');
expect(doc.diagnostics ?? []).toEqual(expect.arrayContaining([
expect.objectContaining({
message: expect.stringMatching('Test name TA should container a number.'),
range: expect.objectContaining({start:expect.objectContaining({line: 1})}) // zero based
range: expect.objectContaining({ start: expect.objectContaining({ line: 1 }) }) // zero based
})
]));
});
Expand All @@ -71,7 +71,7 @@ describe('A requirement shall be covered by at least one test.', () => {
expect(mainDoc.diagnostics ?? []).toEqual(expect.arrayContaining([
expect.objectContaining({
message: expect.stringMatching('Requirement ReqId004_unicorn not covered by a test.'),
range: expect.objectContaining({start:expect.objectContaining({line: 4})}) // zero based
range: expect.objectContaining({ start: expect.objectContaining({ line: 4 }) }) // zero based
})
]));
});
Expand All @@ -80,28 +80,32 @@ describe('A requirement shall be covered by at least one test.', () => {
describe('A referenced environment in a test must be found in one of the referenced requirements.', () => {
test('referenced environment test', async () => {
const services = createRequirementsAndTestsLangServices(NodeFileSystem);
const [,allDocs] = await extractDocuments(
const [, allDocs] = await extractDocuments(
path.join(__dirname, 'files', 'bad2', 'requirements.req'),
services.requirements
);
const doc = allDocs.find(doc=>/tests_part1.tst/.test(doc.uri.fsPath));
const doc = allDocs.find(doc => /tests_part1.tst/.test(doc.uri.fsPath));
expect(doc).toBeDefined();
if (!doc) throw new Error('impossible');
expect((doc.diagnostics ?? [])).toEqual(expect.arrayContaining([
expect.objectContaining({
message: expect.stringMatching('Test T002_badReqId references environment Linux_x86 which is used in any referenced requirement.'),
range: expect.objectContaining({start:expect.objectContaining({
line: 3,
character: 65
})}) // zero based
range: expect.objectContaining({
start: expect.objectContaining({
line: 3,
character: 65
})
}) // zero based
})
]));
expect((doc.diagnostics ?? [])).toEqual(expect.arrayContaining([
expect.objectContaining({
message: expect.stringMatching('Test T004_cov references environment Linux_x86 which is used in any referenced requirement.'),
range: expect.objectContaining({start:expect.objectContaining({
line: 5
})}) // zero based
range: expect.objectContaining({
start: expect.objectContaining({
line: 5
})
}) // zero based
})
]));

Expand Down
4 changes: 3 additions & 1 deletion packages/langium/src/default-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { LangiumParserErrorMessageProvider } from './parser/langium-parser.js';
import { DefaultAsyncParser } from './parser/async-parser.js';
import { DefaultWorkspaceLock } from './workspace/workspace-lock.js';
import { DefaultHydrator } from './serializer/hydrator.js';
import { DefaultEnvironment } from './workspace/environment.js';

/**
* Context required for creating the default language-specific dependency injection module.
Expand Down Expand Up @@ -116,7 +117,8 @@ export function createDefaultSharedCoreModule(context: DefaultSharedCoreModuleCo
WorkspaceManager: (services) => new DefaultWorkspaceManager(services),
FileSystemProvider: (services) => context.fileSystemProvider(services),
WorkspaceLock: () => new DefaultWorkspaceLock(),
ConfigurationProvider: (services) => new DefaultConfigurationProvider(services)
ConfigurationProvider: (services) => new DefaultConfigurationProvider(services),
Environment: () => new DefaultEnvironment()
}
};
}
4 changes: 2 additions & 2 deletions packages/langium/src/lsp/call-hierarchy-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ export abstract class AbstractCallHierarchyProvider implements CallHierarchyProv
return undefined;
}

const declarationNode = this.references.findDeclarationNode(targetNode);
const declarationNode = this.references.findDeclaration(targetNode);
danieldietrich marked this conversation as resolved.
Show resolved Hide resolved
if (!declarationNode) {
return undefined;
}

return this.getCallHierarchyItems(declarationNode.astNode, document);
return this.getCallHierarchyItems(declarationNode, document);
}

protected getCallHierarchyItems(targetNode: AstNode, document: LangiumDocument<AstNode>): CallHierarchyItem[] | undefined {
Expand Down
30 changes: 18 additions & 12 deletions packages/langium/src/lsp/definition-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type { GrammarConfig } from '../languages/grammar-config.js';
import type { NameProvider } from '../references/name-provider.js';
import type { References } from '../references/references.js';
import type { LangiumServices } from './lsp-services.js';
import type { CstNode } from '../syntax-tree.js';
import type { AstNode, CstNode } from '../syntax-tree.js';
import type { MaybePromise } from '../utils/promise-utils.js';
import type { LangiumDocument } from '../workspace/documents.js';
import { LocationLink } from 'vscode-languageserver';
Expand All @@ -37,7 +37,7 @@ export interface DefinitionProvider {

export interface GoToLink {
source: CstNode
target: CstNode
target: AstNode
targetDocument: LangiumDocument
}

Expand Down Expand Up @@ -67,21 +67,27 @@ export class DefaultDefinitionProvider implements DefinitionProvider {

protected collectLocationLinks(sourceCstNode: CstNode, _params: DefinitionParams): MaybePromise<LocationLink[] | undefined> {
const goToLink = this.findLink(sourceCstNode);
if (goToLink) {
return [LocationLink.create(
goToLink.targetDocument.textDocument.uri,
(goToLink.target.astNode.$cstNode ?? goToLink.target).range,
goToLink.target.range,
goToLink.source.range
)];
if (goToLink && goToLink.target.$segments) {
const name = this.nameProvider.getNameProperty(goToLink.target);
if (name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we use the whole composite node as target in case a name is not available?

const nameSegment = goToLink.target.$segments.properties[name]?.[0];
if (nameSegment) {
return [LocationLink.create(
goToLink.targetDocument.textDocument.uri,
goToLink.target.$segments.full.range,
nameSegment.range,
goToLink.source.range
)];
}
}
}
return undefined;
}

protected findLink(source: CstNode): GoToLink | undefined {
const target = this.references.findDeclarationNode(source);
if (target?.astNode) {
const targetDocument = getDocument(target.astNode);
const target = this.references.findDeclaration(source);
if (target) {
const targetDocument = getDocument(target);
if (target && targetDocument) {
return { source, target, targetDocument };
}
Expand Down
26 changes: 22 additions & 4 deletions packages/langium/src/lsp/document-update-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,18 @@
* terms of the MIT License, which is available in the project root.
******************************************************************************/

import type { TextDocumentWillSaveEvent, DidChangeWatchedFilesParams, DidChangeWatchedFilesRegistrationOptions, TextDocumentChangeEvent, TextEdit } from 'vscode-languageserver';
import type { TextDocumentWillSaveEvent, DidChangeWatchedFilesParams, DidChangeWatchedFilesRegistrationOptions, TextDocumentChangeEvent, TextEdit, Connection } from 'vscode-languageserver';
import { DidChangeWatchedFilesNotification, FileChangeType } from 'vscode-languageserver';
import { stream } from '../utils/stream.js';
import { URI } from '../utils/uri-utils.js';
import type { DocumentBuilder } from '../workspace/document-builder.js';
import type { TextDocument } from '../workspace/documents.js';
import type { LangiumDocuments, TextDocument } from '../workspace/documents.js';
import type { WorkspaceLock } from '../workspace/workspace-lock.js';
import type { LangiumSharedServices } from './lsp-services.js';
import type { WorkspaceManager } from '../workspace/workspace-manager.js';
import type { ServiceRegistry } from '../service-registry.js';
import type { MaybePromise } from '../utils/promise-utils.js';
import { discardCst } from '../utils/cst-utils.js';

/**
* Shared service for handling text document changes and watching relevant files.
Expand Down Expand Up @@ -71,13 +72,17 @@ export class DefaultDocumentUpdateHandler implements DocumentUpdateHandler {
protected readonly workspaceManager: WorkspaceManager;
protected readonly documentBuilder: DocumentBuilder;
protected readonly workspaceLock: WorkspaceLock;
protected readonly documents: LangiumDocuments;
protected readonly connection: Connection | undefined;
protected readonly serviceRegistry: ServiceRegistry;

constructor(services: LangiumSharedServices) {
this.workspaceManager = services.workspace.WorkspaceManager;
this.documentBuilder = services.workspace.DocumentBuilder;
this.workspaceLock = services.workspace.WorkspaceLock;
this.serviceRegistry = services.ServiceRegistry;
this.documents = services.workspace.LangiumDocuments;
this.connection = services.lsp.Connection;

let canRegisterFileWatcher = false;
services.lsp.LanguageServer.onInitialize(params => {
Expand All @@ -98,15 +103,14 @@ export class DefaultDocumentUpdateHandler implements DocumentUpdateHandler {
.distinct()
.toArray();
if (fileExtensions.length > 0) {
const connection = services.lsp.Connection;
const options: DidChangeWatchedFilesRegistrationOptions = {
watchers: [{
globPattern: fileExtensions.length === 1
? `**/*.${fileExtensions[0]}`
: `**/*.{${fileExtensions.join(',')}}`
}]
};
connection?.client.register(DidChangeWatchedFilesNotification.type, options);
this.connection?.client.register(DidChangeWatchedFilesNotification.type, options);
}
}

Expand Down Expand Up @@ -141,4 +145,18 @@ export class DefaultDocumentUpdateHandler implements DocumentUpdateHandler {
.toArray();
this.fireDocumentUpdate(changedUris, deletedUris);
}

didCloseDocument(event: TextDocumentChangeEvent<TextDocument>): void {
const document = this.documents.getDocument(URI.parse(event.document.uri));
if (document) {
// Preserve memory by discarding the CST of the document
// Whenever the user reopens the document, the CST will be rebuilt
discardCst(document.parseResult.value);
}
// Discard the diagnostics for the closed document
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we make this configurable (without method overriding)? It could be simply a public property on the service class to turn this behavior on or off?

I imagine there are users/language implementors who would like to keep all problem markers even for closed documents.

this.connection?.sendDiagnostics({
uri: event.document.uri,
diagnostics: []
});
}
}
1 change: 1 addition & 0 deletions packages/langium/src/lsp/language-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ export class DefaultLanguageServer implements LanguageServer {
}

protected fireInitializeOnDefaultServices(params: InitializeParams): void {
this.services.workspace.Environment.initialize(params);
this.services.workspace.ConfigurationProvider.initialize(params);
this.services.workspace.WorkspaceManager.initialize(params);
}
Expand Down
16 changes: 8 additions & 8 deletions packages/langium/src/parser/async-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import type { CancellationToken } from '../utils/cancellation.js';
import type { LangiumCoreServices } from '../services.js';
import type { AstNode } from '../syntax-tree.js';
import type { LangiumParser, ParseResult } from './langium-parser.js';
import type { LangiumParser, ParseResult, ParserOptions } from './langium-parser.js';
import type { Hydrator } from '../serializer/hydrator.js';
import type { Event } from '../utils/event.js';
import { Deferred, OperationCancelled } from '../utils/promise-utils.js';
Expand All @@ -30,7 +30,7 @@ export interface AsyncParser {
*
* @throws `OperationCancelled` if the parsing process is cancelled.
*/
parse<T extends AstNode>(text: string, cancelToken: CancellationToken): Promise<ParseResult<T>>;
parse<T extends AstNode>(text: string, options: ParserOptions | undefined, cancelToken: CancellationToken): Promise<ParseResult<T>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR contains multiple "minor" breaking changes, but in sum I'd say the changes are significant enough to move this to v4.0. Shall we start planning the new major version more concretely?

}

/**
Expand All @@ -47,8 +47,8 @@ export class DefaultAsyncParser implements AsyncParser {
this.syncParser = services.parser.LangiumParser;
}

parse<T extends AstNode>(text: string, _cancelToken: CancellationToken): Promise<ParseResult<T>> {
return Promise.resolve(this.syncParser.parse<T>(text));
parse<T extends AstNode>(text: string, options: ParserOptions | undefined, _cancelToken: CancellationToken): Promise<ParseResult<T>> {
return Promise.resolve(this.syncParser.parse<T>(text, options));
}
}

Expand Down Expand Up @@ -89,7 +89,7 @@ export abstract class AbstractThreadedAsyncParser implements AsyncParser {
}
}

async parse<T extends AstNode>(text: string, cancelToken: CancellationToken): Promise<ParseResult<T>> {
async parse<T extends AstNode>(text: string, options: ParserOptions | undefined, cancelToken: CancellationToken): Promise<ParseResult<T>> {
const worker = await this.acquireParserWorker(cancelToken);
const deferred = new Deferred<ParseResult<T>>();
let timeout: NodeJS.Timeout | undefined;
Expand All @@ -101,7 +101,7 @@ export abstract class AbstractThreadedAsyncParser implements AsyncParser {
this.terminateWorker(worker);
}, this.terminationDelay);
});
worker.parse(text).then(result => {
worker.parse(text, options).then(result => {
const hydrated = this.hydrator.hydrate<T>(result);
deferred.resolve(hydrated);
}).catch(err => {
Expand Down Expand Up @@ -194,13 +194,13 @@ export class ParserWorker {
this.onReadyEmitter.fire();
}

parse(text: string): Promise<ParseResult> {
parse(text: string, options: ParserOptions | undefined): Promise<ParseResult> {
if (this._parsing) {
throw new Error('Parser worker is busy');
}
this._parsing = true;
this.deferred = new Deferred();
this.sendMessage(text);
this.sendMessage([text, options]);
return this.deferred.promise;
}
}
4 changes: 2 additions & 2 deletions packages/langium/src/parser/cst-node-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,20 @@ export class CstNodeBuilder {
}
}

construct(item: { $type: string | symbol | undefined, $cstNode: CstNode }): void {
construct(item: { $type: string | symbol | undefined }): CstNode {
const current: CstNode = this.current;
// The specified item could be a datatype ($type is symbol) or a fragment ($type is undefined)
// Only if the $type is a string, we actually assign the element
if (typeof item.$type === 'string') {
this.current.astNode = <AstNode>item;
}
item.$cstNode = current;
const node = this.nodeStack.pop();
// Empty composite nodes are not valid
// Simply remove the node from the tree
if (node?.content.length === 0) {
this.removeNode(node);
}
return current;
}

addHiddenTokens(hiddenTokens: IToken[]): void {
Expand Down
Loading
Loading