Lint for consistency in multiline vs single-line "if" and "else"

package.json

@@ -1,6 +1,6 @@
 {
 	"name": "ecmarkup",
-	"version": "16.2.0",
+	"version": "17.0.0",
 	"description": "Custom element definitions and core utilities for markup that specifies ECMAScript and related technologies.",
 	"main": "lib/ecmarkup.js",
 	"typings": "lib/ecmarkup.d.ts",

src/lint/collect-algorithm-diagnostics.ts

@@ -1,4 +1,4 @@
-import type { Node as EcmarkdownNode, OrderedListItemNode } from 'ecmarkdown';
+import type { Node as EcmarkdownNode, OrderedListItemNode, OrderedListNode } from 'ecmarkdown';
 
 import type { LintingError, Reporter } from './algorithm-error-reporter-type';
 import type { default as Spec, Warning } from '../Spec';
@@ -11,21 +11,24 @@ import lintAlgorithmStepNumbering from './rules/algorithm-step-numbering';
 import lintAlgorithmStepLabels from './rules/algorithm-step-labels';
 import lintForEachElement from './rules/for-each-element';
 import lintStepAttributes from './rules/step-attributes';
+import lintIfElseConsistency from './rules/if-else-consistency';
 import { checkVariableUsage } from './rules/variable-use-def';
 import { parse, Seq } from '../expr-parser';
 
 type LineRule = (
   report: Reporter,
-  stepSeq: Seq | null,
   step: OrderedListItemNode,
-  algorithmSource: string
+  algorithmSource: string,
+  parsedSteps: Map<OrderedListItemNode, Seq>,
+  parent: OrderedListNode
 ) => void;
 const stepRules: LineRule[] = [
   lintAlgorithmLineStyle,
   lintAlgorithmStepNumbering,
   lintAlgorithmStepLabels,
   lintForEachElement,
   lintStepAttributes,
+  lintIfElseConsistency,
 ];
 
 export function collectAlgorithmDiagnostics(
@@ -70,26 +73,38 @@ export function collectAlgorithmDiagnostics(
     }
     const parsedSteps: Map<OrderedListItemNode, Seq> = new Map();
     let allNodesParsedSuccessfully = true;
-    function walk(visit: LineRule, step: OrderedListItemNode) {
-      // we don't know the names of ops at this point
-      // TODO maybe run later in the process? but not worth worrying about for now
+    function parseStep(step: OrderedListItemNode) {
       const parsed = parse(step.contents, new Set());
-      visit(reporter, parsed.name === 'seq' ? parsed : null, step, algorithmSource); // TODO reconsider algorithmSource
       if (parsed.name === 'failure') {
         allNodesParsedSuccessfully = false;
       } else {
         parsedSteps.set(step, parsed);
       }
       if (step.sublist?.name === 'ol') {
         for (const substep of step.sublist.contents) {
-          walk(visit, substep);
+          parseStep(substep);
+        }
+      }
+    }
+
+    function applyRule(visit: LineRule, step: OrderedListItemNode, parent: OrderedListNode) {
+      // we don't know the names of ops at this point
+      // TODO maybe run later in the process? but not worth worrying about for now
+      visit(reporter, step, algorithmSource, parsedSteps, parent);
+      if (step.sublist?.name === 'ol') {
+        for (const substep of step.sublist.contents) {
+          applyRule(visit, substep, step.sublist);
         }
       }
     }
     if (tree != null && !element.hasAttribute('example')) {
+      for (const step of tree.contents.contents) {
+        parseStep(step);
+      }
+
       for (const rule of stepRules) {
         for (const step of tree.contents.contents) {
-          walk(rule, step);
+          applyRule(rule, step, tree.contents);
         }
       }
       if (allNodesParsedSuccessfully) {

src/lint/rules/algorithm-line-style.ts

@@ -28,10 +28,11 @@ Checks that every algorithm step has one of these forms:
 */
 export default function (
   report: Reporter,
-  stepSeq: Seq | null,
   node: OrderedListItemNode,
-  algorithmSource: string
+  algorithmSource: string,
+  parsedSteps: Map<OrderedListItemNode, Seq>
 ) {
+  const stepSeq = parsedSteps.get(node);
   if (stepSeq == null || stepSeq.items.length === 0) {
     return;
   }

src/lint/rules/algorithm-step-labels.ts

@@ -1,18 +1,12 @@
 import type { OrderedListItemNode } from 'ecmarkdown';
 import type { Reporter } from '../algorithm-error-reporter-type';
-import type { Seq } from '../../expr-parser';
 
 const ruleId = 'algorithm-step-labels';
 
 /*
 Checks that step labels all start with `step-`.
 */
-export default function (
-  report: Reporter,
-  stepSeq: Seq | null,
-  node: OrderedListItemNode,
-  algorithmSource: string
-) {
+export default function (report: Reporter, node: OrderedListItemNode, algorithmSource: string) {
   const idAttr = node.attrs.find(({ key }) => key === 'id');
   if (idAttr != null && !/^step-/.test(idAttr.value)) {
     const itemSource = algorithmSource.slice(

src/lint/rules/algorithm-step-numbering.ts

@@ -1,18 +1,12 @@
 import type { OrderedListItemNode } from 'ecmarkdown';
 import type { Reporter } from '../algorithm-error-reporter-type';
-import type { Seq } from '../../expr-parser';
 
 const ruleId = 'algorithm-step-numbering';
 
 /*
 Checks that step numbers are all `1`.
 */
-export default function (
-  report: Reporter,
-  stepSeq: Seq | null,
-  node: OrderedListItemNode,
-  algorithmSource: string
-) {
+export default function (report: Reporter, node: OrderedListItemNode, algorithmSource: string) {
   const itemSource = algorithmSource.slice(node.location.start.offset, node.location.end.offset);
   const match = itemSource.match(/^(\s*)(\d+\.) /)!;
   if (match[2] !== '1.') {

src/lint/rules/for-each-element.ts

@@ -1,12 +1,19 @@
 import type { Reporter } from '../algorithm-error-reporter-type';
 import type { Seq } from '../../expr-parser';
+import type { OrderedListItemNode } from 'ecmarkdown';
 
 const ruleId = 'for-each-element';
 
 /*
 Checks that "For each" loops name a type or say "element" before the variable.
 */
-export default function (report: Reporter, stepSeq: Seq | null) {
+export default function (
+  report: Reporter,
+  step: OrderedListItemNode,
+  algorithmSource: string,
+  parsedSteps: Map<OrderedListItemNode, Seq>
+) {
+  const stepSeq = parsedSteps.get(step);
   if (stepSeq == null || stepSeq.items.length < 2) {
     return;
   }

src/lint/rules/if-else-consistency.ts

@@ -0,0 +1,57 @@
+import type { Reporter } from '../algorithm-error-reporter-type';
+import type { Seq } from '../../expr-parser';
+import type { OrderedListItemNode, OrderedListNode } from 'ecmarkdown';
+import { offsetToLineAndColumn } from '../../utils';
+
+const ruleId = 'if-else-consistency';
+
+/*
+Checks that `if`/`else` statements are both single-line or both multi-line.
+*/
+export default function (
+  report: Reporter,
+  step: OrderedListItemNode,
+  algorithmSource: string,
+  parsedSteps: Map<OrderedListItemNode, Seq>,
+  parent: OrderedListNode
+) {
+  const stepSeq = parsedSteps.get(step);
+  if (stepSeq == null) {
+    return;
+  }
+  const firstSeqItem = stepSeq.items[0];
+  if (firstSeqItem?.name !== 'text' || !/^(?:If|Else if)\b/.test(firstSeqItem.contents)) {
+    return;
+  }
+  const idx = parent.contents.indexOf(step);
+  if (idx >= parent.contents.length - 1) {
+    return;
+  }
+  const nextStep = parent.contents[idx + 1];
+  const nextSeq = parsedSteps.get(nextStep);
+  if (nextSeq == null) {
+    return;
+  }
+  const nextFirstSeqitem = nextSeq.items[0];
+  if (
+    nextFirstSeqitem?.name !== 'text' ||
+    !/^(?:Else|Otherwise)\b/.test(nextFirstSeqitem.contents)
+  ) {
+    return;
+  }
+  if (step.sublist != null && nextStep.sublist == null) {
+    const location = offsetToLineAndColumn(algorithmSource, nextFirstSeqitem.location.start.offset);
+    report({
+      ruleId,
+      ...location,
+      message: '"Else" steps should be multiline whenever their corresponding "If" is',
+    });
+  } else if (step.sublist == null && nextStep.sublist != null) {
+    const location = offsetToLineAndColumn(algorithmSource, firstSeqItem.location.start.offset);
+    report({
+      ruleId,
+      ...location,
+      message: '"If" steps should be multiline whenever their corresponding "Else" is',
+    });
+  }
+}

src/lint/rules/step-attributes.ts

@@ -1,6 +1,5 @@
 import type { OrderedListItemNode } from 'ecmarkdown';
 import type { Reporter } from '../algorithm-error-reporter-type';
-import type { Seq } from '../../expr-parser';
 
 import { SPECIAL_KINDS } from '../../Clause';
 
@@ -11,7 +10,7 @@ const KNOWN_ATTRIBUTES = ['id', 'fence-effects', 'declared', ...SPECIAL_KINDS];
 /*
 Checks for unknown attributes on steps.
 */
-export default function (report: Reporter, stepSeq: Seq | null, node: OrderedListItemNode) {
+export default function (report: Reporter, node: OrderedListItemNode) {
   for (const attr of node.attrs) {
     if (!KNOWN_ATTRIBUTES.includes(attr.key)) {
       report({

test/lint-algorithms.js

@@ -467,4 +467,132 @@ describe('linting algorithms', () => {
       `);
     });
   });
+
+  describe('if/else consistency', () => {
+    const ruleId = 'if-else-consistency';
+    it('rejects single-line if with multiline else', async () => {
+      await assertLint(
+        positioned`
+        <emu-alg>
+          1. ${M}If some condition holds, do something.
+          1. Else,
+            1. Do something else.
+        </emu-alg>`,
+        {
+          ruleId,
+          nodeType,
+          message: '"If" steps should be multiline whenever their corresponding "Else" is',
+        }
+      );
+    });
+
+    it('rejects single-line if with multiline else-if', async () => {
+      await assertLint(
+        positioned`
+        <emu-alg>
+          1. ${M}If some condition holds, do something.
+          1. Else if another condition holds, then
+            1. Do something else.
+          1. Else,
+            1. Do something yet otherwise.
+        </emu-alg>`,
+        {
+          ruleId,
+          nodeType,
+          message: '"If" steps should be multiline whenever their corresponding "Else" is',
+        }
+      );
+    });
+
+    it('rejects single-line else-if with multiline else', async () => {
+      await assertLint(
+        positioned`
+        <emu-alg>
+          1. If some condition holds, do something.
+          1. ${M}Else if another condition holds, do something else.
+          1. Else,
+            1. Do something yet otherwise.
+        </emu-alg>`,
+        {
+          ruleId,
+          nodeType,
+          message: '"If" steps should be multiline whenever their corresponding "Else" is',
+        }
+      );
+    });
+
+    it('rejects multi-line if with single-line else', async () => {
+      await assertLint(
+        positioned`
+        <emu-alg>
+          1. If some condition holds, then
+            1. Do something.
+          1. ${M}Else do something else.
+        </emu-alg>`,
+        {
+          ruleId,
+          nodeType,
+          message: '"Else" steps should be multiline whenever their corresponding "If" is',
+        }
+      );
+    });
+
+    it('rejects multi-line if with single-line else-f', async () => {
+      await assertLint(
+        positioned`
+        <emu-alg>
+          1. If some condition holds, then
+            1. Do something.
+          1. ${M}Else if another condition holds do something else.
+          1. Else, do something yet otherwise.
+        </emu-alg>`,
+        {
+          ruleId,
+          nodeType,
+          message: '"Else" steps should be multiline whenever their corresponding "If" is',
+        }
+      );
+    });
+
+    it('rejects multi-line else-if with single-line else', async () => {
+      await assertLint(
+        positioned`
+        <emu-alg>
+          1. If some condition holds, then
+            1. Do something.
+          1. Else if another condition holds, then
+            1. Do something else.
+          1. ${M}Else, do something yet otherwise.
+        </emu-alg>`,
+        {
+          ruleId,
+          nodeType,
+          message: '"Else" steps should be multiline whenever their corresponding "If" is',
+        }
+      );
+    });
+
+    it('negative', async () => {
+      await assertLintFree(`
+        <emu-alg>
+          1. If some condition holds, do something simple.
+          1. Else, do something yet otherwise simple.
+          1. If some condition holds, do something simple.
+          1. Else if another condition holds, do something else simple.
+          1. Else, do something yet otherwise simple.
+          1. NOTE: Also works for multiline.
+          1. If some condition holds, then
+            1. Do something simple.
+          1. Else,
+            1. Do something yet otherwise simple.
+          1. If some condition holds, then
+            1. Do something simple.
+          1. Else if another condition holds, then
+            1. Do something else simple.
+          1. Else,
+            1. Do something yet otherwise simple.
+        </emu-alg>
+      `);
+    });
+  });
 });