ByoungSooPark/modeling-app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add warnings for recently deprecated syntax (#4560)

Browse Source 
Signed-off-by: Nick Cameron <nrc@ncameron.org>
This commit is contained in:
Nick Cameron
2024-11-26 14:59:40 +13:00
committed by GitHub
parent 39a2bd685b
commit 30bc85add8
3 changed files with 129 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
src/wasm-lib/kcl/src/ast/types.rs
View File

@ -117,8 +117,12 @@ impl<T> Node<T> {
})
}
pub fn as_source_range(&self) -> SourceRange {
SourceRange([self.start, self.end, self.module_id.as_usize()])
}
pub fn as_source_ranges(&self) -> Vec<SourceRange> {
vec![SourceRange([self.start, self.end, self.module_id.as_usize()])]
vec![self.as_source_range()]
}
}

123
src/wasm-lib/kcl/src/parser/parser_impl.rs
View File

@ -49,7 +49,6 @@ pub fn run_parser(i: TokenSlice) -> super::ParseResult {
#[derive(Debug, Clone, Default)]
pub(crate) struct ParseContext {
pub errors: Vec<ParseError>,
#[allow(dead_code)]
pub warnings: Vec<ParseError>,
}
@ -75,14 +74,34 @@ impl ParseContext {
/// Add an error to the current `ParseContext`, panics if there is none.
fn err(e: ParseError) {
// TODO follow warnings replacement with errors
CTXT.with_borrow_mut(|ctxt| ctxt.as_mut().unwrap().errors.push(e));
}
/// Add a warning to the current `ParseContext`, panics if there is none.
#[allow(dead_code)]
fn warn(mut e: ParseError) {
e.severity = error::Severity::Warning;
CTXT.with_borrow_mut(|ctxt| ctxt.as_mut().unwrap().warnings.push(e));
CTXT.with_borrow_mut(|ctxt| {
// Avoid duplicating warnings. This is possible since the parser can try one path, find
// a warning, then backtrack and decide not to take that path and try another. This can
// happen 'high up the stack', so it's impossible to fix where the warnings are generated.
// Ideally we would pass warnings up the call stack rather than use a context object or
// have some way to mark warnings as speculative or committed, but I don't think Winnow
// is flexible enough for that (or at least, not without significant changes to the
// parser).
let warnings = &mut ctxt.as_mut().unwrap().warnings;
for w in warnings.iter_mut().rev() {
if w.source_range == e.source_range {
*w = e;
return;
}
if w.source_range.start() > e.source_range.end() {
break;
}
}
warnings.push(e);
});
}
}
@ -685,7 +704,7 @@ fn object_property(i: TokenSlice) -> PResult<Node<ObjectProperty>> {
let key = identifier.context(expected("the property's key (the name or identifier of the property), e.g. in 'height = 4', 'height' is the property key")).parse_next(i)?;
ignore_whitespace(i);
// Temporarily accept both `:` and `=` for compatibility.
alt((colon, equals))
let sep = alt((colon, equals))
.context(expected(
"`=`, which separates the property's key from the value you're setting it to, e.g. 'height = 4'",
))
@ -696,7 +715,8 @@ fn object_property(i: TokenSlice) -> PResult<Node<ObjectProperty>> {
"the value which you're setting the property to, e.g. in 'height: 4', the value is 4",
))
.parse_next(i)?;
Ok(Node {
let result = Node {
start: key.start,
end: expr.end(),
module_id: key.module_id,
@ -705,7 +725,18 @@ fn object_property(i: TokenSlice) -> PResult<Node<ObjectProperty>> {
value: expr,
digest: None,
},
})
};
if sep.token_type == TokenType::Colon {
ParseContext::warn(ParseError::with_suggestion(
sep.into(),
Some(result.as_source_range()),
"Using `:` to initialize objects is deprecated, prefer using `=`.",
Some(" ="),
));
}
Ok(result)
}
/// Match something that separates properties of an object.
@ -1919,16 +1950,14 @@ fn double_period(i: TokenSlice) -> PResult<Token> {
.parse_next(i)
}
fn colon(i: TokenSlice) -> PResult<()> {
TokenType::Colon.parse_from(i)?;
Ok(())
fn colon(i: TokenSlice) -> PResult<Token> {
TokenType::Colon.parse_from(i)
}
fn equals(i: TokenSlice) -> PResult<()> {
fn equals(i: TokenSlice) -> PResult<Token> {
one_of((TokenType::Operator, "="))
.context(expected("the equals operator, ="))
.parse_next(i)?;
Ok(())
.parse_next(i)
}
fn question_mark(i: TokenSlice) -> PResult<()> {
@ -2122,6 +2151,23 @@ fn fn_call(i: TokenSlice) -> PResult<Node<CallExpression>> {
}
}
let end = preceded(opt(whitespace), close_paren).parse_next(i)?.end;
// This should really be done with resolved names, but we don't have warning support there
// so we'll hack this in here.
if fn_name.name == "int" {
assert_eq!(args.len(), 1);
let mut arg_str = args[0].recast(&crate::FormatOptions::default(), 0, false);
if arg_str.contains('.') && !arg_str.ends_with(".0") {
arg_str = format!("round({arg_str})");
}
ParseContext::warn(ParseError::with_suggestion(
SourceRange::new(fn_name.start, end, fn_name.module_id),
None,
"`int` function is deprecated. You may not need it at all. If you need to round, consider `round`, `ceil`, or `floor`.",
Some(arg_str),
));
}
Ok(Node {
start: fn_name.start,
end,
@ -2259,7 +2305,7 @@ mod tests {
#[test]
fn test_comments_in_function2() {
let test_program = r#"() => {
const yo = { a: { b: { c: '123' } } } /* block
const yo = { a = { b = { c = '123' } } } /* block
comment */
}"#;
let tokens = crate::token::lexer(test_program, ModuleId::default()).unwrap();
@ -2418,7 +2464,7 @@ const mySk1 = startSketchAt([0, 0])"#;
#[test]
fn many_comments() {
let test_program = r#"// this is a comment
const yo = { a: { b: { c: '123' } } } /* block
const yo = { a = { b = { c = '123' } } } /* block
comment */
const key = 'c'
@ -2455,8 +2501,8 @@ const mySk1 = startSketchAt([0, 0])"#;
},
digest: None,
},
60,
82,
63,
85,
module_id,
),
Node::new(
@ -2464,8 +2510,8 @@ const mySk1 = startSketchAt([0, 0])"#;
value: NonCodeValue::NewLine,
digest: None,
},
82,
86,
85,
89,
module_id,
)
]),
@ -2481,8 +2527,8 @@ const mySk1 = startSketchAt([0, 0])"#;
},
digest: None,
},
103,
129,
106,
132,
module_id,
)]),
non_code_meta.non_code_nodes.get(&1),
@ -2858,7 +2904,7 @@ const mySk1 = startSketchAt([0, 0])"#;
fn test_pipes_on_pipes() {
let test_program = include_str!("../../../tests/executor/inputs/pipes_on_pipes.kcl");
let tokens = crate::token::lexer(test_program, ModuleId::default()).unwrap();
let _actual = program.parse(&tokens).unwrap();
let _ = run_parser(&mut &*tokens).unwrap();
}
#[test]
@ -3128,6 +3174,14 @@ const mySk1 = startSketchAt([0, 0])"#;
assert!(result.is_ok());
}
#[track_caller]
fn assert_no_err(p: &str) -> (Node<Program>, ParseContext) {
let result = crate::parser::top_level_parse(p);
let result = result.0.unwrap();
assert!(result.1.errors.is_empty());
(result.0.unwrap(), result.1)
}
#[track_caller]
fn assert_err(p: &str, msg: &str, src: [usize; 2]) {
let result = crate::parser::top_level_parse(p);
@ -3658,6 +3712,28 @@ let myBox = box([0,0], -3, -16, -10)
"#;
assert_err(some_program_string, "Unexpected token: |>", [57, 59]);
}
#[test]
fn warn_object_expr() {
let some_program_string = "{ foo: bar }";
let (_, ctxt) = assert_no_err(some_program_string);
assert_eq!(ctxt.warnings.len(), 1);
assert_eq!(
ctxt.warnings[0].apply_suggestion(some_program_string).unwrap(),
"{ foo = bar }"
)
}
#[test]
fn warn_fn_int() {
let some_program_string = r#"int(1.0)
int(42.3)"#;
let (_, ctxt) = assert_no_err(some_program_string);
assert_eq!(ctxt.warnings.len(), 2);
let replaced = ctxt.warnings[1].apply_suggestion(some_program_string).unwrap();
let replaced = ctxt.warnings[0].apply_suggestion(&replaced).unwrap();
assert_eq!(replaced, "1.0\nround(42.3)");
}
}
#[cfg(test)]
@ -3673,11 +3749,14 @@ mod snapshot_math_tests {
fn $func_name() {
let module_id = crate::ast::types::ModuleId::default();
let tokens = crate::token::lexer($test_kcl_program, module_id).unwrap();
ParseContext::init();
let actual = match binary_expression.parse(&tokens) {
Ok(x) => x,
Err(_e) => panic!("could not parse test"),
};
insta::assert_json_snapshot!(actual);
let _ = ParseContext::take();
}
};
}
@ -3709,6 +3788,7 @@ mod snapshot_tests {
let module_id = crate::ast::types::ModuleId::default();
let tokens = crate::token::lexer($test_kcl_program, module_id).unwrap();
print_tokens(&tokens);
ParseContext::init();
let actual = match program.parse(&tokens) {
Ok(x) => x,
Err(e) => panic!("could not parse test: {e:?}"),
@ -3718,6 +3798,7 @@ mod snapshot_tests {
settings.bind(|| {
insta::assert_json_snapshot!(actual);
});
let _ = ParseContext::take();
}
};
}

