feat(remap): diagnostic error messages #6023

JeanMertz · 2021-01-13T12:37:49Z

This PR adds the initial infrastructure to support diagnostic error messages in VRL.

Given this source:

if .foo {
	true
}

This is the diagnostic message returned by the compiler (running inside Vector):

I'm fairly happy how this ended up for the initial 0.12 release, but there's plenty of more to do here. I'll be filing separate issues to track future improvements.

On a high-level, here's what has changed:

A new Diagnostic type (and friends) is introduced to record detailed information about an error.
The parser can now continue parsing if it encounters a recoverable parsing error.
- Invalid token errors are currently unrecoverable (although we can improve this by trying to continue to parse the next expression)
- All other non-grammar related errors are recoverable (e.g. providing a non-boolean value to an if-statement condition)
- In the case of recoverable errors, the compiler state is reverted to before parsing began of the given expression, such that any invalid variable/target assignments are removed before continuing.
A Program now returns Result<(Vec<Expression>, DiagnosticList), DiagnosticList>. The Err case is returned if any of the diagnostic recorded are of level "error" or higher. The Ok case only contains diagnostic messages of "warning" or lower (or none at all).
The Runtime now returns a single Abort error if it failed to run. This is the case if you use ! in a function (e.g. parse_json!(.foo)), which would satisfy the error-handling constraint at compile-time, but can still terminate the program at runtime.

closes #4803
closes #6027

lib/remap-lang/src/diagnostic.rs

JeanMertz · 2021-01-14T07:50:28Z

Moving this back to draft, since I'm doing some internal changes to allow multiple diagnostics to be returned (and also in general improving the internal API design).

cc #6027

JeanMertz · 2021-01-18T10:12:29Z

lib/remap-lang/src/expression/path.rs

-        let value = object
-            .get(&self.path)
-            .map_err(|e| E::from(Error::Resolve(e)))?
-            .unwrap_or(Value::Null);
+        let value = object.get(&self.path).ok().flatten().unwrap_or(Value::Null);


I merged (some of) #5941 into this PR, as it made it easier to implement/test the changes in this PR.

JeanMertz · 2021-01-18T10:16:57Z

lib/remap-lang/src/lib.rs

-                Err("remap error: function error: unknown enum variant: baz, must be one of: foo, bar"),
-                Ok(().into()),
-            ),
+            // TODO: move to `remap-tests`


Once this PR and #6105 are merged, I'll be moving these tests to our UI testing set-up.

JeanMertz · 2021-01-18T10:19:59Z

lib/remap-lang/src/parser.rs

+pub struct ParsedExpression {
+    span: Span,
+    expr: Expr,
+}


Eventually, I want to encode span information into our Expr type, but that proved to be a bigger change than needed right now.

JeanMertz · 2021-01-18T10:48:24Z

lib/remap-lang/src/parser.rs

+                            .with_primary(format!("got: {}", got), arguments_span)
+                            .with_context(format!("expected: {} (at most)", max), arguments_span)
+                    }
+                    // TODO: have spans for each individual keyword


This is something I'll tackle in a follow-up PR.

JeanMertz · 2021-01-18T11:00:25Z

lib/remap-tests/tests/assignment_fallible_for_infallible_expression.vrl

+#   = hint: assign to "ok", without assigning to "err"
+#   = see language documentation at: https://vector.dev/docs/reference/vrl/
+
+ok, err = 5;


When reviewing, consider these early prototype messages.

The actual URLs, notes, and exact messages will be iterated on in future PRs. This PR's main purpose is to have the plumbing in place to allow us to improve these messages going forward.

Signed-off-by: Jean Mertz <[email protected]>

StephenWakely · 2021-01-18T12:04:10Z

lib/remap-lang/src/parser.rs

