Merge pull request #509 from jvalue/refactor-block-validation-unused-…

…blocks

Refactor block validation for unused blocks

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;

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
 }

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),
+    );
+  });
 });

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<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 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',
+        },
+      );
+    }
+  }
+}

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),
+    );
+  });
 });