From 8e15a50a5eb269caee7d0eb101750bb0dab0a128 Mon Sep 17 00:00:00 2001 From: Marko Kajzer Date: Tue, 11 Aug 2026 23:10:08 +0200 Subject: [PATCH 1/6] linter: support custom rule options --- .../packages/config/src/config-schema.ts | 12 ++- javascript/packages/config/src/config.ts | 29 ++++--- javascript/packages/config/src/index.ts | 1 + javascript/packages/config/src/utils/omit.ts | 9 ++ .../packages/config/test/config.test.ts | 35 ++++++++ javascript/packages/linter/src/linter.ts | 26 ++++-- javascript/packages/linter/src/types.ts | 83 +++++++++++++++++-- 7 files changed, 164 insertions(+), 31 deletions(-) create mode 100644 javascript/packages/config/src/utils/omit.ts diff --git a/javascript/packages/config/src/config-schema.ts b/javascript/packages/config/src/config-schema.ts index 9334b961d..ac2589bad 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 BaseRuleConfigSchema = 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 = BaseRuleConfigSchema.keyof().options + +// NOTE: Custom options are verified in a second step, and produce `RuleOptionsValidationError` when invalid +const RuleConfigWithOptionsSchema = BaseRuleConfigSchema.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..8f4907618 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 { BaseRuleConfigSchema, 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/src/linter.ts b/javascript/packages/linter/src/linter.ts index 2f385b19f..9ca543054 100644 --- a/javascript/packages/linter/src/linter.ts +++ b/javascript/packages/linter/src/linter.ts @@ -9,8 +9,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" @@ -124,6 +122,18 @@ export class Linter { this.offenses = [] } + /** Create a rule instance with its custom per-rule options applied. */ + protected createRule(ruleClass: RuleClass): Rule { + const rule = new ruleClass() + const configure = (rule as { configure?: (options: Record) => Rule }).configure + + if (typeof configure === "function") { + return configure.call(rule, this.config?.getRuleOptions(ruleClass.ruleName) ?? {}) + } + + return rule + } + /** * Filters rules based on default config, user config overrides, and version gating. * @@ -464,8 +474,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 +503,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 +532,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 +656,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 +729,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/types.ts b/javascript/packages/linter/src/types.ts index c841c9b70..89e7aea89 100644 --- a/javascript/packages/linter/src/types.ts +++ b/javascript/packages/linter/src/types.ts @@ -4,7 +4,7 @@ import type { DiagnosticTag, HerbError } from "@herb-tools/core" import type { rules } from "./rules.js" import type { HerbBackend, Node, ParserOptions } from "@herb-tools/core" import type { AncestorChain, RenderGraph, PartialIndex } from "@herb-tools/analysis" -import type { Framework, RuleConfig, SeverityConfig, LinterMode } from "@herb-tools/config" +import type { Framework, BaseRuleConfig, SeverityConfig, LinterMode } from "@herb-tools/config" import type { Mutable } from "@herb-tools/rewriter" import type { RuleVersion } from "@herb-tools/core" @@ -20,7 +20,9 @@ export const DEFAULT_LINTER_PARSER_OPTIONS: Partial = { track_whitespace: true, } -export type FullRuleConfig = Required> & Omit +export type FullRuleConfig = Required> & Omit + +export type RuleOptions = Record /** * Automatically inferred union type of all available linter rule names. @@ -100,7 +102,10 @@ export const DEFAULT_RULE_CONFIG: FullRuleConfig = { /** * Base class for parser rules. */ -export abstract class ParserRule { +export abstract class ParserRule< + TAutofixContext extends BaseAutofixContext = BaseAutofixContext, + TOptions extends object = RuleOptions +> { static type = "parser" as const static ruleName: string /** The version in which this rule was introduced. Used for version-gated rule filtering. */ @@ -120,6 +125,24 @@ export abstract class ParserRule = {}): this { + this.configuredOptions = { ...this.defaultOptions, ...options } + + return this + } + get ruleName(): string { return (this.constructor as typeof ParserRule).ruleName } @@ -181,7 +204,10 @@ export abstract class ParserRule { +export abstract class LexerRule< + TAutofixContext extends BaseAutofixContext = BaseAutofixContext, + TOptions extends object = RuleOptions +> { static type = "lexer" as const static ruleName: string /** The version in which this rule was introduced. Used for version-gated rule filtering. */ @@ -196,6 +222,24 @@ export abstract class LexerRule = {}): this { + this.configuredOptions = { ...this.defaultOptions, ...options } + + return this + } + get ruleName(): string { return (this.constructor as typeof LexerRule).ruleName } @@ -241,7 +285,7 @@ export abstract class LexerRule ruleName: string introducedIn: RuleVersion autocorrectable?: boolean @@ -285,7 +329,10 @@ export const DEFAULT_LINT_CONTEXT: LintContext = { herb: undefined } as const -export abstract class SourceRule { +export abstract class SourceRule< + TAutofixContext extends BaseAutofixContext = BaseAutofixContext, + TOptions extends object = RuleOptions +> { static type = "source" as const static ruleName: string /** The version in which this rule was introduced. Used for version-gated rule filtering. */ @@ -300,6 +347,24 @@ export abstract class SourceRule = {}): this { + this.configuredOptions = { ...this.defaultOptions, ...options } + + return this + } + get ruleName(): string { return (this.constructor as typeof SourceRule).ruleName } @@ -345,7 +410,7 @@ export abstract class SourceRule ruleName: string introducedIn: RuleVersion autocorrectable?: boolean @@ -359,7 +424,7 @@ export interface SourceRuleConstructor { * The Linter accepts rule classes rather than instances for better performance and memory usage. * Parser rules are the default and don't require static properties. */ -export type ParserRuleClass = (new () => ParserRule) & { +export type ParserRuleClass = (new () => ParserRule) & { type?: "parser" ruleName: string introducedIn: RuleVersion @@ -377,7 +442,7 @@ export type SourceRuleClass = SourceRuleConstructor /** * Union type for any rule instance (Parser/AST, Lexer, or Source) */ -export type Rule = ParserRule | LexerRule | SourceRule +export type Rule = ParserRule | LexerRule | SourceRule /** * Union type for any rule class (Parser/AST, Lexer, or Source) From 390ca038e0bb006983f1e68b48f0bb960d90423c Mon Sep 17 00:00:00 2001 From: Marko Kajzer Date: Tue, 11 Aug 2026 23:10:47 +0200 Subject: [PATCH 2/6] feat(html-allowed-script-type): support allowedTypes --- docs/docs/configuration.md | 8 ++++ .../docs/rules/html-allowed-script-type.md | 15 ++++++++ .../src/rules/html-allowed-script-type.ts | 37 ++++++++++++------- .../rules/html-allowed-script-type.test.ts | 14 +++++++ 4 files changed, 60 insertions(+), 14 deletions(-) diff --git a/docs/docs/configuration.md b/docs/docs/configuration.md index 2e974a424..4ea510524 100644 --- a/docs/docs/configuration.md +++ b/docs/docs/configuration.md @@ -172,6 +172,12 @@ 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 + # Rule with file pattern restrictions html-img-require-alt: # Only apply this rule to files matching these patterns @@ -230,6 +236,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. + ### 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/linter/docs/rules/html-allowed-script-type.md b/javascript/packages/linter/docs/rules/html-allowed-script-type.md index 3e3f56e9d..0c427538c 100644 --- a/javascript/packages/linter/docs/rules/html-allowed-script-type.md +++ b/javascript/packages/linter/docs/rules/html-allowed-script-type.md @@ -72,6 +72,21 @@ An exception is made for `application/ld+json`, which the HTML specification tre ``` +## Configuration + +The rule accepts these options: + +- `allowedTypes` (`string[]`): accepted static `type` values. Defaults to `text/javascript`, `module`, `importmap`, `speculationrules`, and `application/ld+json`. + +```yaml +linter: + rules: + html-allowed-script-type: + allowedTypes: + - text/javascript + - application/json +``` + ## References - [Inspiration: ERB Lint `AllowedScriptType` rule](https://github.com/Shopify/erb_lint/tree/main?tab=readme-ov-file#allowedscripttype) 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..40b855139 100644 --- a/javascript/packages/linter/src/rules/html-allowed-script-type.ts +++ b/javascript/packages/linter/src/rules/html-allowed-script-type.ts @@ -2,15 +2,22 @@ import { ParserRule } from "../types.js" import { BaseRuleVisitor } from "./rule-utils.js" import { getTagLocalName, getAttribute, getStaticAttributeValue, hasAttributeValue } from "@herb-tools/core" -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[] +} 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) @@ -21,10 +28,6 @@ class AllowedScriptTypeVisitor extends BaseRuleVisitor { const typeAttribute = getAttribute(node, "type") if (!typeAttribute) { - if (!ALLOW_BLANK) { - this.addOffense("`type` attribute required for `') + }) + + test("replaces the default allowed types", () => { + customOptions.expectError("Avoid using `text/javascript` as the `type` attribute for the `') + }) + }) }) From f8d7873939c0f14447fda94c7aaa9a798576043d Mon Sep 17 00:00:00 2001 From: Marko Kajzer Date: Tue, 11 Aug 2026 23:11:18 +0200 Subject: [PATCH 3/6] feat(html-allowed-script-type): support allowBlank --- docs/docs/configuration.md | 1 + .../docs/rules/html-allowed-script-type.md | 2 ++ .../src/rules/html-allowed-script-type.ts | 16 +++++++++---- .../rules/html-allowed-script-type.test.ts | 24 +++++++++++++++---- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/docs/configuration.md b/docs/docs/configuration.md index 4ea510524..a3ed61625 100644 --- a/docs/docs/configuration.md +++ b/docs/docs/configuration.md @@ -177,6 +177,7 @@ linter: allowedTypes: - text/javascript - application/json + allowBlank: false # Rule with file pattern restrictions html-img-require-alt: diff --git a/javascript/packages/linter/docs/rules/html-allowed-script-type.md b/javascript/packages/linter/docs/rules/html-allowed-script-type.md index 0c427538c..2c7622c15 100644 --- a/javascript/packages/linter/docs/rules/html-allowed-script-type.md +++ b/javascript/packages/linter/docs/rules/html-allowed-script-type.md @@ -77,6 +77,7 @@ An exception is made for `application/ld+json`, which the HTML specification tre The rule accepts these options: - `allowedTypes` (`string[]`): accepted static `type` values. Defaults to `text/javascript`, `module`, `importmap`, `speculationrules`, and `application/ld+json`. +- `allowBlank` (`boolean`): whether a script may omit the `type` attribute. Defaults to `true`. ```yaml linter: @@ -85,6 +86,7 @@ linter: allowedTypes: - text/javascript - application/json + allowBlank: false ``` ## References 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 40b855139..68773adff 100644 --- a/javascript/packages/linter/src/rules/html-allowed-script-type.ts +++ b/javascript/packages/linter/src/rules/html-allowed-script-type.ts @@ -7,6 +7,7 @@ import type { HTMLAttributeNode, HTMLOpenTagNode, ParseResult } from "@herb-tool export interface HTMLAllowedScriptTypeOptions { allowedTypes: string[] + allowBlank: boolean } class AllowedScriptTypeVisitor extends BaseRuleVisitor { @@ -25,9 +26,14 @@ class AllowedScriptTypeVisitor extends BaseRuleVisitor { } private visitScriptNode(node: HTMLOpenTagNode): void { + const { allowBlank } = this.options const typeAttribute = getAttribute(node, "type") if (!typeAttribute) { + if (!allowBlank) { + this.addOffense("`type` attribute required for `') + configuredAllowedTypes.expectNoOffenses('') }) test("replaces the default allowed types", () => { - customOptions.expectError("Avoid using `text/javascript` as the `type` attribute for the `') + configuredAllowedTypes.expectError("Avoid using `text/javascript` as the `type` attribute for the `') + }) + }) + + describe("with blank types disallowed", () => { + test("can require an explicit type", () => { + blankTypesDisallowed.expectError("`type` attribute required for `") + }) + + test("merges configured options with rule defaults", () => { + blankTypesDisallowed.expectNoOffenses('') }) }) }) From bafe021a81284da59caf41ac061b66bac23d0619 Mon Sep 17 00:00:00 2001 From: Marko Kajzer Date: Tue, 11 Aug 2026 23:11:34 +0200 Subject: [PATCH 4/6] feat(erb-no-unused-expressions): support allowedMethods --- .../docs/rules/erb-no-unused-expressions.md | 14 +++++++++ .../src/rules/erb-no-unused-expressions.ts | 31 +++++++++++++++---- .../rules/erb-no-unused-expressions.test.ts | 11 +++++++ 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md b/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md index b7287e86e..4791ab60f 100644 --- a/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md +++ b/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md @@ -81,3 +81,17 @@ ViewComponent slot setters are intentional side effects and are not flagged, inc ```erb <% User.count %> ``` + +## Configuration + +The rule accepts these options: + +- `allowedMethods` (`string[]`): additional receiverless methods whose return values may be intentionally ignored. Defaults to `[]`. + +```yaml +linter: + rules: + erb-no-unused-expressions: + allowedMethods: + - breadcrumb +``` 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..d3f9893f8 100644 --- a/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts +++ b/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts @@ -5,7 +5,7 @@ import { BaseRuleVisitor } from "./rule-utils.js" 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 +25,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: Set = SIDE_EFFECT_METHODS) { super() this.blockLocalNames = blockLocalNames + this.allowedMethods = allowedMethods } override visit(node: PrismNode): void { @@ -59,7 +65,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 +93,13 @@ class UnusedExpressionCollector extends PrismVisitor { class ERBNoUnusedExpressionsVisitor extends BaseRuleVisitor { private exemptLocalNames: Set = new Set() + private readonly allowedMethods: Set + + constructor(ruleName: string, context: Partial | undefined, options: ERBNoUnusedExpressionsOptions) { + super(ruleName, context) + + this.allowedMethods = new Set([...SIDE_EFFECT_METHODS, ...options.allowedMethods]) + } visitERBRenderNode(node: ERBRenderNode): void { this.visitExemptingBlockArguments(node) @@ -134,7 +147,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.allowedMethods) collector.visit(prismNode) const tagOpening = node.tag_opening?.value ?? "<%" @@ -160,7 +173,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 +187,12 @@ export class ERBNoUnusedExpressionsRule extends ParserRule { } } + get defaultOptions(): ERBNoUnusedExpressionsOptions { + return { + allowedMethods: [] + } + } + get parserOptions(): Partial { return { prism_nodes: true, @@ -182,7 +201,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/test/rules/erb-no-unused-expressions.test.ts b/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts index 51caeb22c..380d5284b 100644 --- a/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts +++ b/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts @@ -5,9 +5,20 @@ import { ERBNoUnusedExpressionsRule } from "../../src/rules/erb-no-unused-expres import { createLinterTest } from "../helpers/linter-test-helper.js" const { expectNoOffenses, expectError, expectWarning, assertOffenses } = createLinterTest(ERBNoUnusedExpressionsRule) +const customOptions = createLinterTest(ERBNoUnusedExpressionsRule, { + allowedMethods: ["breadcrumb"] +}) describe("ERBNoUnusedExpressionsRule", () => { describe("valid cases", () => { + test("passes for configured side-effect methods", () => { + customOptions.expectNoOffenses('<% breadcrumb :projects %>') + }) + + test("keeps the built-in side-effect methods when configured", () => { + customOptions.expectNoOffenses('<% content_for :title, "Projects" %>') + }) + test("passes for output tags with method calls", () => { expectNoOffenses(dedent` <%= @user.name %> From 7458257c6af0a82fb8f65b1ba7c3ad6ed623187d Mon Sep 17 00:00:00 2001 From: Marko Kajzer Date: Tue, 11 Aug 2026 23:19:59 +0200 Subject: [PATCH 5/6] linter: validate custom rule options --- docs/docs/configuration.md | 2 +- javascript/packages/linter/package.json | 3 +- javascript/packages/linter/src/linter.ts | 74 ++++++++- .../src/rules/erb-no-unused-expressions.ts | 7 + .../src/rules/html-allowed-script-type.ts | 8 + javascript/packages/linter/src/types.ts | 37 ++++- .../packages/linter/test/linter.test.ts | 147 +++++++++++++++++- 7 files changed, 265 insertions(+), 13 deletions(-) diff --git a/docs/docs/configuration.md b/docs/docs/configuration.md index a3ed61625..785059140 100644 --- a/docs/docs/configuration.md +++ b/docs/docs/configuration.md @@ -237,7 +237,7 @@ 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. +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 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 9ca543054..f2871db73 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" @@ -63,6 +64,33 @@ export interface FilterRulesOptions { all?: boolean } +export class RuleOptionsValidationError extends Error { + constructor(ruleName: string, problem: string) { + super(`${ruleName}: ${problem}`) + } +} + +function unknownOptionsError(ruleName: string, optionNames: string[], path: PropertyKey[] = []): RuleOptionsValidationError { + const prefix = path.map(String) + const names = optionNames.map(key => [...prefix, key].join(".")) + + return new RuleOptionsValidationError(ruleName, `Unknown options: ${names.join(", ")}`) +} + +function validationErrorFor(ruleName: string, error: ZodError): RuleOptionsValidationError { + const issue = error.issues[0]! + + if (issue.code === "unrecognized_keys") { + return unknownOptionsError(ruleName, issue.keys, issue.path) + } + + const optionName = issue.path.map(String).join(".") + const message = issue.message.replace(/^Invalid input: /, "") + const problem = optionName ? `${optionName}: ${message}` : message + + return new RuleOptionsValidationError(ruleName, `Invalid options: ${problem}`) +} + export class Linter { public rules: RuleClass[] public rulesSkippedByVersion: VersionSkippedRule[] = [] @@ -88,7 +116,13 @@ export class Linter { * @returns A configured Linter instance */ static from(herb: HerbBackend, config?: Config, customRules?: RuleClass[], options?: FilterRulesOptions): Linter { - const allRules = customRules ? [...rules, ...customRules] : rules + const availableRules = new Map(rules.map(ruleClass => [ruleClass.ruleName, ruleClass])) + + for (const ruleClass of customRules ?? []) { + availableRules.set(ruleClass.ruleName, ruleClass) + } + + const allRules = [...availableRules.values()] const configVersion = config?.configVersion const filterResult = Linter.filterRulesByConfig(allRules, config?.linter?.rules, configVersion, options) @@ -120,18 +154,50 @@ 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) ?? {} const configure = (rule as { configure?: (options: Record) => Rule }).configure - if (typeof configure === "function") { - return configure.call(rule, this.config?.getRuleOptions(ruleClass.ruleName) ?? {}) + if (typeof configure !== "function") { + const optionNames = Object.keys(options) + + if (optionNames.length > 0) { + throw unknownOptionsError(ruleClass.ruleName, optionNames) + } + + return rule + } + + try { + return configure.call(rule, options) + } catch (error) { + if (error instanceof ZodError) { + throw validationErrorFor(ruleClass.ruleName, error) + } + + throw error } + } - return rule + /** Validate custom options after built-in and project-local rules are available. */ + protected validateRuleOptions(): void { + const allOptions = this.config?.getRuleOptions(ALL_RULES_KEY) ?? {} + const allOptionNames = Object.keys(allOptions) + + if (allOptionNames.length > 0) { + throw unknownOptionsError(ALL_RULES_KEY, allOptionNames) + } + + const availableRules = new Map(this.allAvailableRules.map(ruleClass => [ruleClass.ruleName, ruleClass])) + + for (const ruleClass of availableRules.values()) { + this.createRule(ruleClass) + } } /** 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 d3f9893f8..c17949cb3 100644 --- a/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts +++ b/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts @@ -1,6 +1,7 @@ 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" @@ -193,6 +194,12 @@ export class ERBNoUnusedExpressionsRule extends ParserRule { + return z.object({ + allowedMethods: z.array(z.string()) + }).strict() + } + get parserOptions(): Partial { return { prism_nodes: true, 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 68773adff..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,6 +1,7 @@ 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 { BaseAutofixContext, UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js" import type { HTMLAttributeNode, HTMLOpenTagNode, ParseResult } from "@herb-tools/core" @@ -92,6 +93,13 @@ export class HTMLAllowedScriptTypeRule extends ParserRule { + return z.object({ + allowedTypes: z.array(z.string()), + allowBlank: z.boolean() + }).strict() + } + check(result: ParseResult, context?: Partial): UnboundLintOffense[] { const visitor = new AllowedScriptTypeVisitor(this.ruleName, context, this.options) diff --git a/javascript/packages/linter/src/types.ts b/javascript/packages/linter/src/types.ts index 89e7aea89..d70658209 100644 --- a/javascript/packages/linter/src/types.ts +++ b/javascript/packages/linter/src/types.ts @@ -1,4 +1,5 @@ import { Diagnostic, LexResult, ParseResult, Location } from "@herb-tools/core" +import { z } from "zod" import type { DiagnosticTag, HerbError } from "@herb-tools/core" import type { rules } from "./rules.js" @@ -131,14 +132,22 @@ export abstract class ParserRule< return {} as TOptions } + get optionsSchema(): z.ZodType { + return z.object({}).strict() as z.ZodType + } + get options(): TOptions { - this.configuredOptions ??= this.defaultOptions + this.configuredOptions ??= this.validateOptions() return this.configuredOptions } + validateOptions(options: Partial = {}): TOptions { + return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) + } + configure(options: Partial = {}): this { - this.configuredOptions = { ...this.defaultOptions, ...options } + this.configuredOptions = this.validateOptions(options) return this } @@ -228,14 +237,22 @@ export abstract class LexerRule< return {} as TOptions } + get optionsSchema(): z.ZodType { + return z.object({}).strict() as z.ZodType + } + get options(): TOptions { - this.configuredOptions ??= this.defaultOptions + this.configuredOptions ??= this.validateOptions() return this.configuredOptions } + validateOptions(options: Partial = {}): TOptions { + return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) + } + configure(options: Partial = {}): this { - this.configuredOptions = { ...this.defaultOptions, ...options } + this.configuredOptions = this.validateOptions(options) return this } @@ -353,14 +370,22 @@ export abstract class SourceRule< return {} as TOptions } + get optionsSchema(): z.ZodType { + return z.object({}).strict() as z.ZodType + } + get options(): TOptions { - this.configuredOptions ??= this.defaultOptions + this.configuredOptions ??= this.validateOptions() return this.configuredOptions } + validateOptions(options: Partial = {}): TOptions { + return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) + } + configure(options: Partial = {}): this { - this.configuredOptions = { ...this.defaultOptions, ...options } + this.configuredOptions = this.validateOptions(options) return this } diff --git a/javascript/packages/linter/test/linter.test.ts b/javascript/packages/linter/test/linter.test.ts index 3ff196031..58b66cbcc 100644 --- a/javascript/packages/linter/test/linter.test.ts +++ b/javascript/packages/linter/test/linter.test.ts @@ -1,5 +1,6 @@ import dedent from "dedent" import { describe, test, expect, beforeAll } from "vitest" +import { z } from "zod" import { Herb } from "@herb-tools/node-wasm" import { Location } from "@herb-tools/core" @@ -11,7 +12,7 @@ import { HTMLAttributeDoubleQuotesRule } from "../src/rules/html-attribute-doubl import { HTMLAttributeValuesRequireQuotesRule } from "../src/rules/html-attribute-values-require-quotes.js" import { ParserRule, SourceRule } from "../src/types.js" -import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../src/types.js" +import type { BaseAutofixContext, UnboundLintOffense, LintContext, FullRuleConfig } from "../src/types.js" import type { ParseResult } from "@herb-tools/core" describe("@herb-tools/linter", () => { @@ -386,6 +387,150 @@ describe("@herb-tools/linter", () => { expect(linter).toBeInstanceOf(Linter) }) + test("uses a custom rule in place of a built-in rule with the same name", () => { + interface CustomOptions { + message: string + } + + class CustomRule extends ParserRule { + static ruleName = HTMLTagNameLowercaseRule.ruleName + static introducedIn = "0.1.0" + + get defaultOptions(): CustomOptions { + return { message: "default" } + } + + get optionsSchema(): z.ZodType { + return z.object({ message: z.string() }).strict() + } + + check(): UnboundLintOffense[] { + return [{ + message: this.options.message, + location: Location.from(1, 1, 1, 1), + rule: this.ruleName, + code: this.ruleName, + source: "linter" + }] + } + } + + const config = Config.fromObject({ + linter: { + rules: { + [CustomRule.ruleName]: { + message: "custom" + } + } + } + }) + const linter = Linter.from(Herb, config, [CustomRule], { only: [CustomRule.ruleName] }) + + expect(linter.rules).toEqual([CustomRule]) + expect(linter.lint("
").offenses.map(offense => offense.message)).toEqual(["custom"]) + }) + + test("gives each rule instance isolated options", () => { + interface MutableOptions { + values: string[] + } + + const observedLengths: number[] = [] + + class MutatingOptionsRule extends ParserRule { + static ruleName = "mutating-options-rule" + static introducedIn = "0.1.0" + + get defaultOptions(): MutableOptions { + return { values: [] } + } + + get optionsSchema(): z.ZodType { + return z.object({ values: z.array(z.string()) }).strict() + } + + check(): UnboundLintOffense[] { + this.options.values.push("value") + observedLengths.push(this.options.values.length) + + return [] + } + } + + const linter = new Linter(Herb, [MutatingOptionsRule]) + + linter.lint("") + linter.lint("") + + expect(observedLengths).toEqual([1, 1]) + }) + + test("validates custom options for disabled rules after the shared config schema", () => { + const config = Config.fromObject({ + linter: { + rules: { + "html-allowed-script-type": { + enabled: false, + allowBlank: "false" + } + } + } + }) + + expect(config.getRuleOptions("html-allowed-script-type")).toEqual({ allowBlank: "false" }) + expect(() => Linter.from(Herb, config)).toThrow( + "html-allowed-script-type: Invalid options: allowBlank: expected boolean, received string" + ) + }) + + test("rejects unknown options for configurable rules", () => { + const config = Config.fromObject({ + linter: { + rules: { + "html-allowed-script-type": { + allowBlakn: false + } + } + } + }) + + expect(() => Linter.from(Herb, config)).toThrow( + "html-allowed-script-type: Unknown options: allowBlakn" + ) + }) + + test("rejects custom options for rules that do not declare any", () => { + const config = Config.fromObject({ + linter: { + rules: { + "html-tag-name-lowercase": { + unexpected: true + } + } + } + }) + + expect(() => new Linter(Herb, [HTMLTagNameLowercaseRule], config)).toThrow( + "html-tag-name-lowercase: Unknown options: unexpected" + ) + }) + + test("rejects custom options on the all pseudo rule", () => { + const config = Config.fromObject({ + linter: { + rules: { + all: { + allowBlank: false + } + } + } + }) + + expect(() => Linter.from(Herb, config)).toThrow( + "all: Unknown options: allowBlank" + ) + }) + test("filters rules based on default config", () => { class EnabledByDefaultRule extends ParserRule { static ruleName = "enabled-by-default-rule" From 31b1ba6b743eb0989b584542675bd0c519b9e2dd Mon Sep 17 00:00:00 2001 From: Marko Kajzer Date: Wed, 12 Aug 2026 02:35:00 +0200 Subject: [PATCH 6/6] refactor: simplify rule options validation --- .../packages/config/src/config-schema.ts | 6 +- javascript/packages/config/src/config.ts | 4 +- .../docs/rules/erb-no-unused-expressions.md | 14 --- .../docs/rules/html-allowed-script-type.md | 17 --- javascript/packages/linter/src/linter.ts | 62 ++++------ .../src/rules/erb-no-unused-expressions.ts | 10 +- javascript/packages/linter/src/types.ts | 112 +++++------------- javascript/packages/linter/test/cli.test.ts | 12 +- .../packages/linter/test/linter.test.ts | 97 +-------------- .../rules/erb-no-unused-expressions.test.ts | 25 ++-- .../rules/html-allowed-script-type.test.ts | 14 ++- 11 files changed, 95 insertions(+), 278 deletions(-) diff --git a/javascript/packages/config/src/config-schema.ts b/javascript/packages/config/src/config-schema.ts index ac2589bad..5266c1c4c 100644 --- a/javascript/packages/config/src/config-schema.ts +++ b/javascript/packages/config/src/config-schema.ts @@ -14,7 +14,7 @@ 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() -export const BaseRuleConfigSchema = 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"), @@ -23,10 +23,10 @@ export const BaseRuleConfigSchema = z.object({ exclude: z.array(z.string()).optional().describe("Don't apply this rule to files matching these glob patterns"), }) -export const BASE_RULE_CONFIG_KEYS = BaseRuleConfigSchema.keyof().options +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 = BaseRuleConfigSchema.catchall(z.unknown()) +const RuleConfigWithOptionsSchema = RuleConfigBaseSchema.catchall(z.unknown()) export const RuleConfigSchema = RuleConfigWithOptionsSchema.optional() diff --git a/javascript/packages/config/src/config.ts b/javascript/packages/config/src/config.ts index 8f4907618..8aed444f2 100644 --- a/javascript/packages/config/src/config.ts +++ b/javascript/packages/config/src/config.ts @@ -14,7 +14,7 @@ import { ZodError, z } from "zod" import { BASE_RULE_CONFIG_KEYS, HerbConfigSchema } from "./config-schema.js" import { omit } from "./utils/omit.js" -import type { BaseRuleConfigSchema, FrameworkSchema, TemplateEngineSchema } from "./config-schema.js" +import type { RuleConfigBaseSchema, FrameworkSchema, TemplateEngineSchema } from "./config-schema.js" import type { DiagnosticSeverity } from "@herb-tools/core" @@ -63,7 +63,7 @@ export function resolveSeverity(severity: SeverityConfig, mode: LinterMode): Dia */ export const ALL_RULES_KEY = "all" -export type BaseRuleConfig = z.infer +export type BaseRuleConfig = z.infer export type RuleConfig = BaseRuleConfig & Record diff --git a/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md b/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md index 4791ab60f..b7287e86e 100644 --- a/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md +++ b/javascript/packages/linter/docs/rules/erb-no-unused-expressions.md @@ -81,17 +81,3 @@ ViewComponent slot setters are intentional side effects and are not flagged, inc ```erb <% User.count %> ``` - -## Configuration - -The rule accepts these options: - -- `allowedMethods` (`string[]`): additional receiverless methods whose return values may be intentionally ignored. Defaults to `[]`. - -```yaml -linter: - rules: - erb-no-unused-expressions: - allowedMethods: - - breadcrumb -``` diff --git a/javascript/packages/linter/docs/rules/html-allowed-script-type.md b/javascript/packages/linter/docs/rules/html-allowed-script-type.md index 2c7622c15..3e3f56e9d 100644 --- a/javascript/packages/linter/docs/rules/html-allowed-script-type.md +++ b/javascript/packages/linter/docs/rules/html-allowed-script-type.md @@ -72,23 +72,6 @@ An exception is made for `application/ld+json`, which the HTML specification tre ``` -## Configuration - -The rule accepts these options: - -- `allowedTypes` (`string[]`): accepted static `type` values. Defaults to `text/javascript`, `module`, `importmap`, `speculationrules`, and `application/ld+json`. -- `allowBlank` (`boolean`): whether a script may omit the `type` attribute. Defaults to `true`. - -```yaml -linter: - rules: - html-allowed-script-type: - allowedTypes: - - text/javascript - - application/json - allowBlank: false -``` - ## References - [Inspiration: ERB Lint `AllowedScriptType` rule](https://github.com/Shopify/erb_lint/tree/main?tab=readme-ov-file#allowedscripttype) diff --git a/javascript/packages/linter/src/linter.ts b/javascript/packages/linter/src/linter.ts index f2871db73..03b266b33 100644 --- a/javascript/packages/linter/src/linter.ts +++ b/javascript/packages/linter/src/linter.ts @@ -70,25 +70,33 @@ export class RuleOptionsValidationError extends Error { } } -function unknownOptionsError(ruleName: string, optionNames: string[], path: PropertyKey[] = []): RuleOptionsValidationError { - const prefix = path.map(String) - const names = optionNames.map(key => [...prefix, key].join(".")) +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(".")) - return new RuleOptionsValidationError(ruleName, `Unknown options: ${names.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 unknownOptionsError(ruleName, issue.keys, issue.path) + return new UnknownRuleOptionsError(ruleName, issue) + } else { + return new InvalidRuleOptionsError(ruleName, issue) } - - const optionName = issue.path.map(String).join(".") - const message = issue.message.replace(/^Invalid input: /, "") - const problem = optionName ? `${optionName}: ${message}` : message - - return new RuleOptionsValidationError(ruleName, `Invalid options: ${problem}`) } export class Linter { @@ -116,13 +124,7 @@ export class Linter { * @returns A configured Linter instance */ static from(herb: HerbBackend, config?: Config, customRules?: RuleClass[], options?: FilterRulesOptions): Linter { - const availableRules = new Map(rules.map(ruleClass => [ruleClass.ruleName, ruleClass])) - - for (const ruleClass of customRules ?? []) { - availableRules.set(ruleClass.ruleName, ruleClass) - } - - const allRules = [...availableRules.values()] + const allRules = customRules ? [...rules, ...customRules] : rules const configVersion = config?.configVersion const filterResult = Linter.filterRulesByConfig(allRules, config?.linter?.rules, configVersion, options) @@ -161,20 +163,9 @@ export class Linter { protected createRule(ruleClass: RuleClass): Rule { const rule = new ruleClass() const options = this.config?.getRuleOptions(ruleClass.ruleName) ?? {} - const configure = (rule as { configure?: (options: Record) => Rule }).configure - - if (typeof configure !== "function") { - const optionNames = Object.keys(options) - - if (optionNames.length > 0) { - throw unknownOptionsError(ruleClass.ruleName, optionNames) - } - - return rule - } try { - return configure.call(rule, options) + return rule.configure(options) } catch (error) { if (error instanceof ZodError) { throw validationErrorFor(ruleClass.ruleName, error) @@ -186,16 +177,7 @@ export class Linter { /** Validate custom options after built-in and project-local rules are available. */ protected validateRuleOptions(): void { - const allOptions = this.config?.getRuleOptions(ALL_RULES_KEY) ?? {} - const allOptionNames = Object.keys(allOptions) - - if (allOptionNames.length > 0) { - throw unknownOptionsError(ALL_RULES_KEY, allOptionNames) - } - - const availableRules = new Map(this.allAvailableRules.map(ruleClass => [ruleClass.ruleName, ruleClass])) - - for (const ruleClass of availableRules.values()) { + for (const ruleClass of this.allAvailableRules) { this.createRule(ruleClass) } } 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 c17949cb3..1f71327a2 100644 --- a/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts +++ b/javascript/packages/linter/src/rules/erb-no-unused-expressions.ts @@ -35,11 +35,11 @@ class UnusedExpressionCollector extends PrismVisitor { private readonly blockLocalNames: Set private readonly allowedMethods: Set - constructor(blockLocalNames: Set = new Set(), allowedMethods: Set = SIDE_EFFECT_METHODS) { + constructor(blockLocalNames: Set = new Set(), allowedMethods: string[] = []) { super() this.blockLocalNames = blockLocalNames - this.allowedMethods = allowedMethods + this.allowedMethods = new Set([...SIDE_EFFECT_METHODS, ...allowedMethods]) } override visit(node: PrismNode): void { @@ -94,12 +94,12 @@ class UnusedExpressionCollector extends PrismVisitor { class ERBNoUnusedExpressionsVisitor extends BaseRuleVisitor { private exemptLocalNames: Set = new Set() - private readonly allowedMethods: Set + private readonly options: ERBNoUnusedExpressionsOptions constructor(ruleName: string, context: Partial | undefined, options: ERBNoUnusedExpressionsOptions) { super(ruleName, context) - this.allowedMethods = new Set([...SIDE_EFFECT_METHODS, ...options.allowedMethods]) + this.options = options } visitERBRenderNode(node: ERBRenderNode): void { @@ -148,7 +148,7 @@ class ERBNoUnusedExpressionsVisitor extends BaseRuleVisitor { const source = node.source if (!source) return - const collector = new UnusedExpressionCollector(this.exemptLocalNames, this.allowedMethods) + const collector = new UnusedExpressionCollector(this.exemptLocalNames, this.options.allowedMethods) collector.visit(prismNode) const tagOpening = node.tag_opening?.value ?? "<%" diff --git a/javascript/packages/linter/src/types.ts b/javascript/packages/linter/src/types.ts index d70658209..8861bb46d 100644 --- a/javascript/packages/linter/src/types.ts +++ b/javascript/packages/linter/src/types.ts @@ -25,6 +25,34 @@ export type FullRuleConfig = Required +abstract class BaseRule { + private configuredOptions: TOptions | undefined + + get defaultOptions(): TOptions { + return {} as TOptions + } + + get optionsSchema(): z.ZodType { + return z.object({}).strict() as z.ZodType + } + + get options(): TOptions { + this.configuredOptions ??= this.validateOptions() + + return this.configuredOptions + } + + validateOptions(options: Partial = {}): TOptions { + return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) + } + + configure(options: Partial = {}): this { + this.configuredOptions = this.validateOptions(options) + + return this + } +} + /** * Automatically inferred union type of all available linter rule names. * This type extracts the 'ruleName' property from each rule class. @@ -106,7 +134,7 @@ export const DEFAULT_RULE_CONFIG: FullRuleConfig = { export abstract class ParserRule< TAutofixContext extends BaseAutofixContext = BaseAutofixContext, TOptions extends object = RuleOptions -> { +> extends BaseRule { static type = "parser" as const static ruleName: string /** The version in which this rule was introduced. Used for version-gated rule filtering. */ @@ -126,32 +154,6 @@ export abstract class ParserRule< /** Indicates that the rule reports about the project rather than the file, so a CLI run collapses its offenses down to the first one. Defaults to false. */ static reportsOncePerRun = false - private configuredOptions: TOptions | undefined - - get defaultOptions(): TOptions { - return {} as TOptions - } - - get optionsSchema(): z.ZodType { - return z.object({}).strict() as z.ZodType - } - - get options(): TOptions { - this.configuredOptions ??= this.validateOptions() - - return this.configuredOptions - } - - validateOptions(options: Partial = {}): TOptions { - return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) - } - - configure(options: Partial = {}): this { - this.configuredOptions = this.validateOptions(options) - - return this - } - get ruleName(): string { return (this.constructor as typeof ParserRule).ruleName } @@ -216,7 +218,7 @@ export abstract class ParserRule< export abstract class LexerRule< TAutofixContext extends BaseAutofixContext = BaseAutofixContext, TOptions extends object = RuleOptions -> { +> extends BaseRule { static type = "lexer" as const static ruleName: string /** The version in which this rule was introduced. Used for version-gated rule filtering. */ @@ -231,32 +233,6 @@ export abstract class LexerRule< /** Indicates that `autofix` can only fix offenses that carry an `autofixContext`. Offenses without one are reported as not correctable. Defaults to false. */ static autofixRequiresContext = false - private configuredOptions: TOptions | undefined - - get defaultOptions(): TOptions { - return {} as TOptions - } - - get optionsSchema(): z.ZodType { - return z.object({}).strict() as z.ZodType - } - - get options(): TOptions { - this.configuredOptions ??= this.validateOptions() - - return this.configuredOptions - } - - validateOptions(options: Partial = {}): TOptions { - return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) - } - - configure(options: Partial = {}): this { - this.configuredOptions = this.validateOptions(options) - - return this - } - get ruleName(): string { return (this.constructor as typeof LexerRule).ruleName } @@ -349,7 +325,7 @@ export const DEFAULT_LINT_CONTEXT: LintContext = { export abstract class SourceRule< TAutofixContext extends BaseAutofixContext = BaseAutofixContext, TOptions extends object = RuleOptions -> { +> extends BaseRule { static type = "source" as const static ruleName: string /** The version in which this rule was introduced. Used for version-gated rule filtering. */ @@ -364,32 +340,6 @@ export abstract class SourceRule< /** Indicates that `autofix` can only fix offenses that carry an `autofixContext`. Offenses without one are reported as not correctable. Defaults to false. */ static autofixRequiresContext = false - private configuredOptions: TOptions | undefined - - get defaultOptions(): TOptions { - return {} as TOptions - } - - get optionsSchema(): z.ZodType { - return z.object({}).strict() as z.ZodType - } - - get options(): TOptions { - this.configuredOptions ??= this.validateOptions() - - return this.configuredOptions - } - - validateOptions(options: Partial = {}): TOptions { - return this.optionsSchema.parse({ ...this.defaultOptions, ...options }) - } - - configure(options: Partial = {}): this { - this.configuredOptions = this.validateOptions(options) - - return this - } - get ruleName(): string { return (this.constructor as typeof SourceRule).ruleName } diff --git a/javascript/packages/linter/test/cli.test.ts b/javascript/packages/linter/test/cli.test.ts index a38e0ff22..567c33c6e 100644 --- a/javascript/packages/linter/test/cli.test.ts +++ b/javascript/packages/linter/test/cli.test.ts @@ -2233,7 +2233,9 @@ describe("CLI Output Formatting", () => { `) writeFileSync(join(tempDir, ".herb/rules/no-hello-world.mjs"), dedent` - export default class NoHelloWorldRule { + import { ParserRule } from "@herb-tools/linter" + + export default class NoHelloWorldRule extends ParserRule { static ruleName = "no-hello-world" check(document, context) { @@ -2283,7 +2285,9 @@ describe("CLI Output Formatting", () => { `) writeFileSync(join(tempDir, ".herb/rules/no-hello-world.mjs"), dedent` - export default class NoHelloWorldRule { + import { ParserRule } from "@herb-tools/linter" + + export default class NoHelloWorldRule extends ParserRule { static ruleName = "no-hello-world" check(document, context) { @@ -2329,7 +2333,9 @@ describe("CLI Output Formatting", () => { `) writeFileSync(join(tempDir, ".herb/rules/no-hello-world.mjs"), dedent` - export default class NoHelloWorldRule { + import { ParserRule } from "@herb-tools/linter" + + export default class NoHelloWorldRule extends ParserRule { static ruleName = "no-hello-world" check(document, context) { diff --git a/javascript/packages/linter/test/linter.test.ts b/javascript/packages/linter/test/linter.test.ts index 58b66cbcc..573123f9c 100644 --- a/javascript/packages/linter/test/linter.test.ts +++ b/javascript/packages/linter/test/linter.test.ts @@ -1,6 +1,5 @@ import dedent from "dedent" import { describe, test, expect, beforeAll } from "vitest" -import { z } from "zod" import { Herb } from "@herb-tools/node-wasm" import { Location } from "@herb-tools/core" @@ -12,7 +11,7 @@ import { HTMLAttributeDoubleQuotesRule } from "../src/rules/html-attribute-doubl import { HTMLAttributeValuesRequireQuotesRule } from "../src/rules/html-attribute-values-require-quotes.js" import { ParserRule, SourceRule } from "../src/types.js" -import type { BaseAutofixContext, UnboundLintOffense, LintContext, FullRuleConfig } from "../src/types.js" +import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../src/types.js" import type { ParseResult } from "@herb-tools/core" describe("@herb-tools/linter", () => { @@ -387,84 +386,6 @@ describe("@herb-tools/linter", () => { expect(linter).toBeInstanceOf(Linter) }) - test("uses a custom rule in place of a built-in rule with the same name", () => { - interface CustomOptions { - message: string - } - - class CustomRule extends ParserRule { - static ruleName = HTMLTagNameLowercaseRule.ruleName - static introducedIn = "0.1.0" - - get defaultOptions(): CustomOptions { - return { message: "default" } - } - - get optionsSchema(): z.ZodType { - return z.object({ message: z.string() }).strict() - } - - check(): UnboundLintOffense[] { - return [{ - message: this.options.message, - location: Location.from(1, 1, 1, 1), - rule: this.ruleName, - code: this.ruleName, - source: "linter" - }] - } - } - - const config = Config.fromObject({ - linter: { - rules: { - [CustomRule.ruleName]: { - message: "custom" - } - } - } - }) - const linter = Linter.from(Herb, config, [CustomRule], { only: [CustomRule.ruleName] }) - - expect(linter.rules).toEqual([CustomRule]) - expect(linter.lint("
").offenses.map(offense => offense.message)).toEqual(["custom"]) - }) - - test("gives each rule instance isolated options", () => { - interface MutableOptions { - values: string[] - } - - const observedLengths: number[] = [] - - class MutatingOptionsRule extends ParserRule { - static ruleName = "mutating-options-rule" - static introducedIn = "0.1.0" - - get defaultOptions(): MutableOptions { - return { values: [] } - } - - get optionsSchema(): z.ZodType { - return z.object({ values: z.array(z.string()) }).strict() - } - - check(): UnboundLintOffense[] { - this.options.values.push("value") - observedLengths.push(this.options.values.length) - - return [] - } - } - - const linter = new Linter(Herb, [MutatingOptionsRule]) - - linter.lint("") - linter.lint("") - - expect(observedLengths).toEqual([1, 1]) - }) - test("validates custom options for disabled rules after the shared config schema", () => { const config = Config.fromObject({ linter: { @@ -515,22 +436,6 @@ describe("@herb-tools/linter", () => { ) }) - test("rejects custom options on the all pseudo rule", () => { - const config = Config.fromObject({ - linter: { - rules: { - all: { - allowBlank: false - } - } - } - }) - - expect(() => Linter.from(Herb, config)).toThrow( - "all: Unknown options: allowBlank" - ) - }) - test("filters rules based on default config", () => { class EnabledByDefaultRule extends ParserRule { static ruleName = "enabled-by-default-rule" diff --git a/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts b/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts index 380d5284b..330860b65 100644 --- a/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts +++ b/javascript/packages/linter/test/rules/erb-no-unused-expressions.test.ts @@ -5,20 +5,9 @@ import { ERBNoUnusedExpressionsRule } from "../../src/rules/erb-no-unused-expres import { createLinterTest } from "../helpers/linter-test-helper.js" const { expectNoOffenses, expectError, expectWarning, assertOffenses } = createLinterTest(ERBNoUnusedExpressionsRule) -const customOptions = createLinterTest(ERBNoUnusedExpressionsRule, { - allowedMethods: ["breadcrumb"] -}) describe("ERBNoUnusedExpressionsRule", () => { describe("valid cases", () => { - test("passes for configured side-effect methods", () => { - customOptions.expectNoOffenses('<% breadcrumb :projects %>') - }) - - test("keeps the built-in side-effect methods when configured", () => { - customOptions.expectNoOffenses('<% content_for :title, "Projects" %>') - }) - test("passes for output tags with method calls", () => { expectNoOffenses(dedent` <%= @user.name %> @@ -311,6 +300,20 @@ describe("ERBNoUnusedExpressionsRule", () => { }) }) + describe("with configured side-effects", () => { + const customOptions = createLinterTest(ERBNoUnusedExpressionsRule, { + allowedMethods: ["breadcrumb"] + }) + + test("passes for configured side-effect methods", () => { + customOptions.expectNoOffenses('<% breadcrumb :projects %>') + }) + + test("keeps the built-in side-effect methods when configured", () => { + customOptions.expectNoOffenses('<% content_for :title, "Projects" %>') + }) + }) + describe("invalid cases", () => { test("fails for bare method call on instance variable", () => { expectError( diff --git a/javascript/packages/linter/test/rules/html-allowed-script-type.test.ts b/javascript/packages/linter/test/rules/html-allowed-script-type.test.ts index fabe7643e..a64da2e5b 100644 --- a/javascript/packages/linter/test/rules/html-allowed-script-type.test.ts +++ b/javascript/packages/linter/test/rules/html-allowed-script-type.test.ts @@ -3,12 +3,6 @@ import { HTMLAllowedScriptTypeRule } from "../../src/rules/html-allowed-script-t import { createLinterTest } from "../helpers/linter-test-helper.js" const { expectNoOffenses, expectError, assertOffenses } = createLinterTest(HTMLAllowedScriptTypeRule) -const configuredAllowedTypes = createLinterTest(HTMLAllowedScriptTypeRule, { - allowedTypes: ["application/json"] -}) -const blankTypesDisallowed = createLinterTest(HTMLAllowedScriptTypeRule, { - allowBlank: false -}) describe("html-allowed-script-type", () => { test("passes when type attribute is blank", () => { @@ -62,6 +56,10 @@ describe("html-allowed-script-type", () => { }) describe("with configured allowed types", () => { + const configuredAllowedTypes = createLinterTest(HTMLAllowedScriptTypeRule, { + allowedTypes: ["application/json"] + }) + test("uses the configured allowed types", () => { configuredAllowedTypes.expectNoOffenses('') }) @@ -73,6 +71,10 @@ describe("html-allowed-script-type", () => { }) describe("with blank types disallowed", () => { + const blankTypesDisallowed = createLinterTest(HTMLAllowedScriptTypeRule, { + allowBlank: false + }) + test("can require an explicit type", () => { blankTypesDisallowed.expectError("`type` attribute required for `")