From 3b4d655feb64018fd6c9097f393fc2ef99fde843 Mon Sep 17 00:00:00 2001 From: Nim G Date: Tue, 1 Sep 2026 22:03:26 -0300 Subject: [PATCH] feat: expose per-check alert counts to metric formulas AddAlert increments a per-check counter in a dedicated File.checkCounts field every time an alert is actually appended. That counter is exposed to metric formulas as an indexable Tengo object: check["Style.Rule"]. A check that's loaded and applicable but hasn't fired yet reads as 0; a name that doesn't correspond to a real, applicable check produces a real error naming it. Which checks are known and applicable is computed per file, respecting the same extension/section/style-toggling rules that already decide whether a check runs at all. Per-check counts live in their own field, decoupled from f.Metrics (which document content can influence via HTML tag-name bookkeeping), so a crafted document can't forge a count for a real check. An earlier version of this branch exposed counters as sanitized Tengo identifiers (check_Style_Rule), which needed to detect when two differently-named checks sanitized to the same identifier via a hand-rolled AST scope resolver. That approach kept finding new scope- tracking edge cases with no way to know when it was complete, and made a misspelled check name silently evaluate to zero instead of erroring. The indexable-object design removes the identifier-flattening step this was rooted in, so there's nothing left to collide. Adds an internal/e2e case covering the check[...] syntax end to end, verified against the pre-feature commit to confirm it fails there with a real Tengo compile error before passing cleanly here. Assisted-by: Claude Code --- cmd/vale/command.go | 11 +- cmd/vale/metrics_test.go | 58 ++ internal/check/check_counts.go | 78 +++ internal/check/check_counts_test.go | 114 +++ internal/check/metric.go | 21 +- internal/core/addalert_test.go | 122 ++++ internal/core/check_object_test.go | 125 ++++ internal/core/file.go | 100 ++- internal/lint/check_object_test.go | 813 ++++++++++++++++++++++ internal/lint/lint.go | 94 ++- internal/lint/loaded_checks_bench_test.go | 146 ++++ internal/lint/metric_check_counts_test.go | 258 +++++++ internal/lint/nested_rule_disable_test.go | 83 +++ testdata/e2e/checks.yaml | 40 ++ 14 files changed, 2025 insertions(+), 38 deletions(-) create mode 100644 cmd/vale/metrics_test.go create mode 100644 internal/check/check_counts.go create mode 100644 internal/check/check_counts_test.go create mode 100644 internal/core/check_object_test.go create mode 100644 internal/lint/check_object_test.go create mode 100644 internal/lint/loaded_checks_bench_test.go create mode 100644 internal/lint/metric_check_counts_test.go create mode 100644 internal/lint/nested_rule_disable_test.go diff --git a/cmd/vale/command.go b/cmd/vale/command.go index aca47b9f..2f59054f 100644 --- a/cmd/vale/command.go +++ b/cmd/vale/command.go @@ -248,7 +248,16 @@ func printMetrics(args []string, _ *core.CLIFlags) error { "'%s' contains no lintable files", args[0])) } - computed, _ := linted[0].ComputeMetrics() + return printMetricsResult(linted[0]) +} + +// printMetricsResult reports f's computed metrics as JSON. +func printMetricsResult(f *core.File) error { + computed, err := f.ComputeMetrics() + if err != nil { + return err + } + return printJSON(computed) } diff --git a/cmd/vale/metrics_test.go b/cmd/vale/metrics_test.go new file mode 100644 index 00000000..8f1f8d33 --- /dev/null +++ b/cmd/vale/metrics_test.go @@ -0,0 +1,58 @@ +package main + +import ( + "testing" + + "github.com/vale-cli/vale/v3/internal/core" +) + +// TestPrintMetricsResultSucceedsWithoutCollision pins the ordinary, +// non-colliding path: printMetricsResult must still succeed and print +// normally when ComputeMetrics finds nothing ambiguous. +func TestPrintMetricsResultSucceedsWithoutCollision(t *testing.T) { + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + f, err := core.NewFile("A document with real prose for a word count.", cfg) + if err != nil { + t.Fatal(err) + } + f.Summary.WriteString("A document with real prose for a word count.") + + if err = printMetricsResult(f); err != nil { + t.Fatalf("expected no error, got: %v", err) + } +} + +// TestPrintMetricsResultNoLongerErrorsOnSanitizedKeyCollision pins the +// metric-check-counts redesign's effect on this CLI surface: with check +// names no longer flattened into Tengo identifiers at all, there is no more +// sanitized-key collision for ComputeMetrics to detect or for +// printMetricsResult to propagate specially -- see item 4 of the redesign +// (cmd/vale/command.go's printMetricsResult should "revert to something +// much simpler, there's no more collision to propagate specially"). +// +// This reproduces the f.Metrics = {"words": 999} setup the deleted +// TestPrintMetricsResultSurfacesCollision (an old-mechanism test pinning +// the now-removed collision machinery) used to assert an error for, but +// asserts the opposite outcome: printMetricsResult must now succeed. +func TestPrintMetricsResultNoLongerErrorsOnSanitizedKeyCollision(t *testing.T) { + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + f, err := core.NewFile("A document with real prose for a word count.", cfg) + if err != nil { + t.Fatal(err) + } + f.Summary.WriteString("A document with real prose for a word count.") + f.Metrics["words"] = 999 + + if err = printMetricsResult(f); err != nil { + t.Fatalf("expected no error under the new design (there is no more "+ + "collision-detection machinery left to trip), got: %v", err) + } +} diff --git a/internal/check/check_counts.go b/internal/check/check_counts.go new file mode 100644 index 00000000..15288719 --- /dev/null +++ b/internal/check/check_counts.go @@ -0,0 +1,78 @@ +package check + +import ( + "fmt" + + "github.com/d5/tengo/v2" +) + +// checkCounts is a Tengo object exposing every loaded check's per-document +// alert count by its real, unflattened name (e.g. "Style.Rule"), added to a +// `metric` formula's parameters under the "check" key. A formula indexes it +// directly: +// +// check["AITells.FigurativeOwns"] + check["AITells.HedgingPhrases"] > 3 +// +// This replaces the old design, where a check name was flattened into a +// sanitized Tengo identifier (check_Style_Rule) for direct reference in a +// formula -- a step where two distinct names could sanitize to the same +// identifier, with no way to tell them apart afterward. Indexing by the +// real name makes that collision structurally impossible: there's no +// flattening step left to collide on. +// +// It also fixes a silent-typo problem the old design had no way to catch: +// IndexGet distinguishes a check that's genuinely loaded but simply never +// fired on this document (reads as 0) from a name that was never loaded at +// all -- almost always a typo in the formula -- which returns a real error +// instead of silently reading 0 either way. +type checkCounts struct { + tengo.ObjectImpl + // counts holds the raw, unsanitized per-check alert count for this + // document, keyed by the check's real name (e.g. "AITells.FigurativeOwns" + // -> 3). A check absent from counts simply never fired. + counts map[string]int + // known holds the set of check names actually loaded for this run -- + // see core.File.LoadedChecks -- which is what lets IndexGet tell a + // never-fired check apart from one that doesn't exist. + known map[string]bool +} + +// newCheckCounts builds a checkCounts object from counts (raw per-check +// alert counts) and known (the set of check names loaded for this run). +func newCheckCounts(counts map[string]int, known map[string]bool) *checkCounts { + return &checkCounts{counts: counts, known: known} +} + +// TypeName returns the name of the type, for Tengo's own error messages and +// debugging output. +func (c *checkCounts) TypeName() string { + return "check-counts" +} + +// String returns a string representation of the object, for Tengo's own +// error messages and debugging output. +func (c *checkCounts) String() string { + return "" +} + +// IndexGet returns index's alert count as a *tengo.Float -- 0 if it names a +// check that's loaded but never fired on this document, its real count +// otherwise -- matching this codebase's existing convention that every other +// metric value a `metric` formula sees is a float64. Indexing a name that +// isn't a check genuinely loaded for this run returns a real error naming +// it, rather than silently reading 0 -- the exact silent-typo failure mode +// the old, shape-only checkCounterRE match could never catch. +func (c *checkCounts) IndexGet(index tengo.Object) (tengo.Object, error) { + name, ok := tengo.ToString(index) + if !ok { + return nil, tengo.ErrInvalidIndexType + } + + if !c.known[name] { + return nil, fmt.Errorf( + "%q is not a known check: it isn't defined by any loaded style, "+ + "so this is likely a typo in the metric formula", name) + } + + return &tengo.Float{Value: float64(c.counts[name])}, nil +} diff --git a/internal/check/check_counts_test.go b/internal/check/check_counts_test.go new file mode 100644 index 00000000..8130858d --- /dev/null +++ b/internal/check/check_counts_test.go @@ -0,0 +1,114 @@ +package check + +import ( + "strings" + "testing" + + "github.com/d5/tengo/v2" +) + +// checkCounts is the target design's replacement for the deleted +// formulaIdentifiers/identScope/checkCounterRE machinery (see metric.go): +// rather than flattening every check name into a sanitized Tengo +// identifier (check_Style_Rule) -- which can collide, since two distinct +// names can sanitize to the same identifier -- a `metric` formula indexes a +// single tengo.Object by the check's real, unflattened name: +// +// check["AITells.FigurativeOwns"] + check["AITells.HedgingPhrases"] > 3 +// +// This makes the collision this branch spent 7 rounds chasing structurally +// impossible (there's no more name-mangling step to collide), and lets +// IndexGet distinguish "this check exists and never fired" (0) from "this +// check name doesn't exist at all" (a real error), fixing the silent-typo +// problem checkCounterRE's shape-only matching could never catch. +// +// newCheckCounts does not exist yet -- this file is the RED-phase +// specification for it, not a passing test. counts holds the raw, +// unsanitized per-check alert count (e.g. "AITells.FigurativeOwns" -> 3, +// the same raw key AddAlert's f.Metrics["check."+a.Check]++ produces once +// the "check." prefix is stripped); known holds the set of check names +// actually loaded for this run, which is what lets IndexGet tell a +// never-fired check apart from a nonexistent one. +func TestCheckCountsIndexGetOnNeverFiredCheckReturnsZero(t *testing.T) { + cc := newCheckCounts( + map[string]int{}, + map[string]bool{"Style.Rule": true}, + ) + + val, err := cc.IndexGet(&tengo.String{Value: "Style.Rule"}) + if err != nil { + t.Fatalf("expected a loaded-but-never-fired check to resolve without "+ + "error, got: %v", err) + } + + got, ok := tengo.ToFloat64(val) + if !ok { + t.Fatalf("expected a numeric result, got %T (%v)", val, val) + } + if got != 0 { + t.Errorf("expected a never-fired check to read as 0, got %v", got) + } +} + +// A check that actually fired must read back its real count, not just a +// truthy/nonzero placeholder -- the whole point of exposing alert counts to +// a formula at all. +func TestCheckCountsIndexGetOnFiredCheckReturnsRealCount(t *testing.T) { + cc := newCheckCounts( + map[string]int{"Style.Rule": 5}, + map[string]bool{"Style.Rule": true}, + ) + + val, err := cc.IndexGet(&tengo.String{Value: "Style.Rule"}) + if err != nil { + t.Fatalf("expected a fired, loaded check to resolve without error, got: %v", err) + } + + got, ok := tengo.ToFloat64(val) + if !ok { + t.Fatalf("expected a numeric result, got %T (%v)", val, val) + } + if got != 5 { + t.Errorf("expected the check's real count 5, got %v", got) + } +} + +// This is the concrete fix for the design review's most-likely-real-mistake +// finding: under the old checkCounterRE (`^check_\w+$`, a shape-only regex +// match against an already-flattened identifier), a misspelled check/rule +// name in a formula silently evaluated to 0 -- indistinguishable from a +// real check that simply never fired. Indexing by the check's actual, +// unflattened name against the set of checks genuinely loaded for this run +// lets IndexGet tell the two apart and surface a real error instead. +func TestCheckCountsIndexGetOnUnknownCheckReturnsError(t *testing.T) { + cc := newCheckCounts( + map[string]int{}, + map[string]bool{"Style.Rule": true}, + ) + + val, err := cc.IndexGet(&tengo.String{Value: "Style.Typo"}) + if err == nil { + t.Fatalf("expected an error for a check name that isn't loaded, got "+ + "value %v with no error -- this is the exact silent-typo failure "+ + "mode the redesign exists to fix", val) + } + if !strings.Contains(err.Error(), "Style.Typo") { + t.Errorf("expected the error to name the unknown check %q, got: %v", + "Style.Typo", err) + } +} + +// TypeName/String only need to be sane for Tengo's own error messages and +// debugging output -- not asserted exhaustively, but they must exist and +// not panic, which ObjectImpl's defaults do (ObjectImpl.TypeName panics +// with ErrNotImplemented), so checkCounts must actually override both. +func TestCheckCountsHasATypeNameAndString(t *testing.T) { + cc := newCheckCounts(map[string]int{}, map[string]bool{}) + + if cc.TypeName() == "" { + t.Error("expected a non-empty TypeName") + } + if cc.String() == "" { + t.Error("expected a non-empty String representation") + } +} diff --git a/internal/check/metric.go b/internal/check/metric.go index 2abf61e6..066b891d 100644 --- a/internal/check/metric.go +++ b/internal/check/metric.go @@ -63,7 +63,7 @@ func measuredScope(declared []string) []string { } // Run calculates the readability level of the given text. -func (o Metric) Run(blk nlp.Block, _ *core.File, _ *core.Config) ([]core.Alert, error) { +func (o Metric) Run(blk nlp.Block, f *core.File, _ *core.Config) ([]core.Alert, error) { alerts := []core.Alert{} // A formula is compiled and run as a Tengo program, so it needs the same @@ -74,7 +74,12 @@ func (o Metric) Run(blk nlp.Block, _ *core.File, _ *core.Config) ([]core.Alert, parameters := core.BlockMetrics(blk.Text, blk.Metrics) if len(parameters) == 0 { - // empty file. + // A heading-and-code-fence-only document (or block), or any other one + // with no prose "words" at all, has nothing for a readability-style + // formula to compute: the built-in values it would need (words, + // characters, sentences, ...) are never populated in that case (see + // BlockMetrics). Evaluating anyway would fail with an opaque Tengo + // "unresolved reference" compile error instead of this graceful no-op. return alerts, nil } @@ -86,6 +91,18 @@ func (o Metric) Run(blk nlp.Block, _ *core.File, _ *core.Config) ([]core.Alert, } } + // Every loaded check's alert count so far is exposed under the "check" + // parameter as an indexable object, rather than flattened into + // individual Tengo identifiers -- see checkCounts. A formula reads it as + // check["Style.Rule"], which resolves a never-fired check to 0 and a + // name that isn't genuinely loaded to a real error, without any risk of + // two distinct check names colliding on the same identifier. See #1163. + // + // "So far" is the whole document only for an unscoped (summary-scoped) + // rule, which runs last. A rule declaring a narrower scope runs + // mid-walk and sees counts as of that point, not the final total. + parameters["check"] = newCheckCounts(f.CheckCounts, f.LoadedChecks) + // The actual result of our formula. // // We need this to allow showing the result in a rule's message. diff --git a/internal/core/addalert_test.go b/internal/core/addalert_test.go index 7d864302..91dbb063 100644 --- a/internal/core/addalert_test.go +++ b/internal/core/addalert_test.go @@ -74,3 +74,125 @@ func TestAddAlertMeasurementStartsBlock(t *testing.T) { t.Errorf("placed at %d:%v, want 3:[1 1]", got.Line, got.Span) } } + +// TestAddAlertNilMetrics verifies the nil-map guard added alongside the new +// per-check counter: AddAlert must not panic when Metrics is left nil, e.g. +// a File built without going through NewFile (which always initializes it). +// Unlike TestAddAlertNegativeSpan, this alert is actually appended to +// f.Alerts, so it exercises the f.Metrics write path directly. +func TestAddAlertNilMetrics(t *testing.T) { + f := &File{ + ChkToCtx: map[string]string{}, + history: map[string]int{}, + limits: map[string]int{}, + } + + blk := nlp.NewBlock("alpha", "alpha", "text.md") + defer func() { + if r := recover(); r != nil { + t.Fatalf("AddAlert panicked with a nil Metrics map: %v", r) + } + }() + f.AddAlert(Alert{ + Check: "Demo.Rule", HasByteOffsets: true, Span: []int{0, 5}, + }, blk, 1, 0, false) +} + +// TestAddAlertCheckCounterIncrementsOnlyForReportedAlerts verifies the new, +// unconditional per-check alert counter proposed in issue #1163: it +// increments once for every alert actually appended to f.Alerts -- the same +// place f.limits increments today, just without the a.Limit > 0 gate -- and +// does NOT increment for an alert a rule marks Hide, or for one f.history +// dedupes as a repeat of an already-reported (Line, Span[0], Check). +// +// The counter is surfaced in f.CheckCounts, a dedicated field keyed by the +// check's real name, kept entirely separate from f.Metrics (which ast.go +// also writes to from document content) so a crafted document can't inject +// a false count under it -- see f.CheckCounts's own doc comment. +// +// Alerts here use HasByteOffsets so AddAlert locates them deterministically +// via locFromByteOffset rather than a text search, making the resulting +// (Line, Span[0]) -- and therefore the dedup outcome -- fully controlled by +// the test rather than incidental to how the search happens to land. +func TestAddAlertCheckCounterIncrementsOnlyForReportedAlerts(t *testing.T) { + f := &File{ + ChkToCtx: map[string]string{}, + history: map[string]int{}, + limits: map[string]int{}, + Metrics: map[string]int{}, + } + + // "alpha beta" -- byte offsets: "alpha" = [0,5), "beta" = [6,10). + blk := nlp.NewBlock("alpha beta", "alpha beta", "text.md") + + // Two genuine, distinct alerts from the same check: both should count. + f.AddAlert(Alert{ + Check: "Demo.Rule", HasByteOffsets: true, Span: []int{0, 5}, + }, blk, 1, 0, false) + f.AddAlert(Alert{ + Check: "Demo.Rule", HasByteOffsets: true, Span: []int{6, 10}, + }, blk, 1, 0, false) + + // A Hide alert from the same check: never reaches f.Alerts, so it must + // not count either. + f.AddAlert(Alert{ + Check: "Demo.Rule", HasByteOffsets: true, Span: []int{0, 5}, Hide: true, + }, blk, 1, 0, false) + + // A repeat of the first alert's exact (Line, Span[0]): f.history dedupes + // this, so it must not count a third time. + f.AddAlert(Alert{ + Check: "Demo.Rule", HasByteOffsets: true, Span: []int{0, 5}, + }, blk, 1, 0, false) + + if len(f.Alerts) != 2 { + t.Fatalf("expected 2 alerts actually reported (Hide and the dedup "+ + "attempt should not add more), got %d", len(f.Alerts)) + } + + if got := f.CheckCounts["Demo.Rule"]; got != 2 { + t.Fatalf("expected CheckCounts[Demo.Rule] to be 2 (one per alert "+ + "actually reported, not per AddAlert call), got %d", got) + } +} + +// TestAddAlertLimitCapUnaffected pins the existing opt-in `limit:`/f.limits +// reporting cap: it must keep behaving exactly as it does today, capping +// f.Alerts at Limit regardless of the new unconditional counter added for +// issue #1163. +// +// The new counter only counts what was actually appended -- the same gate +// f.limits has always used -- so with Limit: 2 and three attempts, both +// f.limits and f.CheckCounts stop at 2, not 3. +func TestAddAlertLimitCapUnaffected(t *testing.T) { + f := &File{ + ChkToCtx: map[string]string{}, + history: map[string]int{}, + limits: map[string]int{}, + Metrics: map[string]int{}, + } + + // "alpha beta gamma" -- three non-overlapping byte-offset spans. + blk := nlp.NewBlock("alpha beta gamma", "alpha beta gamma", "text.md") + spans := [][]int{{0, 5}, {6, 10}, {11, 16}} + + for _, span := range spans { + f.AddAlert(Alert{ + Check: "Demo.Capped", HasByteOffsets: true, Span: span, Limit: 2, + }, blk, 1, 0, false) + } + + if len(f.Alerts) != 2 { + t.Fatalf("expected the existing limit: 2 cap to allow only 2 alerts, got %d", + len(f.Alerts)) + } + + if got := f.limits["Demo.Capped"]; got != 2 { + t.Fatalf("expected the existing f.limits cap counter to stay at 2, got %d", got) + } + + if got := f.CheckCounts["Demo.Capped"]; got != 2 { + t.Fatalf("expected the new counter to count only the 2 alerts actually "+ + "appended (matching where f.limits increments today), got %d", got) + } +} diff --git a/internal/core/check_object_test.go b/internal/core/check_object_test.go new file mode 100644 index 00000000..d634f0af --- /dev/null +++ b/internal/core/check_object_test.go @@ -0,0 +1,125 @@ +package core + +import ( + "testing" + + "github.com/vale-cli/vale/v3/internal/nlp" +) + +// TestComputeMetricsExcludesCheckCountsFromGenericSanitization pins the +// target design from the metric-check-counts redesign: a per-check alert +// count (f.CheckCounts, written by AddAlert -- see its own doc comment for +// why this is a dedicated field rather than a "check."-prefixed f.Metrics +// entry) must never be sanitized into a "check_Style_Rule" Tengo identifier +// and handed out through the generic structural-metrics params map at all. +// It's exposed instead through f.CheckCounts directly, keyed by the check's +// real, unflattened name -- see internal/check/metric.go's checkCounts, +// which wraps it in a separate indexable Tengo object under "check". +func TestComputeMetricsExcludesCheckCountsFromGenericSanitization(t *testing.T) { + f := &File{ + ChkToCtx: map[string]string{}, + history: map[string]int{}, + limits: map[string]int{}, + Metrics: map[string]int{}, + CheckCounts: map[string]int{"Demo.RuleA": 3}, + } + f.Summary.WriteString("Some real prose so the readability builtins are computed too.") + + params, err := f.ComputeMetrics() + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + if _, ok := params["check_Demo_RuleA"]; ok { + t.Errorf(`expected a check count to never surface in the generic `+ + "sanitize-into-Tengo-identifier path at all (it's exposed "+ + "through the new check[...] object instead), got params = %v", + params) + } + + if got, want := f.CheckCounts["Demo.RuleA"], 3; got != want { + t.Errorf("expected CheckCounts[%q] = %d (the raw, unflattened name "+ + "and count), got %d", "Demo.RuleA", want, got) + } +} + +// TestComputeMetricsIgnoresCraftedMetricsKeyShapedLikeACheckCount is the +// regression test for a real integrity bug found in review: f.Metrics is +// also written by ast.go from raw document content (HTML/XML tag names, +// ...), so before per-check counts moved to their own field, a document +// containing a crafted tag literally named e.g. "check.Demo.Forged" -- +// paired with a skip class, default or configured via IgnoredClasses -- +// landed in f.Metrics indistinguishable, by prefix alone, from a genuine +// counter AddAlert would have written. Confirmed directly against a real +// lint run (not just reasoning about the code): such a tag incremented +// f.Metrics["check.Demo.Forged"] to 1 even though Demo.Forged never +// actually fired, and a `metric` formula referencing check["Demo.Forged"] +// read the forged count as real. +// +// This simulates the injected key directly, the shape ast.go's +// f.Metrics[txt]++ would produce for it, and confirms it's now completely +// inert: AddAlert is the only writer of f.CheckCounts, so a "check."-shaped +// f.Metrics key, however it got there, is never read as a check count at +// all -- there's no shared keyspace left for it to collide with. +func TestComputeMetricsIgnoresCraftedMetricsKeyShapedLikeACheckCount(t *testing.T) { + f := &File{ + ChkToCtx: map[string]string{}, + history: map[string]int{}, + limits: map[string]int{}, + Metrics: map[string]int{"check.Demo.Forged": 1}, + } + f.Summary.WriteString("Some real prose so the readability builtins are computed too.") + + _, err := f.ComputeMetrics() + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + if got, ok := f.CheckCounts["Demo.Forged"]; ok { + t.Errorf("expected a crafted \"check.\"-shaped f.Metrics key to "+ + "never be read as a check count, got CheckCounts[%q] = %d", + "Demo.Forged", got) + } +} + +// TestComputeMetricsNoLongerDetectsCheckNameCollisions pins the structural +// claim behind this redesign: two check names that used to sanitize to the +// same identifier (Foo-Bar.Baz and Foo.Bar-Baz both -> check_Foo_Bar_Baz) +// can no longer collide at all, because check names are never flattened +// into identifiers in the first place -- checkCounts, keyed by each check's +// real name, never surfaces in params at all, so there's nothing left to +// detect or report. +func TestComputeMetricsNoLongerDetectsCheckNameCollisions(t *testing.T) { + f := &File{ + ChkToCtx: map[string]string{}, + history: map[string]int{}, + limits: map[string]int{}, + Metrics: map[string]int{}, + } + f.Summary.WriteString("Two differently named checks no longer collide once sanitized.") + + blk := nlp.NewBlock("alpha beta", "alpha beta", "text.md") + f.AddAlert(Alert{ + Check: "Foo-Bar.Baz", HasByteOffsets: true, Span: []int{0, 5}, + }, blk, 1, 0, false) + f.AddAlert(Alert{ + Check: "Foo.Bar-Baz", HasByteOffsets: true, Span: []int{6, 10}, + }, blk, 1, 0, false) + + params, err := f.ComputeMetrics() + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if _, ok := params["check_Foo_Bar_Baz"]; ok { + t.Errorf("expected check counts to be excluded from params "+ + "entirely, not merged or collided into check_Foo_Bar_Baz, got "+ + "params = %v", params) + } + + if got, want := f.CheckCounts["Foo-Bar.Baz"], 1; got != want { + t.Errorf("expected CheckCounts[%q] = %d, got %d", "Foo-Bar.Baz", want, got) + } + if got, want := f.CheckCounts["Foo.Bar-Baz"], 1; got != want { + t.Errorf("expected CheckCounts[%q] = %d, got %d", "Foo.Bar-Baz", want, got) + } +} diff --git a/internal/core/file.go b/internal/core/file.go index 599b605a..387b0995 100755 --- a/internal/core/file.go +++ b/internal/core/file.go @@ -23,6 +23,30 @@ var commentStyleRE = regexp.MustCompile(`^vale styles? = (.*)$`) var commentControlMatchesRE = regexp.MustCompile(`^vale (.+\..+)(\[.+\]) = (YES|NO)$`) +// invalidTengoIdentCharRE matches any character that can't appear in a Tengo +// identifier. An f.Metrics key isn't necessarily one already: it may be an +// HTML tag name (e.g. a hyphenated custom element like "my-component") or a +// "text.heading.h1"-derived scope. Per-check alert counters live in their +// own field (f.CheckCounts, not f.Metrics at all -- see AddAlert), so an +// arbitrary, user-authored check name never needs to survive being +// flattened into an identifier, or even pass through this sanitizer, at +// all; a `metric` formula reads one through a separate indexable object +// keyed by the check's real, unflattened name instead (see +// check.checkCounts). +var invalidTengoIdentCharRE = regexp.MustCompile(`[^A-Za-z0-9_]`) + +// sanitizeMetricKey turns an f.Metrics key into a valid Tengo identifier for +// use as a `metric` formula parameter name: every character that isn't a +// letter, digit, or underscore becomes "_", and a leading digit -- which +// Tengo doesn't allow to start an identifier -- is prefixed with "_". +func sanitizeMetricKey(k string) string { + k = invalidTengoIdentCharRE.ReplaceAllString(k, "_") + if k != "" && k[0] >= '0' && k[0] <= '9' { + k = "_" + k + } + return k +} + // A File represents a linted text file. type File struct { NLP nlp.Info // - @@ -54,18 +78,43 @@ type File struct { // sanShifts records, per line, where the sanitizer's `’` rewrite // shortened the text, so spans can be mapped back to the file's bytes. - sanShifts map[int][]int - regions map[string][]commentRegion // spans covered by comment directives - Comments map[string]bool // comment control statements - Metrics map[string]int // count-based metrics - history map[string]int // - - limits map[string]int // - - tags map[string]*nlp.TokenCache // tagging shared by every rule, per model - lineIdx []int // byte offset of each line start in lineIdxCtx - lineIdxCtx string // the context lineIdx was built from - simple bool // - - Lookup bool // - - MetaScope string // extra scope context, e.g. a YAML key or comment + sanShifts map[int][]int + regions map[string][]commentRegion // spans covered by comment directives + Comments map[string]bool // comment control statements + Metrics map[string]int // count-based metrics, written by ast.go from document content (HTML tag names, structural counts, ...) + + // CheckCounts holds the per-check alert count AddAlert records, keyed by + // the check's real, unflattened name (e.g. "Style.Rule"). This is + // deliberately its own field, not a "check."-prefixed entry sharing + // f.Metrics with ast.go's structural bookkeeping: f.Metrics's other + // writer takes tag names straight out of document content (an HTML/XML + // tag literally named e.g. "check.Style.Rule" would land in f.Metrics + // too, indistinguishable by prefix alone from a genuine counter), so a + // shared map with only a naming convention for a boundary is forgeable + // -- confirmed directly: a crafted `` tag, + // paired with a configured or default skip class, incremented the + // shared key without the check ever actually firing. A dedicated field + // that only AddAlert ever writes to makes that structurally impossible, + // not just unlikely by convention. See check.checkCounts, which wraps + // this in the indexable object a `metric` formula reads as + // check["Style.Rule"]. + CheckCounts map[string]int + + // LoadedChecks is the set of check names loaded for this run (populated + // by lint.lintFile from Manager.Rules() right after NewFile). It's what + // lets check.checkCounts -- the object a `metric` formula indexes as + // check["Style.Rule"] -- tell a check that's genuinely loaded but never + // fired on this document (reads as 0) apart from a typo'd check name + // that was never loaded at all (a real error). + LoadedChecks map[string]bool + history map[string]int // - + limits map[string]int // - + tags map[string]*nlp.TokenCache // tagging shared by every rule, per model + lineIdx []int // byte offset of each line start in lineIdxCtx + lineIdxCtx string // the context lineIdx was built from + simple bool // - + Lookup bool // - + MetaScope string // extra scope context, e.g. a YAML key or comment // Scoped holds the text of every value a View found, by scope name, so // a rule can ask about a scope other than the one it runs in. @@ -281,6 +330,12 @@ func (f *File) ComputeMetrics() (map[string]interface{}, error) { // BlockMetrics computes the metrics of one block: the counts derived from its // text, plus the elements it holds. Empty when the text has no words. +// +// A count's key isn't necessarily a valid Tengo identifier already -- it may +// be an HTML tag name (e.g. a hyphenated custom element like +// "my-component") or a "text.heading.h1"-derived scope -- so it is run +// through sanitizeMetricKey before becoming a `metric` formula parameter +// name. func BlockMetrics(text string, counts map[string]int) map[string]interface{} { params := map[string]interface{}{} @@ -293,8 +348,7 @@ func BlockMetrics(text string, counts map[string]int) map[string]interface{} { if strings.HasPrefix(k, "table") { continue } - k = strings.ReplaceAll(k, ".", "_") - params[k] = float64(v) + params[sanitizeMetricKey(k)] = float64(v) } addTextMetrics(params, doc) @@ -546,6 +600,24 @@ func (f *File) AddAlert(a Alert, blk nlp.Block, lines, pad int, lookup bool) { if a.Limit > 0 { f.limits[a.Check]++ } + + // Unconditional per-check alert counter, exposed to + // `metric` formulas through f.CheckCounts, which + // check.checkCounts wraps as check["Style.Rule"]. This is + // its own field, not a "check."-namespaced f.Metrics + // entry: f.Metrics is also where ast.go writes + // document-content-derived keys (HTML tag names, ...), + // so a shared map guarded only by a prefix convention is + // forgeable by a crafted tag literally named e.g. + // "check.Style.Rule" -- a dedicated field this is the + // only writer of has no such keyspace to inject into. + // Unlike f.limits above, this counts every alert + // actually reported, not just those from a rule opting + // into `limit:`. See #1163. + if f.CheckCounts == nil { + f.CheckCounts = make(map[string]int) + } + f.CheckCounts[a.Check]++ } } } diff --git a/internal/lint/check_object_test.go b/internal/lint/check_object_test.go new file mode 100644 index 00000000..78490757 --- /dev/null +++ b/internal/lint/check_object_test.go @@ -0,0 +1,813 @@ +package lint + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/vale-cli/vale/v3/internal/core" + "github.com/vale-cli/vale/v3/internal/glob" +) + +// buildCheckObjectLinter is the check["Style.Rule"]-syntax analog of +// compositeMetricLinter (metric_check_counts_test.go): a self-contained, +// temp-dir style with two independent `existence` rules (Composite.RuleA on +// "wordA", Composite.RuleB on "wordB") plus a `metric` rule ("Combined") +// whose formula and condition are supplied by the caller, so each test below +// can target a different check["..."] scenario without re-deriving the +// fixture setup. See buildCheckObjectLinter's caller comments for why this +// has to be Markdown, not plain text (a `metric` rule forces `scope: +// summary`, only ever reached from the Markdown/HTML AST walk). +func buildCheckObjectLinter(t *testing.T, formula, condition string) *Linter { + t.Helper() + + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Composite") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + rules := map[string]string{ + "RuleA.yml": "extends: existence\n" + + "message: \"ruleA: '%s'\"\n" + + "level: warning\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordA\n", + "RuleB.yml": "extends: existence\n" + + "message: \"ruleB: '%s'\"\n" + + "level: warning\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordB\n", + "Combined.yml": "extends: metric\n" + + "message: \"combined score: %s\"\n" + + "level: error\n" + + "formula: " + formula + "\n" + + "condition: \"" + condition + "\"\n", + } + + for name, content := range rules { + if err := os.WriteFile(filepath.Join(styleDir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Composite"} + cfg.GBaseStyles = []string{"Composite"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".md" + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + return linter +} + +// alertMessage returns the message of files' first alert matching check, or +// "" if none matched. +func alertMessage(files []*core.File, check string) string { + for _, f := range files { + for _, a := range f.Alerts { + if a.Check == check { + return a.Message + } + } + } + return "" +} + +// TestCheckObjectCombinesTwoChecks is the check["Style.Rule"]-syntax version +// of TestMetricFormulaCombinesCheckCounts (issue #1163's original motivating +// case): a `metric` rule combining two other checks' alert counts in a +// single formula. Under the old design this same case required flattening +// both check names into check_Composite_RuleA / check_Composite_RuleB +// identifiers; here they're indexed by their real names directly. +// +// Today, "check" is not a defined Tengo identifier at all -- ComputeMetrics +// never adds one -- so check["Composite.RuleA"] fails to even compile, +// which LintString surfaces as a non-nil error; every case below currently +// gets that error rather than the pass/fail result asserted here, which is +// the correct RED state. +func TestCheckObjectCombinesTwoChecks(t *testing.T) { + tests := []struct { + name string + text string + wantA int + wantB int + wantFired bool + }{ + { + name: "under threshold", + text: "wordA wordA wordB stays quiet in this paragraph of prose.", //nolint:dupword // intentional repeat + wantA: 2, + wantB: 1, + wantFired: false, + }, + { + name: "over threshold", + text: "wordA wordA wordA wordB crosses the line in this paragraph.", //nolint:dupword // intentional repeat + wantA: 3, + wantB: 1, + wantFired: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + linter := buildCheckObjectLinter(t, + `check["Composite.RuleA"] + check["Composite.RuleB"]`, "> 3") + + files, lintErr := linter.LintString(tt.text) + + if got := countAlerts(files, "Composite.RuleA"); got != tt.wantA { + t.Errorf("Composite.RuleA fired %d times, want %d", got, tt.wantA) + } + if got := countAlerts(files, "Composite.RuleB"); got != tt.wantB { + t.Errorf("Composite.RuleB fired %d times, want %d", got, tt.wantB) + } + + if lintErr != nil { + t.Errorf("LintString returned an unexpected error: %v", lintErr) + } + + fired := countAlerts(files, "Composite.Combined") > 0 + if fired != tt.wantFired { + t.Errorf("Composite.Combined fired = %v, want %v (lint error: %v)", + fired, tt.wantFired, lintErr) + } + }) + } +} + +// TestCheckObjectNeverFiredCheckReadsAsZero verifies that indexing a check +// that IS loaded, but never fired on this document, reads as 0 -- not a +// compile failure, and not silently indistinguishable from a typo (see +// TestCheckObjectUnknownCheckNameSurfacesError below for that distinction). +// wordB never appears, so Composite.RuleB never fires and has no f.Metrics +// entry for it at all. +func TestCheckObjectNeverFiredCheckReadsAsZero(t *testing.T) { + linter := buildCheckObjectLinter(t, `check["Composite.RuleB"]`, "> -1") + + files, lintErr := linter.LintString("wordA appears here in this paragraph of prose.") + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v -- a loaded, "+ + "never-fired check must read as 0, not fail to resolve", lintErr) + } + + if got := countAlerts(files, "Composite.RuleB"); got != 0 { + t.Fatalf("Composite.RuleB fired %d times, want 0 -- this test needs "+ + "RuleB to genuinely never fire", got) + } + + if got := countAlerts(files, "Composite.Combined"); got != 1 { + t.Errorf("Composite.Combined fired %d times, want 1 (0 > -1, treating "+ + "the never-fired check as 0)", got) + } +} + +// TestCheckObjectFiredCheckReturnsRealCount verifies the count read back is +// the check's genuine alert count, not just a truthy placeholder -- asserted +// against the alert's own message, which embeds the formula's numeric +// result (see formatMessages / "%.2f" in Metric.Run), so this fails if the +// object ever returned, say, 1 for "fired" instead of the real count. +func TestCheckObjectFiredCheckReturnsRealCount(t *testing.T) { + linter := buildCheckObjectLinter(t, `check["Composite.RuleA"]`, "> 0") + + files, lintErr := linter.LintString( + "wordA wordA wordA appears three times in this paragraph of prose.") //nolint:dupword // intentional repeat + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v", lintErr) + } + + if got := countAlerts(files, "Composite.RuleA"); got != 3 { + t.Fatalf("Composite.RuleA fired %d times, want 3", got) + } + + msg := alertMessage(files, "Composite.Combined") + if !strings.Contains(msg, "3.00") { + t.Errorf("expected Composite.Combined's message to embed the real "+ + "count 3.00, got %q", msg) + } +} + +// TestCheckObjectUnknownCheckNameSurfacesError is the concrete fix for the +// design review's most-likely-real-mistake finding: a misspelled check/rule +// name in a formula must surface a real, actionable error -- naming the bad +// check -- rather than silently evaluating to 0 the way the old +// checkCounterRE shape-match (`^check_\w+$`, matched regardless of whether +// the name corresponded to any real, loaded check) did. +// +// Composite.NoSuchRule is never defined anywhere in this fixture's style, so +// this is a genuine, unresolvable typo, not a never-fired-but-real check +// (see TestCheckObjectNeverFiredCheckReadsAsZero for that case). +func TestCheckObjectUnknownCheckNameSurfacesError(t *testing.T) { + linter := buildCheckObjectLinter(t, `check["Composite.NoSuchRule"]`, "> -1") + + files, lintErr := linter.LintString("wordA appears here in this paragraph of prose.") + + if lintErr == nil { + t.Fatal("expected LintString to return an error for the unknown " + + "check Composite.NoSuchRule, got nil") + } + if !strings.Contains(lintErr.Error(), "Composite.NoSuchRule") { + t.Errorf("expected the error to name the unknown check "+ + "Composite.NoSuchRule, got: %v", lintErr) + } + // Today, "check" isn't a defined Tengo identifier at all, so *every* + // check["..."] formula -- known-name or not -- fails to even compile, + // with a generic "unresolved reference 'check'" error. That message + // happens to echo the raw formula source (including the literal text + // "Composite.NoSuchRule") as annotated context, which would let the + // assertion above pass for the wrong reason: not because the unknown + // check was actually detected, but because the whole source line is + // quoted verbatim regardless of which check name appears in it. This + // requires the error to NOT be that generic compile-time failure, so the + // test still fails today for the right reason, and will only pass once + // IndexGet genuinely distinguishes an unknown check name at runtime. + if strings.Contains(lintErr.Error(), "unresolved reference") { + t.Errorf("expected a real runtime error identifying the unknown "+ + "check, not Tengo's generic compile-time \"unresolved "+ + "reference\" (which today just means \"check\" isn't a defined "+ + "identifier at all, not that this specific check name was "+ + "looked up and found missing), got: %v", lintErr) + } + + if got := countAlerts(files, "Composite.Combined"); got != 0 { + t.Errorf("Composite.Combined fired %d times, want 0 -- it should "+ + "error, not evaluate the unknown check as 0", got) + } +} + +// TestCheckObjectHandlesHyphenatedStyleName regression-tests a style +// directory name containing "-", like Vale's own bundled `write-good` style +// (see README.md, cmd/vale/pkg_test.go) -- a mainstream, first-class case, +// not a contrived one. +// +// Under the old identifier-flattening design this needed its own dedicated +// fix (see TestMetricFormulaHandlesHyphenatedStyleName in +// metric_check_counts_test.go): "write-good.TooWordy" sanitized to +// "check_write-good_TooWordy", which Tengo parsed as subtraction of two +// undefined identifiers and failed to compile. Indexing by the real, +// unflattened name sidesteps that class of bug entirely -- there's no +// sanitization step left to fail on the hyphen -- so this should be a clean +// pass with no special-casing needed. +func TestCheckObjectHandlesHyphenatedStyleName(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "write-good") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + rules := map[string]string{ + "TooWordy.yml": "extends: existence\n" + + "message: \"wordy: '%s'\"\n" + + "level: warning\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordy\n", + "Combined.yml": "extends: metric\n" + + "message: \"combined score: %s\"\n" + + "level: error\n" + + `formula: check["write-good.TooWordy"]` + "\n" + + "condition: \"> 0\"\n", + } + for name, content := range rules { + if err := os.WriteFile(filepath.Join(styleDir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"write-good"} + cfg.GBaseStyles = []string{"write-good"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".md" + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + files, lintErr := linter.LintString("wordy prose fills this paragraph.") + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v", lintErr) + } + + if got := countAlerts(files, "write-good.TooWordy"); got != 1 { + t.Errorf("write-good.TooWordy fired %d times, want 1 -- if this is 0, "+ + "the fixture's rule isn't loading at all", got) + } + if got := countAlerts(files, "write-good.Combined"); got != 1 { + t.Errorf("write-good.Combined fired %d times, want 1", got) + } +} + +// TestCheckObjectResolvesFormerlyCollidingCheckNamesIndependently is the +// redesign's central selling point, exercised through the real check[...] +// indexing path rather than through the absence of the old collisions map: +// "Foo-Bar.Baz" and "Foo.Bar-Baz" are the exact pair that used to sanitize +// to the same identifier, check_Foo_Bar_Baz (see writeCollisionSourceStyles +// and TestMetricFormulaFailsWhenReferencingCollision in +// metric_check_counts_test.go, which document and pin the OLD failure mode +// for that pair). Under the new design there's no flattening step left to +// collide on -- each name indexes the check object directly -- so both +// counts must come back independent and correct side by side in the same +// formula. +// +// collidesA fires twice and collidesB fires once, deliberately distinct +// counts: if the two were ever merged or one silently overwrote the other +// (the exact old failure mode), the combined formula would read 2 or 1 +// rather than the genuine 3, and both the ">2" condition's outcome and the +// message's embedded total would give it away. This reuses +// writeCollisionSourceStyles from metric_check_counts_test.go (same +// package) so the fixture is the identical colliding-name pair the old +// mechanism's tests exercised, not a fresh, easier-to-satisfy example. +func TestCheckObjectResolvesFormerlyCollidingCheckNamesIndependently(t *testing.T) { + dir := t.TempDir() + stylesDir := filepath.Join(dir, "styles") + writeCollisionSourceStyles(t, stylesDir) + + plainDir := filepath.Join(stylesDir, "Plain") + if err := os.MkdirAll(plainDir, 0o755); err != nil { + t.Fatal(err) + } + + combined := "extends: metric\n" + + "message: \"combined score: %s\"\n" + + "level: error\n" + + `formula: check["Foo-Bar.Baz"] + check["Foo.Bar-Baz"]` + "\n" + + "condition: \"> 2\"\n" + if err := os.WriteFile(filepath.Join(plainDir, "Combined.yml"), []byte(combined), 0o600); err != nil { + t.Fatal(err) + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(stylesDir) + cfg.Styles = []string{"Foo-Bar", "Foo", "Plain"} + cfg.GBaseStyles = []string{"Foo-Bar", "Foo", "Plain"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".md" + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + files, lintErr := linter.LintString( + "collidesA collidesA collidesB appear in this single paragraph together.") //nolint:dupword // intentional repeat + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v", lintErr) + } + + if got := countAlerts(files, "Foo-Bar.Baz"); got != 2 { + t.Fatalf("Foo-Bar.Baz fired %d times, want 2", got) + } + if got := countAlerts(files, "Foo.Bar-Baz"); got != 1 { + t.Fatalf("Foo.Bar-Baz fired %d times, want 1", got) + } + + if got := countAlerts(files, "Plain.Combined"); got != 1 { + t.Errorf("Plain.Combined fired %d times, want 1 (2 + 1 = 3 > 2) -- if "+ + "this is 0, the two formerly-colliding check names aren't "+ + "resolving to their own independent counts", got) + } + + msg := alertMessage(files, "Plain.Combined") + if !strings.Contains(msg, "3.00") { + t.Errorf("expected Plain.Combined's message to embed the genuine "+ + "combined count 3.00 (2 + 1, each check's own independent count), "+ + "got %q -- a mixed or overwritten value would read 2.00 or 1.00 "+ + "instead", msg) + } +} + +// TestCheckObjectDisabledForThisExtensionSurfacesError is the regression +// case for LoadedChecks needing to be scoped per file, not to the whole +// merged config: l.Manager.Rules() covers every rule loaded from every +// style, regardless of section or extension, but a real, already-supported +// Vale feature -- per-extension check toggling, e.g. `Composite.RuleB = NO` +// under a `[*.mdx]` section -- can turn a specific check off for a specific +// file. Composite.RuleB is loaded (it's compiled into the style once, not +// per file) but disabled for *.mdx here, so it can never fire against +// doc.mdx at all; a formula on doc.mdx referencing check["Composite.RuleB"] +// is asking about something that structurally cannot happen on this file, +// and must get the same real error an outright-nonexistent check name +// would, not a silent 0 -- indistinguishable from "loaded here, just never +// fired". The identical formula on doc.md, where RuleB is NOT disabled, +// must behave normally (0 if never fired, the real count if it did). +// +// Both fixtures are Markdown-family formats (.md and .mdx), not .md vs +// .txt: a `metric` rule forces `scope: summary`, only ever reached from the +// Markdown/HTML AST walk (see buildCheckObjectLinter above), so a .txt +// fixture would never even run Composite.UsesB, disabled check or not, and +// the test would pass for the wrong reason. +// +// This drives the scoping through cfg.SChecks/SecToPat directly -- the same +// fields a real `.vale.ini`'s `[*.mdx]` section populates via ini.go's +// processConfig -- rather than parsing an actual .vale.ini file, matching +// the lightweight, direct-field-assignment style the rest of this file's +// fixtures already use. +func TestCheckObjectDisabledForThisExtensionSurfacesError(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Composite") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + rules := map[string]string{ + "RuleA.yml": "extends: existence\n" + + "message: \"ruleA: '%s'\"\n" + + "level: warning\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordA\n", + "RuleB.yml": "extends: existence\n" + + "message: \"ruleB: '%s'\"\n" + + "level: warning\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordB\n", + "UsesB.yml": "extends: metric\n" + + "message: \"uses B: %s\"\n" + + "level: error\n" + + `formula: check["Composite.RuleB"]` + "\n" + + "condition: \"> -1\"\n", + } + for name, content := range rules { + if err := os.WriteFile(filepath.Join(styleDir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Composite"} + cfg.GBaseStyles = []string{"Composite"} + cfg.MinAlertLevel = 0 + // NewFile only infers a real file's format from its own extension when + // Flags.InExt is exactly ".txt" (see NewFile) -- this test lints doc.md + // and doc.mdx in the same run, so each needs its own real extension + // detected rather than a single overriding one. + cfg.Flags.InExt = ".txt" + + // Composite.RuleB = NO under [*.mdx]: the same effect a real .vale.ini + // section has, applied directly to the config fields ini.go's + // processConfig would otherwise populate from it. + mdxPat, err := glob.Compile("*.mdx") + if err != nil { + t.Fatal(err) + } + cfg.SecToPat["*.mdx"] = mdxPat + cfg.RuleKeys = append(cfg.RuleKeys, "*.mdx") + cfg.SChecks["*.mdx"] = map[string]bool{"Composite.RuleB": false} + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + text := "wordB appears once in this paragraph of prose.\n" + mdPath := filepath.Join(dir, "doc.md") + if err = os.WriteFile(mdPath, []byte(text), 0o600); err != nil { + t.Fatal(err) + } + mdxPath := filepath.Join(dir, "doc.mdx") + if err = os.WriteFile(mdxPath, []byte(text), 0o600); err != nil { + t.Fatal(err) + } + + mdFiles, mdErr := linter.Lint([]string{mdPath}, "*") + if mdErr != nil { + t.Fatalf("Lint returned an unexpected error for doc.md, where "+ + "Composite.RuleB is enabled: %v", mdErr) + } + if got := countAlerts(mdFiles, "Composite.RuleB"); got != 1 { + t.Fatalf("Composite.RuleB fired %d times on doc.md, want 1 -- this "+ + "test needs RuleB to genuinely fire there", got) + } + if msg := alertMessage(mdFiles, "Composite.UsesB"); !strings.Contains(msg, "1.00") { + t.Errorf("expected Composite.UsesB's message on doc.md, where "+ + "Composite.RuleB is enabled and fired once, to embed the real "+ + "count 1.00, got %q", msg) + } + + mdxFiles, mdxErr := linter.Lint([]string{mdxPath}, "*") + if mdxErr == nil { + t.Fatal("expected linting doc.mdx to fail: Composite.RuleB is " + + "disabled for *.mdx in this config, so it cannot fire on this " + + "file at all, and Composite.UsesB references it -- reading that " + + "as a silent 0 is exactly the failure mode this redesign exists " + + "to prevent, just scoped to a single file rather than the whole " + + "check name") + } + if !strings.Contains(mdxErr.Error(), "Composite.RuleB") { + t.Errorf("expected the error to name Composite.RuleB, got: %v", mdxErr) + } + if got := countAlerts(mdxFiles, "Composite.UsesB"); got != 0 { + t.Errorf("Composite.UsesB fired %d times on doc.mdx, want 0 -- it "+ + "should error, not evaluate the disabled check as 0", got) + } +} + +// TestCheckObjectBelowMinAlertLevelReadsAsZero is the regression case for +// checkApplies needing to exclude MinAlertLevel, not just reuse shouldRun +// wholesale: shouldRun answers two different questions with one bool -- +// "can this check structurally apply to this file" (extension/section/ +// base-style, what LoadedChecks needs) AND "is this check's severity at or +// above --minAlertLevel" (a display filter on which alerts get shown, see +// MinAlertLevel's doc comment in config.go, not a fact about whether the +// check can run at all). Composite.RuleB here is fully loaded and enabled +// for this file -- nothing disables it structurally -- it's just a +// `suggestion`-level check in a run whose MinAlertLevel is `warning`. Under +// the bug, that made it structurally indistinguishable from an outright +// nonexistent check: check["Composite.RuleB"] would hard-error instead of +// correctly reading 0 (RuleB is filtered out of the run entirely, so it can +// never produce a real count either way -- shouldRun gates it out before +// chk.Run ever executes -- but "never fired because filtered by level" +// still needs to read as a known check's honest 0, not an unknown-check +// error). +// +// wordB does appear in the text, but must not make RuleB actually report: +// if it did, this test would still pass even with the bug (a real, +// nonzero count also isn't the "not a known check" error), silently +// testing nothing about the MinAlertLevel exclusion specifically. +func TestCheckObjectBelowMinAlertLevelReadsAsZero(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Composite") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + rules := map[string]string{ + // suggestion is below the run's warning MinAlertLevel set below, so + // this check is filtered out of every lint run entirely -- but it + // is NOT disabled by any style/extension override, and IS in + // f.BaseStyles: structurally, it's a real, applicable check. + "RuleB.yml": "extends: existence\n" + + "message: \"ruleB: '%s'\"\n" + + "level: suggestion\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordB\n", + "UsesB.yml": "extends: metric\n" + + "message: \"uses B: %s\"\n" + + "level: error\n" + + `formula: check["Composite.RuleB"]` + "\n" + + "condition: \"> -1\"\n", + } + for name, content := range rules { + if err := os.WriteFile(filepath.Join(styleDir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Composite"} + cfg.GBaseStyles = []string{"Composite"} + cfg.Flags.InExt = ".md" + + // The `.vale.ini` path: MinAlertLevel = warning. ini.go's own + // "MinAlertLevel" handler does nothing more than this same assignment + // (see coreOpts["MinAlertLevel"] in ini.go), so setting the field + // directly is equivalent without needing a real ini file round-trip. + cfg.MinAlertLevel = core.LevelToInt["warning"] + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + files, lintErr := linter.LintString("wordB appears here in this paragraph of prose.") + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v -- "+ + "Composite.RuleB is structurally applicable here, just below "+ + "MinAlertLevel, so check[\"Composite.RuleB\"] must read 0, not "+ + "error", lintErr) + } + + if got := countAlerts(files, "Composite.RuleB"); got != 0 { + t.Fatalf("Composite.RuleB fired %d times, want 0 -- this test needs "+ + "MinAlertLevel to genuinely keep it from ever running, or the "+ + "real-vs-error distinction this test targets isn't exercised", + got) + } + if got := countAlerts(files, "Composite.UsesB"); got != 1 { + t.Errorf("Composite.UsesB fired %d times, want 1 (0 > -1, treating "+ + "the below-MinAlertLevel check as 0)", got) + } +} + +// TestCheckObjectBelowMinAlertLevelViaCLIFlagReadsAsZero is +// TestCheckObjectBelowMinAlertLevelReadsAsZero, but driven through the +// `--minAlertLevel` CLI flag's translation into cfg.MinAlertLevel instead +// of `.vale.ini`'s MinAlertLevel key -- the other documented way to set the +// same field (see cmd/vale/flag.go's help text and internal/core/source.go, +// which applies it as `cfg.MinAlertLevel = LevelToInt[cfg.Flags.AlertLevel]` +// once cfg.Flags.AlertLevel is a recognized level). That one-line +// translation is applied directly here, matching source.go's own logic, +// rather than routing through a full ReadPipeline + real .vale.ini file: +// both paths converge on the identical cfg.MinAlertLevel field this test +// (like the one above) actually exercises against checkApplies, and the +// CLI-flag-to-field translation itself is pre-existing, untouched by this +// change, and orthogonal to what's being regression-tested here. +func TestCheckObjectBelowMinAlertLevelViaCLIFlagReadsAsZero(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Composite") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + rules := map[string]string{ + "RuleB.yml": "extends: existence\n" + + "message: \"ruleB: '%s'\"\n" + + "level: suggestion\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordB\n", + "UsesB.yml": "extends: metric\n" + + "message: \"uses B: %s\"\n" + + "level: error\n" + + `formula: check["Composite.RuleB"]` + "\n" + + "condition: \"> -1\"\n", + } + for name, content := range rules { + if err := os.WriteFile(filepath.Join(styleDir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true, AlertLevel: "warning"}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Composite"} + cfg.GBaseStyles = []string{"Composite"} + cfg.Flags.InExt = ".md" + + // The `--minAlertLevel` CLI flag path: internal/core/source.go applies + // exactly this once cfg.Flags.AlertLevel is a recognized level. + if core.StringInSlice(cfg.Flags.AlertLevel, core.AlertLevels) { + cfg.MinAlertLevel = core.LevelToInt[cfg.Flags.AlertLevel] + } + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + files, lintErr := linter.LintString("wordB appears here in this paragraph of prose.") + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v -- "+ + "Composite.RuleB is structurally applicable here, just below "+ + "the --minAlertLevel-derived filter, so "+ + "check[\"Composite.RuleB\"] must read 0, not error", lintErr) + } + + if got := countAlerts(files, "Composite.RuleB"); got != 0 { + t.Fatalf("Composite.RuleB fired %d times, want 0 -- this test needs "+ + "MinAlertLevel to genuinely keep it from ever running", got) + } + if got := countAlerts(files, "Composite.UsesB"); got != 1 { + t.Errorf("Composite.UsesB fired %d times, want 1 (0 > -1, treating "+ + "the below-MinAlertLevel check as 0)", got) + } +} + +// TestCheckObjectCraftedTagCannotForgeACount is the end-to-end regression +// test for a real integrity bug found in review: before per-check counts +// moved to their own field (core.File.checkCounts, decoupled from +// f.Metrics), a document could forge a check's count. ast.go also writes +// f.Metrics from raw document content -- an HTML/XML tag's own name, via +// f.Metrics[txt]++, for a tag treated as a skippable block (its content +// never linted, so nothing in it can trip any real check) -- so a tag +// literally named after a check, e.g. "check.demo.rule", combined with a +// skip class, landed in f.Metrics indistinguishable, by prefix alone, from +// a genuine "check."-namespaced counter AddAlert would have written. +// Confirmed directly (see the investigation that produced this test): +// `.html`-format documents reach ast.go's tag tokenizer directly +// (golang.org/x/net/html permits "." in a tag name), and the crafted tag +// below incremented the old shared f.Metrics key to 1 with demo.rule's own +// token never appearing anywhere -- a real false-positive injection, not +// just a theoretical one. +// +// The style/rule names here are deliberately all-lowercase ("demo"/"rule", +// not "Composite"/"RuleB" like this file's other fixtures): the HTML +// tokenizer folds a tag name to lowercase (confirmed directly -- a +// "" tag tokenizes as "check.composite.ruleb"), so +// this specific vector only lines up with a check name that's already +// lowercase, or with an attacker who names their own style/rule in +// lowercase specifically to exploit it. That's a real, exploitable subset +// of check names (nothing stops a style or rule file from being named in +// lowercase), not a hypothetical case picked to make this test pass; it +// just means a test using capitalized names like "Composite.RuleB" would +// pass even under the vulnerable code, for the wrong reason (case mismatch, +// not the fix), which is why this test doesn't reuse the mixed-case +// fixtures the rest of this file does. +// +// (Markdown specifically was not exploitable this way even before this +// fix, incidentally: CommonMark's raw-HTML-block tag grammar excludes ".", +// so goldmark never recognizes such a tag as an HTML block to begin with -- +// that protection is a property of the Markdown grammar, not of Vale's own +// design, so it doesn't extend to .html or any other format whose +// converter is more permissive.) +// +// wordB never appears in the text below, so demo.rule's own token never +// matches and it does not actually fire; the crafted tag is the only +// possible source of a nonzero count. With the fix, check["demo.rule"] must +// read 0 regardless. +func TestCheckObjectCraftedTagCannotForgeACount(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "demo") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + rules := map[string]string{ + "rule.yml": "extends: existence\n" + + "message: \"rule: '%s'\"\n" + + "level: warning\n" + + "scope: paragraph\n" + + "tokens:\n" + + " - wordB\n", + "UsesIt.yml": "extends: metric\n" + + "message: \"uses it: %s\"\n" + + "level: error\n" + + `formula: check["demo.rule"]` + "\n" + + "condition: \"> 0\"\n", + } + for name, content := range rules { + if err := os.WriteFile(filepath.Join(styleDir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"demo"} + cfg.GBaseStyles = []string{"demo"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".html" + // The exact config path the investigation reproduced this under: a + // user-configured IgnoredClasses skip class (distinct from the + // built-in default skipClasses, which -- confirmed separately -- the + // same crafted tag also reaches). + cfg.IgnoredClasses = []string{"ignore"} + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + doc := "

