fix: keep a rule's level in the section that set it

Fixes #965

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

Author: jdkato

internal/core/config.go

@@ -189,28 +189,29 @@ type CLIFlags struct {
 // Config holds the configuration values from both the CLI and `.vale.ini`.
 type Config struct {
 	// General configuration
-	BlockIgnores      map[string][]string        // A list of blocks to ignore
-	Checks            []string                   // All checks to load
-	Formats           map[string]string          // A map of unknown -> known formats
-	Asciidoctor       map[string]string          // A map of asciidoctor attributes
-	FormatToLang      map[string]string          // A map of format to lang ID
-	GBaseStyles       []string                   // Global base style
-	GChecks           map[string]bool            // Global checks
-	IgnoredClasses    []string                   // A list of HTML classes to ignore
-	IgnoredScopes     []string                   // A list of HTML tags to ignore
-	MinAlertLevel     int                        // Lowest alert level to display
-	Vocab             []string                   // The active project
-	RuleToLevel       map[string]string          // Single-rule level changes
-	SBaseStyles       map[string][]string        // Syntax-specific base styles
-	SChecks           map[string]map[string]bool // Syntax-specific checks
-	SkippedScopes     []string                   // A list of HTML blocks to ignore
-	Stylesheets       map[string]string          // XSLT stylesheet
-	TokenIgnores      map[string][]string        // A list of tokens to ignore
-	CommentDelimiters map[string][2]string       // Strings to treat as comment delimiters. Indicates the start and end delimiters.
-	WordTemplate      string                     // The template used in YAML -> regexp list conversions
-	RootINI           string                     // the path to the project's .vale.ini file
-	Paths             []string                   // A list of paths to search for styles
-	ConfigFiles       []string                   // A list of configuration files to load
+	BlockIgnores      map[string][]string          // A list of blocks to ignore
+	Checks            []string                     // All checks to load
+	Formats           map[string]string            // A map of unknown -> known formats
+	Asciidoctor       map[string]string            // A map of asciidoctor attributes
+	FormatToLang      map[string]string            // A map of format to lang ID
+	GBaseStyles       []string                     // Global base style
+	GChecks           map[string]bool              // Global checks
+	IgnoredClasses    []string                     // A list of HTML classes to ignore
+	IgnoredScopes     []string                     // A list of HTML tags to ignore
+	MinAlertLevel     int                          // Lowest alert level to display
+	Vocab             []string                     // The active project
+	RuleToLevel       map[string]string            // Single-rule level changes
+	SBaseStyles       map[string][]string          // Syntax-specific base styles
+	SChecks           map[string]map[string]bool   // Syntax-specific checks
+	SLevels           map[string]map[string]string // Syntax-specific level changes
+	SkippedScopes     []string                     // A list of HTML blocks to ignore
+	Stylesheets       map[string]string            // XSLT stylesheet
+	TokenIgnores      map[string][]string          // A list of tokens to ignore
+	CommentDelimiters map[string][2]string         // Strings to treat as comment delimiters. Indicates the start and end delimiters.
+	WordTemplate      string                       // The template used in YAML -> regexp list conversions
+	RootINI           string                       // the path to the project's .vale.ini file
+	Paths             []string                     // A list of paths to search for styles
+	ConfigFiles       []string                     // A list of configuration files to load
 
 	AcceptedTokens []string `json:"-"` // Project-specific vocabulary (okay)
 	RejectedTokens []string `json:"-"` // Project-specific vocabulary (avoid)
@@ -242,6 +243,7 @@ func NewConfig(flags *CLIFlags) (*Config, error) {
 	cfg.RuleToLevel = make(map[string]string)
 	cfg.SBaseStyles = make(map[string][]string)
 	cfg.SChecks = make(map[string]map[string]bool)
+	cfg.SLevels = make(map[string]map[string]string)
 	cfg.SecToPat = make(map[string]glob.Glob)
 	cfg.Stylesheets = make(map[string]string)
 	cfg.TokenIgnores = make(map[string][]string)

internal/core/file.go

@@ -42,6 +42,11 @@ type File struct {
 	Checks     map[string]bool   // syntax-specific checks assigned in .vale
 	ChkToCtx   map[string]string // maps a temporary context to a particular check
 
+	// Levels holds the level each rule was given by the sections matching this
+	// file, which is not the same as the level it was compiled with: the same
+	// rule can be an error in one format and a warning in another. See #965.
+	Levels map[string]string
+
 	// chkMasked counts, per check and match text, the occurrences already
 	// masked into ChkToCtx this block. An occurrence a prior alert consumed
 	// is gone from the context, so it must not widen a later alert's
@@ -123,6 +128,7 @@ func NewFile(src string, config *Config) (*File, error) {
 
 	baseStyles := config.GBaseStyles
 	checks := make(map[string]bool)
+	levels := make(map[string]string)
 
 	for _, fp := range filepaths {
 		for _, sec := range config.StyleKeys {
@@ -136,6 +142,11 @@ func NewFile(src string, config *Config) (*File, error) {
 				for k, v := range config.SChecks[sec] {
 					checks[k] = v
 				}
+				// Sections are visited in the order they were written, so a
+				// later one wins -- for this file, and no other. See #965.
+				for k, v := range config.SLevels[sec] {
+					levels[k] = v
+				}
 			}
 		}
 	}
@@ -171,7 +182,8 @@ func NewFile(src string, config *Config) (*File, error) {
 
 	file := File{
 		NormedExt: ext, Format: format, RealExt: filepath.Ext(path),
-		BaseStyles: baseStyles, Checks: checks, Lines: lines, Content: content,
+		BaseStyles: baseStyles, Checks: checks, Levels: levels,
+		Lines: lines, Content: content,
 		Comments: make(map[string]bool), history: make(map[string]int),
 		simple: config.Flags.Simple, Transform: transform,
 		limits: make(map[string]int), Path: path, Metrics: make(map[string]int),
@@ -572,3 +584,17 @@ func (f *File) SetMetaScope(scope string) {
 		f.MetaScope = ""
 	}
 }
+
+// Level returns the level `name` should report at in this file, falling back
+// to the level it was compiled with.
+//
+// A rule's own level covers it; a level set for its style covers the rest of
+// that style, mirroring how the two are resolved at compile time.
+func (f *File) Level(name, compiled string) string {
+	if level, ok := f.Levels[name]; ok {
+		return level
+	} else if level, ok = f.Levels[StyleName(name)]; ok {
+		return level
+	}
+	return compiled
+}

internal/core/ini.go

@@ -85,12 +85,19 @@ func loadVocab(root string, cfg *Config) error {
 	return err
 }
 
-func validateLevel(key, val string, cfg *Config) bool {
+// validateLevel reports whether `key` names a rule that should run, recording
+// any level it was given in `levels`.
+//
+// A level set under a section belongs to that section. Writing every one into
+// a single map made the last section in the file decide the level everywhere,
+// so `Vale.Spelling = warning` for Markdown quietly downgraded HTML too. See
+// #965.
+func validateLevel(key, val string, levels map[string]string) bool {
 	options := []string{"YES", "suggestion", "warning", "error"}
 	if val == "NO" || !StringInSlice(val, options) {
 		return false
 	} else if val != "YES" {
-		cfg.RuleToLevel[key] = val
+		levels[key] = val
 	}
 	return true
 }
@@ -419,7 +426,7 @@ func processConfig(uCfg *ini.File, cfg *Config, dry bool) (*ini.File, error) {
 			msg := fmt.Sprintf("'%s' is a syntax-specific option", k)
 			return nil, NewE201FromTarget(msg, k, cfg.RootINI)
 		} else {
-			cfg.GChecks[k] = validateLevel(k, global.Key(k).String(), cfg)
+			cfg.GChecks[k] = validateLevel(k, global.Key(k).String(), cfg.RuleToLevel)
 			cfg.Checks = append(cfg.Checks, k)
 		}
 	}
@@ -437,6 +444,7 @@ func processConfig(uCfg *ini.File, cfg *Config, dry bool) (*ini.File, error) {
 		cfg.SecToPat[sec] = pat
 
 		syntaxMap := make(map[string]bool)
+		levelMap := make(map[string]string)
 		for _, k := range uCfg.Section(sec).KeyStrings() {
 			if _, option := coreOpts[k]; option {
 				return nil, NewE201FromTarget(fmt.Sprintf(coreError, k), k, cfg.RootINI)
@@ -445,12 +453,13 @@ func processConfig(uCfg *ini.File, cfg *Config, dry bool) (*ini.File, error) {
 					return nil, err
 				}
 			} else {
-				syntaxMap[k] = validateLevel(k, uCfg.Section(sec).Key(k).String(), cfg)
+				syntaxMap[k] = validateLevel(k, uCfg.Section(sec).Key(k).String(), levelMap)
 				cfg.Checks = append(cfg.Checks, k)
 			}
 		}
 		cfg.RuleKeys = append(cfg.RuleKeys, sec)
 		cfg.SChecks[sec] = syntaxMap
+		cfg.SLevels[sec] = levelMap
 	}
 
 	return uCfg, nil

internal/core/ini_test.go

@@ -219,3 +219,136 @@ func Test_shadowLoad_sectionsDoNotInherit(t *testing.T) {
 		t.Errorf("[*.txt] should hold only its own styles; got %v", got)
 	}
 }
+
+// A level set under a section belongs to that section. Collecting them all in
+// one map let the last section in the file decide the level everywhere. See
+// #965.
+func Test_processConfig_sectionLevels(t *testing.T) {
+	cases := []struct {
+		description string
+		body        string
+		levels      map[string]map[string]string
+		global      map[string]string
+	}{
+		{
+			description: "one section overrides a shared style",
+			body: `[*.{html,md}]
+BasedOnStyles = Vale
+
+[*.md]
+Vale.Spelling = warning
+`,
+			levels: map[string]map[string]string{
+				"*.{html,md}": {},
+				"*.md":        {"Vale.Spelling": "warning"},
+			},
+			global: map[string]string{},
+		},
+		{
+			description: "each section keeps its own level",
+			body: `[*.html]
+Vale.Spelling = error
+
+[*.md]
+Vale.Spelling = warning
+`,
+			levels: map[string]map[string]string{
+				"*.html": {"Vale.Spelling": "error"},
+				"*.md":   {"Vale.Spelling": "warning"},
+			},
+			global: map[string]string{},
+		},
+		{
+			description: "a level under [*] stays global",
+			body: `[*]
+Vale.Spelling = suggestion
+
+[*.md]
+BasedOnStyles = Vale
+`,
+			levels: map[string]map[string]string{
+				"*.md": {},
+			},
+			global: map[string]string{"Vale.Spelling": "suggestion"},
+		},
+		{
+			description: "YES and NO carry no level",
+			body: `[*.md]
+Vale.Spelling = YES
+Vale.Repetition = NO
+`,
+			levels: map[string]map[string]string{
+				"*.md": {},
+			},
+			global: map[string]string{},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.description, func(t *testing.T) {
+			uCfg, err := shadowLoad([]byte(c.body))
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			conf, err := NewConfig(&CLIFlags{})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if _, err = processConfig(uCfg, conf, false); err != nil {
+				t.Fatal(err)
+			}
+
+			for sec, want := range c.levels {
+				got := conf.SLevels[sec]
+				if len(got) != len(want) {
+					t.Fatalf("SLevels[%q] = %v, want %v", sec, got, want)
+				}
+				for k, v := range want {
+					if got[k] != v {
+						t.Errorf("SLevels[%q][%q] = %q, want %q", sec, k, got[k], v)
+					}
+				}
+			}
+
+			if len(conf.RuleToLevel) != len(c.global) {
+				t.Fatalf("RuleToLevel = %v, want %v", conf.RuleToLevel, c.global)
+			}
+			for k, v := range c.global {
+				if conf.RuleToLevel[k] != v {
+					t.Errorf("RuleToLevel[%q] = %q, want %q", k, conf.RuleToLevel[k], v)
+				}
+			}
+		})
+	}
+}
+
+// A file reports a rule at the level its own sections gave it, falling back to
+// the level the rule was compiled with.
+func TestFileLevel(t *testing.T) {
+	f := File{Levels: map[string]string{
+		"Vale.Spelling": "warning",
+		"proselint":     "suggestion",
+	}}
+
+	tests := []struct {
+		name     string
+		rule     string
+		compiled string
+		want     string
+	}{
+		{"the rule itself", "Vale.Spelling", "error", "warning"},
+		{"its style", "proselint.Typography", "error", "suggestion"},
+		{"neither", "Microsoft.Wordiness", "error", "error"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := f.Level(tt.rule, tt.compiled); got != tt.want {
+				t.Errorf("Level(%q, %q) = %q, want %q",
+					tt.rule, tt.compiled, got, tt.want)
+			}
+		})
+	}
+}

internal/lint/lint.go

@@ -427,14 +427,28 @@ func (l *Linter) lintBlock(f *core.File, blk nlp.Block, lines, pad int, lookup b
 			if f.QueryComments(r.name + "[" + found[i][j].Match + "]") {
 				continue
 			}
-			core.FormatAlert(&found[i][j], info.Limit, info.Level, r.name)
+			setLevel(&found[i][j], f, info, r.name)
 			f.AddAlert(found[i][j], blk, lines, pad, lookup)
 		}
 	}
 
 	return nil
 }
 
+// setLevel finishes an alert, reporting it at the level this file gives its
+// rule.
+//
+// Assigning the severity rather than leaving it to FormatAlert is what makes a
+// per-format level take effect: a check builds its alerts with the level it was
+// compiled with already set, and FormatAlert only fills a severity that is
+// still empty. See #965.
+func setLevel(a *core.Alert, f *core.File, info check.Definition, name string) {
+	level := f.Level(name, info.Level)
+
+	core.FormatAlert(a, info.Limit, level, name)
+	a.Severity = level
+}
+
 // lintBlockSerial is lintBlock without the concurrency, and without what it
 // costs to set up.
 func (l *Linter) lintBlockSerial(f *core.File, blk nlp.Block, rules []scopedRule, lines, pad int, lookup bool) error {
@@ -454,7 +468,7 @@ func (l *Linter) lintBlockSerial(f *core.File, blk nlp.Block, rules []scopedRule
 			if f.QueryComments(name + "[" + alerts[i].Match + "]") {
 				continue
 			}
-			core.FormatAlert(&alerts[i], info.Limit, info.Level, name)
+			setLevel(&alerts[i], f, info, name)
 			f.AddAlert(alerts[i], blk, lines, pad, lookup)
 		}
 	}
@@ -561,7 +575,9 @@ func (l *Linter) shouldRun(name string, f *core.File, chk check.Rule) bool {
 	if f.QueryComments(name) {
 		// It has been disabled via an in-text comment.
 		return false
-	} else if core.LevelToInt[details.Level] < minLevel {
+	} 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
 	}