TypeScript linter: a warning for logic that is not in a primitive (CUP-I008)

packages/codeupipe-lint-ts/README.md

@@ -16,6 +16,7 @@ from this repository.
 npx codeupipe-lint src                  # files or directories
 npx codeupipe-lint --exclude '*/generated/*' src
 npx codeupipe-lint --disable CUP-I005 src
+npx codeupipe-lint --strict src         # a warning fails the run too
 npx codeupipe-lint --rules              # list the rule ids
 npx codeupipe-lint                      # paths come from cup.config.json
 ```
@@ -50,6 +51,8 @@ The ids are those of the Python linter, where it has the rule.
 | CUP-I005 | one-primitive-per-file | a second primitive class at the top level of a file |
 | CUP-I006 | primitive-file-name | the file name is not the class name in kebab-case |
 | CUP-I007 | primitive-role-suffix | the class name does not end with its role |
+| CUP-I008 | logic-outside-primitive | **a warning.** A function, a class or the code at the top of a file has logic (`if`, `? :`, `switch`, a loop, `try`) and is not a primitive |
+| CUP-I009 | marker-allows-nothing | **a warning.** A `cup: allow-...` comment that allowed no finding: the code that it was for is gone or has moved, or its kind has a spelling error |
 
 A step is the argument of `addFilter`, `addTap` or `addPipeline` (on any
 receiver), and the two arguments of `addValve(predicate, filter)`.
@@ -58,6 +61,30 @@ A class is a primitive by its shape: `extends Pipeline`, `extends Hook`,
 `implements Filter`, `implements Tap`, `implements Predicate`, or a method
 `call`, `observe` or `test` with exactly one required parameter.
 
+## Logic that is not in a primitive (CUP-I008)
+
+A project that uses codeupipe writes its logic as Filters, Predicates, Taps,
+Hooks and Pipelines. Logic in a plain function or a plain class works as
+well, so nothing else shows the mistake. This rule shows it.
+
+```
+path:line:col  CUP-I008 logic-outside-primitive  warning: Function `whoIsDue` has logic and is not a Filter, ...
+```
+
+- **It is a warning.** It is in the report and it is counted apart
+  (`No findings. 3 warnings.`). It does not make the exit code 1. With
+  `--strict`, a warning fails the run as a finding does.
+- **Flagged, once each:** a function at the top of a file, a class at the
+  top of a file that is not a primitive, and the code at the top of a file.
+- **Not flagged:** a primitive, a function or a class with no logic, a type,
+  and a test file.
+- **Not seen:** a loop that is a method call (`map`, `filter`, `forEach`),
+  and `&&` / `||`.
+- For logic that must be plain code (the edge to a model server, the entry
+  of a program), say why in a comment: `// cup: allow-plain <reason>`.
+
+The Python linter has this rule too.
+
 ## Rules of the Python linter that are not here
 
 - **CUP-I001** (a change to the Payload behind its API). The data of a
@@ -69,8 +96,9 @@ A class is a primitive by its shape: `extends Pipeline`, `extends Hook`,
 - **One line.** A comment on the line of the finding, on the line of the
   `add...` call around it, or alone on the line directly above:
   `// cup: allow-anonymous` for a CUP-A finding;
-  `// cup: allow-state-read`, `allow-outside-write`, `allow-shared-file` or
-  `allow-name` for a CUP-I finding. Any text can follow the marker.
+  `// cup: allow-state-read`, `allow-outside-write`, `allow-shared-file`,
+  `allow-name` or `allow-plain` for a CUP-I finding. Any text can follow the marker.
+  A comment that allows no finding is itself a finding (CUP-I009), so an old one does not stay.
 - **Files.** `--exclude GLOB`, or `lint.exclude` in the config file. In a
   glob, `*` is any text, a `/` too.
 - **A rule, for the project.** `--disable RULE`, or `lint.disable` in the

packages/codeupipe-lint-ts/src/analyze-plain-logic-filter.ts

