From bf24cf423c10a2cfa46c1230f4860bbb4ea7ce32 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Tue, 23 Jun 2026 17:01:06 -0500 Subject: [PATCH] fix: tighten tagged error linting --- packages/core/src/npm.ts | 4 +- packages/opencode/src/skill/index.ts | 2 +- script/lint/opencode.mjs | 80 +++++++++++++++------ script/lint/test.ts | 103 ++++++++++++++++++++------- 4 files changed, 140 insertions(+), 49 deletions(-) diff --git a/packages/core/src/npm.ts b/packages/core/src/npm.ts index 01f19479d3..059b68e240 100644 --- a/packages/core/src/npm.ts +++ b/packages/core/src/npm.ts @@ -11,7 +11,6 @@ import { LayerNode } from "./effect/layer-node" import { filesystem } from "./effect/layer-node-platform" import { makeRuntime } from "./effect/runtime" import { NpmConfig } from "./npm-config" -import { errorMessage } from "./util/error" export class InstallFailedError extends Schema.TaggedErrorClass()("NpmInstallFailedError", { add: Schema.Array(Schema.String).pipe(Schema.optional), @@ -19,8 +18,7 @@ export class InstallFailedError extends Schema.TaggedErrorClass()("SkillInvalidError", { path: Schema.String, - message: Schema.optional(Schema.String), + message: Schema.String, issues: Schema.optional(Schema.Array(Issue)), }) {} diff --git a/script/lint/opencode.mjs b/script/lint/opencode.mjs index 777c428496..4803584718 100644 --- a/script/lint/opencode.mjs +++ b/script/lint/opencode.mjs @@ -10,47 +10,78 @@ const taggedErrorMessage = { schema: [], }, create(context) { - const schemas = new Set() - function visitClass(node) { - const fields = taggedErrorFields(node.superClass, schemas) - if ( - !fields || - fields.properties.some((property) => property.type === "SpreadElement" || property.computed) - ) - return - if (fields.properties.some((property) => propertyName(property) === "message")) return + const fields = taggedErrorFields(context, node.superClass) + if (!fields) return + if (fields.properties.some(hasRequiredMessage)) return if (node.body.body.some(hasInstanceMessage)) return context.report({ node, messageId: "missing" }) } return { - ImportDeclaration(node) { - if (node.source.value !== "effect" && node.source.value !== "effect/Schema") return - for (const specifier of node.specifiers) { - if (specifier.type === "ImportSpecifier" && specifier.imported.name === "Schema") schemas.add(specifier.local.name) - if (specifier.type === "ImportNamespaceSpecifier" && node.source.value === "effect/Schema") - schemas.add(specifier.local.name) - } - }, ClassDeclaration: visitClass, ClassExpression: visitClass, } }, } -function taggedErrorFields(superClass, schemas) { +function taggedErrorFields(context, superClass) { if (superClass?.type !== "CallExpression" || superClass.arguments.length < 2) return null if (superClass.arguments[1].type !== "ObjectExpression") return null const factory = superClass.callee if (factory.type !== "CallExpression" || factory.arguments.length !== 0) return null const taggedError = factory.callee if (taggedError.type !== "MemberExpression" || taggedError.computed) return null - if (taggedError.object.type !== "Identifier" || !schemas.has(taggedError.object.name)) return null + if (taggedError.object.type !== "Identifier" || !isEffectSchema(context, taggedError.object)) return null if (taggedError.property.type !== "Identifier" || taggedError.property.name !== "TaggedErrorClass") return null return superClass.arguments[1] } +function isEffectSchema(context, identifier) { + const variable = findVariable(context.sourceCode.getScope(identifier), identifier.name) + if (!variable || variable.defs.length !== 1 || variable.defs[0].type !== "ImportBinding") return false + const definition = variable.defs[0] + if (definition.parent.source.value === "effect") + return definition.node.type === "ImportSpecifier" && definition.node.imported.name === "Schema" + return definition.parent.source.value === "effect/Schema" && definition.node.type === "ImportNamespaceSpecifier" +} + +function findVariable(scope, name) { + return scope.set.get(name) ?? (scope.upper ? findVariable(scope.upper, name) : null) +} + +function hasRequiredMessage(property) { + if (propertyName(property) !== "message" || property.type !== "Property") return false + return isStringSchema(property.value) && !isOptional(property.value) +} + +function isStringSchema(node) { + if (node.type === "MemberExpression") + return ( + node.object.type === "Identifier" && + node.object.name === "Schema" && + node.property.type === "Identifier" && + node.property.name === "String" + ) + if (node.type !== "CallExpression" || node.callee.type !== "MemberExpression") return false + return isStringSchema(node.callee.object) +} + +function isOptional(node) { + if (node.type !== "CallExpression" || node.callee.type !== "MemberExpression") return false + if (node.callee.object.type === "Identifier" && node.callee.object.name === "Schema") + return node.callee.property.type === "Identifier" && node.callee.property.name === "optional" + if (node.callee.property.type !== "Identifier" || node.callee.property.name !== "pipe") return false + return node.arguments.some( + (argument) => + argument.type === "MemberExpression" && + argument.object.type === "Identifier" && + argument.object.name === "Schema" && + argument.property.type === "Identifier" && + argument.property.name === "optional", + ) +} + function propertyName(property) { if (property.type !== "Property" && property.type !== "PropertyDefinition" && property.type !== "MethodDefinition") return null @@ -61,8 +92,15 @@ function propertyName(property) { function hasInstanceMessage(member) { if (member.static || propertyName(member) !== "message") return false - if (member.type === "MethodDefinition") return member.kind === "get" - return member.type === "PropertyDefinition" && member.value !== null + if (member.type === "PropertyDefinition") return member.value !== null && !isEmptyMessage(member.value) + if (member.type !== "MethodDefinition" || member.kind !== "get") return false + if (member.value.body.body.length === 0) return false + const direct = member.value.body.body.length === 1 ? member.value.body.body[0] : null + return direct?.type !== "ReturnStatement" || (direct.argument !== null && !isEmptyMessage(direct.argument)) +} + +function isEmptyMessage(node) { + return (node.type === "Identifier" && node.name === "undefined") || (node.type === "Literal" && !node.value) } export default { diff --git a/script/lint/test.ts b/script/lint/test.ts index 89a2fb50e6..e7effc042d 100644 --- a/script/lint/test.ts +++ b/script/lint/test.ts @@ -32,23 +32,44 @@ export namespace Example { export class Error extends Schema.TaggedErrorClass()("Example", { ...fields }) {}`, }, { name: "computed fields", - errors: 0, + errors: 1, source: `import { Schema } from "effect" const key = "message" class Example extends Schema.TaggedErrorClass()("Example", { [key]: Schema.String }) {}`, + }, + { + name: "optional message", + errors: 1, + source: `import { Schema } from "effect" +class Example extends Schema.TaggedErrorClass()("Example", { message: Schema.optional(Schema.String) }) {}`, + }, + { + name: "piped optional message", + errors: 1, + source: `import { Schema } from "effect" +class Example extends Schema.TaggedErrorClass()("Example", { message: Schema.String.pipe(Schema.optional) }) {}`, }, { name: "unrelated Schema binding", errors: 0, source: `const Schema = getSchema() class Example extends Schema.TaggedErrorClass()("Example", {}) {}`, + }, + { + name: "shadowed Schema binding", + errors: 0, + source: `import { Schema } from "effect" +function example() { + const Schema = getSchema() + return class extends Schema.TaggedErrorClass()("Example", {}) {} +}`, }, { name: "missing message", @@ -67,6 +88,24 @@ class Example extends Schema.TaggedErrorClass()("Example", {}) { static errors: 1, source: `import { Schema } from "effect" class Example extends Schema.TaggedErrorClass()("Example", {}) { declare message: string }`, + }, + { + name: "undefined property", + errors: 1, + source: `import { Schema } from "effect" +class Example extends Schema.TaggedErrorClass()("Example", {}) { override message = undefined }`, + }, + { + name: "empty getter", + errors: 1, + source: `import { Schema } from "effect" +class Example extends Schema.TaggedErrorClass()("Example", {}) { override get message() { return "" } }`, + }, + { + name: "non-string message schema", + errors: 1, + source: `import { Schema } from "effect" +class Example extends Schema.TaggedErrorClass()("Example", { message: Schema.Unknown }) {}`, }, { name: "documented disable", @@ -88,28 +127,31 @@ try { rules: { "opencode/tagged-error-message": "error" }, }), ) - for (const [index, fixture] of cases.entries()) { - const file = path.join(directory, `${index}.ts`) - await Bun.write(file, fixture.source) - const result = Bun.spawnSync([ - path.join(import.meta.dir, "../../node_modules/.bin/oxlint"), - "--config", - config, - "--format", - "json", - file, - ]) - if (!result.stdout.length) throw new Error(`${fixture.name}: ${result.stderr.toString()}`) - const diagnostics: unknown = JSON.parse(result.stdout.toString()) - if (!hasDiagnostics(diagnostics)) throw new Error(`${fixture.name}: invalid Oxlint output`) - const errors = diagnostics.diagnostics.filter( - (item) => typeof item === "object" && item !== null && "code" in item && item.code === "opencode(tagged-error-message)", - ).length - if (errors !== fixture.errors) - throw new Error( - `${fixture.name}: expected ${fixture.errors} errors, received ${errors}\n${result.stdout.toString()}`, - ) - } + await Promise.all( + cases.map(async (fixture, index) => { + const file = path.join(directory, `${index}.ts`) + await Bun.write(file, fixture.source) + const result = Bun.spawnSync([ + path.join(import.meta.dir, "../../node_modules/.bin/oxlint"), + "--config", + config, + "--format", + "json", + file, + ]) + if (!result.stdout.length) throw new Error(`${fixture.name}: ${result.stderr.toString()}`) + const diagnostics: unknown = JSON.parse(result.stdout.toString()) + if (!hasDiagnostics(diagnostics)) throw new Error(`${fixture.name}: invalid Oxlint output`) + const errors = diagnostics.diagnostics.filter(isErrorDiagnostic) + if (errors.length !== fixture.errors || !errors.every(isRuleDiagnostic)) + throw new Error( + `${fixture.name}: expected ${fixture.errors} errors, received ${errors.length}\n${result.stdout.toString()}`, + ) + const exitCode = fixture.errors === 0 ? 0 : 1 + if (result.exitCode !== exitCode || result.signalCode) + throw new Error(`${fixture.name}: expected exit ${exitCode}, received ${result.exitCode}`) + }), + ) } finally { await rm(directory, { recursive: true, force: true }) } @@ -119,3 +161,16 @@ console.log(`Validated ${cases.length} tagged error lint fixtures`) function hasDiagnostics(value: unknown): value is { diagnostics: unknown[] } { return typeof value === "object" && value !== null && "diagnostics" in value && Array.isArray(value.diagnostics) } + +function isRuleDiagnostic(value: unknown) { + return ( + typeof value === "object" && + value !== null && + "code" in value && + value.code === "opencode(tagged-error-message)" + ) +} + +function isErrorDiagnostic(value: unknown) { + return typeof value === "object" && value !== null && "severity" in value && value.severity === "error" +}