Skip to content

Commit

Permalink
Add error messages and test them with line/cols
Browse files Browse the repository at this point in the history
  • Loading branch information
@jhugman
jhugman committed Sep 18, 2023
1 parent 1f8216d commit 5b7a653
Show file tree
Hide file tree
Showing 6 changed files with 295 additions and 69 deletions.
2 changes: 1 addition & 1 deletion components/support/nimbus-fml/fixtures/fe/browser.yaml
View file
Expand Up @@ -68,7 +68,7 @@ features:
type: Option<IconType>
default: letter
description: "Describes the icon of spotlight"
eum-map:
enum-map:
description: A enum map that was causing problem
type: Map<IconType, Int>
default:
Expand Down
188 changes: 179 additions & 9 deletions components/support/nimbus-fml/src/client/inspector.rs
View file
Expand Up @@ -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(|_| ())
}
Expand All @@ -119,7 +130,7 @@ fn find_err(src: &str, path: impl Iterator<Item = String>) -> (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;
Expand All @@ -140,7 +151,9 @@ fn find_err(src: &str, path: impl Iterator<Item = String>) -> (usize, usize) {
}

fn find_index(cur: &str, pattern: &str, start: usize) -> Option<usize> {
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)]
Expand Down Expand Up @@ -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(())
Expand Down Expand Up @@ -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(())
}
}
11 changes: 8 additions & 3 deletions components/support/nimbus-fml/src/defaults_merger.rs
View file
Expand Up @@ -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},
};
Expand Down Expand Up @@ -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::<Result<Vec<_>>>()?;
Expand Down Expand Up @@ -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(())
}
Expand Down
24 changes: 22 additions & 2 deletions components/support/nimbus-fml/src/error.rs
View file
Expand Up @@ -4,6 +4,7 @@
* */

use crate::intermediate_representation::ModuleId;
use std::collections::HashSet;

#[derive(Debug, thiserror::Error)]
pub enum FMLError {
Expand All @@ -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<String>,
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")]
Expand All @@ -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")]
Expand All @@ -65,3 +70,18 @@ pub enum ClientError {
}

pub type Result<T, E = FMLError> = std::result::Result<T, E>;

pub(crate) fn did_you_mean(words: HashSet<String>) -> 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::<Vec<_>>();
let last = words.remove(words.len() - 1);
format!(
"; did you mean one of \"{}\" or \"{last}\"?",
words.join("\", \"")
)
}
}
2 changes: 1 addition & 1 deletion components/support/nimbus-fml/src/fml.udl
View file
Expand Up @@ -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 {
Expand Down

0 comments on commit 5b7a653

Please sign in to comment.