From d2c873b6bc80162901f7a881edc2b9a853aae5f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 30 Sep 2026 15:39:34 -0300 Subject: [PATCH 1/2] fix(core): name the file and key in config value type errors Each config file's values are now type-checked before merging, so a wrong type reports its full key and file instead of a generic serde error from the merged config. --- crates/core/src/config.rs | 167 ++++++++++++++++++++++++++++++++---- docs/releases/UNRELEASED.md | 3 + docs/spec.md | 3 +- 3 files changed, 155 insertions(+), 18 deletions(-) diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index 8570324..09240b7 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -6,12 +6,14 @@ use std::{ use anyhow::{Context, Result, bail}; use globset::{Glob, GlobSet, GlobSetBuilder}; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Serialize, de::DeserializeOwned}; use serde_json::{Map, Value}; use crate::Metric; const DEFAULTS: &str = include_str!("../../../config.default.json"); +const LIMIT: &str = "a non-negative integer or null"; +const LIST: &str = "a list of strings"; /// A `None` limit is `null` in config: the metric is measured but never fails. #[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] @@ -231,30 +233,63 @@ fn validate_keys(value: &Value, path: &Path) -> Result<()> { "", path, )?; - nested_keys(object, "", "limits", &Metric::ALL.map(Metric::name), path)?; - nested_keys(object, "", "tests", &["patterns", "exempt"], path)?; + let limits = nested_keys(object, "", "limits", &Metric::ALL.map(Metric::name), path)?; + section_values::>(limits, "limits", LIMIT, path)?; + let tests = nested_keys(object, "", "tests", &["patterns", "exempt"], path)?; validate_test_exempt(object, path)?; - nested_keys(object, "", "hook", &["max_blocks"], path)?; + section_values::>(tests, "tests", LIST, path)?; + let hook = nested_keys(object, "", "hook", &["max_blocks"], path)?; + section_values::(hook, "hook", "a non-negative integer", path)?; + if let Some(ignore) = object.get("ignore") { + value_type::>(ignore, "ignore", LIST, path)?; + } validate_language_keys(object, path) } -fn nested_keys( - root: &Map, +fn nested_keys<'a>( + root: &'a Map, parent: &str, key: &str, keys: &[&str], path: &Path, +) -> Result>> { + let Some(value) = root.get(key) else { + return Ok(None); + }; + let full = if parent.is_empty() { + key.to_string() + } else { + format!("{parent}.{key}") + }; + let object = value + .as_object() + .ok_or_else(|| anyhow::anyhow!("{full} in {} must be an object", path.display()))?; + allowed(object, keys, &full, path)?; + Ok(Some(object)) +} + +/// Checks each value against the type its `Config` field deserializes into, +/// so a wrong type is reported with its key and file instead of after merging. +fn section_values( + section: Option<&Map>, + prefix: &str, + expected: &str, + path: &Path, ) -> Result<()> { - if let Some(value) = root.get(key) { - let full = if parent.is_empty() { - key.to_string() - } else { - format!("{parent}.{key}") - }; - let object = value - .as_object() - .ok_or_else(|| anyhow::anyhow!("{full} in {} must be an object", path.display()))?; - allowed(object, keys, &full, path)?; + for (key, value) in section.into_iter().flatten() { + value_type::(value, &format!("{prefix}.{key}"), expected, path)?; + } + Ok(()) +} + +fn value_type( + value: &Value, + key: &str, + expected: &str, + path: &Path, +) -> Result<()> { + if serde_json::from_value::(value.clone()).is_err() { + bail!("`{key}` in {} must be {expected}", path.display()); } Ok(()) } @@ -308,13 +343,14 @@ fn validate_language_keys(root: &Map, path: &Path) -> Result<()> })?; let prefix = format!("languages.{name}"); allowed(object, &["limits"], &prefix, path)?; - nested_keys( + let limits = nested_keys( object, &prefix, "limits", &Metric::ALL.map(Metric::name), path, )?; + section_values::>(limits, &format!("{prefix}.limits"), LIMIT, path)?; } Ok(()) } @@ -516,6 +552,103 @@ mod tests { ); } + fn config_error(json: &str) -> (String, PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.json"); + fs::write(&path, json).unwrap(); + let error = load_config(dir.path(), Some(&path)) + .unwrap_err() + .to_string(); + (error, path) + } + + #[test] + fn string_global_limit_names_key_and_config_path() { + let (error, path) = config_error(r#"{"limits":{"depth":"4"}}"#); + assert_eq!( + error, + format!( + "`limits.depth` in {} must be a non-negative integer or null", + path.display() + ) + ); + } + + #[test] + fn string_language_limit_names_full_path() { + let (error, path) = config_error(r#"{"languages":{"go":{"limits":{"depth":"4"}}}}"#); + assert_eq!( + error, + format!( + "`languages.go.limits.depth` in {} must be a non-negative integer or null", + path.display() + ) + ); + } + + #[test] + fn float_limit_is_rejected() { + let (error, path) = config_error(r#"{"limits":{"lines":1.5}}"#); + assert_eq!( + error, + format!( + "`limits.lines` in {} must be a non-negative integer or null", + path.display() + ) + ); + } + + #[test] + fn negative_max_blocks_names_key_and_config_path() { + let (error, path) = config_error(r#"{"hook":{"max_blocks":-1}}"#); + assert_eq!( + error, + format!( + "`hook.max_blocks` in {} must be a non-negative integer", + path.display() + ) + ); + } + + #[test] + fn non_list_ignore_and_patterns_are_rejected() { + let (error, path) = config_error(r#"{"ignore":"dist/**"}"#); + assert_eq!( + error, + format!("`ignore` in {} must be a list of strings", path.display()) + ); + let (error, path) = config_error(r#"{"tests":{"patterns":["a",1]}}"#); + assert_eq!( + error, + format!( + "`tests.patterns` in {} must be a list of strings", + path.display() + ) + ); + let (error, path) = config_error(r#"{"tests":{"exempt":"lines"}}"#); + assert_eq!( + error, + format!( + "`tests.exempt` in {} must be a list of strings", + path.display() + ) + ); + } + + #[test] + fn null_limits_still_load() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.json"); + fs::write( + &path, + r#"{"limits":{"depth":null},"languages":{"go":{"limits":{"lines":null}}}}"#, + ) + .unwrap(); + let config = load_config(dir.path(), Some(&path)).unwrap().config; + assert_eq!(config.limits.depth, None); + assert_eq!(config.limits_for("go").lines, None); + } + #[test] fn unknown_language_names_config_path() { let dir = tempfile::tempdir().unwrap(); diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 1defc9c..9347c8a 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -12,6 +12,9 @@ reported with the config file it came from, so it is clear whether the user or the repo config holds the typo. An unknown language is reported before any error inside its entry. (#43) +- A config value of the wrong type, such as a string limit or a negative + `hook.max_blocks`, is now reported with its full key and the config file it + came from, instead of a generic type error after the files are merged. (#46) ## Validation diff --git a/docs/spec.md b/docs/spec.md index d06b92b..c7540a4 100644 --- a/docs/spec.md +++ b/docs/spec.md @@ -792,7 +792,8 @@ have no hunks, and never quotes a path. `languages..limits` overrides limits for one language (`javascript`, `typescript`, `svelte`, `dart`, `rust`, `python`, `go`). Unknown keys → exit 2 -with the key named. Any limit, global or per language, accepts `null` to turn +with the key named. A value of the wrong type → exit 2 naming the key and the +config file. Any limit, global or per language, accepts `null` to turn that check off; a later layer can turn it back on with a number, and a language override of `null` turns it off for that language only. From b48c1c571938017ed42157cd9eb881f3aab5fc34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 30 Sep 2026 15:45:15 -0300 Subject: [PATCH 2/2] fix(core): type-check tests.exempt before its value check --- crates/core/src/config.rs | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index 09240b7..7ed5847 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -236,8 +236,8 @@ fn validate_keys(value: &Value, path: &Path) -> Result<()> { let limits = nested_keys(object, "", "limits", &Metric::ALL.map(Metric::name), path)?; section_values::>(limits, "limits", LIMIT, path)?; let tests = nested_keys(object, "", "tests", &["patterns", "exempt"], path)?; - validate_test_exempt(object, path)?; section_values::>(tests, "tests", LIST, path)?; + validate_test_exempt(object, path)?; let hook = nested_keys(object, "", "hook", &["max_blocks"], path)?; section_values::(hook, "hook", "a non-negative integer", path)?; if let Some(ignore) = object.get("ignore") { @@ -288,7 +288,7 @@ fn value_type( expected: &str, path: &Path, ) -> Result<()> { - if serde_json::from_value::(value.clone()).is_err() { + if T::deserialize(value).is_err() { bail!("`{key}` in {} must be {expected}", path.display()); } Ok(()) @@ -611,12 +611,16 @@ mod tests { } #[test] - fn non_list_ignore_and_patterns_are_rejected() { + fn string_ignore_is_rejected_as_non_list() { let (error, path) = config_error(r#"{"ignore":"dist/**"}"#); assert_eq!( error, format!("`ignore` in {} must be a list of strings", path.display()) ); + } + + #[test] + fn non_string_test_pattern_is_rejected() { let (error, path) = config_error(r#"{"tests":{"patterns":["a",1]}}"#); assert_eq!( error, @@ -625,6 +629,10 @@ mod tests { path.display() ) ); + } + + #[test] + fn string_test_exempt_is_rejected_as_non_list() { let (error, path) = config_error(r#"{"tests":{"exempt":"lines"}}"#); assert_eq!( error, @@ -635,6 +643,18 @@ mod tests { ); } + #[test] + fn non_string_test_exempt_entry_gets_list_message() { + let (error, path) = config_error(r#"{"tests":{"exempt":["lines",1]}}"#); + assert_eq!( + error, + format!( + "`tests.exempt` in {} must be a list of strings", + path.display() + ) + ); + } + #[test] fn null_limits_still_load() { let dir = tempfile::tempdir().unwrap();