diff --git a/docs/docs/configuration.md b/docs/docs/configuration.md index 2e974a424..785059140 100644 --- a/docs/docs/configuration.md +++ b/docs/docs/configuration.md @@ -172,6 +172,13 @@ linter: html-tag-name-lowercase: severity: warning # Options: error, warning, info, hint + # Set options defined by a specific rule + html-allowed-script-type: + allowedTypes: + - text/javascript + - application/json + allowBlank: false + # Rule with file pattern restrictions html-img-require-alt: # Only apply this rule to files matching these patterns @@ -230,6 +237,8 @@ Each rule can be configured with the following options: - **`only`**: Array of glob patterns - Restrict rule to ONLY these files (can override parent excludes, overrides `include`) - **`exclude`**: Array of glob patterns - Exclude files from this rule (always applied) +Rules may also define their own options. These are documented on the individual rule page and can be set alongside the common options above. Herb validates those options after built-in and project-local rules are loaded, so misspelled option names and values of the wrong type produce configuration errors. + ### Setting the Default for All Rules The `all` pseudo rule sets the default `enabled` state for every rule you don't list explicitly. It's the way to opt into a fully explicit rule set without having to know (and repeat) which rules are on by default: diff --git a/javascript/packages/config/src/config-schema.ts b/javascript/packages/config/src/config-schema.ts index 9334b961d..5266c1c4c 100644 --- a/javascript/packages/config/src/config-schema.ts +++ b/javascript/packages/config/src/config-schema.ts @@ -14,15 +14,21 @@ export const FilesConfigSchema = z.object({ exclude: z.array(z.string()).optional().describe("Glob patterns to exclude (e.g., ['node_modules/**/*', 'vendor/**/*', '**/*.html.erb'])"), }).strict().optional() -const RuleConfigBaseSchema = z.object({ +export const RuleConfigBaseSchema = z.object({ enabled: z.boolean().optional().describe("Whether the rule is enabled"), severity: SeverityConfigSchema.optional().describe("Severity level for the rule"), + autoCorrect: z.boolean().optional().describe("Whether autocorrection is enabled for the rule"), include: z.array(z.string()).optional().describe("Additional glob patterns to include for this rule (additive, ignored when 'only' is present)"), only: z.array(z.string()).optional().describe("Only apply this rule to files matching these glob patterns (overrides all 'include' patterns)"), exclude: z.array(z.string()).optional().describe("Don't apply this rule to files matching these glob patterns"), }) -export const RuleConfigSchema = RuleConfigBaseSchema.optional() +export const BASE_RULE_CONFIG_KEYS = RuleConfigBaseSchema.keyof().options + +// NOTE: Custom options are verified in a second step, and produce `RuleOptionsValidationError` when invalid +const RuleConfigWithOptionsSchema = RuleConfigBaseSchema.catchall(z.unknown()) + +export const RuleConfigSchema = RuleConfigWithOptionsSchema.optional() export const LinterConfigSchema = z.object({ enabled: z.boolean().optional().describe("Whether the linter is enabled"), @@ -30,7 +36,7 @@ export const LinterConfigSchema = z.object({ logLevel: SeveritySchema.optional().describe("Only report diagnostics of this severity or higher (e.g., 'warning' hides info and hint diagnostics from the output and from CI annotations)"), include: z.array(z.string()).optional().describe("Additional glob patterns to include beyond defaults (e.g., ['**/*.xml.erb', 'custom/**/*.html'])"), exclude: z.array(z.string()).optional().describe("Glob patterns to exclude from linting"), - rules: z.record(z.string(), RuleConfigBaseSchema).optional().describe("Per-rule configuration"), + rules: z.record(z.string(), RuleConfigWithOptionsSchema).optional().describe("Per-rule configuration"), }).strict().optional() const RewriterConfigSchema = z.object({ diff --git a/javascript/packages/config/src/config.ts b/javascript/packages/config/src/config.ts index 12a1c7a67..8aed444f2 100644 --- a/javascript/packages/config/src/config.ts +++ b/javascript/packages/config/src/config.ts @@ -4,16 +4,17 @@ import packageJson from "../package.json" import configTemplate from "./config-template.yml" import defaultsYaml from "../../../../lib/herb/defaults.yml" -import { stringify, parse, parseDocument, isMap, isScalar, isAlias, visit } from "yaml" +import { stringify, parse, parseDocument, isMap, isScalar, visit } from "yaml" import { semverGreaterThan } from "@herb-tools/core" import { promises as fs } from "fs" import { fromZodError } from "zod-validation-error" import { deepMerge } from "./merge.js" import { ZodError, z } from "zod" -import { HerbConfigSchema } from "./config-schema.js" +import { BASE_RULE_CONFIG_KEYS, HerbConfigSchema } from "./config-schema.js" +import { omit } from "./utils/omit.js" -import type { FrameworkSchema, TemplateEngineSchema } from "./config-schema.js" +import type { RuleConfigBaseSchema, FrameworkSchema, TemplateEngineSchema } from "./config-schema.js" import type { DiagnosticSeverity } from "@herb-tools/core" @@ -62,14 +63,9 @@ export function resolveSeverity(severity: SeverityConfig, mode: LinterMode): Dia */ export const ALL_RULES_KEY = "all" -export type RuleConfig = { - enabled?: boolean - severity?: SeverityConfig - autoCorrect?: boolean - include?: string[] - only?: string[] - exclude?: string[] -} +export type BaseRuleConfig = z.infer + +export type RuleConfig = BaseRuleConfig & Record export type LinterConfig = { enabled?: boolean @@ -273,6 +269,17 @@ export class Config { return !this.isRuleDisabled(ruleName) } + /** + * Return custom options configured for a rule. + * @param ruleName - The name of the rule to check + * @returns The custom options for the rule + */ + public getRuleOptions(ruleName: string): Record { + const ruleConfig = this.config.linter?.rules?.[ruleName] || {} + + return omit(ruleConfig, BASE_RULE_CONFIG_KEYS) + } + /** * Get the files configuration for a specific tool. * Both include and exclude patterns are additive: diff --git a/javascript/packages/config/src/index.ts b/javascript/packages/config/src/index.ts index dbe462f1b..8a21020b4 100644 --- a/javascript/packages/config/src/index.ts +++ b/javascript/packages/config/src/index.ts @@ -10,6 +10,7 @@ export type { LinterConfig, FormatterConfig, EngineConfig, + BaseRuleConfig, RuleConfig, FilesConfig, LoadOptions, diff --git a/javascript/packages/config/src/utils/omit.ts b/javascript/packages/config/src/utils/omit.ts new file mode 100644 index 000000000..551625968 --- /dev/null +++ b/javascript/packages/config/src/utils/omit.ts @@ -0,0 +1,9 @@ +export function omit(object: Record, keys: readonly string[]): Record { + const result = { ...object } + + for (const key of keys) { + delete result[key] + } + + return result +} diff --git a/javascript/packages/config/test/config.test.ts b/javascript/packages/config/test/config.test.ts index 323942430..da6dc464e 100644 --- a/javascript/packages/config/test/config.test.ts +++ b/javascript/packages/config/test/config.test.ts @@ -1610,6 +1610,41 @@ describe("@herb-tools/config", () => { }) }) + describe("custom rule options", () => { + test("loads arbitrary options from YAML", async () => { + createTestFile(testDir, ".herb.yml", dedent` + version: 0.10.3 + + linter: + rules: + html-allowed-script-type: + enabled: true + severity: warning + autoCorrect: true + include: + - app/views/**/*.erb + only: + - app/views/scripts/**/*.erb + exclude: + - app/views/scripts/legacy/**/*.erb + allowedTypes: + - text/javascript + - application/json + allowBlank: false + nested: + mode: strict + `) + + const config = await Config.load(testDir, { version: "0.10.3", silent: true }) + + expect(config.getRuleOptions("html-allowed-script-type")).toEqual({ + allowedTypes: ["text/javascript", "application/json"], + allowBlank: false, + nested: { mode: "strict" } + }) + }) + }) + describe("YAML anchors and aliases", () => { test("loads configuration using YAML anchors and aliases", async () => { createTestFile(testDir, ".herb.yml", dedent` diff --git a/javascript/packages/linter/package.json b/javascript/packages/linter/package.json index 7912f180e..6384e55d0 100644 --- a/javascript/packages/linter/package.json +++ b/javascript/packages/linter/package.json @@ -56,7 +56,8 @@ "@herb-tools/rewriter": "0.10.3", "@ruby/prism": "^1.9.0", "picomatch": "^4.0.5", - "tinyglobby": "^0.2.15" + "tinyglobby": "^0.2.15", + "zod": "^4.4.3" }, "files": [ "package.json", diff --git a/javascript/packages/linter/src/linter.ts b/javascript/packages/linter/src/linter.ts index 2f385b19f..03b266b33 100644 --- a/javascript/packages/linter/src/linter.ts +++ b/javascript/packages/linter/src/linter.ts @@ -1,4 +1,5 @@ import picomatch from "picomatch" +import { ZodError } from "zod" import { Location, semverGreaterThan } from "@herb-tools/core" import { IdentityPrinter, IndentPrinter } from "@herb-tools/printer" @@ -9,8 +10,6 @@ import { parseHerbDisableLine } from "./herb-disable-comment-utils.js" import { hasLinterIgnoreDirective } from "./linter-ignore.js" import { ParseCache } from "./parse-cache.js" -import { ParserNoErrorsRule } from "./rules/parser-no-errors.js" - import { DEFAULT_RULE_CONFIG } from "./types.js" import { resolveSeverity, ALL_RULES_KEY } from "@herb-tools/config" @@ -65,6 +64,41 @@ export interface FilterRulesOptions { all?: boolean } +export class RuleOptionsValidationError extends Error { + constructor(ruleName: string, problem: string) { + super(`${ruleName}: ${problem}`) + } +} + +export class UnknownRuleOptionsError extends RuleOptionsValidationError { + constructor(ruleName: string, issue: Extract) { + const prefix = issue.path.map(String) + const names = issue.keys.map(key => [...prefix, key].join(".")) + + super(ruleName, `Unknown options: ${names.join(", ")}`) + } +} + +export class InvalidRuleOptionsError extends RuleOptionsValidationError { + constructor(ruleName: string, issue: ZodError["issues"][number]) { + const optionName = issue.path.map(String).join(".") + const message = issue.message.replace(/^Invalid input: /, "") + const problem = optionName ? `${optionName}: ${message}` : message + + super(ruleName, `Invalid options: ${problem}`) + } +} + +function validationErrorFor(ruleName: string, error: ZodError): RuleOptionsValidationError { + const issue = error.issues[0]! + + if (issue.code === "unrecognized_keys") { + return new UnknownRuleOptionsError(ruleName, issue) + } else { + return new InvalidRuleOptionsError(ruleName, issue) + } +} + export class Linter { public rules: RuleClass[] public rulesSkippedByVersion: VersionSkippedRule[] = [] @@ -122,6 +156,30 @@ export class Linter { this.rules = rules !== undefined ? rules : this.getDefaultRules() this.allAvailableRules = allAvailableRules !== undefined ? allAvailableRules : this.rules this.offenses = [] + this.validateRuleOptions() + } + + /** Create a rule instance with its custom per-rule options applied. */ + protected createRule(ruleClass: RuleClass): Rule { + const rule = new ruleClass() + const options = this.config?.getRuleOptions(ruleClass.ruleName) ?? {} + + try { + return rule.configure(options) + } catch (error) { + if (error instanceof ZodError) { + throw validationErrorFor(ruleClass.ruleName, error) + } + + throw error + } + } + + /** Validate custom options after built-in and project-local rules are available. */ + protected validateRuleOptions(): void { + for (const ruleClass of this.allAvailableRules) { + this.createRule(ruleClass) + } } /** @@ -464,8 +522,8 @@ export class Linter { const hasParserRule = this.findRuleClass("parser-no-errors") if (hasParserRule) { - const rule = new ParserNoErrorsRule() - const offenses = rule.check(parseResult) + const rule = this.createRule(hasParserRule) as ParserRule + const offenses = rule.check(parseResult) as LintOffense[] this.offenses.push(...offenses) } @@ -493,7 +551,7 @@ export class Linter { const regularRules = this.rules.filter(ruleClass => ruleClass.ruleName !== "herb-disable-comment-unnecessary") for (const ruleClass of regularRules) { - const rule = new ruleClass() + const rule = this.createRule(ruleClass) const parserOptions = this.isParserRuleClass(ruleClass) ? (rule as ParserRule).parserOptions : {} const parseResult = this.parseCache.get(source, parserOptions) @@ -522,7 +580,7 @@ export class Linter { const unnecessaryRuleClass = this.findRuleClass("herb-disable-comment-unnecessary") if (unnecessaryRuleClass) { - const unnecessaryRule = new unnecessaryRuleClass() as ParserRule + const unnecessaryRule = this.createRule(unnecessaryRuleClass) as ParserRule const parseResult = this.parseCache.get(source, unnecessaryRule.parserOptions) const unboundOffenses = unnecessaryRule.check(parseResult, context) const boundOffenses = this.bindSeverity(unboundOffenses, unnecessaryRuleClass.ruleName) @@ -646,7 +704,7 @@ export class Linter { continue } - const rule = new ruleClass() as ParserRule + const rule = this.createRule(ruleClass) as ParserRule const isUnsafe = (ruleClass as any).unsafeAutocorrectable === true || offense.autofixContext?.unsafe === true if (!rule.autofix) { @@ -719,7 +777,7 @@ export class Linter { continue } - const rule = new ruleClass() as SourceRule + const rule = this.createRule(ruleClass) as SourceRule const isUnsafe = (ruleClass as any).unsafeAutocorrectable === true || offense.autofixContext?.unsafe === true if (!rule.autofix) { diff --git a/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts b/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts index 1d2a8db83..1f71327a2 100644 --- a/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts +++ b/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts @@ -1,11 +1,12 @@ import { ParserRule } from "../types.js" import { PrismVisitor, substringFromByteOffset , locationFromByteOffset } from "@herb-tools/core" import { BaseRuleVisitor } from "./rule-utils.js" +import { z } from "zod" import { isERBOutputNode, isRubyParameterNode, isPrismNodeType } from "@herb-tools/core" import { isAssignmentNode, isDebugOutputCall, isSleepCall, isCallOnLocal, SIDE_EFFECT_METHODS } from "./prism-rule-utils.js" -import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js" +import type { BaseAutofixContext, UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js" import type { ParseResult, ERBContentNode, ERBRenderNode, ERBBlockNode, ParserOptions, PrismNode } from "@herb-tools/core" const MUTATION_METHODS = new Set([ @@ -25,14 +26,20 @@ const MUTATION_METHODS = new Set([ "assert_valid_keys", ]) +export interface ERBNoUnusedExpressionsOptions { + allowedMethods: string[] +} + class UnusedExpressionCollector extends PrismVisitor { public readonly expressions: PrismNode[] = [] private readonly blockLocalNames: Set + private readonly allowedMethods: Set - constructor(blockLocalNames: Set = new Set()) { + constructor(blockLocalNames: Set = new Set(), allowedMethods: string[] = []) { super() this.blockLocalNames = blockLocalNames + this.allowedMethods = new Set([...SIDE_EFFECT_METHODS, ...allowedMethods]) } override visit(node: PrismNode): void { @@ -59,7 +66,7 @@ class UnusedExpressionCollector extends PrismVisitor { private isSideEffectCall(node: PrismNode): boolean { if (node.receiver) return false - return SIDE_EFFECT_METHODS.has(node.name) + return this.allowedMethods.has(node.name) } private isUnusedExpression(node: PrismNode): boolean { @@ -87,6 +94,13 @@ class UnusedExpressionCollector extends PrismVisitor { class ERBNoUnusedExpressionsVisitor extends BaseRuleVisitor { private exemptLocalNames: Set = new Set() + private readonly options: ERBNoUnusedExpressionsOptions + + constructor(ruleName: string, context: Partial | undefined, options: ERBNoUnusedExpressionsOptions) { + super(ruleName, context) + + this.options = options + } visitERBRenderNode(node: ERBRenderNode): void { this.visitExemptingBlockArguments(node) @@ -134,7 +148,7 @@ class ERBNoUnusedExpressionsVisitor extends BaseRuleVisitor { const source = node.source if (!source) return - const collector = new UnusedExpressionCollector(this.exemptLocalNames) + const collector = new UnusedExpressionCollector(this.exemptLocalNames, this.options.allowedMethods) collector.visit(prismNode) const tagOpening = node.tag_opening?.value ?? "<%" @@ -160,7 +174,7 @@ class ERBNoUnusedExpressionsVisitor extends BaseRuleVisitor { } } -export class ERBNoUnusedExpressionsRule extends ParserRule { +export class ERBNoUnusedExpressionsRule extends ParserRule { static ruleName = "erb-no-unused-expressions" static introducedIn = this.version("0.9.3") @@ -174,6 +188,18 @@ export class ERBNoUnusedExpressionsRule extends ParserRule { } } + get defaultOptions(): ERBNoUnusedExpressionsOptions { + return { + allowedMethods: [] + } + } + + get optionsSchema(): z.ZodType { + return z.object({ + allowedMethods: z.array(z.string()) + }).strict() + } + get parserOptions(): Partial { return { prism_nodes: true, @@ -182,7 +208,7 @@ export class ERBNoUnusedExpressionsRule extends ParserRule { } check(result: ParseResult, context?: Partial): UnboundLintOffense[] { - const visitor = new ERBNoUnusedExpressionsVisitor(this.ruleName, context) + const visitor = new ERBNoUnusedExpressionsVisitor(this.ruleName, context, this.options) visitor.visit(result.value) diff --git a/javascript/packages/linter/src/rules/html-allowed-script-type.ts b/javascript/packages/linter/src/rules/html-allowed-script-type.ts index a2279c47e..362e7c618 100644 --- a/javascript/packages/linter/src/rules/html-allowed-script-type.ts +++ b/javascript/packages/linter/src/rules/html-allowed-script-type.ts @@ -1,16 +1,25 @@ import { ParserRule } from "../types.js" import { BaseRuleVisitor } from "./rule-utils.js" import { getTagLocalName, getAttribute, getStaticAttributeValue, hasAttributeValue } from "@herb-tools/core" +import { z } from "zod" -import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js" +import type { BaseAutofixContext, UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js" import type { HTMLAttributeNode, HTMLOpenTagNode, ParseResult } from "@herb-tools/core" -// NOTE: Rules are not configurable for now, keep some sane defaults -// See https://github.com/marcoroth/herb/issues/1204 -const ALLOW_BLANK = true -const ALLOWED_TYPES = ["text/javascript", "module", "importmap", "speculationrules", "application/ld+json"] +export interface HTMLAllowedScriptTypeOptions { + allowedTypes: string[] + allowBlank: boolean +} class AllowedScriptTypeVisitor extends BaseRuleVisitor { + private readonly options: HTMLAllowedScriptTypeOptions + + constructor(ruleName: string, context: Partial | undefined, options: HTMLAllowedScriptTypeOptions) { + super(ruleName, context) + + this.options = options + } + visitHTMLOpenTagNode(node: HTMLOpenTagNode): void { if (getTagLocalName(node) === "script") { this.visitScriptNode(node) @@ -18,10 +27,11 @@ class AllowedScriptTypeVisitor extends BaseRuleVisitor { } private visitScriptNode(node: HTMLOpenTagNode): void { + const { allowBlank } = this.options const typeAttribute = getAttribute(node, "type") if (!typeAttribute) { - if (!ALLOW_BLANK) { + if (!allowBlank) { this.addOffense("`type` attribute required for `') + }) + + test("replaces the default allowed types", () => { + configuredAllowedTypes.expectError("Avoid using `text/javascript` as the `type` attribute for the `') + }) + }) + + describe("with blank types disallowed", () => { + const blankTypesDisallowed = createLinterTest(HTMLAllowedScriptTypeRule, { + allowBlank: false + }) + + test("can require an explicit type", () => { + blankTypesDisallowed.expectError("`type` attribute required for `") + }) + + test("merges configured options with rule defaults", () => { + blankTypesDisallowed.expectNoOffenses('') + }) + }) })