From 0d1747985369088bbe0acf7c8ffe75b3f97c30d2 Mon Sep 17 00:00:00 2001 From: Kevin Gibbons Date: Wed, 19 Apr 2023 23:50:45 -0700 Subject: [PATCH] add lint rule for multiline if/else consistency --- src/lint/collect-algorithm-diagnostics.ts | 15 ++- src/lint/rules/if-else-consistency.ts | 57 ++++++++++ test/lint-algorithms.js | 128 ++++++++++++++++++++++ 3 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 src/lint/rules/if-else-consistency.ts diff --git a/src/lint/collect-algorithm-diagnostics.ts b/src/lint/collect-algorithm-diagnostics.ts index d60046ff..8bf77855 100644 --- a/src/lint/collect-algorithm-diagnostics.ts +++ b/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,6 +11,7 @@ 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'; @@ -18,7 +19,8 @@ type LineRule = ( report: Reporter, step: OrderedListItemNode, algorithmSource: string, - parsedSteps: Map + parsedSteps: Map, + parent: OrderedListNode ) => void; const stepRules: LineRule[] = [ lintAlgorithmLineStyle, @@ -26,6 +28,7 @@ const stepRules: LineRule[] = [ 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) { diff --git a/src/lint/rules/if-else-consistency.ts b/src/lint/rules/if-else-consistency.ts new file mode 100644 index 00000000..44aaddd3 --- /dev/null +++ b/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, + 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', + }); + } +} diff --git a/test/lint-algorithms.js b/test/lint-algorithms.js index e61150dc..87b77b7c 100644 --- a/test/lint-algorithms.js +++ b/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` + + 1. ${M}If some condition holds, do something. + 1. Else, + 1. Do something else. + `, + { + 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` + + 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. + `, + { + 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` + + 1. If some condition holds, do something. + 1. ${M}Else if another condition holds, do something else. + 1. Else, + 1. Do something yet otherwise. + `, + { + 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` + + 1. If some condition holds, then + 1. Do something. + 1. ${M}Else do something else. + `, + { + 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` + + 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. + `, + { + 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` + + 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. + `, + { + ruleId, + nodeType, + message: '"Else" steps should be multiline whenever their corresponding "If" is', + } + ); + }); + + it('negative', async () => { + await assertLintFree(` + + 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. + + `); + }); + }); });