diff --git a/components/support/nimbus-fml/fixtures/fe/browser.yaml b/components/support/nimbus-fml/fixtures/fe/browser.yaml index b85a79d288..68a31c8801 100644 --- a/components/support/nimbus-fml/fixtures/fe/browser.yaml +++ b/components/support/nimbus-fml/fixtures/fe/browser.yaml @@ -68,7 +68,7 @@ features: type: Option default: letter description: "Describes the icon of spotlight" - eum-map: + enum-map: description: A enum map that was causing problem type: Map default: diff --git a/components/support/nimbus-fml/src/client/inspector.rs b/components/support/nimbus-fml/src/client/inspector.rs index d41c78bb88..13164b8a47 100644 --- a/components/support/nimbus-fml/src/client/inspector.rs +++ b/components/support/nimbus-fml/src/client/inspector.rs @@ -90,14 +90,25 @@ impl FmlFeatureInspector { } } - fn get_semantic_error(&self, _string: &str, value: Value) -> Result<(), FmlEditorError> { + fn get_semantic_error(&self, src: &str, value: Value) -> Result<(), FmlEditorError> { self.manifest .validate_feature_config(&self.feature_id, value) - .map_err(|e| FmlEditorError { - message: e.to_string(), - hightlight: None, - line: 1, - col: 1, + .map_err(|e| match e { + FMLError::FeatureValidationError { + literals, message, .. + } => { + let hightlight = literals.last().cloned(); + let (line, col) = find_err(src, literals.into_iter()); + FmlEditorError { + message, + line: line as u32, + col: col as u32, + hightlight, + } + } + _ => { + unreachable!("Error {e:?} should be caught as FeatureValidationError"); + } }) .map(|_| ()) } @@ -119,7 +130,7 @@ fn find_err(src: &str, path: impl Iterator) -> (usize, usize) { let start = if !first_match { 0 } else { col_no + 1 }; // if let Some(i) = cur[start..].find(&p).map(|i| i + start) { - if let Some(i) = find_index(&mut cur, &p, start) { + if let Some(i) = find_index(cur, &p, start) { col_no = i; first_match = true; break; @@ -140,7 +151,9 @@ fn find_err(src: &str, path: impl Iterator) -> (usize, usize) { } fn find_index(cur: &str, pattern: &str, start: usize) -> Option { - cur.match_indices(pattern).find(|(i, _)| i >= &start).map(|(i, _)| i) + cur.match_indices(pattern) + .find(|(i, _)| i >= &start) + .map(|(i, _)| i) } #[derive(Debug, PartialEq)] @@ -206,7 +219,7 @@ mod unit_tests { test_syntax_error(&f, "[]", 0, true); test_syntax_error(&f, "1", 0, true); test_syntax_error(&f, "true", 0, true); - test_syntax_error(&f, "\"string\"", 0,true); + test_syntax_error(&f, "\"string\"", 0, true); assert!(f.get_first_error("{}".to_string()).is_none()); Ok(()) @@ -315,4 +328,161 @@ mod unit_tests { // assert_eq!(find_index("åéîø token", "token", 0), Some(5)); Ok(()) } + + #[test] + fn test_semantic_errors() -> Result<()> { + let client = client("./browser.yaml", "release")?; + let inspector = client + .get_feature_inspector("nimbus-validation".to_string()) + .unwrap(); + + let do_test = |lines: &[&str], token: &str, expected: (u32, u32)| { + let input = lines.join("\n"); + let err = inspector + .get_first_error(input.clone()) + .unwrap_or_else(|| unreachable!("No error for \"{input}\"")); + + assert_eq!( + err.hightlight, + Some(token.to_string()), + "Token {token} not detected in error in {input}" + ); + + let observed = (err.line, err.col); + assert_eq!( + expected, observed, + "Error at {token} in the wrong place in {input}" + ); + }; + + // invalid property name. + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "invalid": 1"#, // 1 + r#"}"#, // 2 + ], + "\"invalid\"", + (1, 2), + ); + + // simple type mismatch + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "icon-type": 1"#, // 1 + r#"}"#, // 2 + ], + "1", + (1, 15), + ); + + // enum mismatch + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "icon-type": "invalid""#, // 1 + r#"}"#, // 2 + ], + "\"invalid\"", + (1, 15), + ); + + // invalid field within object + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "nested": {"#, // 1 + r#" "invalid": true"#, // 2 + r#" }"#, // 3 + r#"}"#, // 4 + ], + "\"invalid\"", + (2, 4), + ); + + // nested in an object type mismatch + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "nested": {"#, // 1 + r#" "is-useful": 256"#, // 2 + r#" }"#, // 3 + r#"}"#, // 4 + ], + "256", + (2, 17), + ); + + // nested in a map type mismatch + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "string-int-map": {"#, // 1 + r#" "valid": "invalid""#, // 2 + r#" }"#, // 3 + r#"}"#, // 4 + ], + "\"invalid\"", + (2, 13), + ); + + // invalid key in enum map + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "enum-map": {"#, // 1 + r#" "invalid": 42"#, // 2 + r#" }"#, // 3 + r#"}"#, // 4 + ], + "\"invalid\"", + (2, 4), + ); + + // type mismatch in list + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "nested-list": ["#, // 1 + r#" {"#, // 2 + r#" "is-useful": true"#, // 3 + r#" },"#, // 4 + r#" false"#, // 5 + r#" ]"#, // 6 + r#"}"#, // 7 + ], + "false", + (5, 5), + ); + + // Difficult! + do_test( + &[ + // 012345678901234567890 + r#"{"#, // 0 + r#" "string-int-map": {"#, // 1 + r#" "nested": 1,"#, // 2 + r#" "is-useful": 2,"#, // 3 + r#" "invalid": 3"#, // 4 error is not here! + r#" },"#, // 5 + r#" "nested": {"#, // 6 + r#" "is-useful": "invalid""#, // 7 error is here! + r#" }"#, // 8 + r#"}"#, // 9 + ], + "\"invalid\"", + (7, 17), + ); + + Ok(()) + } } diff --git a/components/support/nimbus-fml/src/defaults_merger.rs b/components/support/nimbus-fml/src/defaults_merger.rs index 5c95c3dc49..25b6ba33be 100644 --- a/components/support/nimbus-fml/src/defaults_merger.rs +++ b/components/support/nimbus-fml/src/defaults_merger.rs @@ -7,7 +7,7 @@ use std::collections::{BTreeMap, HashMap}; use serde_json::json; use crate::{ - error::{FMLError, Result}, + error::{did_you_mean, FMLError, Result}, frontend::DefaultBlock, intermediate_representation::{FeatureDef, ObjectDef, PropDef, TypeRef}, }; @@ -192,7 +192,12 @@ impl<'object> DefaultsMerger<'object> { res.default = v.clone(); Ok(res) } else { - Err(FMLError::InvalidPropertyError(k.clone(), res.name.clone())) + let valid = res.props.iter().map(|p| p.name()).collect(); + Err(FMLError::FeatureValidationError { + literals: vec![format!("\"{k}\"")], + path: format!("features/{}", res.name), + message: format!("Invalid property \"{k}\"{}", did_you_mean(valid)), + }) } }) .collect::>>()?; @@ -1232,7 +1237,7 @@ mod unit_tests { assert!(result.is_err()); assert_eq!( result.err().unwrap().to_string(), - "Property `secondary-button-color` not found on feature `feature`" + "Validation Error at features/feature: Invalid property \"secondary-button-color\"; did you mean \"button-color\"?" ); Ok(()) } diff --git a/components/support/nimbus-fml/src/error.rs b/components/support/nimbus-fml/src/error.rs index 042a74370e..3cfa19d84b 100644 --- a/components/support/nimbus-fml/src/error.rs +++ b/components/support/nimbus-fml/src/error.rs @@ -4,6 +4,7 @@ * */ use crate::intermediate_representation::ModuleId; +use std::collections::HashSet; #[derive(Debug, thiserror::Error)] pub enum FMLError { @@ -30,6 +31,12 @@ pub enum FMLError { InternalError(&'static str), #[error("Validation Error at {0}: {1}")] ValidationError(String, String), + #[error("Validation Error at {path}: {message}")] + FeatureValidationError { + literals: Vec, + path: String, + message: String, + }, #[error("Type Parsing Error: {0}")] TypeParsingError(String), #[error("Invalid Channel error: The channel `{0}` is specified, but only {1:?} are supported for the file")] @@ -47,8 +54,6 @@ pub enum FMLError { #[error("Feature `{0}` not found on manifest")] InvalidFeatureError(String), - #[error("Property `{0}` not found on feature `{1}`")] - InvalidPropertyError(String, String), } #[cfg(feature = "client-lib")] @@ -65,3 +70,18 @@ pub enum ClientError { } pub type Result = std::result::Result; + +pub(crate) fn did_you_mean(words: HashSet) -> String { + if words.is_empty() { + "".to_string() + } else if words.len() == 1 { + format!("; did you mean \"{}\"?", words.iter().next().unwrap()) + } else { + let mut words = words.into_iter().collect::>(); + let last = words.remove(words.len() - 1); + format!( + "; did you mean one of \"{}\" or \"{last}\"?", + words.join("\", \"") + ) + } +} diff --git a/components/support/nimbus-fml/src/fml.udl b/components/support/nimbus-fml/src/fml.udl index 50e9fb65d7..3551bae188 100644 --- a/components/support/nimbus-fml/src/fml.udl +++ b/components/support/nimbus-fml/src/fml.udl @@ -8,7 +8,7 @@ enum FMLError { "IOError", "JSONError", "YAMLError", "UrlError", "FetchError", "InvalidPath", "TemplateProblem", "Fatal", "InternalError", "ValidationError", "TypeParsingError", "InvalidChannelError", "FMLModuleError", "CliError", "ClientError", "InvalidFeatureError", - "InvalidPropertyError" + "FeatureValidationError", }; dictionary MergedJsonWithErrors { diff --git a/components/support/nimbus-fml/src/intermediate_representation.rs b/components/support/nimbus-fml/src/intermediate_representation.rs index b5b797b2ac..09eb7cb3dd 100644 --- a/components/support/nimbus-fml/src/intermediate_representation.rs +++ b/components/support/nimbus-fml/src/intermediate_representation.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use crate::defaults_merger::DefaultsMerger; use crate::error::FMLError::InvalidFeatureError; -use crate::error::{FMLError, Result}; +use crate::error::{did_you_mean, FMLError, Result}; use crate::frontend::AboutBlock; use crate::util::loaders::FilePath; use anyhow::{bail, Error, Result as AnyhowResult}; @@ -433,13 +433,15 @@ impl FeatureManifest { fn validate_feature_def(&self, feature_def: &FeatureDef) -> Result<()> { for prop in &feature_def.props { let path = format!("features/{}.{}", feature_def.name, prop.name); - self.validate_prop_defaults(&path, prop)?; + let literals = vec![prop.name.to_string()]; + self.validate_default_by_typ(&path, &prop.typ, &prop.default, &literals)?; } Ok(()) } fn validate_prop_defaults(&self, path: &str, prop: &PropDef) -> Result<()> { - self.validate_default_by_typ(path, &prop.typ, &prop.default) + let literals = Default::default(); + self.validate_default_by_typ(path, &prop.typ, &prop.default, &literals) } pub fn validate_default_by_typ( @@ -447,7 +449,20 @@ impl FeatureManifest { path: &str, type_ref: &TypeRef, default: &Value, + literals: &Vec, ) -> Result<()> { + let add_literals = |new: &[String]| -> Vec { + let mut clone = literals.clone(); + for s in new { + clone.push(s.clone()); + } + clone + }; + let add_literal = |new: String| -> Vec { + let mut clone = literals.clone(); + clone.push(new); + clone + }; match (type_ref, default) { (TypeRef::Boolean, Value::Bool(_)) | (TypeRef::BundleImage(_), Value::String(_)) @@ -462,28 +477,28 @@ impl FeatureManifest { "Nested options".into(), )); } - self.validate_default_by_typ(path, inner, v) + self.validate_default_by_typ(path, inner, v, literals) } (TypeRef::Enum(enum_name), Value::String(s)) => { let enum_def = self.find_enum(enum_name).ok_or_else(|| { FMLError::ValidationError( path.to_string(), - format!("Type `{}` is not a type. Perhaps you need to declare an enum of that name.", enum_name) + format!("Type `{enum_name}` is not a type. Perhaps you need to declare an enum of that name.") ) })?; + let mut valid = HashSet::new(); for variant in enum_def.variants() { - if *s == variant.name() { + let name = variant.name(); + if *s == name { return Ok(()); } + valid.insert(name); } - Err(FMLError::ValidationError( - path.to_string(), - format!( - "Default value `{value}` is not declared a variant of {enum_type}", - value = s, - enum_type = enum_name - ), - )) + Err(FMLError::FeatureValidationError { + path: path.to_string(), + message: format!("\"{s}\" is not a valid {enum_name}{}", did_you_mean(valid)), + literals: add_literal(format!("\"{s}\"")), + }) } (TypeRef::EnumMap(enum_type, map_type), Value::Object(map)) => { let enum_name = if let TypeRef::Enum(name) = enum_type.as_ref() { @@ -500,48 +515,59 @@ impl FeatureManifest { })?; let mut seen = HashSet::new(); let mut unseen = HashSet::new(); + let mut valid = HashSet::new(); for variant in enum_def.variants() { - let map_value = map.get(&variant.name()); + let nm = variant.name(); + valid.insert(nm.clone()); + + let map_value = map.get(&nm); match (map_type.as_ref(), map_value) { (TypeRef::Option(_), None) => (), (_, None) => { unseen.insert(variant.name()); } (_, Some(inner)) => { - let path = format!("{}[{}#{}]", path, enum_def.name, variant.name); - self.validate_default_by_typ(&path, map_type, inner)?; - seen.insert(variant.name()); + let path = format!("{path}[{}#{nm}]", enum_def.name); + let literals = add_literals(&["{".to_string(), format!("\"{nm}\"")]); + self.validate_default_by_typ(&path, map_type, inner, &literals)?; + seen.insert(nm); } } } if !unseen.is_empty() { - return Err(FMLError::ValidationError( - path.to_string(), - format!( - "Default for enum map {} doesn't contain variant(s) {:?}", - enum_name, unseen - ), - )); + return Err(FMLError::FeatureValidationError { + path: path.to_string(), + message: format!("Enum map {enum_name} is missing values for {unseen:?}"), + // Can we be more specific that just the opening brace? + literals: add_literal("{".to_string()), + }); } for map_key in map.keys() { if !seen.contains(map_key) { - return Err(FMLError::ValidationError(path.to_string(), format!("Enum map default contains key {} that doesn't exist in the enum definition", map_key))); + return Err(FMLError::FeatureValidationError { + path: path.to_string(), + message: format!("Invalid key \"{map_key}\"{}", did_you_mean(valid)), + literals: add_literals(&["{".to_string(), format!("\"{map_key}\"")]), + }); } } Ok(()) } (TypeRef::StringMap(map_type), Value::Object(map)) => { for (key, value) in map { - let path = format!("{}['{}']", path, key); - self.validate_default_by_typ(&path, map_type, value)?; + let path = format!("{path}['{key}']"); + let literals = add_literals(&["{".to_string(), format!("\"{key}\"")]); + self.validate_default_by_typ(&path, map_type, value, &literals)?; } Ok(()) } (TypeRef::List(list_type), Value::Array(arr)) => { + let mut literals = add_literal("[".to_string()); for (index, value) in arr.iter().enumerate() { - let path = format!("{}['{}']", path, index); - self.validate_default_by_typ(&path, list_type, value)?; + let path = format!("{path}['{index}']"); + self.validate_default_by_typ(&path, list_type, value, &literals)?; + literals.push(",".to_string()); } Ok(()) } @@ -549,43 +575,48 @@ impl FeatureManifest { let obj_def = self.find_object(obj_name).ok_or_else(|| { FMLError::ValidationError( path.to_string(), - format!("Object {} is not defined in the manifest", obj_name), + format!("Object {obj_name} is not defined in the manifest"), ) })?; - let mut seen = HashSet::new(); - let path = format!("{}#{}", path, obj_name); + let mut valid = HashSet::new(); + let mut unseen = HashSet::new(); + let path = format!("{path}#{obj_name}"); for prop in &obj_def.props { // We only check the defaults overriding the property defaults // from the object's own property defaults. // We check the object property defaults previously. - if let Some(map_val) = map.get(&prop.name) { - let path = format!("{}.{}", path, prop.name); - self.validate_default_by_typ(&path, &prop.typ, map_val)?; + let nm = prop.name(); + if let Some(map_val) = map.get(&nm) { + let path = format!("{path}.{}", prop.name); + let literals = + add_literals(&["{".to_string(), format!("\"{}\"", &prop.name)]); + self.validate_default_by_typ(&path, &prop.typ, map_val, &literals)?; + } else { + unseen.insert(nm.clone()); } - seen.insert(prop.name()); + valid.insert(nm); } for map_key in map.keys() { - if !seen.contains(map_key) { - return Err(FMLError::ValidationError( + if !valid.contains(map_key) { + return Err(FMLError::FeatureValidationError { path, - format!( - "Default includes key {} that doesn't exist in {}'s object definition", - map_key, obj_name - ), - )); + message: format!( + "Invalid key \"{map_key}\" for object {obj_name}{}", + did_you_mean(valid) + ), + literals: add_literal(format!("\"{map_key}\"")), + }); } } Ok(()) } - _ => Err(FMLError::ValidationError( - path.to_string(), - format!( - "Mismatch between type {:?} and default {}", - type_ref, default - ), - )), + _ => Err(FMLError::FeatureValidationError { + path: path.to_string(), + message: format!("Mismatch between type {type_ref:?} and default {default}"), + literals: add_literal(default.to_string()), + }), } } @@ -2110,7 +2141,7 @@ pub mod unit_tests { assert!(result.is_err()); assert_eq!( result.err().unwrap().to_string(), - "Property `prop` not found on feature `feature`" + "Validation Error at features/feature: Invalid property \"prop\"; did you mean \"prop_1\"?" ); Ok(()) @@ -2189,7 +2220,7 @@ pub mod unit_tests { assert!(result.is_err()); assert_eq!( result.err().unwrap().to_string(), - "Validation Error at features/feature.prop_1#SampleObj: Default includes key invalid-prop that doesn't exist in SampleObj's object definition" + "Validation Error at features/feature.prop_1#SampleObj: Invalid key \"invalid-prop\" for object SampleObj; did you mean \"string\"?" ); Ok(())