Skip to content

Commit f09bf47

Browse files
JeanMertzJeanMertz
authored
chore(remap): improve operation type defs (#6471)
1 parent d8ac64d commit f09bf47

File tree

5 files changed

+114
-45
lines changed

5 files changed

+114
-45
lines changed

‎lib/vrl/compiler/src/compiler.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,10 +192,13 @@ impl<'a> Compiler<'a> {
192192
let op = node.into_inner();
193193
let ast::Op(lhs, opcode, rhs) = op;
194194

195-
let lhs = self.compile_expr(*lhs);
196-
let rhs = self.compile_expr(*rhs);
195+
let lhs_span = lhs.span();
196+
let lhs = Node::new(lhs_span, self.compile_expr(*lhs));
197197

198-
Op::new(lhs, opcode, rhs).unwrap_or_else(|err| {
198+
let rhs_span = rhs.span();
199+
let rhs = Node::new(rhs_span, self.compile_expr(*rhs));
200+
201+
Op::new(lhs, opcode, rhs, &self.state).unwrap_or_else(|err| {
199202
self.errors.push(Box::new(err));
200203
Op::noop()
201204
})

‎lib/vrl/compiler/src/expression/op.rs

Lines changed: 77 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
use crate::expression::{Expr, Noop, Resolved};
1+
use crate::expression::{self, Expr, Noop, Resolved};
22
use crate::parser::{ast, Node};
33
use crate::{value, Context, Expression, State, TypeDef, Value};
4-
use diagnostic::{DiagnosticError, Label, Span};
4+
use diagnostic::{DiagnosticError, Label, Note, Span};
55
use std::fmt;
66

77
#[derive(Clone, PartialEq)]
@@ -12,20 +12,42 @@ pub struct Op {
1212
}
1313

1414
impl Op {
15-
pub fn new(lhs: Expr, opcode: Node<ast::Opcode>, rhs: Expr) -> Result<Self, Error> {
16-
use ast::Opcode::*;
15+
pub fn new(
16+
lhs: Node<Expr>,
17+
opcode: Node<ast::Opcode>,
18+
rhs: Node<Expr>,
19+
state: &State,
20+
) -> Result<Self, Error> {
21+
use ast::Opcode::{Eq, Ge, Gt, Le, Lt, Ne};
22+
23+
let (op_span, opcode) = opcode.take();
24+
25+
let (lhs_span, lhs) = lhs.take();
26+
let lhs_type_def = lhs.type_def(state);
1727

18-
let (span, opcode) = opcode.take();
28+
let (rhs_span, rhs) = rhs.take();
29+
let rhs_type_def = rhs.type_def(state);
1930

2031
if matches!(opcode, Eq | Ne | Lt | Le | Gt | Ge) {
2132
if let Expr::Op(op) = &lhs {
2233
if matches!(op.opcode, Eq | Ne | Lt | Le | Gt | Ge) {
23-
let error = Error::ChainedComparison { span };
24-
return std::result::Result::Err(error);
34+
return Err(Error::ChainedComparison { span: op_span });
2535
}
2636
}
2737
}
2838

39+
if let ast::Opcode::Err = opcode {
40+
if lhs_type_def.is_infallible() {
41+
return Err(Error::ErrInfallible {
42+
lhs_span,
43+
rhs_span,
44+
op_span,
45+
});
46+
} else if rhs_type_def.is_fallible() {
47+
return Err(expression::Error::Fallible { span: rhs_span }.into());
48+
}
49+
}
50+
2951
Ok(Op {
3052
lhs: Box::new(lhs),
3153
rhs: Box::new(rhs),
@@ -85,11 +107,13 @@ impl Expression for Op {
85107

86108
let lhs_kind = lhs_def.kind();
87109
let rhs_kind = rhs_def.kind();
88-
let merged_kind = merged_def.kind();
89110

90111
match self.opcode {
91-
// null || null
92-
Or if merged_kind.is_null() => TypeDef::new().infallible().null(),
112+
// ok/err ?? ok
113+
Err if rhs_def.is_infallible() => merged_def.infallible(),
114+
115+
// ... ?? ...
116+
Err => merged_def,
93117

94118
// null || ...
95119
Or if lhs_kind.is_null() => rhs_def,
@@ -107,15 +131,6 @@ impl Expression for Op {
107131
// ... || ...
108132
Or => merged_def,
109133

110-
// ok ?? ...
111-
Err if lhs_def.is_infallible() => lhs_def,
112-
113-
// ok/err ?? ok
114-
Err if rhs_def.is_infallible() => merged_def.infallible(),
115-
116-
// ... ?? ...
117-
Err => merged_def,
118-
119134
// null && ...
120135
And if lhs_kind.is_null() => rhs_def.scalar(K::Boolean),
121136

@@ -162,18 +177,14 @@ impl Expression for Op {
162177
// 1 * 1
163178
// 1 % 1
164179
Add | Sub | Mul | Rem if lhs_kind.is_integer() && rhs_kind.is_integer() => {
165-
merged_def.infallible().scalar(K::Integer)
180+
merged_def.scalar(K::Integer)
166181
}
167182

168183
// "bar" * 1
169-
Mul if lhs_kind.is_bytes() && rhs_kind.is_integer() => {
170-
merged_def.infallible().scalar(K::Bytes)
171-
}
184+
Mul if lhs_kind.is_bytes() && rhs_kind.is_integer() => merged_def.scalar(K::Bytes),
172185

173186
// 1 * "bar"
174-
Mul if lhs_kind.is_integer() && rhs_kind.is_bytes() => {
175-
merged_def.infallible().scalar(K::Bytes)
176-
}
187+
Mul if lhs_kind.is_integer() && rhs_kind.is_bytes() => merged_def.scalar(K::Bytes),
177188

178189
// ... + ...
179190
// ... * ...
@@ -206,6 +217,16 @@ impl fmt::Debug for Op {
206217
pub enum Error {
207218
#[error("comparison operators cannot be chained")]
208219
ChainedComparison { span: Span },
220+
221+
#[error("unneeded error-coalesce operation")]
222+
ErrInfallible {
223+
lhs_span: Span,
224+
rhs_span: Span,
225+
op_span: Span,
226+
},
227+
228+
#[error("fallible operation")]
229+
Expr(#[from] expression::Error),
209230
}
210231

211232
impl DiagnosticError for Error {
@@ -214,6 +235,17 @@ impl DiagnosticError for Error {
214235

215236
match self {
216237
ChainedComparison { .. } => 650,
238+
ErrInfallible { .. } => 651,
239+
Expr(err) => err.code(),
240+
}
241+
}
242+
243+
fn message(&self) -> String {
244+
use Error::*;
245+
246+
match self {
247+
Expr(err) => err.message(),
248+
err => err.to_string(),
217249
}
218250
}
219251

@@ -222,6 +254,25 @@ impl DiagnosticError for Error {
222254

223255
match self {
224256
ChainedComparison { span } => vec![Label::primary("", span)],
257+
ErrInfallible {
258+
lhs_span,
259+
rhs_span,
260+
op_span,
261+
} => vec![
262+
Label::primary("this expression cannot fail", lhs_span),
263+
Label::context("this expression never resolves", rhs_span),
264+
Label::context("remove this error coalesce operation", op_span),
265+
],
266+
Expr(err) => err.labels(),
267+
}
268+
}
269+
270+
fn notes(&self) -> Vec<Note> {
271+
use Error::*;
272+
273+
match self {
274+
Expr(err) => err.notes(),
275+
_ => vec![],
225276
}
226277
}
227278
}
@@ -389,20 +440,6 @@ mod tests {
389440
want: TypeDef::new().fallible().boolean(),
390441
}
391442

392-
error_or_lhs_infallible {
393-
expr: |_| Op {
394-
lhs: Box::new(Literal::from("foo").into()),
395-
rhs: Box::new(Op {
396-
lhs: Box::new(Literal::from("foo").into()),
397-
rhs: Box::new(Literal::from(1).into()),
398-
opcode: Div,
399-
}.into(),
400-
),
401-
opcode: Err,
402-
},
403-
want: TypeDef::new().bytes(),
404-
}
405-
406443
error_or_rhs_infallible {
407444
expr: |_| Op {
408445
lhs: Box::new(Op {

‎lib/vrl/tests/tests/diagnostics/unneeded_error_coalesce.vrl

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# result:
2+
#
3+
# error[E651]: unneeded error-coalesce operation
4+
# ┌─ :2:1
5+
# │
6+
# 2 │ "foo" ?? "bar"
7+
# │ ^^^^^ -- ----- this expression never resolves
8+
# │ │ │
9+
# │ │ remove this error coalesce operation
10+
# │ this expression cannot fail
11+
# │
12+
# = see language documentation at: https://vector.dev/docs/reference/vrl/
13+
14+
"foo" ?? "bar"

‎lib/vrl/tests/tests/expressions/logical/err.vrl

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
1-
# result: ["foo", 2.5, "yes", false]
1+
# result: [2.5, "yes", false]
22

33
[
4-
"foo" ?? "bar",
54
5 / 2 ?? 0,
65
parse_json(s'"yes"') ?? false,
76
parse_json("nope") ?? false,

‎lib/vrl/tests/tests/issues/6469_fallible_operations_marked_as_infallible.vrl

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# result:
2+
#
3+
# error[E100]: unhandled error
4+
# ┌─ :2:1
5+
# │
6+
# 2 │ 5 + to_int(.foo)
7+
# │ ^^^^^^^^^^^^^^^^
8+
# │ │
9+
# │ expression can result in runtime error
10+
# │ handle the error case to ensure runtime success
11+
# │
12+
# = see error handling documentation at: https://vector.dev/docs/reference/vrl/
13+
# = see language documentation at: https://vector.dev/docs/reference/vrl/
14+
# = learn more at: https://errors.vrl.dev/100
15+
16+
5 + to_int(.foo)

0 commit comments

Comments
 (0)