fix: don't treat substitution replacements as regexes (#1038)

Signed-off-by: Joseph Kato <joseph@jdkato.io>

Author: jdkato

internal/check/definition.go

@@ -325,6 +325,9 @@ func makeRegexp(
 	return regex
 }
 
+// matchToken reports whether `observed` already conforms to `expected`. When
+// `expected` is a plain phrase it's an exact comparison; otherwise `expected`
+// is treated as a regex (e.g., a vocab term like `[pP]y.*\b`).
 func matchToken(expected, observed string, ignorecase bool) bool {
 	p := expected
 	if ignorecase {

internal/check/substitution.go

@@ -119,7 +119,23 @@ func (s Substitution) Run(blk nlp.Block, _ *core.File, cfg *core.Config) ([]core
 					return alerts, msgErr
 				}
 
-				same := matchToken(expected, observed, false)
+				// Determine whether `observed` is already in an acceptable
+				// form (a no-op suggestion). For a `replace` action the swap
+				// value is literal replacement text -- one or more `|`-separated
+				// suggestions (the same split used to build `action.Params`
+				// below) -- so we compare each option literally. Otherwise an
+				// option like `...` would be read as a regex matching any three
+				// characters (e.g. `,,,`), suppressing a real finding (#1038).
+				//
+				// For non-`replace` rules the value may instead be a regex that
+				// describes the acceptable forms (e.g. a vocab term `[pP]y.*\b`,
+				// or a `LookAround`-style pattern), which we still match as one.
+				var same bool
+				if s.Fields().Action.Name == "replace" {
+					same = core.StringInSlice(observed, getOptions(expected))
+				} else {
+					same = matchToken(expected, observed, false)
+				}
 				if !same && !isMatch(s.exceptRe, observed) {
 					action := s.Fields().Action
 					if action.Name == "replace" && len(action.Params) == 0 {

testdata/features/checks.feature

@@ -110,6 +110,8 @@ Feature: Checks
             test.md:25:1:Bugs.SameCase:Use 'MPL 2.0' instead of 'mpl 2.0'
             test.md:27:1:Bugs.SameCase:Use 'MPL 2.0' instead of 'MPL2.0'
             test.md:31:15:Bugs.EmptyReplace:Use  instead of 'obvious'.
+            test.md:33:4:Bugs.Commas:Use a single comma or an ellipsis, not multiple consecutive commas.
+            test.md:33:23:Bugs.Commas:Use a single comma or an ellipsis, not multiple consecutive commas.
             test2.md:3:1:demo.CapSub:Use 'Change to the `/etc` directory' instead of 'Change into the `/etc` directory'.
             test2.md:7:1:demo.CapSub:Use 'Change to the `/home/user` directory' instead of 'Change into the `/home/user` directory'.
             test2.md:9:1:demo.CapSub:Use 'Change to the `/etc/X11` directory' instead of 'Change into the `/etc/X11` directory'.

testdata/fixtures/checks/Substitution/.vale.ini

@@ -10,6 +10,7 @@ Bugs.KeepCase = YES
 Bugs.TermCase = YES
 Bugs.SameCase = YES
 Bugs.EmptyReplace = YES
+Bugs.Commas = YES
 
 [test2.md]
 demo.CapSub = YES

testdata/fixtures/checks/Substitution/test.md

@@ -29,3 +29,5 @@ MPL2.0
 MPL 2.0
 
 The answer is obvious.
+
+Two,, commas and three,,, commas.

testdata/styles/Bugs/Commas.yml

@@ -0,0 +1,8 @@
+extends: substitution
+message: "Use a single comma or an ellipsis, not multiple consecutive commas."
+level: error
+nonword: true
+action:
+  name: replace
+swap:
+  ',{2,}': ',|...'