+                    span.clone(),
+                );
+
+                match () {


In my opinion, this would be nicer as an if .. else if .. else.

Signed-off-by: Jean Mertz <[email protected]>

StephenWakely · 2021-01-18T13:32:10Z

lib/remap-lang/src/program.rs

+            .filter(|e| e.type_def(&state).is_fallible())
+            .for_each(|e| {
+                diagnostics.push(
+                    Diagnostic::error("unhandled error")


Sometimes the expression results in a runtime error because the function is a fallible one (eg. parse_json). But other times it can because it has been passed a parameter that it can't determine is the correct type.

I think ultimately it would be nice to be able to distinguish between these. So for example with this:

if .ook == true { x = 2 } else { x = "boo" } .thing = round(.x)

It would be nice for the error reporting to say :

error: unhandled error ┌─ :2:2 │ 2 │ .thing = round(.x) │ ^^^^^^^^^^^^^^^^^^ │ │ │ expression can't determine the correct types for the parameters and so may fail if it is passed │ an incorrect type. │ handle the error case to ensure runtime success │

However that probably requires a bit of change to the fallibility checks to make fallibilty more than just a simple boolean, which would be out of scope for this PR.

Perhaps would could just change the below error message to say expression can result in runtime error, possibly because it can't determine the correct types for the parameters? That would help point the user in the right direction without having to jump to the docs..

Actually, as long as we use the try_boolean() etc functions in the function implementation, this will already display something like unexpected type "string" for value. But I agree that this can use a further refinement (specifically to mention the parameter name), and indeed it requires a bit more work that I considered to be outside the scope of this PR.

StephenWakely

Nice! Will all the commented out tests be moved out in a future PR, or is this still to do?

JeanMertz · 2021-01-18T14:05:59Z

Will all the commented out tests be moved out in a future PR, or is this still to do?

They will! See my earlier comment: #6023 (comment)

Signed-off-by: Jean Mertz <[email protected]>

JeanMertz added the domain: vrl Anything related to the Vector Remap Language label Jan 13, 2021

JeanMertz added this to the 2021-01-04 Xenomass Well milestone Jan 13, 2021

JeanMertz requested review from StephenWakely and jszwedko January 13, 2021 12:37

JeanMertz self-assigned this Jan 13, 2021

JeanMertz mentioned this pull request Jan 13, 2021

Support continued parsing even when error occurs #6027

Closed

binarylogic reviewed Jan 13, 2021

View reviewed changes

lib/remap-lang/src/diagnostic.rs Outdated Show resolved Hide resolved

JeanMertz force-pushed the jean/remap-test-harness branch from 12b879c to fe2f7df Compare January 13, 2021 21:03

JeanMertz force-pushed the jean/remap-diagnostics branch from b7d2e8f to cb820f0 Compare January 13, 2021 21:03

JeanMertz marked this pull request as draft January 14, 2021 07:50

JeanMertz force-pushed the jean/remap-test-harness branch from 4c2a879 to 1076a54 Compare January 14, 2021 08:29

Base automatically changed from jean/remap-test-harness to master January 14, 2021 08:52

JeanMertz force-pushed the jean/remap-diagnostics branch from cb820f0 to e1b81f5 Compare January 14, 2021 08:53

This was referenced Jan 15, 2021

add support for error codes in VRL #6070

Closed

improve span accuracy during parsing #6072

Closed

JeanMertz commented Jan 18, 2021

View reviewed changes

JeanMertz added 12 commits January 18, 2021 12:04

feat(remap): diagnostic error messages

2a91547

Signed-off-by: Jean Mertz <[email protected]>

add span tracking support to parser

bfc4089

Signed-off-by: Jean Mertz <[email protected]>

clean up parser token error handling

9f74f7a

Signed-off-by: Jean Mertz <[email protected]>

remove unused parser impls

dd118db

Signed-off-by: Jean Mertz <[email protected]>

add initial if-condition diagnostic

cb74373

Signed-off-by: Jean Mertz <[email protected]>

make it easy to print colored diagnostics output

66f6b5d

Signed-off-by: Jean Mertz <[email protected]>

add support for multiple diagnostic return values

297728a

Signed-off-by: Jean Mertz <[email protected]>

extend diagnostics impls

ea233f0

Signed-off-by: Jean Mertz <[email protected]>

add more ui tests

e58ea29

Signed-off-by: Jean Mertz <[email protected]>

cleanup and fixes

9fdbae0

Signed-off-by: Jean Mertz <[email protected]>

fix remap-cli

1c275e7

Signed-off-by: Jean Mertz <[email protected]>

more cleanup and improvements

153e1af

Signed-off-by: Jean Mertz <[email protected]>

JeanMertz force-pushed the jean/remap-diagnostics branch from 60d09b5 to 153e1af Compare January 18, 2021 11:06

JeanMertz marked this pull request as ready for review January 18, 2021 11:06

JeanMertz removed the request for review from jszwedko January 18, 2021 11:06

lint fixes

f7d5406

Signed-off-by: Jean Mertz <[email protected]>

JeanMertz requested a review from jamtur01 as a code owner January 18, 2021 11:39

StephenWakely reviewed Jan 18, 2021

View reviewed changes

JeanMertz added 3 commits January 18, 2021 13:19

more linting fixes

301f22c

Signed-off-by: Jean Mertz <[email protected]>

fix coercer behavior test

3f80d2e

Signed-off-by: Jean Mertz <[email protected]>

share compiler state in REPL

6ac1b73

Signed-off-by: Jean Mertz <[email protected]>

StephenWakely reviewed Jan 18, 2021

View reviewed changes

StephenWakely approved these changes Jan 18, 2021

View reviewed changes

JeanMertz added 5 commits January 18, 2021 15:09

fix filter behavior test

2812c02

Signed-off-by: Jean Mertz <[email protected]>

fix behavior tests

9cab943

Signed-off-by: Jean Mertz <[email protected]>

more test fixes

92a208b

Signed-off-by: Jean Mertz <[email protected]>

temporary disable test

2e7916d

Signed-off-by: Jean Mertz <[email protected]>

fix test run

5215f84

Signed-off-by: Jean Mertz <[email protected]>

JeanMertz merged commit a2107bb into master Jan 18, 2021

JeanMertz deleted the jean/remap-diagnostics branch January 18, 2021 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remap): diagnostic error messages #6023

feat(remap): diagnostic error messages #6023

JeanMertz commented Jan 13, 2021 •

edited

Loading

JeanMertz commented Jan 14, 2021

JeanMertz Jan 18, 2021

JeanMertz Jan 18, 2021

JeanMertz Jan 18, 2021

JeanMertz Jan 18, 2021

JeanMertz Jan 18, 2021

StephenWakely Jan 18, 2021

StephenWakely Jan 18, 2021

JeanMertz Jan 18, 2021 •

edited

Loading

StephenWakely left a comment

JeanMertz commented Jan 18, 2021 •

edited

Loading

feat(remap): diagnostic error messages #6023

feat(remap): diagnostic error messages #6023

Conversation

JeanMertz commented Jan 13, 2021 • edited Loading

JeanMertz commented Jan 14, 2021

JeanMertz Jan 18, 2021

Choose a reason for hiding this comment

JeanMertz Jan 18, 2021

Choose a reason for hiding this comment

JeanMertz Jan 18, 2021

Choose a reason for hiding this comment

JeanMertz Jan 18, 2021

Choose a reason for hiding this comment

JeanMertz Jan 18, 2021

Choose a reason for hiding this comment

StephenWakely Jan 18, 2021

Choose a reason for hiding this comment

StephenWakely Jan 18, 2021

Choose a reason for hiding this comment

JeanMertz Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

StephenWakely left a comment

Choose a reason for hiding this comment

JeanMertz commented Jan 18, 2021 • edited Loading

JeanMertz commented Jan 13, 2021 •

edited

Loading

JeanMertz Jan 18, 2021 •

edited

Loading

JeanMertz commented Jan 18, 2021 •

edited

Loading