From 35e5247e72e47c3b29eb1a699d8128c0153a83be Mon Sep 17 00:00:00 2001 From: Georg Schwarz Date: Tue, 16 Jan 2024 10:36:21 +0100 Subject: [PATCH 1/5] Simplify checking for incorrect extractor and loader blocks --- .../lib/validation/checks/block-definition.ts | 31 ++++++------------- .../lib/validation/checks/pipe-definition.ts | 10 +++--- 2 files changed, 14 insertions(+), 27 deletions(-) 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 87da1ae64..3907febde 100644 --- a/libs/language-server/src/lib/validation/checks/block-definition.ts +++ b/libs/language-server/src/lib/validation/checks/block-definition.ts @@ -40,24 +40,6 @@ function checkPipesOfBlock( const blockType = new BlockTypeWrapper(block?.type); const pipes = collectPipes(block, whatToCheck); - const isStartOrEnd = - (whatToCheck === 'input' && !blockType.hasInput()) || - (whatToCheck === 'output' && !blockType.hasOutput()); - if (isStartOrEnd) { - if (pipes.length > 0) { - for (const pipe of pipes) { - context.accept( - 'error', - `Blocks of type ${blockType.type} do not have an ${whatToCheck}`, - whatToCheck === 'input' - ? pipe.getToDiagnostic() - : pipe.getFromDiagnostic(), - ); - } - } - return; - } - const hasMultipleInputPorts = pipes.length > 1 && whatToCheck === 'input'; if (hasMultipleInputPorts) { for (const pipe of pipes) { @@ -70,8 +52,6 @@ function checkPipesOfBlock( return; } - // above: there should be pipes (defined by blocktype) - // but there aren't any if (pipes.length === 0) { const isLastBlockOfCompositeBlocktype = isCompositeBlocktypeDefinition(block.$container) && @@ -81,9 +61,18 @@ function checkPipesOfBlock( 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 - if (!isLastBlockOfCompositeBlocktype && !isFirstBlockOfCompositeBlocktype) { + // 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`, diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.ts index 76f24acbe..9c1b6abb0 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.ts @@ -37,12 +37,10 @@ function checkBlockCompatibility( const fromBlockType = new BlockTypeWrapper(fromBlockTypeDefinition); const toBlockType = new BlockTypeWrapper(toBlockTypeDefinition); - if (fromBlockType.hasOutput() && toBlockType.hasInput()) { - if (!fromBlockType.canBeConnectedTo(toBlockType)) { - const errorMessage = `The output type "${fromBlockType.outputType}" of ${fromBlockType.type} is incompatible with the input type "${toBlockType.inputType}" of ${toBlockType.type}`; - context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic()); - context.accept('error', errorMessage, pipeWrapper.getToDiagnostic()); - } + if (!fromBlockType.canBeConnectedTo(toBlockType)) { + const errorMessage = `The output type "${fromBlockType.outputType}" of ${fromBlockType.type} is incompatible with the input type "${toBlockType.inputType}" of ${toBlockType.type}`; + context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic()); + context.accept('error', errorMessage, pipeWrapper.getToDiagnostic()); } } } From c78387091c46ee0a369b0d3d414595bb990e91e5 Mon Sep 17 00:00:00 2001 From: Georg Schwarz Date: Tue, 16 Jan 2024 11:26:44 +0100 Subject: [PATCH 2/5] Move test with moved validation --- .../validation/checks/block-definition.spec.ts | 15 --------------- .../lib/validation/checks/pipe-definition.spec.ts | 15 +++++++++++++++ .../invalid-output-block-as-input.jv | 0 3 files changed, 15 insertions(+), 15 deletions(-) rename libs/language-server/src/test/assets/{block-definition => pipe-definition}/invalid-output-block-as-input.jv (100%) 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 index a8da55530..a6b5f426c 100644 --- a/libs/language-server/src/lib/validation/checks/block-definition.spec.ts +++ b/libs/language-server/src/lib/validation/checks/block-definition.spec.ts @@ -76,21 +76,6 @@ describe('Validation of BlockDefinition', () => { ); }); - it('should diagnose error on block as output without having an output', async () => { - const text = readJvTestAsset( - 'block-definition/invalid-output-block-as-input.jv', - ); - - await parseAndValidateBlock(text); - - expect(validationAcceptorMock).toHaveBeenCalledTimes(1); - expect(validationAcceptorMock).toHaveBeenCalledWith( - 'error', - 'Blocks of type TestTableLoader do not have an output', - expect.any(Object), - ); - }); - it('should diagnose error on block as input for multiple pipes', async () => { const text = readJvTestAsset( 'block-definition/invalid-block-as-multiple-pipe-inputs.jv', diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts index fb6c43a9a..97e1a2b25 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts @@ -95,5 +95,20 @@ describe('Validation of PipeDefinition', () => { expect.any(Object), ); }); + + it('should diagnose error on ingoing blocktype having no output but has pipe', async () => { + const text = readJvTestAsset( + 'pipe-definition/invalid-output-block-as-input.jv', + ); + + await parseAndValidatePipe(text); + + expect(validationAcceptorMock).toHaveBeenCalledTimes(1); + expect(validationAcceptorMock).toHaveBeenCalledWith( + 'error', + 'Blocks of type TestTableLoader do not have an output', + expect.any(Object), + ); + }); }); }); diff --git a/libs/language-server/src/test/assets/block-definition/invalid-output-block-as-input.jv b/libs/language-server/src/test/assets/pipe-definition/invalid-output-block-as-input.jv similarity index 100% rename from libs/language-server/src/test/assets/block-definition/invalid-output-block-as-input.jv rename to libs/language-server/src/test/assets/pipe-definition/invalid-output-block-as-input.jv From c98e6aa335aad3424add7d34fed3ff732a4f2a4b Mon Sep 17 00:00:00 2001 From: Georg Schwarz Date: Tue, 16 Jan 2024 11:27:12 +0100 Subject: [PATCH 3/5] Add better failure messages for exractor and loader blocks --- .../src/lib/validation/checks/pipe-definition.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.ts index 9c1b6abb0..28b99541c 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.ts @@ -37,6 +37,19 @@ function checkBlockCompatibility( const fromBlockType = new BlockTypeWrapper(fromBlockTypeDefinition); const toBlockType = new BlockTypeWrapper(toBlockTypeDefinition); + const isFromBlockLoader = !fromBlockType.hasOutput(); + const isToBlockExtractor = !toBlockType.hasInput(); + + if (isFromBlockLoader) { + const errorMessage = `Block "${pipeWrapper.from?.name}" cannot be connected to other blocks. Its blocktype "${fromBlockType.astNode.name}" has output type "${fromBlockType.outputType}".`; + context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic()); + } + + if (isToBlockExtractor) { + const errorMessage = `Block "${pipeWrapper.to?.name}" cannot be connected from other blocks. Its blocktype "${toBlockType.astNode.name}" has output type "${toBlockType.outputType}".`; + context.accept('error', errorMessage, pipeWrapper.getToDiagnostic()); + } + if (!fromBlockType.canBeConnectedTo(toBlockType)) { const errorMessage = `The output type "${fromBlockType.outputType}" of ${fromBlockType.type} is incompatible with the input type "${toBlockType.inputType}" of ${toBlockType.type}`; context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic()); From 7506f8f6bd50a2bac898f956e00262368e982b7f Mon Sep 17 00:00:00 2001 From: Georg Schwarz Date: Tue, 16 Jan 2024 12:43:58 +0100 Subject: [PATCH 4/5] Improve error messages on wrongly connected extractor and loader blocks --- .../lib/validation/checks/pipe-definition.spec.ts | 15 +++++++++++---- .../src/lib/validation/checks/pipe-definition.ts | 2 +- .../invalid-output-block-as-input.jv | 6 ++---- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts index 97e1a2b25..840acef44 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts @@ -96,17 +96,24 @@ describe('Validation of PipeDefinition', () => { ); }); - it('should diagnose error on ingoing blocktype having no output but has pipe', async () => { + it('should diagnose error on connecting loader block to extractor block with a pipe', async () => { const text = readJvTestAsset( 'pipe-definition/invalid-output-block-as-input.jv', ); await parseAndValidatePipe(text); - expect(validationAcceptorMock).toHaveBeenCalledTimes(1); - expect(validationAcceptorMock).toHaveBeenCalledWith( + expect(validationAcceptorMock).toHaveBeenCalledTimes(2); + expect(validationAcceptorMock).toHaveBeenNthCalledWith( + 1, + 'error', + 'Block "BlockTo" cannot be connected to other blocks. Its blocktype "TestFileLoader" has output type "None".', + expect.any(Object), + ); + expect(validationAcceptorMock).toHaveBeenNthCalledWith( + 2, 'error', - 'Blocks of type TestTableLoader do not have an output', + 'Block "BlockFrom" cannot be connected to from other blocks. Its blocktype "TestFileExtractor" has input type "None".', expect.any(Object), ); }); diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.ts index 28b99541c..fd320f2b7 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.ts @@ -46,7 +46,7 @@ function checkBlockCompatibility( } if (isToBlockExtractor) { - const errorMessage = `Block "${pipeWrapper.to?.name}" cannot be connected from other blocks. Its blocktype "${toBlockType.astNode.name}" has output type "${toBlockType.outputType}".`; + const errorMessage = `Block "${pipeWrapper.to?.name}" cannot be connected to from other blocks. Its blocktype "${toBlockType.astNode.name}" has input type "${toBlockType.inputType}".`; context.accept('error', errorMessage, pipeWrapper.getToDiagnostic()); } diff --git a/libs/language-server/src/test/assets/pipe-definition/invalid-output-block-as-input.jv b/libs/language-server/src/test/assets/pipe-definition/invalid-output-block-as-input.jv index 40f836c9b..1bec9407d 100644 --- a/libs/language-server/src/test/assets/pipe-definition/invalid-output-block-as-input.jv +++ b/libs/language-server/src/test/assets/pipe-definition/invalid-output-block-as-input.jv @@ -3,14 +3,12 @@ // SPDX-License-Identifier: AGPL-3.0-only pipeline Pipeline { - block BlockTo oftype TestTableLoader { + block BlockTo oftype TestFileLoader { } block BlockFrom oftype TestFileExtractor { } - BlockFrom -> BlockTo; - BlockTo -> BlockFrom; } @@ -19,7 +17,7 @@ builtin blocktype TestFileExtractor { output outPort oftype File; } -builtin blocktype TestTableLoader { +builtin blocktype TestFileLoader { input inPort oftype File; output outPort oftype None; } From b43c3794cdf5a937696f97c1dfd363ffbd607722 Mon Sep 17 00:00:00 2001 From: Georg Schwarz Date: Tue, 16 Jan 2024 12:50:49 +0100 Subject: [PATCH 5/5] Improve validation message on incompatible types in pipe --- .../src/lib/validation/checks/pipe-definition.spec.ts | 2 +- .../src/lib/validation/checks/pipe-definition.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts index 840acef44..28952ae66 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.spec.ts @@ -91,7 +91,7 @@ describe('Validation of PipeDefinition', () => { expect(validationAcceptorMock).toHaveBeenNthCalledWith( 2, 'error', - `The output type "File" of TestFileExtractor is incompatible with the input type "Table" of TestTableLoader`, + 'The output type "File" of block "TestExtractor" (of type "TestFileExtractor") is not compatible with the input type "Table" of block "TestLoader" (of type "TestTableLoader")', expect.any(Object), ); }); diff --git a/libs/language-server/src/lib/validation/checks/pipe-definition.ts b/libs/language-server/src/lib/validation/checks/pipe-definition.ts index fd320f2b7..c1fc581dd 100644 --- a/libs/language-server/src/lib/validation/checks/pipe-definition.ts +++ b/libs/language-server/src/lib/validation/checks/pipe-definition.ts @@ -51,7 +51,7 @@ function checkBlockCompatibility( } if (!fromBlockType.canBeConnectedTo(toBlockType)) { - const errorMessage = `The output type "${fromBlockType.outputType}" of ${fromBlockType.type} is incompatible with the input type "${toBlockType.inputType}" of ${toBlockType.type}`; + const errorMessage = `The output type "${fromBlockType.outputType}" of block "${pipeWrapper.from?.name}" (of type "${fromBlockType.astNode.name}") is not compatible with the input type "${toBlockType.inputType}" of block "${pipeWrapper.to?.name}" (of type "${toBlockType.astNode.name}")`; context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic()); context.accept('error', errorMessage, pipeWrapper.getToDiagnostic()); }