Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(diagnostics): improve fallibility error notes #523

Merged
merged 4 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
- 'from_unix_timestamp' now accepts a new unit: Microseconds.
- `parse_nginx_log` no longer fails if `upstream_response_length`, `upstream_response_time`, `upstream_status` are missing (https://github.com/vectordotdev/vrl/pull/498)
- added `parse_float` function (https://github.com/vectordotdev/vrl/pull/484)

- improved fallibility diagnostics (https://github.com/vectordotdev/vrl/pull/523)
## `0.7.0` (2023-09-25)

#### Bug Fixes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
# │ │ │
# │ │ this expression is fallible
# │ │ update the expression to be infallible
# │ │ note if an argument type is invalid it can render a function fallible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great if we could go even further and say, for cases where the function is fallible due to the argument type not being known to be valid, that:

this expression is fallible because argument `.result[0].an` is of type `any` and this function expects a `string`. It will fail at runtime if `.result[0].an` is not a `string`

The currently added message is definitely an improvement, but I think users will remain largely confused by this behavior of VRL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jszwedko: Agreed, good point.

However, I don't have time to do it before this upcoming release. Two options:

  1. Get this improvement in and follow up with another PR (to be included in the next release)
  2. Leave this PR open

What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'm fine with merging this since it is an improvement and then following up. I just want to make sure we don't close out #23 until we do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Added a reminder here.

Merging this PR won't close the ticket, so if you give this a ✅ I will include it in the VRL release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was more involved than expected but implemented this here: #553

# │ or change this to an infallible assignment:
# │ .a, err = sha3(.result[0].an)
# │
# = see documentation about error handling at https://errors.vrl.dev/#handling
# = see functions characteristics documentation at https://vrl.dev/expressions/#function-call-characteristics
# = learn more about error code 103 at https://errors.vrl.dev/103
# = see language documentation at https://vrl.dev
# = try your code in the VRL REPL, learn more at https://vrl.dev/examples
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
# │ │ │
# │ │ this expression is fallible
# │ │ update the expression to be infallible
# │ │ note if an argument type is invalid it can render a function fallible
# │ or change this to an infallible assignment:
# │ ., err = parse_common_log(.log)
# │
# = see documentation about error handling at https://errors.vrl.dev/#handling
# = see functions characteristics documentation at https://vrl.dev/expressions/#function-call-characteristics
# = learn more about error code 103 at https://errors.vrl.dev/103
# = see language documentation at https://vrl.dev
# = try your code in the VRL REPL, learn more at https://vrl.dev/examples
Expand Down
20 changes: 14 additions & 6 deletions src/compiler/expression/assignment.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
use std::{convert::TryFrom, fmt};

use crate::diagnostic::{DiagnosticMessage, Label, Note};
use crate::path::{OwnedSegment, OwnedTargetPath};
use crate::path::{OwnedValuePath, PathPrefix};
use crate::value::{Kind, Value};

use crate::compiler::{
expression::{assignment::ErrorVariant::InvalidParentPathSegment, Expr, Resolved},
parser::{
Expand All @@ -16,6 +11,10 @@ use crate::compiler::{
value::kind::DefaultValue,
CompileConfig, Context, Expression, Span, TypeDef,
};
use crate::diagnostic::{DiagnosticMessage, Label, Note};
use crate::path::{OwnedSegment, OwnedTargetPath};
use crate::path::{OwnedValuePath, PathPrefix};
use crate::value::{Kind, Value};

#[derive(Clone, PartialEq)]
pub struct Assignment {
Expand Down Expand Up @@ -671,6 +670,10 @@ impl DiagnosticMessage for Error {
],
FallibleAssignment(target, expr) => vec![
Label::primary("this expression is fallible", self.expr_span),
Label::context(
"note if an argument type is invalid it can render a function fallible",
self.expr_span,
),
Label::context("update the expression to be infallible", self.expr_span),
Label::context(
"or change this to an infallible assignment:",
Expand Down Expand Up @@ -718,7 +721,12 @@ impl DiagnosticMessage for Error {
use ErrorVariant::{FallibleAssignment, InfallibleAssignment};

match &self.variant {
FallibleAssignment(..) | InfallibleAssignment(..) => vec![Note::SeeErrorDocs],
FallibleAssignment(..) => {
vec![Note::SeeErrorDocs, Note::SeeFunctionCharacteristicsDocs]
}
InfallibleAssignment(..) => {
vec![Note::SeeErrorDocs]
}
InvalidParentPathSegment {
variant,
parent_str,
Expand Down
19 changes: 12 additions & 7 deletions src/diagnostic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@
clippy::needless_pass_by_value, // allowed in initial deny commit
)]

pub use diagnostic::{Diagnostic, DiagnosticList};
pub use formatter::Formatter;
pub use label::Label;
pub use note::Note;
pub use severity::Severity;
pub use span::{span, Span};

#[allow(clippy::module_inception)]
mod diagnostic;
mod formatter;
Expand All @@ -24,13 +31,6 @@ mod note;
mod severity;
mod span;

pub use diagnostic::{Diagnostic, DiagnosticList};
pub use formatter::Formatter;
pub use label::Label;
pub use note::Note;
pub use severity::Severity;
pub use span::{span, Span};

const VRL_DOCS_ROOT_URL: &str = "https://vrl.dev";
const VRL_ERROR_DOCS_ROOT_URL: &str = "https://errors.vrl.dev";
const VRL_FUNCS_ROOT_URL: &str = "https://functions.vrl.dev";
Expand Down Expand Up @@ -97,4 +97,9 @@ impl Urls {
fn example_docs() -> String {
format!("{VRL_DOCS_ROOT_URL}/examples")
}

#[must_use]
pub fn func_characteristics() -> String {
format!("{VRL_DOCS_ROOT_URL}/expressions/#function-call-characteristics")
}
}
10 changes: 8 additions & 2 deletions src/diagnostic/note.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pub enum Note {
SeeErrorDocs,
SeeCodeDocs(usize),
SeeLangDocs,
SeeFunctionCharacteristicsDocs,
SeeRepl,

#[doc(hidden)]
Expand All @@ -37,8 +38,9 @@ impl Note {
impl fmt::Display for Note {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use Note::{
Basic, CoerceValue, Example, Hint, SeeCodeDocs, SeeDocs, SeeErrorDocs, SeeFunctionDocs,
SeeLangDocs, SeeRepl, UserErrorMessage,
Basic, CoerceValue, Example, Hint, SeeCodeDocs, SeeDocs, SeeErrorDocs,
SeeFunctionCharacteristicsDocs, SeeFunctionDocs, SeeLangDocs, SeeRepl,
UserErrorMessage,
};

match self {
Expand All @@ -65,6 +67,10 @@ impl fmt::Display for Note {

write!(f, "see language documentation at {url}")
}
SeeFunctionCharacteristicsDocs => {
let url = Urls::func_characteristics();
write!(f, "see functions characteristics documentation at {url}")
}
SeeRepl => {
let url = Urls::example_docs();

Expand Down
Loading