diff --git a/.agents/skills/pgsql-lint/SKILL.md b/.agents/skills/pgsql-lint/SKILL.md index 7fb4c333..1cf0fa93 100644 --- a/.agents/skills/pgsql-lint/SKILL.md +++ b/.agents/skills/pgsql-lint/SKILL.md @@ -80,7 +80,14 @@ await lintFiles(['./migrations']); // files/dirs on disk `lintSqlText`/`lintFiles` slice out each top-level `CREATE FUNCTION` using the parser's `stmt_location`/`stmt_len`, lint each in isolation, and **re-anchor** findings to absolute file lines — so a mixed migration is never treated as one -malformed definition. +malformed definition. Those offsets are **bytes** into the UTF-8 source, not +string indices: slice and count lines over `utf8(text)` (`src/util.ts`), never +over the JS string, or one `→` in a comment shifts every later definition. + +A definition the parser rejects is a `parse-error` (`P0`) finding, never a +silent skip — no rule ran, so it is not clean. `lintDefinition` returns it as +`parseError` on the result; the file runner turns it into a finding whose +severity is configured like any rule (`--warn parse-error`). ## Authoring a new rule diff --git a/packages/lint/README.md b/packages/lint/README.md index 1c28fd74..3de278ea 100644 --- a/packages/lint/README.md +++ b/packages/lint/README.md @@ -36,6 +36,10 @@ Agent skill (rules, CLI, authoring new rules): | `C3` | `require-qualified-refs` | an unqualified relation reference (`FROM users` → `FROM app_public.users`) | | `C4` | `no-dynamic-sql` | `EXECUTE`, `EXECUTE … USING`, `FOR … IN EXECUTE` | +A definition the parser rejects is reported as `parse-error` (`P0`) rather than +skipped: no rule ran over it, so it is not clean. Downgrade it like any rule +(`--warn parse-error`) when the parser, not the SQL, is at fault. + The rules encode a single discipline: never depend on `search_path` — fully qualify everything (`C1` + `C3`) — don't paper over ambiguity (`C2`), and treat dynamic SQL as opaque and exceptional (`C4`). diff --git a/packages/lint/__tests__/__fixtures__/multibyte.sql b/packages/lint/__tests__/__fixtures__/multibyte.sql new file mode 100644 index 00000000..f5c9e817 --- /dev/null +++ b/packages/lint/__tests__/__fixtures__/multibyte.sql @@ -0,0 +1,25 @@ +-- Arrows (→) and dashes (—) are multi-byte in UTF-8: every parser offset +-- after this comment is a byte offset, not a UTF-16 index. +CREATE SCHEMA app_public; + +CREATE FUNCTION app_public.pinned() RETURNS void +LANGUAGE sql +SET search_path = app_public +AS $$ + SELECT 1 +$$; + +CREATE FUNCTION app_public.dynamic(x text) RETURNS void +LANGUAGE plpgsql +AS $$ +BEGIN + EXECUTE format('SELECT %s', x); +END; +$$; + +CREATE FUNCTION app_public.dirty() RETURNS setof app_public.users +LANGUAGE sql +AS $$ + -- 日本語 + SELECT * FROM users +$$; diff --git a/packages/lint/__tests__/file-runner.test.ts b/packages/lint/__tests__/file-runner.test.ts index c5b16ced..49fdab07 100644 --- a/packages/lint/__tests__/file-runner.test.ts +++ b/packages/lint/__tests__/file-runner.test.ts @@ -73,3 +73,58 @@ describe('lintSource (adapters)', () => { expect(reports.some((r) => r.file.endsWith('migration.sql'))).toBe(true); }); }); + +describe('multi-byte characters before a definition', () => { + it('finds every rule after a non-ASCII comment', async () => { + const [report] = await lintFiles([path.join(FIXTURES, 'multibyte.sql')]); + expect(report.parseError).toBeUndefined(); + const byCode = Object.fromEntries( + report.findings.map((f) => [f.code, { line: f.line, subject: f.subject }]) + ); + expect(byCode).toEqual({ + C1: { line: 7, subject: 'app_public.pinned' }, + C4: { line: 16, subject: 'app_public.dynamic' }, + C3: { line: 24, subject: 'app_public.dirty' } + }); + }); + + it('is byte-for-byte equivalent to the ASCII-folded source', async () => { + const source = `-- → —\nCREATE FUNCTION f(x text) RETURNS void LANGUAGE plpgsql AS $$\nBEGIN\n EXECUTE format('SELECT %s', x);\nEND;\n$$;`; + const folded = await lintSqlText(source.replace('→', '->').replace('—', '-')); + const report = await lintSqlText(source); + expect(report.findings).toEqual(folded.findings); + expect(report.findings.map((f) => [f.code, f.line])).toEqual([['C4', 4]]); + }); +}); + +describe('definitions that fail to parse', () => { + it('surface as a finding instead of being dropped', async () => { + const report = await lintSource( + sqlTextAdapter( + `CREATE FUNCTION f() RETURNS void LANGUAGE plpgsql AS $$ BEGIN this is not plpgsql; END; $$;`, + 'bad.sql' + ) + ); + expect(report).toHaveLength(1); + expect(report[0].findings).toHaveLength(1); + expect(report[0].findings[0]).toMatchObject({ + ruleId: 'parse-error', + code: 'P0', + severity: 'error', + subject: 'f', + line: 1 + }); + }); + + it('are not reported when parse-error is configured off', async () => { + const report = await lintSource( + sqlTextAdapter( + `CREATE FUNCTION f() RETURNS void LANGUAGE plpgsql AS $$ BEGIN this is not plpgsql; END; $$;`, + 'bad.sql' + ), + { severity: { 'parse-error': 'off' } } + ); + expect(report).toHaveLength(1); + expect(report[0].findings).toEqual([]); + }); +}); diff --git a/packages/lint/src/cli.ts b/packages/lint/src/cli.ts index 23bc84a2..75c06f73 100644 --- a/packages/lint/src/cli.ts +++ b/packages/lint/src/cli.ts @@ -194,10 +194,6 @@ async function main(): Promise { let warnings = 0; let acknowledged = 0; for (const report of reports) { - if (report.parseError) { - if (!argv.json) console.error(chalk.yellow(`⚠ ${report.file}: ${report.parseError}`)); - continue; - } const shown = report.findings.filter((f) => !f.acknowledged || !argv.quiet); if (!argv.json && shown.length > 0) { console.log(chalk.underline(report.file)); diff --git a/packages/lint/src/engine.ts b/packages/lint/src/engine.ts index 3a2eb7ce..3a80781d 100644 --- a/packages/lint/src/engine.ts +++ b/packages/lint/src/engine.ts @@ -15,6 +15,7 @@ import type { LintProblem, LintResult, LintRule, + LintRuleMeta, Severity, SeverityMap, SuppressedProblem @@ -38,10 +39,22 @@ export interface LintOptions { } /** Resolve a rule's configured severity (id wins over code; default `error`). */ -export function severityOf(rule: LintRule, severity: SeverityMap = {}): Severity { +export function severityOf(rule: LintRuleMeta, severity: SeverityMap = {}): Severity { return severity[rule.id] ?? severity[rule.code] ?? 'error'; } +/** + * The pseudo-rule a parse failure is reported under. Not a {@link LintRule} + * (it has no `run`): it exists so the failure has an id/code a consumer can + * configure (`off` / `warn`) exactly like a rule. + */ +export const PARSE_ERROR: LintRuleMeta = { + id: 'parse-error', + code: 'P0', + title: 'Definition could not be parsed', + reasonRequired: false +}; + /** Lint a single function definition. */ export async function lintDefinition( text: string, @@ -63,9 +76,10 @@ export async function lintDefinition( if (selected.length === 0) return { problems: active, suppressed }; const unit = await parseUnit(text, language, name); - // An unparseable definition produces no lint findings — dynamic/opaque bodies - // are a downstream concern (e.g. a call-graph), not the linter's. - if (unit.parseError) return { problems: active, suppressed }; + // An unparseable definition was checked by no rule. That is never a pass: + // the failure is returned so the caller reports it (the file runner turns + // it into a `parse-error` finding). + if (unit.parseError) return { problems: active, suppressed, parseError: unit.parseError }; const keywords = options.keyword ? (Array.isArray(options.keyword) ? options.keyword : [options.keyword]) diff --git a/packages/lint/src/file-runner.ts b/packages/lint/src/file-runner.ts index e20b2981..c58e3889 100644 --- a/packages/lint/src/file-runner.ts +++ b/packages/lint/src/file-runner.ts @@ -14,7 +14,7 @@ import { promises as fs } from 'fs'; import * as path from 'path'; import { parse } from 'pgsql-parser'; -import { lintDefinition, LintOptions } from './engine'; +import { lintDefinition, LintOptions, PARSE_ERROR, severityOf } from './engine'; import { applyIgnore } from './ignore'; import type { LintDefinitionInput, @@ -23,6 +23,7 @@ import type { SourceAdapter, SuppressionScope } from './types'; +import { newlinesBeforeByte, sliceBytes, utf8 } from './util'; import { findAll } from './walk'; /** A finding re-anchored to a source file. */ @@ -55,14 +56,6 @@ export interface FileReport { parseError?: string; } -/** Count newlines before `offset` — the 0-based line the offset sits on. */ -function lineAtOffset(text: string, offset: number): number { - let n = 0; - const end = Math.min(Math.max(offset, 0), text.length); - for (let i = 0; i < end; i++) if (text.charCodeAt(i) === 10) n++; - return n; -} - /** Printable `schema.name` of a `CreateFunctionStmt.funcname`. */ function funcName(createFnStmt: Record): string { const funcname = createFnStmt.funcname; @@ -98,6 +91,9 @@ function funcLanguage(createFnStmt: Record): string { * `CREATE FUNCTION` is sliced back out with its absolute start line, so a * mixed migration (schema + tables + functions) yields one input per function * rather than being treated as a single malformed definition. + * + * `stmt_location` / `stmt_len` are byte offsets into the UTF-8 source, so the + * slicing happens over the encoded bytes (see `util.ts`). */ export async function sqlTextDefinitions( source: string, @@ -106,18 +102,19 @@ export async function sqlTextDefinitions( const parsed = (await parse(source)) as { stmts?: Array<{ stmt?: unknown; stmt_location?: number; stmt_len?: number }>; }; + const bytes = utf8(source); const out: LintDefinitionInput[] = []; for (const entry of parsed.stmts ?? []) { const createFnStmt = entry.stmt ? findAll(entry.stmt, 'CreateFunctionStmt')[0] : undefined; if (!createFnStmt) continue; const start = entry.stmt_location ?? 0; - const len = entry.stmt_len ?? source.length - start; + const len = entry.stmt_len ?? bytes.length - start; out.push({ - text: source.slice(start, start + len), + text: sliceBytes(bytes, start, len), language: funcLanguage(createFnStmt), name: funcName(createFnStmt), file, - startLine: lineAtOffset(source, start) // 0-based line the definition begins on + startLine: newlinesBeforeByte(bytes, start) // 0-based line the definition begins on }); } return out; @@ -132,7 +129,9 @@ export async function lintSqlText(source: string, options: LintOptions = {}): Pr try { inputs = await sqlTextDefinitions(source); } catch (err) { - return { file: '', findings: [], parseError: (err as Error).message }; + const message = (err as Error).message; + const finding = parseErrorFinding(message, '', 1, options); + return { file: '', findings: finding ? [finding] : [], parseError: message }; } const findings = await lintInputs(inputs, options); return { file: '', findings }; @@ -147,12 +146,16 @@ async function lintInputs( for (const input of inputs) { const subject = input.name ?? ''; const startLine = input.startLine ?? 0; - const { problems, suppressed } = await lintDefinition( + const { problems, suppressed, parseError } = await lintDefinition( input.text, input.language, subject, options ); + if (parseError) { + const finding = parseErrorFinding(parseError, subject, startLine + 1, options); + if (finding) findings.push(finding); + } const absLine = (p: LintProblem): number => startLine + p.line; // p.line is 1-based within input.text for (const p of problems) findings.push(toFinding(p, subject, absLine(p), false)); for (const s of suppressed) { @@ -190,6 +193,7 @@ function toFinding( } const RULE_CODE: Record = { + [PARSE_ERROR.id]: PARSE_ERROR.code, 'no-set-search-path': 'C1', 'no-variable-conflict': 'C2', 'require-qualified-refs': 'C3', @@ -199,6 +203,33 @@ function codeFor(ruleId: string): string { return RULE_CODE[ruleId] ?? ruleId; } +/** + * A definition (or file) the parser rejected. Nothing in it was checked, so + * the failure is reported as a finding rather than dropped: an unparseable + * function would otherwise pass every rule by default. Severity is configured + * like any rule (`parse-error` / `P0`): `error` unless downgraded, and `off` + * drops it like any other rule turned off. + */ +function parseErrorFinding( + message: string, + subject: string, + line: number, + options: LintOptions +): FileFinding | undefined { + const severity = severityOf(PARSE_ERROR, options.severity); + if (severity === 'off') return undefined; + return { + ruleId: PARSE_ERROR.id, + code: PARSE_ERROR.code, + line, + message: `Definition could not be parsed, so no rule was checked: ${message}`, + hint: 'Fix the syntax (or, if the parser is wrong, downgrade `parse-error` in the lint config).', + severity, + subject, + acknowledged: false + }; +} + /** Recursively collect `.sql` files under a directory. */ async function sqlFilesUnder(dir: string): Promise { const out: string[] = []; diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 5a156d62..20211211 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -16,7 +16,7 @@ export { changedFiles, changedSqlFiles, resolveChangedBase } from './changed'; export type { LintConfigFile, LoadedLintConfig } from './config'; export { CONFIG_FILENAMES, configDir, findConfigFile, loadLintConfig } from './config'; export type { LintOptions } from './engine'; -export { lintDefinition, severityOf } from './engine'; +export { lintDefinition, PARSE_ERROR, severityOf } from './engine'; export { FileFinding, FileLintOptions, diff --git a/packages/lint/src/parse-unit.ts b/packages/lint/src/parse-unit.ts index f74bb38d..f412f554 100644 --- a/packages/lint/src/parse-unit.ts +++ b/packages/lint/src/parse-unit.ts @@ -13,19 +13,15 @@ import { parsePlPgSQL } from 'libpg-query'; import { parse } from 'pgsql-parser'; import type { DynamicSqlSite, LintUnit, SqlFragment } from './types'; +import { newlinesBeforeByte, utf8 } from './util'; import { findAll } from './walk'; -/** Count newlines in `s[0..offset)` — i.e. how many lines precede `offset`. */ -function newlinesBefore(s: string, offset: number): number { - let n = 0; - const end = Math.min(offset, s.length); - for (let i = 0; i < end; i++) if (s.charCodeAt(i) === 10) n++; - return n; -} - /** Absolute 1-based line of a char offset within `text`. */ function lineOf(text: string, offset: number): number { - return newlinesBefore(text, offset) + 1; + let n = 0; + const end = Math.min(offset, text.length); + for (let i = 0; i < end; i++) if (text.charCodeAt(i) === 10) n++; + return n + 1; } /** The body string of a `CreateFunctionStmt` (`AS $$ … $$`), or null. */ @@ -138,13 +134,14 @@ export async function parseUnit( const lang = language.toLowerCase(); if (lang === 'sql' && body !== null && bodyStartLine !== undefined) { - // A SQL-language body is itself SQL: parse it whole. Locations are - // relative to the body, so re-anchor them onto the definition. + // A SQL-language body is itself SQL: parse it whole. Locations are byte + // offsets relative to the body, so re-anchor them onto the definition. try { const ast = await parse(body); + const bodyBytes = utf8(body); fragments.push({ ast, - lineForOffset: (offset) => bodyStartLine + newlinesBefore(body, offset) + lineForOffset: (offset) => bodyStartLine + newlinesBeforeByte(bodyBytes, offset) }); } catch { // Body may contain positional parameters etc. that don't parse alone — diff --git a/packages/lint/src/types.ts b/packages/lint/src/types.ts index b816ff27..1fef7203 100644 --- a/packages/lint/src/types.ts +++ b/packages/lint/src/types.ts @@ -84,6 +84,12 @@ export interface LintResult { problems: LintProblem[]; /** Suppressed findings, kept for the "accepted risk" report bucket. */ suppressed: SuppressedProblem[]; + /** + * Set when the definition could not be parsed. `problems` is then empty + * because no rule ran — not because the definition is clean — so a consumer + * must report this rather than treat the result as a pass. + */ + parseError?: string; } /** Static metadata for a lint rule. */ diff --git a/packages/lint/src/util.ts b/packages/lint/src/util.ts index ed5ef70b..ef3cfb94 100644 --- a/packages/lint/src/util.ts +++ b/packages/lint/src/util.ts @@ -1,7 +1,34 @@ -/** Absolute 1-based line of a char offset within `text`. */ -export function lineOfOffset(text: string, offset: number): number { +/** + * Offset arithmetic. Every location libpg_query reports — `stmt_location`, + * `stmt_len`, a node's `location` — is a **byte** offset into the UTF-8 source, + * while a JS string is indexed by UTF-16 code unit. The two agree only while + * the text is pure ASCII: one `→` or `—` earlier in a file shifts every later + * offset, and a definition sliced by character then fails to parse. So slice + * and count newlines over the UTF-8 bytes, never over the string. + */ + +const encoder = new TextEncoder(); +const decoder = new TextDecoder(); + +/** The UTF-8 bytes of `text` — the coordinate space parser offsets live in. */ +export function utf8(text: string): Uint8Array { + return encoder.encode(text); +} + +/** `bytes[start, start + len)` decoded back to a string. */ +export function sliceBytes(bytes: Uint8Array, start: number, len: number): string { + return decoder.decode(bytes.subarray(start, start + len)); +} + +/** Count newline bytes in `bytes[0, byteOffset)` — the 0-based line the offset sits on. */ +export function newlinesBeforeByte(bytes: Uint8Array, byteOffset: number): number { let n = 0; - const end = Math.min(Math.max(offset, 0), text.length); - for (let i = 0; i < end; i++) if (text.charCodeAt(i) === 10) n++; - return n + 1; + const end = Math.min(Math.max(byteOffset, 0), bytes.length); + for (let i = 0; i < end; i++) if (bytes[i] === 10) n++; + return n; +} + +/** Absolute 1-based line of a parser (byte) offset within `text`. */ +export function lineOfOffset(text: string, byteOffset: number): number { + return newlinesBeforeByte(utf8(text), byteOffset) + 1; }