27
src/wasm-lib/kcl/src/parser/parser_impl/error.rs
View File

@ -24,9 +24,11 @@ pub struct ContextError<C = StrContext> {
#[derive(Debug, Clone)]
pub(crate) struct ParseError {
pub source_range: SourceRange,
#[allow(dead_code)]
pub context_range: Option<SourceRange>,
pub message: String,
#[allow(dead_code)]
pub suggestion: String,
pub suggestion: Option<String>,
pub severity: Severity,
}
@ -34,25 +36,38 @@ impl ParseError {
pub(super) fn err(source_range: SourceRange, message: impl ToString) -> ParseError {
ParseError {
source_range,
context_range: None,
message: message.to_string(),
suggestion: String::new(),
suggestion: None,
severity: Severity::Error,
}
}
#[allow(dead_code)]
pub(super) fn with_suggestion(
source_range: SourceRange,
context_range: Option<SourceRange>,
message: impl ToString,
suggestion: impl ToString,
suggestion: Option<impl ToString>,
) -> ParseError {
ParseError {
source_range,
context_range,
message: message.to_string(),
suggestion: suggestion.to_string(),
suggestion: suggestion.map(|s| s.to_string()),
severity: Severity::Error,
}
}
#[cfg(test)]
pub fn apply_suggestion(&self, src: &str) -> Option<String> {
let suggestion = self.suggestion.as_ref()?;
Some(format!(
"{}{}{}",
&src[0..self.source_range.start()],
suggestion,
&src[self.source_range.end()..]
))
}
}
impl From<ParseError> for KclError {
@ -73,6 +88,8 @@ pub(crate) enum Severity {
/// Helper enum for the below conversion of Winnow errors into either a parse error or an unexpected
/// error.
// TODO we should optimise the size of SourceRange and thus ParseError
#[allow(clippy::large_enum_variant)]
pub(super) enum ErrorKind {
Parse(ParseError),
Internal(KclError),