diff --git a/packages/codeupipe-lint-ts/README.md b/packages/codeupipe-lint-ts/README.md index 4573218..15d02d8 100644 --- a/packages/codeupipe-lint-ts/README.md +++ b/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 `. + +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 diff --git a/packages/codeupipe-lint-ts/src/analyze-plain-logic-filter.ts b/packages/codeupipe-lint-ts/src/analyze-plain-logic-filter.ts new file mode 100644 index 0000000..f037ef8 --- /dev/null +++ b/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("findings") ?? [])]; + for (const source of payload.get("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); + } +} diff --git a/packages/codeupipe-lint-ts/src/analyze-stale-markers-filter.ts b/packages/codeupipe-lint-ts/src/analyze-stale-markers-filter.ts new file mode 100644 index 0000000..b162ef9 --- /dev/null +++ b/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("usedMarkers") ?? []); + const findings = [...(payload.get("findings") ?? [])]; + for (const source of payload.get("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(); + 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; + } +} diff --git a/packages/codeupipe-lint-ts/src/apply-suppressions-filter.ts b/packages/codeupipe-lint-ts/src/apply-suppressions-filter.ts index fc00328..859714f 100644 --- a/packages/codeupipe-lint-ts/src/apply-suppressions-filter.ts +++ b/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(); for (const finding of payload.get("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]); } } diff --git a/packages/codeupipe-lint-ts/src/cli.ts b/packages/codeupipe-lint-ts/src/cli.ts index 6b8efb4..c85812f 100644 --- a/packages/codeupipe-lint-ts/src/cli.ts +++ b/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 { 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 { 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("report")); const errorReport = out.get("errorReport"); if (errorReport) console.error(errorReport); diff --git a/packages/codeupipe-lint-ts/src/drop-disabled-filter.ts b/packages/codeupipe-lint-ts/src/drop-disabled-filter.ts index b99f6ce..3b998b0 100644 --- a/packages/codeupipe-lint-ts/src/drop-disabled-filter.ts +++ b/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("disable") ?? []); - const findings = payload.get("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("findings") ?? []).filter(on)) + .insert("suppressed", (payload.get("suppressed") ?? []).filter(on)); } } diff --git a/packages/codeupipe-lint-ts/src/finding.ts b/packages/codeupipe-lint-ts/src/finding.ts index d4e3088..7bfb50e 100644 --- a/packages/codeupipe-lint-ts/src/finding.ts +++ b/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}`; } diff --git a/packages/codeupipe-lint-ts/src/format-report-filter.ts b/packages/codeupipe-lint-ts/src/format-report-filter.ts index 43522f4..5c6c2ec 100644 --- a/packages/codeupipe-lint-ts/src/format-report-filter.ts +++ b/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("suppressed") ?? []; const errors = payload.get("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); } } diff --git a/packages/codeupipe-lint-ts/src/lint-paths.ts b/packages/codeupipe-lint-ts/src/lint-paths.ts index a6415bb..f5af58f 100644 --- a/packages/codeupipe-lint-ts/src/lint-paths.ts +++ b/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 { +export function lintPaths(paths: string[], options: { exclude?: string[]; disable?: string[]; strict?: boolean } = {}): Promise { 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, })); } diff --git a/packages/codeupipe-lint-ts/src/lint-pipeline.ts b/packages/codeupipe-lint-ts/src/lint-pipeline.ts index f472e70..dc3002f 100644 --- a/packages/codeupipe-lint-ts/src/lint-pipeline.ts +++ b/packages/codeupipe-lint-ts/src/lint-pipeline.ts @@ -1,7 +1,9 @@ import { Pipeline } from "codeupipe"; import { AnalyzeOutsideWritesFilter } from "./analyze-outside-writes-filter.ts"; +import { AnalyzePlainLogicFilter } from "./analyze-plain-logic-filter.ts"; import { AnalyzePrimitiveNamesFilter } from "./analyze-primitive-names-filter.ts"; +import { AnalyzeStaleMarkersFilter } from "./analyze-stale-markers-filter.ts"; import { AnalyzePrimitivesPerFileFilter } from "./analyze-primitives-per-file-filter.ts"; import { AnalyzeStateReadsFilter } from "./analyze-state-reads-filter.ts"; import { AnalyzeStepsFilter } from "./analyze-steps-filter.ts"; @@ -22,8 +24,10 @@ export class LintPipeline extends Pipeline { this.addFilter(new AnalyzeOutsideWritesFilter()); // CUP-I004 this.addFilter(new AnalyzePrimitivesPerFileFilter()); // CUP-I005 this.addFilter(new AnalyzePrimitiveNamesFilter()); // CUP-I006, CUP-I007 - this.addFilter(new DropDisabledFilter()); + this.addFilter(new AnalyzePlainLogicFilter()); // CUP-I008, a warning this.addFilter(new ApplySuppressionsFilter()); + this.addFilter(new AnalyzeStaleMarkersFilter()); // CUP-I009, a warning + this.addFilter(new DropDisabledFilter()); // last, so that a comment for a rule that is off is not stale this.addFilter(new FormatReportFilter()); } } diff --git a/packages/codeupipe-lint-ts/src/rules.ts b/packages/codeupipe-lint-ts/src/rules.ts index af7d9d5..f8e15f3 100644 --- a/packages/codeupipe-lint-ts/src/rules.ts +++ b/packages/codeupipe-lint-ts/src/rules.ts @@ -15,6 +15,11 @@ export const WRITE_OUTSIDE_FILTER = "CUP-I004"; export const ONE_PRIMITIVE_PER_FILE = "CUP-I005"; export const PRIMITIVE_FILE_NAME = "CUP-I006"; export const PRIMITIVE_ROLE_SUFFIX = "CUP-I007"; +export const LOGIC_OUTSIDE_PRIMITIVE = "CUP-I008"; +export const MARKER_ALLOWS_NOTHING = "CUP-I009"; + +/** The rules whose findings are warnings. */ +export const WARNINGS = new Set([LOGIC_OUTSIDE_PRIMITIVE, MARKER_ALLOWS_NOTHING]); /** Rule id to its short name. The ids and names are those of the Python linter, where it has the rule. */ export const RULES: Record = { @@ -30,6 +35,8 @@ export const RULES: Record = { [ONE_PRIMITIVE_PER_FILE]: "one-primitive-per-file", [PRIMITIVE_FILE_NAME]: "primitive-file-name", [PRIMITIVE_ROLE_SUFFIX]: "primitive-role-suffix", + [LOGIC_OUTSIDE_PRIMITIVE]: "logic-outside-primitive", + [MARKER_ALLOWS_NOTHING]: "marker-allows-nothing", }; const NAMED_CLASS = "Define it at module level as a named class (a Valve predicate: a class with test(payload)) " @@ -54,12 +61,22 @@ const FIX: Record = { [PRIMITIVE_FILE_NAME]: "The file name of a primitive is its class name in kebab-case, so one name gives the other.", [PRIMITIVE_ROLE_SUFFIX]: "A primitive's class name ends with its role (Filter, Tap, Hook, Predicate, " + "Pipeline), so the name tells what the class is.", + [LOGIC_OUTSIDE_PRIMITIVE]: "Write it as a primitive, so that the pipeline has the logic and it can be tested as a step. " + + "For logic that must be plain code, say why: // cup: allow-plain ", + [MARKER_ALLOWS_NOTHING]: "Remove it, or put it on the line of the finding that it is for. A comment that allows nothing " + + "says that the code has an exception that it does not have.", }; +/** What to do about a finding of `rule`. */ +export function fixOf(rule: string): string { + return FIX[rule] ?? ""; +} + /** A Finding at `node`. Its message reads `. `. */ export function makeFinding(source: Source, node: ts.Node, rule: string, what: string, callNode?: ts.Node): Finding { const { line, col } = position(source, node); const finding: Finding = { path: source.path, line, col, rule, slug: RULES[rule] ?? "", message: `${what}. ${FIX[rule]}` }; + if (WARNINGS.has(rule)) finding.severity = "warning"; if (callNode !== undefined) { const callLine = position(source, callNode).line; if (callLine !== line) finding.callLine = callLine; diff --git a/packages/codeupipe-lint-ts/tests/cli.test.ts b/packages/codeupipe-lint-ts/tests/cli.test.ts index 20d530c..e561683 100644 --- a/packages/codeupipe-lint-ts/tests/cli.test.ts +++ b/packages/codeupipe-lint-ts/tests/cli.test.ts @@ -77,6 +77,7 @@ test("the linter is clean on the TypeScript code of this repository", () => { const here = join(import.meta.dirname, ".."); const done = run(here, "src", "../codeupipe-ts/src", "../codeupipe-ts/examples", "../codeupipe-compile-ts/src", "../../conformance/runners/typescript"); - assert.equal(done.out.trim(), "No findings."); + // The plain logic of this repository is reported as warnings. A warning does not fail the run. + assert.match(done.out, /^No findings[ .]/m); assert.equal(done.code, 0); }); diff --git a/packages/codeupipe-lint-ts/tests/lint-in.ts b/packages/codeupipe-lint-ts/tests/lint-in.ts index 4101b47..5a1feed 100644 --- a/packages/codeupipe-lint-ts/tests/lint-in.ts +++ b/packages/codeupipe-lint-ts/tests/lint-in.ts @@ -10,7 +10,7 @@ export type Linted = { found: string[]; findings: Finding[]; suppressed: number; /** `found` holds one string for each finding: `file:line rule`. */ export async function lintIn(files: Record, - options: { exclude?: string[]; disable?: string[] } = {}): Promise { + options: { exclude?: string[]; disable?: string[]; strict?: boolean } = {}): Promise { const root = mkdtempSync(join(tmpdir(), "cup-lint-")); for (const [name, text] of Object.entries(files)) { mkdirSync(dirname(join(root, name)), { recursive: true }); diff --git a/packages/codeupipe-lint-ts/tests/plain-logic.test.ts b/packages/codeupipe-lint-ts/tests/plain-logic.test.ts new file mode 100644 index 0000000..a41392f --- /dev/null +++ b/packages/codeupipe-lint-ts/tests/plain-logic.test.ts @@ -0,0 +1,85 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { lintIn } from "./lint-in.ts"; + +const warnings = (out: Awaited>) => + out.findings.filter((finding) => finding.severity === "warning").map((finding) => finding.message.split(" has logic")[0]); + +test("CUP-I008: a function with a decision that is not in a primitive is a warning", async () => { + const out = await lintIn({ + "who-is-due.ts": `export function whoIsDue(entries) { if (entries.length === 0) return "say"; return "model"; } +export const pick = (list) => { for (const each of list) { if (each) return each; } return null; }; +export const choose = (flag) => (flag ? "a" : "b"); +export async function load(path) { try { return await read(path); } catch { return null; } } +export function plain(a, b) { return a + b; } +export const double = (n) => n * 2; +`, + }); + assert.deepEqual(out.found, ["who-is-due.ts:1 CUP-I008", "who-is-due.ts:2 CUP-I008", "who-is-due.ts:3 CUP-I008", "who-is-due.ts:4 CUP-I008"]); + assert.deepEqual(warnings(out), ["Function `whoIsDue`", "Function `pick`", "Function `choose`", "Function `load`"]); + assert.match(out.findings[0]?.message ?? "", /is not a Filter, a Predicate, a Tap, a Hook or a Pipeline\. Write it as a primitive/); +}); + +test("CUP-I008: a class that is not a primitive, and code at the top of a file", async () => { + const out = await lintIn({ + "record-store.ts": `export class RecordStore { + list() { return this.files.filter((file) => file.endsWith(".jsonl")); } + read(id) { if (!this.has(id)) throw new Error("none"); return []; } +} +export class Point { constructor(x) { this.x = x; } get double() { return this.x * 2; } } +`, + "main.ts": `import { run } from "./run.ts"; +const args = process.argv.slice(2); +if (args.length === 0) process.exit(2); +for (const arg of args) run(arg); +`, + }); + assert.deepEqual(out.found, ["main.ts:3 CUP-I008", "record-store.ts:1 CUP-I008"]); + assert.deepEqual(warnings(out), ["The code at the top of this file", "Class `RecordStore`"]); +}); + +test("CUP-I008: logic in a primitive is where it must be, and types have no logic", async () => { + const out = await lintIn({ + "pick-filter.ts": `export class PickFilter { call(payload) { if (payload.has("a")) return payload.insert("b", 1); for (const k of []) {} return payload; } }\n`, + "is-on-predicate.ts": `export class IsOnPredicate { test(payload) { return payload.get("on") ? true : false; } }\n`, + "see-tap.ts": `export class SeeTap { observe(payload) { if (payload.has("x")) console.log(1); } }\n`, + "log-hook.ts": `import { Hook } from "codeupipe";\nexport class LogHook extends Hook { override before(step) { if (step) console.log(step); } }\n`, + "main-pipeline.ts": `import { Pipeline } from "codeupipe";\nexport class MainPipeline extends Pipeline { constructor() { super(); if (process.env.X) this.observe(); } }\n`, + "types.ts": `export type Due = "say" | "model";\nexport interface Model { reply(text: string): string }\nexport const LIMIT = 20000;\nexport const NAMES = ["a", "b"];\n`, + }); + assert.deepEqual(out.found, []); +}); + +test("a warning does not fail the run, unless the run is strict", async () => { + const files = { "helper.ts": `export function pick(a) { return a ? 1 : 2; }\n` }; + const soft = await lintIn(files); + assert.equal(soft.exitCode, 0); + assert.match(soft.report, /helper\.ts:1:17 CUP-I008 logic-outside-primitive warning: Function `pick` has logic/); + assert.match(soft.report, /^No findings\. 1 warning\.$/m); + const strict = await lintIn(files, { strict: true }); + assert.equal(strict.exitCode, 1); +}); + +test("a warning and a finding are counted apart", async () => { + const out = await lintIn({ "discount.ts": `export class Discount { call(payload) { return payload; } }\nexport function pick(a) { return a ? 1 : 2; }\n` }); + assert.equal(out.exitCode, 1); + assert.match(out.report, /^2 findings\. 1 warning\.$/m); +}); + +test("cup: allow-plain with a reason suppresses the warning, and test files are not checked", async () => { + const out = await lintIn({ + "helper.ts": `// cup: allow-plain a pure helper of two Predicates\nexport function pick(a) { return a ? 1 : 2; }\n`, + "helper.test.ts": `export function pick(a) { return a ? 1 : 2; }\n`, + "tests/support.ts": `export function pick(a) { return a ? 1 : 2; }\n`, + }); + assert.deepEqual(out.found, []); + assert.equal(out.suppressed, 1); + assert.match(out.report, /^No findings \(1 suppressed\)\.$/m); +}); + +test("the rule can be switched off", async () => { + const out = await lintIn({ "helper.ts": `export function pick(a) { return a ? 1 : 2; }\n` }, { disable: ["CUP-I008"] }); + assert.deepEqual(out.found, []); + assert.match(out.report, /^No findings\.$/m); +}); diff --git a/packages/codeupipe-lint-ts/tests/stale-markers.test.ts b/packages/codeupipe-lint-ts/tests/stale-markers.test.ts new file mode 100644 index 0000000..4a329d4 --- /dev/null +++ b/packages/codeupipe-lint-ts/tests/stale-markers.test.ts @@ -0,0 +1,44 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { lintIn } from "./lint-in.ts"; + +test("CUP-I009: a comment that allows nothing is a warning", async () => { + const out = await lintIn({ + "add.ts": `// cup: allow-plain it had a loop before +export function add(a, b) { return a + b; } +export const one = 1; // cup: allow-state-read nothing here reads State +/* + * cup: allow-plian a kind that does not exist + */ +export const two = 2; +`, + }); + assert.deepEqual(out.found, ["add.ts:1 CUP-I009", "add.ts:3 CUP-I009", "add.ts:5 CUP-I009"]); + assert.equal(out.findings[0]?.severity, "warning"); + assert.match(out.findings[0]?.message ?? "", /^The comment `cup: allow-plain` allows nothing\. Remove it/); + assert.equal(out.exitCode, 0); + assert.equal((await lintIn({ "add.ts": "// cup: allow-plain no reason\nexport const one = 1;\n" }, { strict: true })).exitCode, 1); +}); + +test("CUP-I009: a comment that allows a finding is not reported, and text that only names a marker is not a comment", async () => { + const out = await lintIn({ + "pick.ts": `// cup: allow-plain the entry of the program +export function pick(list) { for (const each of list) { if (each) return each; } return null; } +export function choose(flag) { return flag ? "a" : "b"; } // cup: allow-plain a small helper +/** The marker \`// cup: allow-plain\` allows a finding. */ +export const HELP = "say why: // cup: allow-plain "; +export const MORE = \`\${HELP} // cup: allow-plain\`; +`, + }); + assert.deepEqual(out.found, []); + assert.equal(out.suppressed, 2); +}); + +test("CUP-I009: a comment for a rule that is switched off is not reported, and the rule can be switched off", async () => { + const files = { "pick.ts": "// cup: allow-plain the entry\nexport function pick(flag) { return flag ? 1 : 2; }\n" }; + const off = await lintIn(files, { disable: ["CUP-I008"] }); + assert.deepEqual(off.found, []); + assert.equal(off.suppressed, 0); + assert.deepEqual((await lintIn({ "one.ts": "// cup: allow-plain no reason\nexport const one = 1;\n" }, { disable: ["CUP-I009"] })).found, []); +}); diff --git a/packages/codeupipe-lint/README.md b/packages/codeupipe-lint/README.md index 84d53f6..8f2a807 100644 --- a/packages/codeupipe-lint/README.md +++ b/packages/codeupipe-lint/README.md @@ -13,7 +13,7 @@ Each rule can be switched off. There are three ways. | For | How | |---|---| -| One line | A comment on the line: `# cup: allow-anonymous`, `# cup: allow-mutation`, `# cup: allow-state-read`, `# cup: allow-outside-write`, `# cup: allow-shared-file`, `# cup: allow-name` | +| One line | A comment on the line: `# cup: allow-anonymous`, `# cup: allow-mutation`, `# cup: allow-state-read`, `# cup: allow-outside-write`, `# cup: allow-shared-file`, `# cup: allow-name`, `# cup: allow-plain`. A comment that allows no finding is itself a finding (CUP-I009) | | Some files | `--exclude GLOB`, or `lint.exclude` in `cup.config.json` | | The whole project | `--disable RULE`, or `lint.disable` in `cup.config.json` | @@ -124,6 +124,8 @@ a path could not be read. From Python: | CUP-I005 | one-primitive-per-file | a second Filter, Predicate, Tap, Hook or named Pipeline class at module level in one file | | CUP-I006 | primitive-file-name | the file of a primitive is not its class name in snake_case (`DiscountFilter` is in `discount_filter.py`) | | CUP-I007 | primitive-role-suffix | the class name of a primitive does not end with its role: `Filter`, `Tap`, `Hook`, `Predicate` or `Pipeline` | +| CUP-I008 | logic-outside-primitive | **a warning.** A function, a class or the code at module level has logic (`if`, `a if c else b`, `match`, 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 | **Why CUP-I002.** A Payload owns its keys, not its values. `payload.get("items")` returns the live list, shared with every copy of the @@ -159,6 +161,26 @@ the standard style for a Python module, so one name gives the other. A subclass of `Valve` can end with `Valve`. Test files are not checked. Suppress with `cup: allow-name`. +**Why 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. Flagged, once each: +a function at module level, a class at module level that is not a primitive, +and the code at module level. Not flagged: a primitive, a function or a class +with no logic, a test file, and the guards `if __name__ == "__main__":` and +`if TYPE_CHECKING:`. Not seen: a comprehension, and `and` / `or`. For logic +that must be plain code (the edge to another program, the entry of a +program), say why: `# cup: allow-plain `. + +**Warnings.** CUP-I008 and CUP-I009 are warnings. A warning is in the report +and is counted apart (`No invariant violations. 3 warnings.`). It does not +make the exit code 1. With `--strict`, a warning fails the run as a finding +does. + +**Why CUP-I009.** A comment that allows nothing says that the code has an +exception that it does not have. Only a comment that starts with the marker +counts. A `cup: allow-anonymous` comment is for the anonymous-step linter and +is not looked at. + **Why CUP-I003.** A Filter's result and a Valve's decision must depend only on the Payload. A step is never handed State, so the only way to read it is to hold a pipeline or a State, and that makes the step's behaviour depend on diff --git a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/apply_suppressions_filter.py b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/apply_suppressions_filter.py index 860f026..d406663 100644 --- a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/apply_suppressions_filter.py +++ b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/apply_suppressions_filter.py @@ -17,7 +17,8 @@ class ApplySuppressionsFilter: """Filter (sync). Input keys: ``findings``. - Output keys: ``findings`` (kept), ``suppressed`` (dropped). + Output keys: ``findings`` (kept), ``suppressed`` (dropped), ``used_markers`` + (the place of each comment that allowed a finding, as ``(path, line)``). ``marker`` is the comment that suppresses; other linters pass their own. """ @@ -26,7 +27,7 @@ def __init__(self, marker=MARKER): def call(self, payload: Payload) -> Payload: cache = {} - kept, suppressed = [], [] + kept, suppressed, used = [], [], set() for finding in payload.get("findings", []): lines = cache.get(finding.path) if lines is None: @@ -36,17 +37,25 @@ def call(self, payload: Payload) -> Payload: except OSError: lines = [] cache[finding.path] = lines - (suppressed if _is_suppressed(finding, lines, self.marker) else kept).append(finding) - return payload.insert("findings", kept).insert("suppressed", suppressed) + places = _marker_lines(finding, lines, self.marker) + used.update((finding.path, place) for place in places) + (suppressed if places else kept).append(finding) + return (payload.insert("findings", kept).insert("suppressed", suppressed) + .insert("used_markers", sorted(used))) def _line(lines, number): return lines[number - 1] if number and 0 < number <= len(lines) else "" -def _is_suppressed(finding, lines, marker=MARKER) -> bool: - for number in (finding.line, finding.call_line): - if marker.search(_line(lines, number)): - return True +def _marker_lines(finding, lines, marker=MARKER): + """The lines of the comments that allow this finding. Empty when none does.""" + places = [number for number in (finding.line, finding.call_line) if marker.search(_line(lines, number))] above = _line(lines, finding.line - 1) - return bool(_COMMENT_ONLY.match(above) and marker.search(above)) + if _COMMENT_ONLY.match(above) and marker.search(above): + places.append(finding.line - 1) + return places + + +def _is_suppressed(finding, lines, marker=MARKER) -> bool: + return bool(_marker_lines(finding, lines, marker)) diff --git a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/drop_disabled_filter.py b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/drop_disabled_filter.py index 60390c7..906a400 100644 --- a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/drop_disabled_filter.py +++ b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/drop_disabled_filter.py @@ -6,13 +6,17 @@ class DropDisabledFilter: """Filter (sync). - Input keys: ``findings``, optional ``disable`` (rule ids). - Output keys: ``findings`` without the findings of a disabled rule. A - disabled rule is not counted as suppressed: it was not asked for. + Input keys: ``findings``, optional ``suppressed``, optional ``disable`` (rule ids). + Output keys: ``findings`` and ``suppressed`` without the findings of a + disabled rule. A disabled rule is not counted as suppressed: it was not + asked for. """ def call(self, payload: Payload) -> Payload: disabled = set(payload.get("disable") or ()) if not disabled: return payload - return payload.insert("findings", [f for f in payload.get("findings", []) if f.rule not in disabled]) + payload.insert("findings", [f for f in payload.get("findings", []) if f.rule not in disabled]) + if payload.has("suppressed"): + payload.insert("suppressed", [f for f in payload.get("suppressed", []) if f.rule not in disabled]) + return payload diff --git a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/finding.py b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/finding.py index cfb9f1c..db0eb29 100644 --- a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/finding.py +++ b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/finding.py @@ -8,6 +8,8 @@ class Finding(NamedTuple): ``call_line`` is the line of the enclosing ``add_*`` / Valve call, when it differs from ``line``: a suppression comment may sit on either. + ``severity`` is ``"warning"`` for a finding that is reported and does not + fail the run, unless the run is strict. """ path: str @@ -18,6 +20,8 @@ class Finding(NamedTuple): message: str lang: str call_line: Optional[int] = None + severity: Optional[str] = None def format(self) -> str: - return "%s:%d:%d %s %s %s" % (self.path, self.line, self.col, self.rule, self.slug, self.message) + warning = "warning: " if self.severity == "warning" else "" + return "%s:%d:%d %s %s %s%s" % (self.path, self.line, self.col, self.rule, self.slug, warning, self.message) diff --git a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/format_report_filter.py b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/format_report_filter.py index dd8a49f..d8c6c20 100644 --- a/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/format_report_filter.py +++ b/packages/codeupipe-lint/codeupipe_lint/anonymous_steps/format_report_filter.py @@ -6,11 +6,14 @@ class FormatReportFilter: """Filter (sync). - Input keys: ``findings``, ``suppressed``, ``errors``. + Input keys: ``findings``, ``suppressed``, ``errors``, optional ``strict``. Output keys: ``findings`` (sorted), ``report`` (stdout text), ``error_report`` (stderr text), ``exit_code``: 0 clean, 1 anonymous steps found, 2 a path could not be read (the check is incomplete). ``noun`` and ``plural`` name what a finding is; other linters pass their own. + + A warning is in the report and is counted apart. It does not make the + exit code 1, unless ``strict`` is true. """ def __init__(self, noun="anonymous step", plural="anonymous steps"): @@ -22,14 +25,20 @@ def call(self, payload: Payload) -> Payload: suppressed = payload.get("suppressed", []) errors = payload.get("errors", []) lines = [f.format() for f in findings] - if findings: - summary = "%d %s" % (len(findings), self.noun if len(findings) == 1 else self.plural) + warnings = sum(1 for f in findings if f.severity == "warning") + real = len(findings) - warnings + if real: + summary = "%d %s" % (real, self.noun if real == 1 else self.plural) else: summary = "No %s" % self.plural if suppressed: summary += " (%d suppressed)" % len(suppressed) - lines.append(summary + ".") - exit_code = 2 if errors else (1 if findings else 0) + summary += "." + if warnings: + summary += " %d %s." % (warnings, "warning" if warnings == 1 else "warnings") + lines.append(summary) + fails = real or (warnings and payload.get("strict") is True) + exit_code = 2 if errors else (1 if fails else 0) error_report = "\n".join("error: " + e for e in errors) return (payload.insert("findings", findings).insert("report", "\n".join(lines)) .insert("error_report", error_report).insert("exit_code", exit_code)) diff --git a/packages/codeupipe-lint/codeupipe_lint/invariants/__main__.py b/packages/codeupipe-lint/codeupipe_lint/invariants/__main__.py index fba6d0a..8cdf99a 100644 --- a/packages/codeupipe-lint/codeupipe_lint/invariants/__main__.py +++ b/packages/codeupipe-lint/codeupipe_lint/invariants/__main__.py @@ -1,8 +1,8 @@ -"""``python -m codeupipe_lint.invariants [--exclude GLOB] [--config FILE] [--rules] [paths...]`` +"""``python -m codeupipe_lint.invariants [--exclude GLOB] [--strict] [--config FILE] [--rules] [paths...]`` With no paths, they come from ``cup.config.json`` (packages/codeupipe/codeupipe/config.py). -Exit code: 0 clean, 1 invariant violations found, 2 a path could not be read +Exit code: 0 clean, 1 invariant violations found (with ``--strict``, warnings too), 2 a path could not be read or the config cannot be used. """ @@ -24,6 +24,7 @@ def main(argv=None) -> int: help="skip files whose path matches this fnmatch glob (repeatable)") parser.add_argument("--disable", action="append", metavar="RULE", help="switch this rule off (repeatable), e.g. CUP-I005") + parser.add_argument("--strict", action="store_true", help="a warning fails the run, as a finding does") parser.add_argument("--config", metavar="FILE", help="read paths and excludes from FILE, not %s" % DEFAULT_NAME) parser.add_argument("--rules", action="store_true", help="list the rule ids and exit") args = parser.parse_args(argv) @@ -49,7 +50,7 @@ def main(argv=None) -> int: print("error: no such rule: %s (the rules are listed by --rules of each linter)" % ", ".join(unknown), file=sys.stderr) return 2 - out = lint_paths(paths, exclude, disable) + out = lint_paths(paths, exclude, disable, strict=args.strict) print(out.get("report")) if out.get("error_report"): print(out.get("error_report"), file=sys.stderr) diff --git a/packages/codeupipe-lint/codeupipe_lint/invariants/analyze_plain_logic_filter.py b/packages/codeupipe-lint/codeupipe_lint/invariants/analyze_plain_logic_filter.py new file mode 100644 index 0000000..726e9e4 --- /dev/null +++ b/packages/codeupipe-lint/codeupipe_lint/invariants/analyze_plain_logic_filter.py @@ -0,0 +1,69 @@ +"""AnalyzePlainLogicFilter: 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``, ``a if c else b``, ``match``, +``for``, ``while``, ``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 test file, and the guards +``if __name__ == "__main__":`` and ``if TYPE_CHECKING:``. Not seen: a +comprehension, and ``and`` / ``or``. +""" + +import ast + +from codeupipe import Payload + +from .primitive_kind import is_test_file, primitive_kind +from .rules import LOGIC_OUTSIDE_PRIMITIVE, make_finding + +_WHAT = "has logic and is not a Filter, a Predicate, a Tap, a Hook or a Pipeline" +# ast.Match is new in Python 3.10 and ast.TryStar in 3.11. +_LOGIC = tuple(getattr(ast, name) for name in ("If", "IfExp", "For", "AsyncFor", "While", "Try", "TryStar", "Match") + if hasattr(ast, name)) +_GUARDS = ("__name__", "TYPE_CHECKING") + + +class AnalyzePlainLogicFilter: + """Filter (sync). + + Input keys: ``trees`` ({path: ast.Module}), ``findings`` (list). + Output keys: ``findings`` with CUP-I008 warnings appended. + """ + + def call(self, payload: Payload) -> Payload: + findings = list(payload.get("findings", [])) + for path, tree in payload.get("trees", {}).items(): + if is_test_file(path): + continue + module_code = None + for node in tree.body: + what = None + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + what = "Function `%s`" % node.name if self.has_logic(node.body) else None + elif isinstance(node, ast.ClassDef): + if primitive_kind(node) is None and self.has_logic(node.body): + what = "Class `%s`" % node.name + elif module_code is None and self.has_logic(self.without_guard(node)): + module_code = node + if what is not None: + findings.append(make_finding(path, node.lineno, node.col_offset + 1, LOGIC_OUTSIDE_PRIMITIVE, + "%s %s" % (what, _WHAT))) + if module_code is not None: + findings.append(make_finding(path, module_code.lineno, module_code.col_offset + 1, + LOGIC_OUTSIDE_PRIMITIVE, "The code at the top of this file %s" % _WHAT)) + return payload.insert("findings", findings) + + def has_logic(self, nodes) -> bool: + return any(isinstance(inner, _LOGIC) for node in nodes for inner in ast.walk(node)) + + def without_guard(self, node): + """The statements to look in: for a guard of the language, those in it, and not the ``if`` itself.""" + if isinstance(node, ast.If) and not node.orelse and any( + isinstance(name, ast.Name) and name.id in _GUARDS or isinstance(name, ast.Attribute) and name.attr in _GUARDS + for name in ast.walk(node.test)): + return node.body + return [node] diff --git a/packages/codeupipe-lint/codeupipe_lint/invariants/analyze_stale_markers_filter.py b/packages/codeupipe-lint/codeupipe_lint/invariants/analyze_stale_markers_filter.py new file mode 100644 index 0000000..7c5e5a1 --- /dev/null +++ b/packages/codeupipe-lint/codeupipe_lint/invariants/analyze_stale_markers_filter.py @@ -0,0 +1,57 @@ +"""AnalyzeStaleMarkersFilter: CUP-I009, a warning: a comment that allows nothing. + +A ``# cup: allow-...`` comment that allowed no finding stays when the code +that it was for is gone or has moved, or when its kind has a spelling error. +It then says that the code has an exception that it does not have. + +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. A +``cup: allow-anonymous`` comment is for the other linter +(``codeupipe_lint.anonymous_steps``) and is not looked at here. +""" + +import io +import re +import tokenize + +from codeupipe import Payload + +from .rules import MARKER_ALLOWS_NOTHING, make_finding + +_MARKER = re.compile(r"#+\s*cup:\s*(allow-[\w-]*)") +_OTHER_LINTER = ("allow-anonymous",) + + +class AnalyzeStaleMarkersFilter: + """Filter (sync). + + Input keys: ``trees`` ({path: ast.Module}), ``findings``, ``used_markers`` + (from ApplySuppressionsFilter). + Output keys: ``findings`` with CUP-I009 warnings appended. + """ + + def call(self, payload: Payload) -> Payload: + used = {tuple(place) for place in payload.get("used_markers", [])} + findings = list(payload.get("findings", [])) + for path in payload.get("trees", {}): + for line, col, kind in self.markers_of(path): + if (path, line) in used or kind in _OTHER_LINTER: + continue + findings.append(make_finding(path, line, col, MARKER_ALLOWS_NOTHING, + "The comment `cup: %s` allows nothing" % kind)) + return payload.insert("findings", findings) + + def markers_of(self, path): + """``(line, col, kind)`` for each comment of the file that starts with a marker.""" + try: + with open(path, encoding="utf-8", errors="replace") as handle: + text = handle.read() + tokens = list(tokenize.generate_tokens(io.StringIO(text).readline)) + except (OSError, tokenize.TokenError, SyntaxError): + return [] + markers = [] + for token in tokens: + match = _MARKER.match(token.string) if token.type == tokenize.COMMENT else None + if match: + markers.append((token.start[0], token.start[1] + 1, match.group(1))) + return markers diff --git a/packages/codeupipe-lint/codeupipe_lint/invariants/lint_pipeline.py b/packages/codeupipe-lint/codeupipe_lint/invariants/lint_pipeline.py index d625bb0..00befb0 100644 --- a/packages/codeupipe-lint/codeupipe_lint/invariants/lint_pipeline.py +++ b/packages/codeupipe-lint/codeupipe_lint/invariants/lint_pipeline.py @@ -7,12 +7,15 @@ 4. AnalyzeStateReadsFilter CUP-I003 5. AnalyzeOutsideWritesFilter CUP-I004 6. AnalyzePrimitivesPerFileFilter CUP-I005 - 7. AnalyzePrimitiveNamesFilter CUP-I006, CUP-I007 - 8. DropDisabledFilter rules switched off for the project + 7. AnalyzePrimitiveNamesFilter CUP-I006, CUP-I007 + 8. AnalyzePlainLogicFilter CUP-I008, a warning 9. ApplySuppressionsFilter ``cup: allow-mutation``, ``cup: allow-state-read``, - ``cup: allow-outside-write``, ``cup: allow-shared-file``, - ``cup: allow-name`` - 10. FormatReportFilter report lines + exit code + ``cup: allow-outside-write``, ``cup: allow-shared-file``, + ``cup: allow-name``, ``cup: allow-plain`` + 10. AnalyzeStaleMarkersFilter CUP-I009, a warning + 11. DropDisabledFilter rules switched off for the project. Last, so that a + comment for a rule that is off is not stale + 12. FormatReportFilter report lines + exit code """ import re @@ -27,11 +30,13 @@ from .analyze_outside_writes_filter import AnalyzeOutsideWritesFilter from .analyze_primitive_names_filter import AnalyzePrimitiveNamesFilter from .analyze_payload_mutation_filter import AnalyzePayloadMutationFilter +from .analyze_plain_logic_filter import AnalyzePlainLogicFilter +from .analyze_stale_markers_filter import AnalyzeStaleMarkersFilter from .analyze_primitives_per_file_filter import AnalyzePrimitivesPerFileFilter from .analyze_state_reads_filter import AnalyzeStateReadsFilter from .parse_python_filter import ParsePythonFilter -MARKER = re.compile(r"cup:\s*allow-(mutation|state-read|outside-write|shared-file|name)\b") +MARKER = re.compile(r"cup:\s*allow-(mutation|state-read|outside-write|shared-file|name|plain)\b") def build_lint_pipeline() -> Pipeline: @@ -43,14 +48,16 @@ def build_lint_pipeline() -> Pipeline: pipeline.add_filter(AnalyzeOutsideWritesFilter()) pipeline.add_filter(AnalyzePrimitivesPerFileFilter()) pipeline.add_filter(AnalyzePrimitiveNamesFilter()) - pipeline.add_filter(DropDisabledFilter()) + pipeline.add_filter(AnalyzePlainLogicFilter()) pipeline.add_filter(ApplySuppressionsFilter(MARKER)) + pipeline.add_filter(AnalyzeStaleMarkersFilter()) + pipeline.add_filter(DropDisabledFilter()) pipeline.add_filter(FormatReportFilter("invariant violation", "invariant violations")) return pipeline def lint_paths(paths: Iterable[str], exclude: Optional[Iterable[str]] = None, - disable: Optional[Iterable[str]] = None) -> Payload: + disable: Optional[Iterable[str]] = None, strict: bool = False) -> Payload: """Run the linter over ``paths``; the result Payload holds ``findings``, ``suppressed``, ``errors``, ``report``, ``error_report`` and ``exit_code``.""" data = {"paths": list(paths), "findings": [], "errors": []} @@ -58,4 +65,6 @@ def lint_paths(paths: Iterable[str], exclude: Optional[Iterable[str]] = None, data["exclude"] = list(exclude) if disable: data["disable"] = list(disable) + if strict: + data["strict"] = True return build_lint_pipeline().run(Payload(data)) diff --git a/packages/codeupipe-lint/codeupipe_lint/invariants/rules.py b/packages/codeupipe-lint/codeupipe_lint/invariants/rules.py index 07ce597..8fc085c 100644 --- a/packages/codeupipe-lint/codeupipe_lint/invariants/rules.py +++ b/packages/codeupipe-lint/codeupipe_lint/invariants/rules.py @@ -4,7 +4,7 @@ __all__ = ["RULES", "PARSE_ERROR", "PAYLOAD_INTERNALS", "SHARED_VALUE_MUTATION", "STATE_READ", "WRITE_OUTSIDE_FILTER", "ONE_PRIMITIVE_PER_FILE", "PRIMITIVE_FILE_NAME", - "PRIMITIVE_ROLE_SUFFIX", "make_finding"] + "PRIMITIVE_ROLE_SUFFIX", "LOGIC_OUTSIDE_PRIMITIVE", "MARKER_ALLOWS_NOTHING", "WARNINGS", "make_finding"] PARSE_ERROR = "CUP-I000" PAYLOAD_INTERNALS = "CUP-I001" @@ -14,6 +14,11 @@ ONE_PRIMITIVE_PER_FILE = "CUP-I005" PRIMITIVE_FILE_NAME = "CUP-I006" PRIMITIVE_ROLE_SUFFIX = "CUP-I007" +LOGIC_OUTSIDE_PRIMITIVE = "CUP-I008" +MARKER_ALLOWS_NOTHING = "CUP-I009" + +# The rules whose findings are warnings. +WARNINGS = frozenset([LOGIC_OUTSIDE_PRIMITIVE, MARKER_ALLOWS_NOTHING]) RULES = { PARSE_ERROR: "parse-error", @@ -24,6 +29,8 @@ ONE_PRIMITIVE_PER_FILE: "one-primitive-per-file", PRIMITIVE_FILE_NAME: "primitive-file-name", PRIMITIVE_ROLE_SUFFIX: "primitive-role-suffix", + LOGIC_OUTSIDE_PRIMITIVE: "logic-outside-primitive", + MARKER_ALLOWS_NOTHING: "marker-allows-nothing", } _FIX = { @@ -43,9 +50,14 @@ "the other."), PRIMITIVE_ROLE_SUFFIX: ("A primitive's class name ends with its role (Filter, Tap, Hook, Predicate, " "Pipeline), so the name tells what the class is."), + LOGIC_OUTSIDE_PRIMITIVE: ("Write it as a primitive, so that the pipeline has the logic and it can be tested " + "as a step. For logic that must be plain code, say why: # cup: allow-plain "), + MARKER_ALLOWS_NOTHING: ("Remove it, or put it on the line of the finding that it is for. A comment that " + "allows nothing says that the code has an exception that it does not have."), } def make_finding(path: str, line: int, col: int, rule: str, what: str) -> Finding: """A Finding whose message reads ``. ``.""" - return Finding(path, line, col, rule, RULES[rule], "%s. %s" % (what, _FIX[rule]), "python") + return Finding(path, line, col, rule, RULES[rule], "%s. %s" % (what, _FIX[rule]), "python", + severity="warning" if rule in WARNINGS else None) diff --git a/packages/codeupipe-ts/src/payload.ts b/packages/codeupipe-ts/src/payload.ts index c77fbe1..3432561 100644 --- a/packages/codeupipe-ts/src/payload.ts +++ b/packages/codeupipe-ts/src/payload.ts @@ -5,7 +5,7 @@ * step can write and return in one line. `copy()` gives a separate Payload * when you want to keep the one you have. */ -export class Payload { +export class Payload { // cup: allow-plain the framework itself: a primitive is made of this #data: Map; #traceId: string | null = null; #lineage: string[] = []; diff --git a/packages/codeupipe-ts/src/pipeline.ts b/packages/codeupipe-ts/src/pipeline.ts index 1436487..c1fa4b2 100644 --- a/packages/codeupipe-ts/src/pipeline.ts +++ b/packages/codeupipe-ts/src/pipeline.ts @@ -28,7 +28,7 @@ function has(value: unknown, method: string): boolean { } /** Refuse a step that does not have the method of its role. */ -function require(step: unknown, method: string, what: string): void { +function require(step: unknown, method: string, what: string): void { // cup: allow-plain the framework itself: a primitive is made of this if (!has(step, method)) { const given = step === null || step === undefined ? String(step) : typeof step === "function" ? "a function" : `a ${labelOf(step)}`; diff --git a/packages/codeupipe-ts/src/step-label.ts b/packages/codeupipe-ts/src/step-label.ts index 9b83e94..79accda 100644 --- a/packages/codeupipe-ts/src/step-label.ts +++ b/packages/codeupipe-ts/src/step-label.ts @@ -2,7 +2,7 @@ * The label of a step: the name of its class. State and Hooks report a step * by this label. */ -export function labelOf(step: unknown): string { +export function labelOf(step: unknown): string { // cup: allow-plain the framework itself: a primitive is made of this const type = (step as { constructor?: { name?: unknown } } | null | undefined)?.constructor; const name = type?.name; return typeof name === "string" && name !== "" ? name : "anonymous"; diff --git a/tests/lint/invariants/test_plain_logic.py b/tests/lint/invariants/test_plain_logic.py new file mode 100644 index 0000000..afcb78f --- /dev/null +++ b/tests/lint/invariants/test_plain_logic.py @@ -0,0 +1,79 @@ +"""CUP-I008: logic that is not in a primitive. A warning.""" + +from codeupipe_lint.invariants.lint_pipeline import lint_paths +from codeupipe_lint.invariants.__main__ import main + + +def lint(tmp_path, files, **options): + for name, text in files.items(): + (tmp_path / name).write_text(text) + out = lint_paths([str(tmp_path)], **options) + found = ["%s:%d %s" % (f.path[len(str(tmp_path)) + 1:], f.line, f.rule) for f in out.get("findings")] + return out, found + + +def test_a_function_with_a_decision_that_is_not_in_a_primitive_is_a_warning(tmp_path): + out, found = lint(tmp_path, {"who_is_due.py": ( + 'def who_is_due(entries):\n if not entries:\n return "say"\n return "model"\n' + "def pick(items):\n for each in items:\n return each\n" + 'def choose(flag):\n return "a" if flag else "b"\n' + "async def load(path):\n try:\n return await read(path)\n except OSError:\n return None\n" + "def wait(queue):\n while queue:\n queue.pop()\n" + "def plain(a, b):\n return a + b\n" + "def evens(items):\n return [each for each in items if each % 2 == 0]\n")}) + assert found == ["who_is_due.py:1 CUP-I008", "who_is_due.py:5 CUP-I008", "who_is_due.py:8 CUP-I008", + "who_is_due.py:10 CUP-I008", "who_is_due.py:15 CUP-I008"] + first = out.get("findings")[0] + assert first.severity == "warning" + assert first.message.startswith("Function `who_is_due` has logic and is not a Filter, a Predicate, a Tap, " + "a Hook or a Pipeline. Write it as a primitive") + assert "CUP-I008 logic-outside-primitive warning: Function `who_is_due`" in out.get("report") + + +def test_a_class_that_is_not_a_primitive_and_code_at_the_top_of_a_file(tmp_path): + _, found = lint(tmp_path, { + "record_store.py": ( + "class RecordStore:\n def read(self, name):\n if name not in self.files:\n" + " raise KeyError(name)\n return []\n" + "class Point:\n def __init__(self, x):\n self.x = x\n"), + "main.py": "import sys\nARGS = sys.argv[1:]\nif not ARGS:\n sys.exit(2)\nfor arg in ARGS:\n print(arg)\n"}) + assert found == ["main.py:3 CUP-I008", "record_store.py:1 CUP-I008"] + + +def test_logic_in_a_primitive_is_where_it_must_be(tmp_path): + _, found = lint(tmp_path, { + "pick_filter.py": ("class PickFilter:\n def call(self, payload):\n if payload.get('a'):\n" + " return payload.insert('b', 1)\n return payload\n"), + "is_on_predicate.py": "class IsOnPredicate:\n def test(self, payload):\n return True if payload.get('on') else False\n", + "order_pipeline.py": ("from codeupipe import Pipeline\nclass OrderPipeline(Pipeline):\n def __init__(self):\n" + " super().__init__()\n for step in ():\n self.add_filter(step)\n")}) + assert found == [] + + +def test_the_usual_guards_at_the_top_of_a_file_are_not_logic(tmp_path): + _, found = lint(tmp_path, {"entry.py": ( + "from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n import ast\n" + "def main():\n return 0\n" + 'if __name__ == "__main__":\n raise SystemExit(main())\n')}) + assert found == [] + + +def test_a_test_file_is_not_checked(tmp_path): + _, found = lint(tmp_path, {"test_pick.py": "def test_pick():\n for each in ():\n assert each\n"}) + assert found == [] + + +def test_a_comment_allows_plain_logic(tmp_path): + out, found = lint(tmp_path, {"shell.py": ( + "# cup: allow-plain the edge to the shell\n" + "def run(command):\n try:\n return start(command)\n except OSError:\n return None\n")}) + assert found == [] + assert len(out.get("suppressed")) == 1 + + +def test_a_warning_fails_the_run_only_when_it_is_strict(tmp_path, capsys): + (tmp_path / "pick.py").write_text("def pick(flag):\n return 1 if flag else 2\n") + assert main([str(tmp_path)]) == 0 + assert "No invariant violations. 1 warning." in capsys.readouterr().out + assert main(["--strict", str(tmp_path)]) == 1 + assert main(["--strict", "--disable", "CUP-I008", str(tmp_path)]) == 0 diff --git a/tests/lint/invariants/test_repo_is_clean.py b/tests/lint/invariants/test_repo_is_clean.py index f2a1568..53fc856 100644 --- a/tests/lint/invariants/test_repo_is_clean.py +++ b/tests/lint/invariants/test_repo_is_clean.py @@ -12,5 +12,6 @@ def test_repo_python_has_no_invariant_violations(): out = lint_paths([os.path.join(REPO, p) for p in CHECKED], exclude=["*/tests/lint/fixtures/*"]) assert out.get("errors") == [] - assert [f.format() for f in out.get("findings")] == [] + # CUP-I008 is a warning, and the framework has logic that cannot be a primitive: it makes the primitives. + assert [f.format() for f in out.get("findings") if f.rule != "CUP-I008"] == [] assert out.get("suppressed") == [] diff --git a/tests/lint/invariants/test_stale_markers.py b/tests/lint/invariants/test_stale_markers.py new file mode 100644 index 0000000..b2929c2 --- /dev/null +++ b/tests/lint/invariants/test_stale_markers.py @@ -0,0 +1,45 @@ +"""CUP-I009: a comment that allows nothing. A warning.""" + +from codeupipe_lint.invariants.__main__ import main + +from .test_plain_logic import lint + + +def test_a_comment_that_allows_nothing_is_a_warning(tmp_path): + out, found = lint(tmp_path, {"add.py": ( + "# cup: allow-plain it had a loop before\n" + "def add(a, b):\n return a + b\n" + "ONE = 1 # cup: allow-state-read nothing here reads State\n" + "# cup: allow-plian a kind that does not exist\n" + "TWO = 2\n")}) + assert found == ["add.py:1 CUP-I009", "add.py:4 CUP-I009", "add.py:5 CUP-I009"] + first = out.get("findings")[0] + assert first.severity == "warning" + assert first.message.startswith("The comment `cup: allow-plain` allows nothing. Remove it") + assert out.get("exit_code") == 0 + + +def test_a_stale_comment_fails_a_strict_run(tmp_path): + (tmp_path / "one.py").write_text("# cup: allow-plain no reason\nONE = 1\n") + assert main(["--strict", str(tmp_path)]) == 1 + assert main(["--strict", "--disable", "CUP-I009", str(tmp_path)]) == 0 + + +def test_a_comment_that_allows_a_finding_is_not_reported_and_a_string_is_not_a_comment(tmp_path): + out, found = lint(tmp_path, {"pick.py": ( + "# cup: allow-plain the entry of the program\n" + "def pick(items):\n for each in items:\n return each\n" + "def choose(flag): # cup: allow-plain a small helper\n return 1 if flag else 2\n" + '"""The marker ``# cup: allow-plain`` allows a finding."""\n' + 'HELP = "say why: # cup: allow-plain "\n' + "# For plain logic, write cup: allow-plain and the reason.\n")}) + assert found == [] + assert len(out.get("suppressed")) == 2 + + +def test_a_comment_for_a_rule_that_is_off_or_for_the_other_linter_is_not_reported(tmp_path): + out, found = lint(tmp_path, {"pick.py": ( + "# cup: allow-plain the entry\ndef pick(flag):\n return 1 if flag else 2\n" + "STEP = 1 # cup: allow-anonymous the other linter reads this one\n")}, disable=["CUP-I008"]) + assert found == [] + assert out.get("suppressed") == []