@@ -0,0 +1,92 @@
+import type { Filter, Payload } from "codeupipe";
+import ts from "typescript";
+
+import type { Finding } from "./finding.ts";
+import { isTestFile, primitiveKind } from "./primitive-kind.ts";
+import { LOGIC_OUTSIDE_PRIMITIVE, makeFinding } from "./rules.ts";
+import type { Source } from "./source.ts";
+
+/** A decision or a loop: the logic that a pipeline is for. */
+function isLogic(node: ts.Node): boolean {
+  return ts.isIfStatement(node) || ts.isConditionalExpression(node) || ts.isSwitchStatement(node)
+    || ts.isForStatement(node) || ts.isForInStatement(node) || ts.isForOfStatement(node)
+    || ts.isWhileStatement(node) || ts.isDoStatement(node) || ts.isTryStatement(node);
+}
+
+function hasLogic(node: ts.Node): boolean {
+  let found = false;
+  const visit = (inner: ts.Node) => {
+    if (found) return;
+    if (isLogic(inner)) {
+      found = true;
+      return;
+    }
+    ts.forEachChild(inner, visit);
+  };
+  visit(node);
+  return found;
+}
+
+const WHAT = "has logic and is not a Filter, a Predicate, a Tap, a Hook or a Pipeline";
+
+/**
+ * CUP-I008, a warning: logic that is not in a primitive.
+ *
+ * A project that uses codeupipe writes its logic as Filters, Predicates,
+ * Taps, Hooks and Pipelines. Logic in a plain function or a plain class works
+ * as well, so nothing else shows the mistake. This rule shows it.
+ *
+ * Logic is a decision or a loop: `if`, `? :`, `switch`, `for`, `while`,
+ * `do`, `try`. Flagged, once each: a function at the top of a file, a class
+ * at the top of a file that is not a primitive, and the code at the top of a
+ * file itself. Not flagged: a primitive (its logic is where it must be), a
+ * function or a class with no logic, a type, and a test file. Not seen: a
+ * loop that is a method call (`map`, `filter`, `forEach`), and `&&` / `||`.
+ *
+ * Input keys: `sources`, `findings`.
+ * Output keys: `findings`, with CUP-I008 warnings appended.
+ */
+export class AnalyzePlainLogicFilter implements Filter {
+  call(payload: Payload): Payload {
+    const findings = [...(payload.get<Finding[]>("findings") ?? [])];
+    for (const source of payload.get<Source[]>("sources") ?? []) {
+      if (isTestFile(source.path)) continue;
+      let moduleCode: ts.Node | undefined;
+      for (const statement of source.tree.statements) {
+        if (ts.isFunctionDeclaration(statement)) {
+          if (statement.body !== undefined && hasLogic(statement.body)) {
+            findings.push(makeFinding(source, statement.name ?? statement, LOGIC_OUTSIDE_PRIMITIVE,
+              `Function \`${statement.name?.text ?? "default"}\` ${WHAT}`));
+          }
+        } else if (ts.isClassDeclaration(statement)) {
+          if (primitiveKind(statement) === undefined && hasLogic(statement)) {
+            findings.push(makeFinding(source, statement.name ?? statement, LOGIC_OUTSIDE_PRIMITIVE,
+              `Class \`${statement.name?.text ?? "default"}\` ${WHAT}`));
+          }
+        } else if (ts.isVariableStatement(statement)) {
+          for (const declaration of statement.declarationList.declarations) {
+            const value = declaration.initializer;
+            if (value === undefined) continue;
+            if (ts.isArrowFunction(value) || ts.isFunctionExpression(value)) {
+              if (hasLogic(value.body)) {
+                findings.push(makeFinding(source, declaration.name, LOGIC_OUTSIDE_PRIMITIVE,
+                  `Function \`${declaration.name.getText()}\` ${WHAT}`));
+              }
+            } else if (moduleCode === undefined && hasLogic(value)) {
+              moduleCode = declaration;
+            }
+          }
+        } else if (ts.isInterfaceDeclaration(statement) || ts.isTypeAliasDeclaration(statement)
+          || ts.isImportDeclaration(statement) || ts.isExportDeclaration(statement)) {
+          // no logic
+        } else if (moduleCode === undefined && (isLogic(statement) || hasLogic(statement))) {
+          moduleCode = statement;
+        }
+      }
+      if (moduleCode !== undefined) {
+        findings.push(makeFinding(source, moduleCode, LOGIC_OUTSIDE_PRIMITIVE, `The code at the top of this file ${WHAT}`));
+      }
+    }
+    return payload.insert("findings", findings);
+  }
+}