Some real prose here.

\n" + + `forged` + "\n" + + "\n" + + files, lintErr := linter.LintString(doc) + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v", lintErr) + } + + if got := countAlerts(files, "demo.rule"); got != 0 { + t.Fatalf("demo.rule fired %d times, want 0 -- this test needs "+ + "demo.rule to genuinely never fire, or the crafted-tag-vs-real-"+ + "alert distinction this test targets isn't exercised", got) + } + if got := countAlerts(files, "demo.UsesIt"); got != 0 { + t.Errorf("demo.UsesIt fired %d times, want 0 -- the crafted tag "+ + "must not forge a nonzero count for demo.rule", got) + } +} diff --git a/internal/lint/lint.go b/internal/lint/lint.go index e26f263e..509ef924 100755 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -232,11 +232,42 @@ func (l *Linter) lintFile(src string) lintResult { return lintResult{err: err} } else if len(file.Checks) == 0 && len(file.BaseStyles) == 0 { if len(l.Manager.Config.GBaseStyles) == 0 && len(l.Manager.Config.GChecks) == 0 { - // There's nothing to do; bail early. + // There's nothing to do; bail early. No rule could apply to this + // file either way (see the LoadedChecks comment below), so this + // also saves the shouldRun pass over every loaded rule that + // would otherwise just produce an empty map. return lintResult{file: file} } } + // The set of check names that can actually run against THIS file, not + // every check loaded anywhere in the merged config: l.Manager.Rules() + // covers every style loaded across every section/extension, but a + // per-section or per-extension override (f.Checks/GChecks) can turn a + // given check off for this file specifically. Using the raw, unfiltered + // rule set here would let check["Style.Rule"] read a + // structurally-impossible check as a silent 0 instead of the real error + // it deserves -- the exact class of bug this object exists to prevent, + // just narrower. + // + // This deliberately uses checkApplies, not the fuller shouldRun a + // block-scoped rule is gated by below: shouldRun also excludes a check + // disabled via an in-text comment (a mid-document runtime toggle, not a + // fact about this file, and always a no-op here regardless since + // f.Comments is still empty at this point -- block-scoped in-text + // comments aren't parsed until the walk below) and one below + // --minAlertLevel (a display filter on severity, not a fact about + // whether the check runs at all -- conflating the two used to make a + // check["..."] reference to a fully-loaded, enabled check that simply + // sits below the run's alert-level filter hard-error instead of + // correctly reading 0). See checkApplies's own doc comment. + file.LoadedChecks = make(map[string]bool, len(l.Manager.Rules())) + for name := range l.Manager.Rules() { + if l.checkApplies(name, file) { + file.LoadedChecks[name] = true + } + } + // Determine what NLP tasks this particular file needs; the goal is to do // the least amount of work possible. file.NLP = l.Manager.AssignNLP(file) @@ -571,28 +602,27 @@ func (l *Linter) inScopeFor(blk nlp.Block) []scopedRule { return found } -func (l *Linter) shouldRun(name string, f *core.File, chk check.Rule) bool { - minLevel := l.Manager.Config.MinAlertLevel - run := false - - details := chk.Fields() - - // Configuration addresses the defining rule: a `consistency` alert's - // name carries a matched term, and a rule's own name may span - // subdirectories, so the rule is found by name, not by dot-count. - // See #129. +// checkApplies reports whether name could ever run against f at all, based +// solely on structural applicability -- which extensions/sections/styles +// it's enabled for (f.Checks, GChecks, f.BaseStyles) -- deliberately +// excluding two things shouldRun also weighs that aren't structural facts +// about this check and this file: f.QueryComments (an in-text opt-out, +// evaluated per-block at lint time, not a property of the file as a whole) +// and MinAlertLevel (a display filter on alert severity -- see its doc +// comment in config.go -- not a fact about whether the check runs at all). +// +// This exists for lintFile's LoadedChecks: a check["Style.Rule"] formula +// needs to know whether Style.Rule is capable of firing on this file, not +// whether today's --minAlertLevel would end up hiding its alerts if it did +// -- conflating the two would make a check["..."] reference to a check +// that's fully loaded and enabled, just below the run's alert-level filter, +// hard-error as "not a known check" instead of correctly reading 0. See +// shouldRun, which layers both of those exclusions back on top of this for +// the per-block gate a rule actually needs. +func (l *Linter) checkApplies(name string, f *core.File) bool { name = l.Manager.RuleForAlert(name) - - if f.QueryComments(name) { - // It has been disabled via an in-text comment. - return false - } else if core.LevelToInt[f.Level(name, details.Level)] < minLevel { - // The level this file gives the rule, which a section may have changed - // for this format alone. See #965. - return false - } - style := core.StyleName(name) + run := false // Has the check been disabled for this extension? // @@ -621,6 +651,28 @@ func (l *Linter) shouldRun(name string, f *core.File, chk check.Rule) bool { return true } +func (l *Linter) shouldRun(name string, f *core.File, chk check.Rule) bool { + minLevel := l.Manager.Config.MinAlertLevel + details := chk.Fields() + + // Configuration addresses the defining rule: a `consistency` alert's + // name carries a matched term, and a rule's own name may span + // subdirectories, so the rule is found by name, not by dot-count. + // See #129. + name = l.Manager.RuleForAlert(name) + + if f.QueryComments(name) { + // It has been disabled via an in-text comment. + return false + } else if core.LevelToInt[f.Level(name, details.Level)] < minLevel { + // The level this file gives the rule, which a section may have changed + // for this format alone. See #965. + return false + } + + return l.checkApplies(name, f) +} + func (l *Linter) match(s string) bool { if l.glob == nil { return true diff --git a/internal/lint/loaded_checks_bench_test.go b/internal/lint/loaded_checks_bench_test.go new file mode 100644 index 00000000..88ecc2f2 --- /dev/null +++ b/internal/lint/loaded_checks_bench_test.go @@ -0,0 +1,146 @@ +package lint + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/vale-cli/vale/v3/internal/core" +) + +// syntheticAITellsRuleCount matches the PR's own motivating case for +// exposing per-check alert counts to `metric` formulas: a style package like +// `tbhb/vale-ai-tells`, which ships about 110 independent pattern rules, +// each an isolated per-instance match. +const syntheticAITellsRuleCount = 110 + +// buildSyntheticAITellsStyle writes count independent `existence` rules into +// a new "AITells" style directory under dir, one per file, each matching a +// token no other rule -- and no word in aiTellsDocument -- matches. This is +// the shape LoadedChecks construction is benchmarked against below: a style +// with many independent rules, rather than a handful of rules extending each +// other or overlapping in what they match. +func buildSyntheticAITellsStyle(tb testing.TB, dir string, count int) { + tb.Helper() + + styleDir := filepath.Join(dir, "AITells") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + tb.Fatal(err) + } + + for i := 0; i < count; i++ { + rule := fmt.Sprintf( + "extends: existence\n"+ + "message: \"Avoid the tell-word 'aitellword%d'.\"\n"+ + "level: warning\n"+ + "scope: sentence\n"+ + "ignorecase: true\n"+ + "tokens:\n"+ + " - aitellword%d\n", i, i) + + path := filepath.Join(styleDir, fmt.Sprintf("Rule%03d.yml", i)) + if err := os.WriteFile(path, []byte(rule), 0o600); err != nil { + tb.Fatal(err) + } + } +} + +// aiTellsDocument builds realistic prose at least size bytes long, none of +// which contains any of the synthetic style's tell-words. The point of both +// benchmarks below is the cost of loading and running 110 independent rules +// against an ordinary document, not the cost of reporting alerts. +func aiTellsDocument(size int) string { + var b strings.Builder + for i := 0; b.Len() < size; i++ { + fmt.Fprintf(&b, "Paragraph %d walks through the change in plain "+ + "terms, noting what moved and why the team made the call it "+ + "did.\n\n", i) + fmt.Fprintf(&b, "The rollout went smoothly, and the team is now "+ + "watching the dashboards for anything unexpected over the next "+ + "few days.\n\n") + } + return b.String() +} + +// syntheticAITellsLinter returns a Linter loaded with exactly the synthetic +// AITells style built by buildSyntheticAITellsStyle -- no built-in Vale +// style alongside it, so l.Manager.Rules() is exactly the synthetic rule +// set -- plus the path to a realistic ~5KB document linted against it, +// matching the PR's own "synthetic 110-rule style ... realistic ~5KB +// document" benchmark description. +func syntheticAITellsLinter(tb testing.TB, count int) (*Linter, string) { + tb.Helper() + + dir := tb.TempDir() + buildSyntheticAITellsStyle(tb, dir, count) + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + tb.Fatal(err) + } + + cfg.AddStylesPath(dir) + cfg.Styles = []string{"AITells"} + cfg.GBaseStyles = []string{"AITells"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".md" + + linter, err := NewLinter(cfg) + if err != nil { + tb.Fatal(err) + } + + docPath := filepath.Join(dir, "bench.md") + if writeErr := os.WriteFile(docPath, []byte(aiTellsDocument(5*1024)), 0o600); writeErr != nil { + tb.Fatal(writeErr) + } + + return linter, docPath +} + +// BenchmarkLoadedChecksConstruction measures the cost of the per-file +// LoadedChecks build in lintFile (see lint.go): a loop over every loaded +// check name, calling checkApplies, which does a couple of map lookups and +// one short scan over f.BaseStyles per check. +// +// It replicates that exact loop rather than calling lintFile itself, so it +// isolates LoadedChecks construction from parsing, NLP assignment, and the +// walk over blocks that a full lint pass also does. See +// BenchmarkLintSyntheticAITells for that full pass, under the same +// synthetic style and document, for direct comparison. +func BenchmarkLoadedChecksConstruction(b *testing.B) { + linter, docPath := syntheticAITellsLinter(b, syntheticAITellsRuleCount) + + file, err := core.NewFile(docPath, linter.Manager.Config) + if err != nil { + b.Fatal(err) + } + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + file.LoadedChecks = make(map[string]bool, len(linter.Manager.Rules())) + for name := range linter.Manager.Rules() { + if linter.checkApplies(name, file) { + file.LoadedChecks[name] = true + } + } + } +} + +// BenchmarkLintSyntheticAITells lints the same synthetic 110-rule style and +// ~5KB document as BenchmarkLoadedChecksConstruction, but the full lint +// pass, so the two numbers are directly comparable: LoadedChecks +// construction against the total cost it is one part of. +func BenchmarkLintSyntheticAITells(b *testing.B) { + linter, docPath := syntheticAITellsLinter(b, syntheticAITellsRuleCount) + + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := linter.Lint([]string{docPath}, "*"); err != nil { + b.Fatal(err) + } + } +} diff --git a/internal/lint/metric_check_counts_test.go b/internal/lint/metric_check_counts_test.go new file mode 100644 index 00000000..3cb9bff9 --- /dev/null +++ b/internal/lint/metric_check_counts_test.go @@ -0,0 +1,258 @@ +package lint + +import ( + "os" + "path/filepath" + "testing" + + "github.com/vale-cli/vale/v3/internal/core" +) + +// countAlerts returns how many of files' alerts match check. +func countAlerts(files []*core.File, check string) int { + n := 0 + for _, f := range files { + for _, a := range f.Alerts { + if a.Check == check { + n++ + } + } + } + return n +} + +// writeCollisionSourceStyles writes two styles whose check names would have +// collided under the old identifier-flattening design -- style "Foo-Bar" +// rule "Baz" (check "Foo-Bar.Baz") and style "Foo" rule "Bar-Baz" (check +// "Foo.Bar-Baz"), both of which used to sanitize to the same identifier, +// check_Foo_Bar_Baz -- into stylesDir. Shared with +// TestCheckObjectResolvesFormerlyCollidingCheckNamesIndependently in +// check_object_test.go (same package), which exercises the real check[...] +// indexing path against this exact pair to confirm the redesign resolves +// their counts independently now that there's no flattening step to collide +// on. +func writeCollisionSourceStyles(t *testing.T, stylesDir string) { + t.Helper() + + fooBarDir := filepath.Join(stylesDir, "Foo-Bar") + fooDir := filepath.Join(stylesDir, "Foo") + if err := os.MkdirAll(fooBarDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(fooDir, 0o755); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(filepath.Join(fooBarDir, "Baz.yml"), []byte( + "extends: existence\n"+ + "message: \"baz: '%s'\"\n"+ + "level: warning\n"+ + "scope: paragraph\n"+ + "tokens:\n"+ + " - collidesA\n"), 0o600); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(filepath.Join(fooDir, "Bar-Baz.yml"), []byte( + "extends: existence\n"+ + "message: \"barbaz: '%s'\"\n"+ + "level: warning\n"+ + "scope: paragraph\n"+ + "tokens:\n"+ + " - collidesB\n"), 0o600); err != nil { + t.Fatal(err) + } +} + +// TestMetricFormulaSkipsWordlessDocumentWithoutError is the exact scenario +// that broke Vale's own shipped Readability style after the round-4 fix (in +// the now-deleted collision-detection machinery): a heading-and-code-fence- +// only document -- no prose "words" at all -- linted with a real, +// division-based readability formula (the shape of the bundled +// AutomatedReadability/LIX styles, referencing "characters", "words", and +// "sentences"). +// +// The built-in readability values themselves mean nothing without real +// prose and stay absent from ComputeMetrics's params for such a document; +// evaluating the formula anyway would fail with a Tengo "unresolved +// reference" compile error instead of the graceful skip this rule has +// always had for such a document. +// +// This must lint clean, with the readability rule skipped (no alert, no +// error) -- exactly matching testdata/fixtures/styles/Readability/test2.md, +// the actual shipped fixture this regression was caught against in +// internal/e2e's TestScenarios/styles/readability. +func TestMetricFormulaSkipsWordlessDocumentWithoutError(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Readability") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + // The same shape as testdata/styles/Readability/AutomatedReadability.yml: + // a division-based formula that would hit "unresolved reference" for any + // operand ComputeMetrics leaves out of params. + automatedReadability := "extends: metric\n" + + "message: \"Try to keep the Automated Readability Index (%s) below 8.\"\n" + + "formula: |\n" + + " (4.71 * (characters / words)) + (0.5 * (words / sentences)) - 21.43\n" + + "condition: \"> 8\"\n" + if err := os.WriteFile(filepath.Join(styleDir, "AutomatedReadability.yml"), []byte(automatedReadability), 0o600); err != nil { + t.Fatal(err) + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Readability"} + cfg.GBaseStyles = []string{"Readability"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".md" + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + // Heading + code fence only, no prose -- the exact shape of + // testdata/fixtures/styles/Readability/test2.md. + files, lintErr := linter.LintString("# A section with only code\n\n``` shell\nls\n```\n") + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v -- a wordless "+ + "document should skip a readability formula cleanly, not fail "+ + "it with an unresolved-reference compile error", lintErr) + } + + if got := countAlerts(files, "Readability.AutomatedReadability"); got != 0 { + t.Errorf("Readability.AutomatedReadability fired %d times, want 0 -- "+ + "it should be skipped entirely for a wordless document", got) + } +} + +// TestMetricParagraphScopeSeesPartialCheckCountNotFinalTotal pins the "so +// far" behavior described in measuredScope's doc comment and in the comment +// right above `parameters["check"] = newCheckCounts(...)` in metric.go: a +// `metric` rule that declares a scope narrower than the default `summary` -- +// here, `scope: paragraph` -- runs once per paragraph, in document order, and +// each run sees only the check counts recorded up to that point in the walk, +// not the document's eventual final total. A `summary`-scoped rule (the +// default, exercised elsewhere in this file) runs last and would see the +// final total instead. +// +// Empirically confirmed by running this test against the unmodified code +// before picking the expected values below, per the task's instruction not +// to assume the exact dispatch order: +// +// - Rules that apply to the same block run in ascending alphabetical order +// of their full check name (internal/lint/lint.go's inScopeFor sorts +// scopedRule by name), and every paragraph block here is far under +// parallelFloor, so it takes the serial path (lintBlockSerial): each +// rule's Run is called and its alerts are added -- incrementing +// f.CheckCounts -- one rule fully before the next rule for that same +// block even runs. +// - "Style.TheExistenceRule" sorts before "Style.TheMetricRule", so on any +// paragraph where the existence rule fires, its own alert for THAT +// paragraph is already counted by the time the metric rule evaluates the +// very same paragraph. This was the ambiguous case the task called out: +// confirmed empirically to be "already counted", not "not yet counted". +// That is why paragraph 1 below reads 1, not 0. +// +// The formula's condition (">= 0") is deliberately always true -- a count is +// never negative -- so the metric rule fires on every paragraph, giving one +// alert per paragraph whose message bakes in the exact check[...] value that +// paragraph saw. The three paragraphs fire the existence rule in the 1st and +// 3rd, but not the 2nd, giving three distinct sample points: +// +// - Paragraph 1: the existence rule fires here -> count so far = 1. +// - Paragraph 2: no new alert -> the same count carries forward = 1. +// - Paragraph 3: the existence rule fires again -> count so far = 2. +// +// Paragraphs 1 and 2 both read 1, which is NOT the document's final total of +// 2 (asserted separately below): a summary-scoped rule would have read 2 +// everywhere, which is exactly the "partial, not final" distinction this +// test exists to pin. +func TestMetricParagraphScopeSeesPartialCheckCountNotFinalTotal(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Style") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(filepath.Join(styleDir, "TheExistenceRule.yml"), []byte( + "extends: existence\n"+ + "message: \"found: '%s'\"\n"+ + "level: warning\n"+ + "scope: paragraph\n"+ + "tokens:\n"+ + " - trigger\n"), 0o600); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(filepath.Join(styleDir, "TheMetricRule.yml"), []byte( + "extends: metric\n"+ + "message: \"count so far: %s\"\n"+ + "scope: paragraph\n"+ + "formula: check[\"Style.TheExistenceRule\"]\n"+ + "condition: \">= 0\"\n"), 0o600); err != nil { + t.Fatal(err) + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Style"} + cfg.GBaseStyles = []string{"Style"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".md" + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + doc := "First paragraph with a trigger word.\n\n" + + "Second paragraph without it.\n\n" + + "Third paragraph has trigger again.\n" + + files, lintErr := linter.LintString(doc) + if lintErr != nil { + t.Fatalf("LintString returned an unexpected error: %v", lintErr) + } + + if got := countAlerts(files, "Style.TheExistenceRule"); got != 2 { + t.Fatalf("Style.TheExistenceRule fired %d times, want 2 (paragraphs 1 and 3)", got) + } + + var metricMessages []string + for _, f := range files { + for _, a := range f.Alerts { + if a.Check == "Style.TheMetricRule" { + metricMessages = append(metricMessages, a.Message) + } + } + } + + want := []string{ + "count so far: 1.00", + "count so far: 1.00", + "count so far: 2.00", + } + if len(metricMessages) != len(want) { + t.Fatalf("Style.TheMetricRule fired %d times, want %d: got %v", + len(metricMessages), len(want), metricMessages) + } + for i, msg := range metricMessages { + if msg != want[i] { + t.Errorf("paragraph %d: metric rule read %q, want %q -- a "+ + "paragraph-scoped metric rule must see only the check count "+ + "recorded so far at its position in the document, not the "+ + "final whole-document total (2)", i+1, msg, want[i]) + } + } +} diff --git a/internal/lint/nested_rule_disable_test.go b/internal/lint/nested_rule_disable_test.go new file mode 100644 index 00000000..83b064b6 --- /dev/null +++ b/internal/lint/nested_rule_disable_test.go @@ -0,0 +1,83 @@ +package lint + +import ( + "os" + "path/filepath" + "testing" + + "github.com/vale-cli/vale/v3/internal/core" + "github.com/vale-cli/vale/v3/internal/glob" +) + +// TestNestedRuleDirectoryDisableRegression is a probe for the +// checkApplies/RuleForAlert reconciliation done while rebasing this branch +// onto the "nested rule directories" feature: a rule loaded from a nested +// style directory has a name with more than one dot (e.g. +// "Std.dates.TimeFormat"), same as a `consistency` check's per-term alert +// name. checkApplies must resolve such a name via l.Manager.RuleForAlert +// (which knows the real, loaded rule names), not by blindly truncating to +// the first two dot-separated segments -- that would turn +// "Std.dates.TimeFormat" into "Std.dates" before looking it up in +// cfg.SChecks, missing a real, exact-name extension override entirely and +// letting the rule run where it should have been disabled. +func TestNestedRuleDirectoryDisableRegression(t *testing.T) { + dir := t.TempDir() + styleDir := filepath.Join(dir, "styles", "Std", "dates") + if err := os.MkdirAll(styleDir, 0o755); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(filepath.Join(styleDir, "TimeFormat.yml"), []byte( + "extends: existence\n"+ + "message: \"found: '%s'\"\n"+ + "level: warning\n"+ + "scope: paragraph\n"+ + "tokens:\n"+ + " - badtime\n"), 0o600); err != nil { + t.Fatal(err) + } + + cfg, err := core.NewConfig(&core.CLIFlags{IgnoreGlobal: true}) + if err != nil { + t.Fatal(err) + } + + cfg.AddStylesPath(filepath.Join(dir, "styles")) + cfg.Styles = []string{"Std"} + cfg.GBaseStyles = []string{"Std"} + cfg.MinAlertLevel = 0 + cfg.Flags.InExt = ".txt" + + // Std.dates.TimeFormat = NO under [*.md]: the same effect a real + // .vale.ini section has, applied directly to the config fields ini.go's + // processConfig would otherwise populate from it. + mdPat, err := glob.Compile("*.md") + if err != nil { + t.Fatal(err) + } + cfg.SecToPat["*.md"] = mdPat + cfg.RuleKeys = append(cfg.RuleKeys, "*.md") + cfg.SChecks["*.md"] = map[string]bool{"Std.dates.TimeFormat": false} + + linter, err := NewLinter(cfg) + if err != nil { + t.Fatal(err) + } + + path := filepath.Join(dir, "doc.md") + if err = os.WriteFile(path, []byte("A paragraph mentioning badtime here.\n"), 0o600); err != nil { + t.Fatal(err) + } + + files, lintErr := linter.Lint([]string{path}, "*") + if lintErr != nil { + t.Fatalf("Lint returned an unexpected error: %v", lintErr) + } + + if got := countAlerts(files, "Std.dates.TimeFormat"); got != 0 { + t.Errorf("Std.dates.TimeFormat fired %d times, want 0 -- it was "+ + "disabled for .md via a section override, and a nested-directory "+ + "rule name must not be truncated past what the override actually "+ + "named", got) + } +} diff --git a/testdata/e2e/checks.yaml b/testdata/e2e/checks.yaml index 861c3305..d5db81ae 100644 --- a/testdata/e2e/checks.yaml +++ b/testdata/e2e/checks.yaml @@ -19,6 +19,46 @@ cases: want: | test.md:1:1:Checks.MetricValue:This topic has 1.00 H2s in it. + - name: metric/check-counts + about: "#1163 -- a `metric` formula reads other checks' per-document + alert counts as check[\"Style.Rule\"], so a rule combining several + independent signals becomes expressible." + files: + .vale.ini: | + StylesPath = styles + MinAlertLevel = suggestion + + [*.md] + BasedOnStyles = T + styles/T/WordA.yml: | + extends: existence + message: "found an A-word" + level: suggestion + scope: paragraph + tokens: + - foo + styles/T/WordB.yml: | + extends: existence + message: "found a B-word" + level: suggestion + scope: paragraph + tokens: + - bar + styles/T/Combined.yml: | + extends: metric + message: "combined signal too high (%s)" + level: warning + formula: check["T.WordA"] + check["T.WordB"] + condition: "> 1" + test.md: | + A paragraph mentioning foo and bar together. + args: test.md + exit: 0 + want: | + test.md:1:1:T.Combined:combined signal too high (2.00) + test.md:1:24:T.WordA:found an A-word + test.md:1:32:T.WordB:found a B-word + - name: conditional dir: Conditional args: .