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.

Sign up for GitHub

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 assignment fallibility compilation error #553

Merged
merged 3 commits into from
Nov 13, 2023
Merged

feat(diagnostics): improve assignment fallibility compilation error #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19ab369
feat(diagnostics): improve assignment fallibility compilation error
pront Nov 10, 2023
ab51d3d
update the argument type
pront Nov 13, 2023
4825e34
improve grammar
pront Nov 13, 2023
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 lib/tests/tests/diagnostics/unhandled_parse_regex_all_type.vrl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
# │ │ │
# │ │ this expression is fallible
# │ │ update the expression to be infallible
# │ │ note if an argument type is invalid it can render a function fallible
# │ │ '.result[0].an' argument type is 'string or undefined' and this function expected a parameter 'value' of type 'string'
# │ or change this to an infallible assignment:
# │ .a, err = sha3(.result[0].an)
Comment on lines 10 to 14
Copy link
Member

Choose a reason for hiding this comment

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

#   │ │    this expression is fallible because at least one argument's type cannot be verified to be valid
#   │ │    '.result[0].an' argument type is 'string or undefined' and this function expected a parameter 'value' of type 'string'
#   │  update the expression to be infallible by adding a `!`: `.a = sha3!(.result[0].an)`
#   │ or change this to an infallible assignment:
#   │ .a, err = sha3(.result[0].an)

What you have here is a great step forward, but do you think we can make it even clearer with something like what I put? I still worry that the current error message will be confusing to users. I'm not sure exactly what's possible though.

Copy link
Member Author

@pront pront Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If I read the above correctly you want to add two more lines:

this expression is fallible because at least one argument's type cannot be verified to be valid
// ...
update the expression to be infallible by adding a `!`: `.a = sha3!(.result[0].an)`

which should be doable and I will follow up on this.

Ultimately users should have some basic understanding about fallibility and fail safety when using VRL (which practically means adding a ! or handling the returned err).

P.S. Where users might still be confused, is with code blocks which contain fallible expressions example here. However, that part of the codebase is not easy to maintain and is need of refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

#   │ │    this expression is fallible because at least one argument's type cannot be verified to be valid
#   │ │    '.result[0].an' argument type is 'string or undefined' and this function expected a parameter 'value' of type 'string'

is actually the more important update I would like to see. I think my verbiage makes it more clear exactly what the issue is: that the function is fallible because one of the arguments cannot be verified, and this is the argument (or arguments) in particular, their types, and what the function expects.

The hints on how to fix are then a cherry on top.

Copy link
Member

Choose a reason for hiding this comment

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

The hints could also include casting the arguments like:

#   │ .a = sha3(string!(.result[0].an))

I think that is closer to the "best-practice" that we want to guide people into.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, this makes sense.