packages/codeupipe-lint-ts/src/analyze-stale-markers-filter.ts

@@ -0,0 +1,67 @@
+import type { Filter, Payload } from "codeupipe";
+import ts from "typescript";
+
+import type { Finding } from "./finding.ts";
+import { fixOf, MARKER_ALLOWS_NOTHING, RULES } from "./rules.ts";
+import type { Source } from "./source.ts";
+
+const MARKER = /^cup:\s*(allow-[\w-]*)/;
+
+/**
+ * Finds each `cup: allow-...` comment that allowed no finding (CUP-I009, a
+ * warning). Such a comment stays when the code that it was for is gone or
+ * has moved, or when its kind has a spelling error.
+ *
+ * Only a comment that starts with the marker counts, so text that names a
+ * marker in a string or in the middle of a sentence is not reported.
+ *
+ * Input keys: `findings`, `sources`, `usedMarkers` (from ApplySuppressionsFilter).
+ * Output keys: `findings`.
+ */
+export class AnalyzeStaleMarkersFilter implements Filter {
+  call(payload: Payload): Payload {
+    const used = new Set(payload.get<string[]>("usedMarkers") ?? []);
+    const findings = [...(payload.get<Finding[]>("findings") ?? [])];
+    for (const source of payload.get<Source[]>("sources") ?? []) {
+      for (const marker of this.markersOf(source)) {
+        if (used.has(`${source.path}:${marker.line}`)) continue;
+        findings.push({
+          path: source.path, line: marker.line, col: marker.col, rule: MARKER_ALLOWS_NOTHING, slug: RULES[MARKER_ALLOWS_NOTHING] ?? "",
+          message: `The comment \`cup: ${marker.kind}\` allows nothing. ${fixOf(MARKER_ALLOWS_NOTHING)}`, severity: "warning",
+        });
+      }
+    }
+    findings.sort((a, b) => a.path.localeCompare(b.path) || a.line - b.line || a.col - b.col);
+    return payload.insert("findings", findings);
+  }
+
+  /** Each line of a comment of the file that starts with a marker. */
+  markersOf(source: Source): Array<{ line: number; col: number; kind: string }> {
+    const ranges = new Map<number, ts.CommentRange>();
+    const visit = (node: ts.Node) => {
+      const found = [
+        ...(ts.getLeadingCommentRanges(source.text, node.getFullStart()) ?? []),
+        ...(ts.getTrailingCommentRanges(source.text, node.getEnd()) ?? []),
+      ];
+      for (const range of found) ranges.set(range.pos, range);
+      // The text of a string or a template can look like a comment. It has no tokens to look in.
+      if (!ts.isStringLiteralLike(node) && !ts.isTemplateExpression(node)) node.getChildren(source.tree).forEach(visit);
+    };
+    visit(source.tree);
+
+    const markers: Array<{ line: number; col: number; kind: string }> = [];
+    for (const range of [...ranges.values()].sort((a, b) => a.pos - b.pos)) {
+      let at = range.pos;
+      for (const text of source.text.slice(range.pos, range.end).split("\n")) {
+        const lead = /^\s*(\/\/+|\/\*+|\*(?!\/))?\s*/.exec(text)?.[0].length ?? 0;
+        const kind = MARKER.exec(text.slice(lead))?.[1];
+        if (kind !== undefined) {
+          const place = source.tree.getLineAndCharacterOfPosition(at + lead);
+          markers.push({ line: place.line + 1, col: place.character + 1, kind });
+        }
+        at += text.length + 1;
+      }
+    }
+    return markers;
+  }
+}

packages/codeupipe-lint-ts/src/apply-suppressions-filter.ts

@@ -4,20 +4,21 @@ import type { Finding } from "./finding.ts";
 import type { Source } from "./source.ts";
 
 const ANONYMOUS = /cup:\s*allow-anonymous\b/;
