diff --git a/libs/language-server/src/lib/ast/wrappers/pipeline-wrapper.ts b/libs/language-server/src/lib/ast/wrappers/pipeline-wrapper.ts index f5d40c123..aa4e2810b 100644 --- a/libs/language-server/src/lib/ast/wrappers/pipeline-wrapper.ts +++ b/libs/language-server/src/lib/ast/wrappers/pipeline-wrapper.ts @@ -24,6 +24,12 @@ export class PipelineWrapper< constructor(pipesContainer: T) { this.astNode = pipesContainer; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (pipesContainer.pipes === undefined) { + this.allPipes = []; + return; + } + this.allPipes = pipesContainer.pipes.flatMap((pipe) => createWrappersFromPipeChain(pipe), ); @@ -32,6 +38,11 @@ export class PipelineWrapper< static canBeWrapped( pipesContainer: PipelineDefinition | CompositeBlocktypeDefinition, ): boolean { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (pipesContainer.pipes === undefined) { + return true; + } + for (const pipeDefinition of pipesContainer.pipes) { for ( let chainIndex = 0; diff --git a/libs/language-server/src/lib/validation/checks/block-definition.spec.ts b/libs/language-server/src/lib/validation/checks/block-definition.spec.ts deleted file mode 100644 index e4228a6f4..000000000 --- a/libs/language-server/src/lib/validation/checks/block-definition.spec.ts +++ /dev/null @@ -1,86 +0,0 @@ -// SPDX-FileCopyrightText: 2023 Friedrich-Alexander-Universitat Erlangen-Nurnberg -// -// SPDX-License-Identifier: AGPL-3.0-only - -import { AstNode, AstNodeLocator, LangiumDocument } from 'langium'; -import { NodeFileSystem } from 'langium/node'; - -import { - BlockDefinition, - ValidationContext, - createJayveeServices, -} from '../..'; -import { - ParseHelperOptions, - expectNoParserAndLexerErrors, - parseHelper, - readJvTestAssetHelper, - validationAcceptorMockImpl, -} from '../../../test'; - -import { validateBlockDefinition } from './block-definition'; - -describe('Validation of BlockDefinition', () => { - let parse: ( - input: string, - options?: ParseHelperOptions, - ) => Promise>; - - const validationAcceptorMock = jest.fn(validationAcceptorMockImpl); - - let locator: AstNodeLocator; - - const readJvTestAsset = readJvTestAssetHelper( - __dirname, - '../../../test/assets/', - ); - - async function parseAndValidateBlock(input: string) { - const document = await parse(input); - expectNoParserAndLexerErrors(document); - - const block = locator.getAstNode( - document.parseResult.value, - 'pipelines@0/blocks@0', - ) as BlockDefinition; - - validateBlockDefinition( - block, - new ValidationContext(validationAcceptorMock), - ); - } - - beforeAll(() => { - // Create language services - const services = createJayveeServices(NodeFileSystem).Jayvee; - locator = services.workspace.AstNodeLocator; - // Parse function for Jayvee (without validation) - parse = parseHelper(services); - }); - - afterEach(() => { - // Reset mock - validationAcceptorMock.mockReset(); - }); - - it('should diagnose error on block without pipe', async () => { - const text = readJvTestAsset('block-definition/invalid-missing-pipe.jv'); - - await parseAndValidateBlock(text); - - expect(validationAcceptorMock).toHaveBeenCalledTimes(1); - expect(validationAcceptorMock).toHaveBeenCalledWith( - 'warning', - 'A pipe should be connected to the output of this block', - expect.any(Object), - ); - }); - - it('should have no error on valid block definition', async () => { - const text = readJvTestAsset('block-definition/valid-block-definition.jv'); - - await parseAndValidateBlock(text); - - expect(validationAcceptorMock).toHaveBeenCalledTimes(0); - }); -}); diff --git a/libs/language-server/src/lib/validation/checks/block-definition.ts b/libs/language-server/src/lib/validation/checks/block-definition.ts index 661393f3a..3a176cc8d 100644 --- a/libs/language-server/src/lib/validation/checks/block-definition.ts +++ b/libs/language-server/src/lib/validation/checks/block-definition.ts @@ -2,96 +2,6 @@ // // SPDX-License-Identifier: AGPL-3.0-only -/** - * See https://jvalue.github.io/jayvee/docs/dev/guides/working-with-the-ast/ for why the following ESLint rule is disabled for this file. - */ -/* eslint-disable @typescript-eslint/no-unnecessary-condition */ - -import { assertUnreachable } from 'langium'; - -import { PipelineWrapper } from '../../ast'; -import { - BlockDefinition, - isCompositeBlocktypeDefinition, -} from '../../ast/generated/ast'; -import { PipeWrapper } from '../../ast/wrappers/pipe-wrapper'; -import { BlockTypeWrapper } from '../../ast/wrappers/typed-object/blocktype-wrapper'; -import { ValidationContext } from '../validation-context'; - -export function validateBlockDefinition( - block: BlockDefinition, - context: ValidationContext, -): void { - checkPipesOfBlock(block, 'input', context); - checkPipesOfBlock(block, 'output', context); -} - -function checkPipesOfBlock( - block: BlockDefinition, - whatToCheck: 'input' | 'output', - context: ValidationContext, -): void { - if ( - !BlockTypeWrapper.canBeWrapped(block?.type) || - !PipelineWrapper.canBeWrapped(block.$container) - ) { - return; - } - const blockType = new BlockTypeWrapper(block?.type); - const pipes = collectPipes(block, whatToCheck); - - if (pipes.length === 0) { - const isLastBlockOfCompositeBlocktype = - isCompositeBlocktypeDefinition(block.$container) && - block.$container.blocks.at(-1)?.name === block.name; - - const isFirstBlockOfCompositeBlocktype = - isCompositeBlocktypeDefinition(block.$container) && - block.$container.blocks.at(0)?.name === block.name; - - const isExtractorBlock = !blockType.hasInput() && whatToCheck === 'input'; - const isLoaderBlock = !blockType.hasOutput() && whatToCheck === 'output'; - - // exception: the first block in a composite block is connected to the input, not another block - // exception: The last block in a composite block is connected to the output, not another block - // exception: the extractor / loader block in a pipeline - if ( - !isLastBlockOfCompositeBlocktype && - !isFirstBlockOfCompositeBlocktype && - !isExtractorBlock && - !isLoaderBlock - ) { - context.accept( - 'warning', - `A pipe should be connected to the ${whatToCheck} of this block`, - { - node: block, - property: 'name', - }, - ); - } - } -} - -function collectPipes( - block: BlockDefinition, - whatToCheck: 'input' | 'output', -): PipeWrapper[] { - const pipelineWrapper = new PipelineWrapper(block.$container); - - let pipes: PipeWrapper[]; - switch (whatToCheck) { - case 'input': { - pipes = pipelineWrapper.getIngoingPipes(block); - break; - } - case 'output': { - pipes = pipelineWrapper.getOutgoingPipes(block); - break; - } - default: { - assertUnreachable(whatToCheck); - } - } - return pipes; +export function validateBlockDefinition(): void { + // Nothing to do } diff --git a/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.spec.ts b/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.spec.ts index 302f44335..5f719bd85 100644 --- a/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.spec.ts +++ b/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.spec.ts @@ -138,4 +138,24 @@ describe('Validation of CompositeBlocktypeDefinition', () => { expect.any(Object), ); }); + + it('should diagnose error on block without pipe', async () => { + const text = readJvTestAsset( + 'composite-blocktype-definition/invalid-unconnected-block.jv', + ); + + await parseAndValidateBlocktype(text); + + expect(validationAcceptorMock).toHaveBeenCalledTimes(2); + expect(validationAcceptorMock).toHaveBeenCalledWith( + 'warning', + 'A pipe should be connected to the input of this block', + expect.any(Object), + ); + expect(validationAcceptorMock).toHaveBeenCalledWith( + 'warning', + 'A pipe should be connected to the output of this block', + expect.any(Object), + ); + }); }); diff --git a/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.ts b/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.ts index de33c0c9b..5686a9d6c 100644 --- a/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.ts +++ b/libs/language-server/src/lib/validation/checks/composite-blocktype-definition.ts @@ -2,8 +2,12 @@ // // SPDX-License-Identifier: AGPL-3.0-only +import { BlockTypeWrapper, PipelineWrapper } from '../../ast'; import { EvaluationContext } from '../../ast/expressions/evaluation'; -import { CompositeBlocktypeDefinition } from '../../ast/generated/ast'; +import { + BlockDefinition, + CompositeBlocktypeDefinition, +} from '../../ast/generated/ast'; import { ValidationContext } from '../validation-context'; import { validateBlocktypeDefinition } from './blocktype-definition'; @@ -19,12 +23,18 @@ export function validateCompositeBlockTypeDefinition( checkExactlyOnePipeline(blockType, validationContext); checkMultipleBlockInputs(blockType, validationContext); + checkDefinedBlocksAreUsed(blockType, validationContext); } function checkHasPipeline( blockType: CompositeBlocktypeDefinition, context: ValidationContext, ): void { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (blockType.pipes === undefined) { + return; + } + if (blockType.pipes.length === 0) { context.accept( 'error', @@ -41,6 +51,11 @@ function checkExactlyOnePipeline( blockType: CompositeBlocktypeDefinition, context: ValidationContext, ): void { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (blockType.pipes === undefined) { + return; + } + if (blockType.pipes.length > 1) { blockType.pipes.forEach((pipe) => { context.accept( @@ -53,3 +68,73 @@ function checkExactlyOnePipeline( }); } } + +export function checkDefinedBlocksAreUsed( + blocktypeDefinition: CompositeBlocktypeDefinition, + context: ValidationContext, +): void { + if (!PipelineWrapper.canBeWrapped(blocktypeDefinition)) { + return; + } + const pipelineWrapper = new PipelineWrapper(blocktypeDefinition); + + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (blocktypeDefinition.blocks === undefined) { + return; + } + + const containedBlocks = blocktypeDefinition.blocks; + for (const block of containedBlocks) { + doCheckDefinedBlockIsUsed(pipelineWrapper, block, context); + } +} + +function doCheckDefinedBlockIsUsed( + pipelineWrapper: PipelineWrapper, + block: BlockDefinition, + context: ValidationContext, +): void { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (block.type === undefined || !BlockTypeWrapper.canBeWrapped(block.type)) { + return; + } + const pipes = pipelineWrapper.astNode.pipes; + + const isConnectedToInput = pipes.some( + (pipe) => + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + pipe?.blocks?.at(0)?.ref === block, + ); + if (!isConnectedToInput) { + const parents = pipelineWrapper.getParentBlocks(block); + if (parents.length === 0) { + context.accept( + 'warning', + `A pipe should be connected to the input of this block`, + { + node: block, + property: 'name', + }, + ); + } + } + + const isConnectedToOutput = pipes.some( + (pipeline) => + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + pipeline?.blocks?.at(-1)?.ref === block, + ); + if (!isConnectedToOutput) { + const children = pipelineWrapper.getChildBlocks(block); + if (children.length === 0) { + context.accept( + 'warning', + `A pipe should be connected to the output of this block`, + { + node: block, + property: 'name', + }, + ); + } + } +} diff --git a/libs/language-server/src/lib/validation/checks/pipeline-definition.spec.ts b/libs/language-server/src/lib/validation/checks/pipeline-definition.spec.ts index 0626d2baf..5988ad1e7 100644 --- a/libs/language-server/src/lib/validation/checks/pipeline-definition.spec.ts +++ b/libs/language-server/src/lib/validation/checks/pipeline-definition.spec.ts @@ -85,7 +85,7 @@ describe('Validation of PipelineDefinition', () => { await parseAndValidatePipeline(text); - expect(validationAcceptorMock).toHaveBeenCalledTimes(1); + expect(validationAcceptorMock).toHaveBeenCalledTimes(2); // one warning for unused blocks expect(validationAcceptorMock).toHaveBeenCalledWith( 'error', `An extractor block is required for this pipeline`, @@ -116,4 +116,17 @@ describe('Validation of PipelineDefinition', () => { expect.any(Object), ); }); + + it('should diagnose error on block without pipe', async () => { + const text = readJvTestAsset('pipeline-definition/invalid-missing-pipe.jv'); + + await parseAndValidatePipeline(text); + + expect(validationAcceptorMock).toHaveBeenCalledTimes(2); // one error since missing extractor + expect(validationAcceptorMock).toHaveBeenCalledWith( + 'warning', + 'A pipe should be connected to the output of this block', + expect.any(Object), + ); + }); }); diff --git a/libs/language-server/src/lib/validation/checks/pipeline-definition.ts b/libs/language-server/src/lib/validation/checks/pipeline-definition.ts index 552c0f43f..4c3811b8b 100644 --- a/libs/language-server/src/lib/validation/checks/pipeline-definition.ts +++ b/libs/language-server/src/lib/validation/checks/pipeline-definition.ts @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: AGPL-3.0-only -import { PipeWrapper, PipelineWrapper } from '../../ast'; +import { BlockTypeWrapper, PipeWrapper, PipelineWrapper } from '../../ast'; import { BlockDefinition, CompositeBlocktypeDefinition, @@ -22,6 +22,7 @@ export function validatePipelineDefinition( checkUniqueNames(pipeline.constraints, context); checkMultipleBlockInputs(pipeline, context); + checkDefinedBlocksAreUsed(pipeline, context); } function checkStartingBlocks( @@ -119,3 +120,62 @@ function doCheckMultipleBlockInputs( return alreadyMarkedPipes; } + +export function checkDefinedBlocksAreUsed( + pipeline: PipelineDefinition | CompositeBlocktypeDefinition, + context: ValidationContext, +): void { + if (!PipelineWrapper.canBeWrapped(pipeline)) { + return; + } + const pipelineWrapper = new PipelineWrapper(pipeline); + + const containedBlocks = pipeline.blocks; + for (const block of containedBlocks) { + doCheckDefinedBlockIsUsed(pipelineWrapper, block, context); + } +} + +function doCheckDefinedBlockIsUsed( + pipelineWrapper: PipelineWrapper< + PipelineDefinition | CompositeBlocktypeDefinition + >, + block: BlockDefinition, + context: ValidationContext, +): void { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (block.type === undefined || !BlockTypeWrapper.canBeWrapped(block.type)) { + return; + } + const blockType = new BlockTypeWrapper(block.type); + + const isExtractorBlock = !blockType.hasInput(); + if (!isExtractorBlock) { + const parents = pipelineWrapper.getParentBlocks(block); + if (parents.length === 0) { + context.accept( + 'warning', + `A pipe should be connected to the input of this block`, + { + node: block, + property: 'name', + }, + ); + } + } + + const isLoaderBlock = !blockType.hasOutput(); + if (!isLoaderBlock) { + const children = pipelineWrapper.getChildBlocks(block); + if (children.length === 0) { + context.accept( + 'warning', + `A pipe should be connected to the output of this block`, + { + node: block, + property: 'name', + }, + ); + } + } +} diff --git a/libs/language-server/src/test/assets/block-definition/valid-block-definition.jv b/libs/language-server/src/test/assets/block-definition/valid-block-definition.jv deleted file mode 100644 index 272e0f9f7..000000000 --- a/libs/language-server/src/test/assets/block-definition/valid-block-definition.jv +++ /dev/null @@ -1,23 +0,0 @@ -// SPDX-FileCopyrightText: 2023 Friedrich-Alexander-Universitat Erlangen-Nurnberg -// -// SPDX-License-Identifier: AGPL-3.0-only - -pipeline Pipeline { - block TestExtractor oftype TestFileExtractor { - } - - block TestLoader oftype TestTableLoader { - } - - TestExtractor -> TestLoader; -} - -builtin blocktype TestFileExtractor { - input inPort oftype None; - output outPort oftype File; -} - -builtin blocktype TestTableLoader { - input inPort oftype File; - output outPort oftype None; -} \ No newline at end of file diff --git a/libs/language-server/src/test/assets/composite-blocktype-definition/invalid-unconnected-block.jv b/libs/language-server/src/test/assets/composite-blocktype-definition/invalid-unconnected-block.jv new file mode 100644 index 000000000..252667e32 --- /dev/null +++ b/libs/language-server/src/test/assets/composite-blocktype-definition/invalid-unconnected-block.jv @@ -0,0 +1,18 @@ +// SPDX-FileCopyrightText: 2023 Friedrich-Alexander-Universitat Erlangen-Nurnberg +// +// SPDX-License-Identifier: AGPL-3.0-only + +composite blocktype TestBlockType { + input inPort oftype Table; + output outPort oftype File; + + block TestExtractor1 oftype TestFileExtractor { } + block TestExtractor2 oftype TestFileExtractor { } + + inPort -> TestExtractor1 -> outPort; +} + +builtin blocktype TestFileExtractor { + input inPort oftype Table; + output outPort oftype File; +} diff --git a/libs/language-server/src/test/assets/block-definition/invalid-missing-pipe.jv b/libs/language-server/src/test/assets/pipeline-definition/invalid-missing-pipe.jv similarity index 100% rename from libs/language-server/src/test/assets/block-definition/invalid-missing-pipe.jv rename to libs/language-server/src/test/assets/pipeline-definition/invalid-missing-pipe.jv