diff --git a/cmd/vale/command.go b/cmd/vale/command.go index 871d9bee..06331c27 100644 --- a/cmd/vale/command.go +++ b/cmd/vale/command.go @@ -244,7 +244,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 77ba4c93..84d7cb70 100644 --- a/internal/check/metric.go +++ b/internal/check/metric.go @@ -59,11 +59,19 @@ func (o Metric) Run(_ nlp.Block, f *core.File, _ *core.Config) ([]core.Alert, er ctx, cancel := context.WithTimeout(context.Background(), tengoTimeout) defer cancel() - parameters, err := f.ComputeMetrics() + parameters, rawCheckCounts, hasProse, err := f.ComputeMetrics() if err != nil { return alerts, err - } else if len(parameters) == 0 { - // empty file. + } + + if !hasProse { + // A heading-and-code-fence-only document, 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 + // ComputeMetrics). Evaluating anyway would fail with an opaque Tengo + // "unresolved reference" compile error instead of the graceful no-op + // this rule has always had for such a document. return alerts, nil } @@ -75,6 +83,14 @@ func (o Metric) Run(_ nlp.Block, f *core.File, _ *core.Config) ([]core.Alert, er } } + // Every loaded check's per-document alert count 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. + parameters["check"] = newCheckCounts(rawCheckCounts, 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 1fb54e9b..6c2f2036 100644 --- a/internal/core/addalert_test.go +++ b/internal/core/addalert_test.go @@ -27,3 +27,125 @@ func TestAddAlertNegativeSpan(t *testing.T) { }() f.AddAlert(a, blk, 1, 0, false) } + +// 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..56ee6f89 --- /dev/null +++ b/internal/core/check_object_test.go @@ -0,0 +1,126 @@ +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 the second, raw-counts return value, 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, checkCounts, _, 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 := 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.") + + _, checkCounts, _, err := f.ComputeMetrics() + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + if got, ok := 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, checkCounts, _, 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 := checkCounts["Foo-Bar.Baz"], 1; got != want { + t.Errorf("expected checkCounts[%q] = %d, got %d", "Foo-Bar.Baz", want, got) + } + if got, want := 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 c7a16288..97cdbb4a 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,17 +78,42 @@ 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 - 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 + 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 ComputeMetrics, which returns + // this alongside params, and check.checkCounts, which wraps it 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 } // lineStarts returns the byte offset at which each line of ctx begins. @@ -269,32 +318,52 @@ func (f *File) SortedAlerts() []Alert { return f.Alerts } -// ComputeMetrics returns all of f's metrics. -func (f *File) ComputeMetrics() (map[string]interface{}, error) { +// ComputeMetrics returns all of f's metrics, plus f's raw per-check alert +// counts (f.checkCounts, populated by AddAlert -- see its own doc comment +// for why this lives in a field of its own rather than sharing f.Metrics +// with ast.go's document-content-derived bookkeeping), plus whether f's +// Summary has any prose at all. +// +// The caller -- check.Metric.Run -- builds a separate indexable +// check.checkCounts object from the returned counts and adds it to the +// formula's parameters under "check", so a formula reads a check's count as +// check["Style.Rule"] by its real, unflattened name, rather than as a +// sanitized Tengo identifier that a second, differently-named check could +// collide on (e.g. "Foo-Bar.Baz" and "Foo.Bar-Baz" both sanitizing to +// "check_Foo_Bar_Baz" -- see sanitizeMetricKey). +// +// hasProse reports whether f.Summary contains any prose "words" at all (a +// heading-and-code-fence-only document has none); the built-in readability +// values (words, characters, sentences, ...) mean nothing without real prose +// and are never computed in that case. A caller that needs to know "is there +// anything readability-relevant to compute here" -- e.g. Metric.Run, +// deciding whether to skip a formula entirely rather than fail it with an +// unhelpful Tengo "unresolved reference" error -- asks for that explicitly +// rather than inferring it from params being non-empty. +func (f *File) ComputeMetrics() (map[string]interface{}, map[string]int, bool, error) { params := map[string]interface{}{} doc := summarize.NewDocument(f.Summary.String()) if doc.NumWords == 0 { - return params, nil + return params, f.checkCounts, false, nil } for k, v := range f.Metrics { if strings.HasPrefix(k, "table") { continue } - k = strings.ReplaceAll(k, ".", "_") - params[k] = float64(v) + params[sanitizeMetricKey(k)] = float64(v) } - params["complex_words"] = doc.NumComplexWords - params["long_words"] = doc.NumLongWords - params["sentences"] = doc.NumSentences - params["characters"] = doc.NumCharacters - params["words"] = doc.NumWords - params["polysyllabic_words"] = doc.NumPolysylWords - params["syllables"] = doc.NumSyllables + params["complex_words"] = float64(doc.NumComplexWords) + params["long_words"] = float64(doc.NumLongWords) + params["sentences"] = float64(doc.NumSentences) + params["characters"] = float64(doc.NumCharacters) + params["words"] = float64(doc.NumWords) + params["polysyllabic_words"] = float64(doc.NumPolysylWords) + params["syllables"] = float64(doc.NumSyllables) - return params, nil + return params, f.checkCounts, true, nil } // FindLoc calculates the line and span of an Alert. @@ -516,6 +585,23 @@ 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 ComputeMetrics's checkCounts + // return value. This is its own field (f.checkCounts), + // 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 f8a6e4d8..f0f70168 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) @@ -566,29 +597,40 @@ 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() +// consistencyBaseName strips a consistency check's variant suffix down to +// its base "Style.Rule" name -- see #129 -- so lookups keyed by the plain +// check name (f.Checks, GChecks, f.Comments, f.Levels) find the entry the +// user actually wrote rather than missing it over an internal, dotted +// sub-variant name. A name with at most one dot is returned unchanged. +func consistencyBaseName(name string) string { if strings.Count(name, ".") > 1 { - // NOTE: This fixes the loading issue with consistency checks. - // - // See #129. list := strings.Split(name, ".") - name = strings.Join([]string{list[0], list[1]}, ".") - } - - 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 strings.Join([]string{list[0], list[1]}, ".") } + return name +} +// 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 = consistencyBaseName(name) style := core.StyleName(name) + run := false // Has the check been disabled for this extension? // @@ -617,6 +659,23 @@ 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() + name = consistencyBaseName(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/metric_check_counts_test.go b/internal/lint/metric_check_counts_test.go new file mode 100644 index 00000000..56bd7455 --- /dev/null +++ b/internal/lint/metric_check_counts_test.go @@ -0,0 +1,133 @@ +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) + } +} diff --git a/testdata/e2e/checks.yaml b/testdata/e2e/checks.yaml index 7f225900..25c29061 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: .