-const INVARIANT = /cup:\s*allow-(state-read|outside-write|shared-file|name)\b/;
+const INVARIANT = /cup:\s*allow-(state-read|outside-write|shared-file|name|plain)\b/;
 const COMMENT_ONLY = /^\s*(\/\/|\/\*|\*)/;
 
 /**
  * Drops the findings that a comment allows.
  *
  * `// cup: allow-anonymous` allows a CUP-A finding. `// cup: allow-state-read`,
- * `allow-outside-write`, `allow-shared-file` and `allow-name` allow a CUP-I
- * finding. The comment is on the line of the finding, on the line of the
+ * `allow-outside-write`, `allow-shared-file`, `allow-name` and `allow-plain`
+ * allow a CUP-I finding. The comment is on the line of the finding, on the line of the
  * `add...` call around it, or alone on the line directly above. Any text can
  * follow the marker.
  *
  * Input keys: `findings`, `sources`.
- * Output keys: `findings` (kept), `suppressed` (dropped).
+ * Output keys: `findings` (kept), `suppressed` (dropped), `usedMarkers` (the
+ * place of each comment that allowed a finding, as `path:line`).
  */
 export class ApplySuppressionsFilter implements Filter {
   call(payload: Payload): Payload {
@@ -27,15 +28,17 @@ export class ApplySuppressionsFilter implements Filter {
     }
     const kept: Finding[] = [];
     const suppressed: Finding[] = [];
+    const used = new Set<string>();
     for (const finding of payload.get<Finding[]>("findings") ?? []) {
       const marker = finding.rule.startsWith("CUP-A") ? ANONYMOUS : INVARIANT;
       const text = lines.get(finding.path) ?? [];
       const line = (number: number | undefined) => (number === undefined ? "" : text[number - 1] ?? "");
       const above = line(finding.line - 1);
-      const allowed = marker.test(line(finding.line)) || marker.test(line(finding.callLine))
-        || (COMMENT_ONLY.test(above) && marker.test(above));
-      (allowed ? suppressed : kept).push(finding);
+      const places = [finding.line, finding.callLine].filter((number) => marker.test(line(number)));
+      if (COMMENT_ONLY.test(above) && marker.test(above)) places.push(finding.line - 1);
+      for (const place of places) used.add(`${finding.path}:${place}`);
+      (places.length > 0 ? suppressed : kept).push(finding);
     }
-    return payload.insert("findings", kept).insert("suppressed", suppressed);
+    return payload.insert("findings", kept).insert("suppressed", suppressed).insert("usedMarkers", [...used]);
   }
 }

packages/codeupipe-lint-ts/src/cli.ts

@@ -1,9 +1,10 @@
 #!/usr/bin/env node
 /**
- * codeupipe-lint [--exclude GLOB] [--disable RULE] [--config FILE] [--rules] [paths...]
+ * codeupipe-lint [--exclude GLOB] [--disable RULE] [--strict] [--config FILE] [--rules] [paths...]
  *
  * With no paths, they come from cup.config.json. Exit code: 0 no findings,
  * 1 findings, 2 a path could not be read or the arguments cannot be used.
+ * A warning does not make the exit code 1, unless --strict is given.
  */
 import { parseArgs } from "node:util";
 
