diff --git a/src/lint/collect-algorithm-diagnostics.ts b/src/lint/collect-algorithm-diagnostics.ts index 581cf14d..39d47c5b 100644 --- a/src/lint/collect-algorithm-diagnostics.ts +++ b/src/lint/collect-algorithm-diagnostics.ts @@ -13,6 +13,7 @@ import lintEnumCasing from './rules/enum-casing'; import lintForEachElement from './rules/for-each-element'; import lintStepAttributes from './rules/step-attributes'; import lintIfElseConsistency from './rules/if-else-consistency'; +import lintAlgorithmSpelling from './rules/algorithm-spelling'; import { checkVariableUsage } from './rules/variable-use-def'; import type { Seq } from '../expr-parser'; import { parse } from '../expr-parser'; @@ -32,6 +33,7 @@ const stepRules: LineRule[] = [ lintForEachElement, lintStepAttributes, lintIfElseConsistency, + lintAlgorithmSpelling, ]; export function collectAlgorithmDiagnostics( diff --git a/src/lint/rules/algorithm-spelling.ts b/src/lint/rules/algorithm-spelling.ts new file mode 100644 index 00000000..628bb98c --- /dev/null +++ b/src/lint/rules/algorithm-spelling.ts @@ -0,0 +1,133 @@ +import type { Reporter } from '../algorithm-error-reporter-type'; +import type { OrderedListItemNode } from 'ecmarkdown'; +import type { Seq } from '../../expr-parser'; +import { walk as walkExpr } from '../../expr-parser'; +import { offsetToLineAndColumn } from '../../utils'; + +const ruleId = 'algorithm-spelling'; + +// Note that these will be composed, so cannot contain backreferences +const matchers = [ + { + pattern: /\b[Ii]ncrement\b/gu, + message: 'prefer "set _x_ to _x_ + 1" over "increment"', + }, + { + pattern: /\b[Dd]ecrement\b/gu, + message: 'prefer "set _x_ to _x_ - 1" over "decrement"', + }, + { + pattern: /\b[Ii]ncrease _/gu, + message: 'prefer "set _x_ to _x_ + _y_" over "increase _x_"', + }, + { + pattern: /\b[Dd]ecrease _/gu, + message: 'prefer "set _x_ to _x_ - _y_" over "decrease _x_"', + }, + { + pattern: /\bis an element of\b/gu, + message: 'prefer "_list_ contains _element_" over "_element_ is an element of _list_"', + }, + { + pattern: /(?<=\.\[\[\w+\]\] )is present\b/gu, + message: 'prefer "_record_ has a [[Field]] field" over "_record_.[[Field]] is present"', + }, + { + pattern: /(?<=\.\[\[Type\]\] )is ~(?:throw|normal|return|continue|break)~/gu, + message: + 'prefer "is a throw completion", "is a normal completion", etc. over accessing [[Type]]', + }, + { + pattern: /\bis either null or undefined\b/gu, + message: 'prefer "is either undefined or null" for consistency', + }, + { + pattern: /\bis that of\b/gu, + message: 'prefer "whose _ is _" over "whose _ is that of _"', + }, + { + pattern: /\bare both\b/gu, + message: 'prefer "if _a_ is _c_ and _b_ is _c_" over "if _a_ and _b_ are both _c_"', + }, + { + pattern: + /\b(?:a|an) (?:String|Number|Boolean|BigInt|Symbol|Object|List|Record|Set|Relation|Enum|Property Descriptor|(?:Reference|Completion|Environment|ClassFieldDefinition|ClassStaticBlockDefinition) Record|Abstract Closure|PrivateElement) value\b/gu, + message: + 'do not say "value" after the name of a spec type when following a determiner: prefer "a String" over "a String value"', + }, +]; + +const composed = new RegExp(matchers.map(m => `(?:${m.pattern.source})`).join('|'), 'u'); + +export default function ( + report: Reporter, + step: OrderedListItemNode, + algorithmSource: string, + parsedSteps: Map, +) { + if (step.contents.length === 0) { + return; + } + const stepSource = algorithmSource.slice( + step.contents[0].location.start.offset, + step.contents[step.contents.length - 1].location.end.offset, + ); + if (/^(?:NOTE:|Assert:)/.test(stepSource)) { + return; + } + + const baseOffset = step.contents[0].location.start.offset; + + // structural checks using the parsed expression tree + const stepSeq = parsedSteps.get(step); + if (stepSeq != null) { + walkExpr((expr, path) => { + // check *""* outside of AO argument lists + if ( + expr.name === 'star' && + expr.contents.length === 1 && + expr.contents[0].name === 'text' && + expr.contents[0].contents === '""' + ) { + const inArgList = path.some(p => p.parent.name === 'call' || p.parent.name === 'sdo-call'); + if (!inArgList) { + report({ + ruleId: 'prefer-empty-string', + message: 'prefer "the empty String" over *""*', + ...offsetToLineAndColumn(algorithmSource, expr.location.start.offset), + }); + } + } + }, stepSeq); + } + + // check "or if" / "and if" — but only when the other connective is not also present + const orIfPattern = /(?<=\bIf .+) (or|and) if\b/gu; + let orIfMatch = orIfPattern.exec(stepSource); + while (orIfMatch !== null) { + const otherPattern = orIfMatch[1] === 'or' ? /\band\b/ : /\bor\b/; + if (!otherPattern.test(stepSource)) { + report({ + ruleId, + message: `prefer "if _a_ ${orIfMatch[1]} _b_" over "if _a_ ${orIfMatch[1]} if _b_"`, + ...offsetToLineAndColumn(algorithmSource, baseOffset + orIfMatch.index), + }); + } + orIfMatch = orIfPattern.exec(stepSource); + } + + if (!composed.test(stepSource)) { + return; + } + for (const { pattern, message } of matchers) { + let match = pattern.exec(stepSource); + while (match !== null) { + report({ + ruleId, + message, + ...offsetToLineAndColumn(algorithmSource, baseOffset + match.index), + }); + match = pattern.exec(stepSource); + } + } +} diff --git a/test/lint-algorithm-spelling.ts b/test/lint-algorithm-spelling.ts new file mode 100644 index 00000000..f1d886db --- /dev/null +++ b/test/lint-algorithm-spelling.ts @@ -0,0 +1,362 @@ +import { describe, it } from 'node:test'; +import { assertLint, assertLintFree, lintLocationMarker as M, positioned } from './utils.ts'; + +const nodeType = 'emu-alg'; +const ruleId = 'algorithm-spelling'; + +describe('algorithm spelling', () => { + it('empty string literal', async () => { + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. If _x_ is ${M}*""*, return _x_. + `, + { + ruleId: 'prefer-empty-string', + nodeType, + message: 'prefer "the empty String" over *""*', + }, + ); + }); + + it('empty string literal in AO argument list is allowed', async () => { + await assertLintFree(` + +

SomeOperation ( _v_ )

+
+
description
Example.
+
+ + 1. Return _v_. + + + +

Caller ( )

+
+
description
Example.
+
+ + 1. Let _x_ be SomeOperation(*""*). + 1. Return _x_. + + + `); + }); + + it('increment', async () => { + await assertLint( + positioned` + 1. Let _k_ be 0. + 1. ${M}Increment _k_. + `, + { + ruleId, + nodeType, + message: 'prefer "set _x_ to _x_ + 1" over "increment"', + }, + ); + }); + + it('decrement', async () => { + await assertLint( + positioned` + 1. Let _k_ be 0. + 1. ${M}Decrement _k_. + `, + { + ruleId, + nodeType, + message: 'prefer "set _x_ to _x_ - 1" over "decrement"', + }, + ); + }); + + it('increase', async () => { + await assertLint( + positioned` + 1. Let _k_ be 0. + 1. Let _n_ be 1. + 1. ${M}Increase _k_ by _n_. + `, + { + ruleId, + nodeType, + message: 'prefer "set _x_ to _x_ + _y_" over "increase _x_"', + }, + ); + }); + + it('decrease', async () => { + await assertLint( + positioned` + 1. Let _k_ be 0. + 1. Let _n_ be 1. + 1. ${M}Decrease _k_ by _n_. + `, + { + ruleId, + nodeType, + message: 'prefer "set _x_ to _x_ - _y_" over "decrease _x_"', + }, + ); + }); + + it('is an element of', async () => { + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. Let _list_ be a new empty List. + 1. If _x_ ${M}is an element of _list_, return _x_. + `, + { + ruleId, + nodeType, + message: 'prefer "_list_ contains _element_" over "_element_ is an element of _list_"', + }, + ); + }); + + it('is present', async () => { + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. If _x_.[[Foo]] ${M}is present, return _x_. + `, + { + ruleId, + nodeType, + message: 'prefer "_record_ has a [[Field]] field" over "_record_.[[Field]] is present"', + }, + ); + }); + + it('[[Type]] is', async () => { + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. If _x_.[[Type]] ${M}is ~throw~, return _x_. + `, + { + ruleId, + nodeType, + message: + 'prefer "is a throw completion", "is a normal completion", etc. over accessing [[Type]]', + }, + ); + }); + + it('is either null or undefined', async () => { + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. If _x_ ${M}is either null or undefined, return _x_. + `, + { + ruleId, + nodeType, + message: 'prefer "is either undefined or null" for consistency', + }, + ); + }); + + it('is that of', async () => { + await assertLint( + positioned` + 1. Let _list_ be a new empty List. + 1. Let _y_ be a List whose length ${M}is that of _list_. + 1. Return _y_. + `, + { + ruleId, + nodeType, + message: 'prefer "whose _ is _" over "whose _ is that of _"', + }, + ); + }); + + it('are both', async () => { + await assertLint( + positioned` + 1. Let _a_ be *true*. + 1. Let _b_ be *true*. + 1. If _a_ and _b_ ${M}are both *true*, return _a_. + `, + { + ruleId, + nodeType, + message: 'prefer "if _a_ is _c_ and _b_ is _c_" over "if _a_ and _b_ are both _c_"', + }, + ); + }); + + it('or if / and if', async () => { + await assertLint( + positioned` + 1. Let _a_ be *true*. + 1. Let _b_ be *true*. + 1. If _a_ is *true*${M} or if _b_ is *true*, return _a_. + `, + { + ruleId, + nodeType, + message: 'prefer "if _a_ or _b_" over "if _a_ or if _b_"', + }, + ); + + await assertLint( + positioned` + 1. Let _a_ be *true*. + 1. Let _b_ be *true*. + 1. If _a_ is *true*${M} and if _b_ is *true*, return _a_. + `, + { + ruleId, + nodeType, + message: 'prefer "if _a_ and _b_" over "if _a_ and if _b_"', + }, + ); + }); + + it('or if / and if with mixed connectives is allowed', async () => { + await assertLintFree(` + + 1. Let _a_ be *true*. + 1. Let _b_ be *true*. + 1. Let _c_ be *true*. + 1. If _a_ is *true* and _b_ is *true* or if _c_ is *true*, return _a_. + 1. If _a_ is *true* or _b_ is *true* and if _c_ is *true*, return _a_. + 1. If _a_ is *true*, or if _b_ is *true* and _c_ is *true*, return _a_. + + `); + }); + + it('a Type value', async () => { + await assertLint( + positioned` + 1. Let _x_ be ${M}a String value. + 1. Return _x_. + `, + { + ruleId, + nodeType, + message: + 'do not say "value" after the name of a spec type when following a determiner: prefer "a String" over "a String value"', + }, + ); + + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. If _x_ is ${M}a Number value, return _x_. + `, + { + ruleId, + nodeType, + message: + 'do not say "value" after the name of a spec type when following a determiner: prefer "a String" over "a String value"', + }, + ); + + await assertLint( + positioned` + 1. Let _x_ be ${M}a List value. + 1. Return _x_. + `, + { + ruleId, + nodeType, + message: + 'do not say "value" after the name of a spec type when following a determiner: prefer "a String" over "a String value"', + }, + ); + + await assertLint( + positioned` + 1. Let _x_ be ${M}a Completion Record value. + 1. Return _x_. + `, + { + ruleId, + nodeType, + message: + 'do not say "value" after the name of a spec type when following a determiner: prefer "a String" over "a String value"', + }, + ); + + await assertLint( + positioned` + 1. Let _x_ be ${M}a Property Descriptor value. + 1. Return _x_. + `, + { + ruleId, + nodeType, + message: + 'do not say "value" after the name of a spec type when following a determiner: prefer "a String" over "a String value"', + }, + ); + }); + + it('NOTE and Assert steps are excluded', async () => { + // these steps would trigger lints without the NOTE:/Assert: prefix + await assertLintFree(` + + 1. Let _x_ be *"foo"*. + 1. Let _list_ be a new empty List. + 1. NOTE: _x_ is an element of _list_. + 1. Assert: _x_ is either null or undefined. + 1. Return _x_. + + `); + + // verify the same text does trigger lints without the prefix + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. Let _list_ be a new empty List. + 1. If _x_ ${M}is an element of _list_, return _x_. + `, + { + ruleId, + nodeType, + message: 'prefer "_list_ contains _element_" over "_element_ is an element of _list_"', + }, + ); + + await assertLint( + positioned` + 1. Let _x_ be *"foo"*. + 1. If _x_ ${M}is either null or undefined, return _x_. + `, + { + ruleId, + nodeType, + message: 'prefer "is either undefined or null" for consistency', + }, + ); + }); + + it('negative', async () => { + await assertLintFree(` + + 1. Let _x_ be *"foo"*. + 1. Let _list_ be a new empty List. + 1. Let _n_ be 1. + 1. Let _a_ be *true*. + 1. Let _b_ be *false*. + 1. If _x_ is the empty String, return _x_. + 1. Set _x_ to _x_ + 1. + 1. If _list_ contains _x_, return _x_. + 1. If _x_ has a [[Foo]] field, return _x_. + 1. If _x_ is a throw completion, return _x_. + 1. If _x_ is either undefined or null, return _x_. + 1. Let _y_ be a List whose length is _n_. + 1. If _a_ is *true* and _b_ is *true*, return _a_. + 1. If _a_ is *true* or _b_ is *true*, return _a_. + 1. If _a_ is a String, return _a_. + 1. Let _z_ be a Number. + 1. Return « _y_, _z_ ». + + `); + }); +}); diff --git a/test/lint-variable-use-def.ts b/test/lint-variable-use-def.ts index cf539786..2afa8069 100644 --- a/test/lint-variable-use-def.ts +++ b/test/lint-variable-use-def.ts @@ -317,7 +317,7 @@ describe('variables are declared and used appropriately', () => {
a Declarative Environment Record _envRec_
- 1. If _envRec_ has a binding for the name that is the value of _N_, return *true*. + 1. If _envRec_ has a binding for _N_, return *true*. 1. Return *false*.