# │
Expand Down
2 changes: 1 addition & 1 deletion lib/tests/tests/examples/blog_vector_remap_language_error_diagnostic.vrl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
# │ │ │
# │ │ this expression is fallible
# │ │ update the expression to be infallible
# │ │ note if an argument type is invalid it can render a function fallible
# │ │ '.log' argument type is 'any' and this function expected a parameter 'value' of type 'string'
# │ or change this to an infallible assignment:
# │ ., err = parse_common_log(.log)
# │
Expand Down
51 changes: 42 additions & 9 deletions src/compiler/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::compiler::expression::function_call::FunctionCallError;
use crate::compiler::expression::ExpressionError;
use crate::compiler::{
expression::{
assignment, function_call, literal, predicate, query, Abort, Array, Assignment, Block,
Expand All @@ -12,7 +14,7 @@ use crate::diagnostic::{DiagnosticList, DiagnosticMessage, Note};
use crate::parser::ast::{self, Node, QueryTarget};
use crate::path::PathPrefix;
use crate::path::{OwnedTargetPath, OwnedValuePath};
use crate::prelude::ArgumentList;
use crate::prelude::{expression, ArgumentList};
use crate::value::Value;

use super::state::TypeState;
Expand Down Expand Up @@ -52,11 +54,35 @@ pub struct Compiler<'a> {
// This should probably be kept on the call stack as the "compile_*" functions are called
// otherwise some expressions may remove it when they shouldn't (such as the RHS of an operation removing
// the error from the LHS)
fallible_expression_error: Option<Box<dyn DiagnosticMessage>>,
fallible_expression_error: Option<CompilerError>,

config: CompileConfig,
}

// TODO: The diagnostic related code is in dire need of refactoring.
// This is a workaround to avoid doing this work upfront.
#[derive(Debug)]
pub(crate) enum CompilerError {
FunctionCallError(FunctionCallError),
ExpressionError(ExpressionError),
}

impl CompilerError {
fn to_diagnostic(&self) -> &dyn DiagnosticMessage {
match self {
CompilerError::FunctionCallError(e) => e,
CompilerError::ExpressionError(e) => e,
}
}

fn into_diagnostic_boxed(self) -> Box<dyn DiagnosticMessage> {
match self {
CompilerError::FunctionCallError(e) => Box::new(e),
CompilerError::ExpressionError(e) => Box::new(e),
}
}
}

impl<'a> Compiler<'a> {
pub fn compile(
fns: &'a [Box<dyn Function>],
Expand Down Expand Up @@ -151,8 +177,9 @@ impl<'a> Compiler<'a> {

let type_def = expr.type_info(&original_state).result;
if type_def.is_fallible() && self.fallible_expression_error.is_none() {
let error = super::expression::Error::Fallible { span };
self.fallible_expression_error = Some(Box::new(error) as _);
self.fallible_expression_error = Some(CompilerError::ExpressionError(
expression::ExpressionError::Fallible { span },
));
}

Some(expr)
Expand Down Expand Up @@ -234,7 +261,7 @@ impl<'a> Compiler<'a> {

if let Some(expr) = self.compile_expr(node_expr, state) {
if let Some(error) = self.fallible_expression_error.take() {
self.diagnostics.push(error);
self.diagnostics.push(error.into_diagnostic_boxed());
}

node_exprs.push(expr);
Expand Down Expand Up @@ -353,7 +380,9 @@ impl<'a> Compiler<'a> {
Some(Predicate::new(
Node::new(span, exprs),
state,
self.fallible_expression_error.as_deref(),
self.fallible_expression_error
.as_ref()
.map(CompilerError::to_diagnostic),
))
}

Expand Down Expand Up @@ -510,7 +539,7 @@ impl<'a> Compiler<'a> {
let assignment = Assignment::new(
node,
state,
self.fallible_expression_error.as_deref(),
self.fallible_expression_error.as_ref(),
&self.config,
)
.map_err(|err| self.diagnostics.push(Box::new(err)))
Expand Down Expand Up @@ -696,12 +725,16 @@ impl<'a> Compiler<'a> {
state,
block,
local_snapshot,
&mut self.fallible_expression_error,
&mut self.config,
)
.map_err(|err| self.diagnostics.push(Box::new(err)))
.ok()
.map(|func| (arg_list, func))
.map(|result| {
if let Some(e) = result.error {
self.fallible_expression_error = Some(CompilerError::FunctionCallError(e));
}
(arg_list, result.function_call)
})
});

if let Some((args, function)) = &function_info {
Expand Down
58 changes: 29 additions & 29 deletions src/compiler/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,33 @@
use std::fmt;

use dyn_clone::{clone_trait_object, DynClone};

pub use abort::Abort;
pub use array::Array;
pub use assignment::Assignment;
pub use block::Block;
pub use container::{Container, Variant};
pub use function::FunctionExpression;
pub use function_argument::FunctionArgument;
pub use function_call::FunctionCall;
pub use group::Group;
pub use if_statement::IfStatement;
pub use literal::Literal;
pub use noop::Noop;
pub use not::Not;
pub use object::Object;
pub use op::Op;
pub use predicate::Predicate;
pub use query::{Query, Target};
pub use unary::Unary;
pub use variable::Variable;

use crate::diagnostic::{DiagnosticMessage, Label, Note};
use crate::value::Value;
use dyn_clone::{clone_trait_object, DynClone};

use super::state::{TypeInfo, TypeState};
use super::{Context, Span, TypeDef};
pub use super::{ExpressionError2, Resolved};

mod abort;
mod array;
Expand All @@ -28,29 +51,6 @@ pub(crate) mod literal;
pub(crate) mod predicate;
pub mod query;

pub use super::{ExpressionError, Resolved};

use super::state::{TypeInfo, TypeState};
pub use abort::Abort;
pub use array::Array;
pub use assignment::Assignment;
pub use block::Block;
pub use container::{Container, Variant};
pub use function::FunctionExpression;
pub use function_argument::FunctionArgument;
pub use function_call::FunctionCall;
pub use group::Group;
pub use if_statement::IfStatement;
pub use literal::Literal;
pub use noop::Noop;
pub use not::Not;
pub use object::Object;
pub use op::Op;
pub use predicate::Predicate;
pub use query::{Query, Target};
pub use unary::Unary;
pub use variable::Variable;

pub trait Expression: Send + Sync + fmt::Debug + DynClone {
/// Resolve an expression to a concrete [`Value`].
///
Expand Down Expand Up @@ -373,17 +373,17 @@ impl From<Value> for Expr {
// -----------------------------------------------------------------------------

#[derive(thiserror::Error, Debug)]
pub enum Error {
pub enum ExpressionError {
#[error("unhandled error")]
Fallible { span: Span },

#[error("expression type unavailable")]
Missing { span: Span, feature: &'static str },
}

impl DiagnosticMessage for Error {
impl DiagnosticMessage for ExpressionError {
fn code(&self) -> usize {
use Error::{Fallible, Missing};
use ExpressionError::{Fallible, Missing};

match self {
Fallible { .. } => 100,
Expand All @@ -392,7 +392,7 @@ impl DiagnosticMessage for Error {
}

fn labels(&self) -> Vec<Label> {
use Error::{Fallible, Missing};
use ExpressionError::{Fallible, Missing};

match self {
Fallible { span } => vec![
Expand All @@ -410,7 +410,7 @@ impl DiagnosticMessage for Error {
}

fn notes(&self) -> Vec<Note> {
use Error::{Fallible, Missing};
use ExpressionError::{Fallible, Missing};

match self {
Fallible { .. } => vec![Note::SeeErrorDocs],
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/expression/abort.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
use std::fmt;

use crate::diagnostic::{DiagnosticMessage, Label, Note, Urls};
use crate::parser::ast::Node;

use super::Expr;
use crate::compiler::{
expression::{ExpressionError, Resolved},
expression::{ExpressionError2, Resolved},
state::{TypeInfo, TypeState},
value::{Kind, VrlValueConvert},
Context, Expression, Span, TypeDef,
};
use crate::diagnostic::{DiagnosticMessage, Label, Note, Urls};
use crate::parser::ast::Node;

use super::Expr;

#[derive(Debug, Clone, PartialEq)]
pub struct Abort {
Expand Down Expand Up @@ -53,12 +53,12 @@ impl Expression for Abort {
let message = self
.message
.as_ref()
.map::<Result<_, ExpressionError>, _>(|expr| {
.map::<Result<_, ExpressionError2>, _>(|expr| {
Ok(expr.resolve(ctx)?.try_bytes_utf8_lossy()?.to_string())
})
.transpose()?;

Err(ExpressionError::Abort {
Err(ExpressionError2::Abort {
span: self.span,
message,
})
Expand Down
Loading
Loading