add lint rule for multiline if/else consistency

Author: bakkot

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,
   step: OrderedListItemNode,
   algorithmSource: string,
-  parsedSteps: Map<OrderedListItemNode, Seq>
+  parsedSteps: Map<OrderedListItemNode, Seq>,
+  parent: OrderedListNode
 ) => void;
 const stepRules: LineRule[] = [
   lintAlgorithmLineStyle,
   lintAlgorithmStepNumbering,
   lintAlgorithmStepLabels,
   lintForEachElement,
   lintStepAttributes,
+  lintIfElseConsistency,
 ];
 
 export function collectAlgorithmDiagnostics(
@@ -84,13 +87,13 @@ export function collectAlgorithmDiagnostics(
       }
     }
 
-    function applyRule(visit: LineRule, step: OrderedListItemNode) {
+    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);
+      visit(reporter, step, algorithmSource, parsedSteps, parent);
       if (step.sublist?.name === 'ol') {
         for (const substep of step.sublist.contents) {
-          applyRule(visit, substep);
+          applyRule(visit, substep, step.sublist);
         }
       }
     }
@@ -101,7 +104,7 @@ export function collectAlgorithmDiagnostics(
 
       for (const rule of stepRules) {
         for (const step of tree.contents.contents) {
-          applyRule(rule, step);
+          applyRule(rule, step, tree.contents);
         }
       }
       if (allNodesParsedSuccessfully) {

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

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