@@ -21,6 +22,7 @@ async function main(argv: string[]): Promise<number> {
         disable: { type: "string", multiple: true },
         config: { type: "string" },
         rules: { type: "boolean" },
+        strict: { type: "boolean" },
       },
     });
   } catch (error) {
@@ -58,7 +60,7 @@ async function main(argv: string[]): Promise<number> {
     return 2;
   }
 
-  const out = await lintPaths(paths, { exclude, disable });
+  const out = await lintPaths(paths, { exclude, disable, strict: args.values.strict === true });
   console.log(out.get<string>("report"));
   const errorReport = out.get<string>("errorReport");
   if (errorReport) console.error(errorReport);

packages/codeupipe-lint-ts/src/drop-disabled-filter.ts

@@ -3,15 +3,17 @@ import type { Filter, Payload } from "codeupipe";
 import type { Finding } from "./finding.ts";
 
 /**
- * Drops the findings of the rules that the project switched off.
+ * Drops the findings of the rules that the project switched off, from those
+ * that are reported and from those that a comment allowed.
  *
- * Input keys: `findings`, optional `disable` (rule ids).
- * Output keys: `findings`.
+ * Input keys: `findings`, `suppressed`, optional `disable` (rule ids).
+ * Output keys: `findings`, `suppressed`.
  */
 export class DropDisabledFilter implements Filter {
   call(payload: Payload): Payload {
     const disabled = new Set(payload.get<string[]>("disable") ?? []);
-    const findings = payload.get<Finding[]>("findings") ?? [];
-    return payload.insert("findings", findings.filter((finding) => !disabled.has(finding.rule)));
+    const on = (finding: Finding) => !disabled.has(finding.rule);
+    return payload.insert("findings", (payload.get<Finding[]>("findings") ?? []).filter(on))
+      .insert("suppressed", (payload.get<Finding[]>("suppressed") ?? []).filter(on));
   }
 }

packages/codeupipe-lint-ts/src/finding.ts

@@ -12,8 +12,11 @@ export type Finding = {
   slug: string;
   message: string;
   callLine?: number;
+  /** A warning is reported and does not fail the run, unless the run is strict. */
+  severity?: "warning";
 };
 
 export function formatFinding(finding: Finding): string {
-  return `${finding.path}:${finding.line}:${finding.col}  ${finding.rule} ${finding.slug}  ${finding.message}`;
+  const warning = finding.severity === "warning" ? "warning: " : "";
+  return `${finding.path}:${finding.line}:${finding.col}  ${finding.rule} ${finding.slug}  ${warning}${finding.message}`;
 }

packages/codeupipe-lint-ts/src/format-report-filter.ts

@@ -6,10 +6,13 @@ import type { Finding } from "./finding.ts";
 /**
  * The findings as sorted report lines, and the exit code.
  *
- * Input keys: `findings`, `suppressed`, `errors`.
+ * Input keys: `findings`, `suppressed`, `errors`, optional `strict`.
  * Output keys: `findings` (sorted), `report` (text for stdout),
  * `errorReport` (text for stderr), `exitCode`: 0 no findings, 1 findings,
  * 2 a path could not be read, so the check is not complete.
+ *
+ * A warning is in the report and is counted apart. It does not make the
+ * exit code 1, unless `strict` is true.
  */
 export class FormatReportFilter implements Filter {
   call(payload: Payload): Payload {
@@ -19,11 +22,16 @@ export class FormatReportFilter implements Filter {
     const suppressed = payload.get<Finding[]>("suppressed") ?? [];
     const errors = payload.get<string[]>("errors") ?? [];
     const lines = findings.map(formatFinding);
-    let summary = findings.length === 0 ? "No findings" : `${findings.length} ${findings.length === 1 ? "finding" : "findings"}`;
+    const warnings = findings.filter((finding) => finding.severity === "warning").length;
+    const real = findings.length - warnings;
+    let summary = real === 0 ? "No findings" : `${real} ${real === 1 ? "finding" : "findings"}`;
     if (suppressed.length > 0) summary += ` (${suppressed.length} suppressed)`;
-    lines.push(summary + ".");
+    summary += ".";
+    if (warnings > 0) summary += ` ${warnings} ${warnings === 1 ? "warning" : "warnings"}.`;
+    lines.push(summary);
+    const fails = real > 0 || (warnings > 0 && payload.get("strict") === true);
     return payload.insert("findings", findings).insert("report", lines.join("\n"))
       .insert("errorReport", errors.map((error) => "error: " + error).join("\n"))
-      .insert("exitCode", errors.length > 0 ? 2 : findings.length > 0 ? 1 : 0);
+      .insert("exitCode", errors.length > 0 ? 2 : fails ? 1 : 0);
   }
 }

packages/codeupipe-lint-ts/src/lint-paths.ts

@@ -6,8 +6,8 @@ import { LintPipeline } from "./lint-pipeline.ts";
  * Run the linter over `paths`. The Payload that comes back holds `findings`,
  * `suppressed`, `errors`, `report`, `errorReport` and `exitCode`.
  */
-export function lintPaths(paths: string[], options: { exclude?: string[]; disable?: string[] } = {}): Promise<Payload> {
+export function lintPaths(paths: string[], options: { exclude?: string[]; disable?: string[]; strict?: boolean } = {}): Promise<Payload> {
   return new LintPipeline().run(new Payload({
-    paths, findings: [], errors: [], exclude: options.exclude ?? [], disable: options.disable ?? [],
+    paths, findings: [], errors: [], exclude: options.exclude ?? [], disable: options.disable ?? [], strict: options.strict === true,
   }));
 }