Skip to content

Commit 5c4a12c

Browse files
fuchsnjjszwedko
authored andcommitted
fix(vrl): fix if statement type definitions (#12954)
1 parent d304ed3 commit 5c4a12c

17 files changed

+257
-35
lines changed

lib/vrl/compiler/src/compiler.rs

+49-6
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ impl<'a> Compiler<'a> {
237237
// the block, and merge back into it any mutations that happened to
238238
// state the snapshot was already tracking. Then, revert the compiler
239239
// local state to the updated snapshot.
240-
self.local = local_snapshot.merge_mutations(self.local.clone());
240+
self.local = local_snapshot.apply_child_scope(self.local.clone());
241241

242242
block
243243
}
@@ -266,6 +266,8 @@ impl<'a> Compiler<'a> {
266266
node: Node<ast::IfStatement>,
267267
external: &mut ExternalEnv,
268268
) -> IfStatement {
269+
use crate::type_def::{Details, TypeDef};
270+
269271
let ast::IfStatement {
270272
predicate,
271273
consequent,
@@ -280,13 +282,54 @@ impl<'a> Compiler<'a> {
280282
}
281283
};
282284

285+
let original_locals = self.local.clone();
286+
let original_external = external.target().cloned();
287+
283288
let consequent = self.compile_block(consequent, external);
284-
let alternative = alternative.map(|block| self.compile_block(block, external));
285289

286-
IfStatement {
287-
predicate,
288-
consequent,
289-
alternative,
290+
match alternative {
291+
Some(block) => {
292+
let consequent_locals = self.local.clone();
293+
let consequent_external = external.target().cloned();
294+
295+
self.local = original_locals;
296+
297+
let else_block = self.compile_block(block, external);
298+
299+
// assignments must be the result of either the if or else block, but not the original value
300+
self.local = self.local.clone().merge(consequent_locals);
301+
if let Some(details) = Details::merge_optional(
302+
consequent_external,
303+
external.target().cloned(),
304+
TypeDef::any(),
305+
) {
306+
external.update_target(details);
307+
}
308+
309+
IfStatement {
310+
predicate,
311+
consequent,
312+
alternative: Some(else_block),
313+
}
314+
}
315+
None => {
316+
// assignments must be the result of either the if block or the original value
317+
self.local = self.local.clone().merge(original_locals);
318+
319+
if let Some(details) = Details::merge_optional(
320+
original_external,
321+
external.target().cloned(),
322+
TypeDef::any(),
323+
) {
324+
external.update_target(details);
325+
}
326+
327+
IfStatement {
328+
predicate,
329+
consequent,
330+
alternative: None,
331+
}
332+
}
290333
}
291334
}
292335

lib/vrl/compiler/src/expression/if_statement.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ impl Expression for IfStatement {
4949
let type_def = self.consequent.type_def(state);
5050

5151
match &self.alternative {
52-
None => type_def,
52+
None => type_def.add_null(),
5353
Some(alternative) => type_def.merge_deep(alternative.type_def(state)),
5454
}
5555
}

lib/vrl/compiler/src/state.rs

+17-4
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,29 @@ impl LocalEnv {
3030
self.bindings.remove(ident)
3131
}
3232

33-
/// Merge state present in both `self` and `other`.
34-
pub(crate) fn merge_mutations(mut self, other: Self) -> Self {
35-
for (ident, other_details) in other.bindings.into_iter() {
33+
/// Any state the child scope modified that was part of the parent is copied to the parent scope
34+
pub(crate) fn apply_child_scope(mut self, child: Self) -> Self {
35+
for (ident, child_details) in child.bindings {
3636
if let Some(self_details) = self.bindings.get_mut(&ident) {
37-
*self_details = other_details;
37+
*self_details = child_details;
3838
}
3939
}
4040

4141
self
4242
}
43+
44+
/// Merges two local envs together. This is useful in cases such as if statements
45+
/// where different LocalEnv's can be created, and the result is decided at runtime.
46+
/// The compile-time type must be the union of the options.
47+
#[cfg(feature = "expr-if_statement")]
48+
pub(crate) fn merge(mut self, other: Self) -> Self {
49+
for (ident, other_details) in other.bindings {
50+
if let Some(self_details) = self.bindings.get_mut(&ident) {
51+
*self_details = self_details.clone().merge(other_details);
52+
}
53+
}
54+
self
55+
}
4356
}
4457

4558
/// A lexical scope within the program.

lib/vrl/compiler/src/type_def.rs

+76-15
Original file line numberDiff line numberDiff line change
@@ -350,21 +350,7 @@ impl TypeDef {
350350

351351
pub fn merge(&mut self, other: Self, strategy: merge::Strategy) {
352352
self.fallible |= other.fallible;
353-
354-
// NOTE: technically we shouldn't do this, but to keep backward compatibility with the
355-
// "old" `Kind` implementation, we consider a type that is marked as "any" equal to the old
356-
// implementation's `unknown`, which, when merging, would discard that type.
357-
//
358-
// see: https://github.com/vectordotdev/vector/blob/18415050b60b08197e8135b7659390256995e844/lib/vrl/compiler/src/type_def.rs#L428-L429
359-
if !self.is_any() && other.is_any() {
360-
// keep `self`'s `kind` definition
361-
} else if self.is_any() && !other.is_any() {
362-
// overwrite `self`'s `kind` definition with `others`'s
363-
self.kind = other.kind;
364-
} else {
365-
// merge the two `kind`s
366-
self.kind.merge(other.kind, strategy);
367-
}
353+
self.kind.merge(other.kind, strategy);
368354
}
369355

370356
pub fn merge_overwrite(mut self, other: Self) -> Self {
@@ -399,3 +385,78 @@ pub(crate) struct Details {
399385
pub(crate) type_def: TypeDef,
400386
pub(crate) value: Option<Value>,
401387
}
388+
389+
impl Details {
390+
/// Returns the union of 2 possible states
391+
#[cfg(any(test, feature = "expr-if_statement"))]
392+
pub(crate) fn merge(self, other: Self) -> Self {
393+
Self {
394+
type_def: self.type_def.merge_deep(other.type_def),
395+
value: if self.value == other.value {
396+
self.value
397+
} else {
398+
None
399+
},
400+
}
401+
}
402+
403+
/// Returns the union of 2 possible states
404+
#[cfg(any(feature = "expr-if_statement"))]
405+
pub(crate) fn merge_optional(
406+
a: Option<Self>,
407+
b: Option<Self>,
408+
none_type: TypeDef,
409+
) -> Option<Self> {
410+
match (a, b) {
411+
(Some(a), Some(b)) => Some(a.merge(b)),
412+
(Some(_), None) | (None, Some(_)) => Some(Details {
413+
type_def: none_type,
414+
value: None,
415+
}),
416+
(None, None) => None,
417+
}
418+
}
419+
}
420+
421+
#[cfg(test)]
422+
mod test {
423+
use super::*;
424+
425+
#[test]
426+
fn merge_details_same_literal() {
427+
let a = Details {
428+
type_def: TypeDef::integer(),
429+
value: Some(Value::from(5)),
430+
};
431+
let b = Details {
432+
type_def: TypeDef::float(),
433+
value: Some(Value::from(5)),
434+
};
435+
assert_eq!(
436+
a.merge(b),
437+
Details {
438+
type_def: TypeDef::integer().add_float(),
439+
value: Some(Value::from(5))
440+
}
441+
)
442+
}
443+
444+
#[test]
445+
fn merge_details_different_literal() {
446+
let a = Details {
447+
type_def: TypeDef::any(),
448+
value: Some(Value::from(5)),
449+
};
450+
let b = Details {
451+
type_def: TypeDef::object(Collection::empty()),
452+
value: Some(Value::from(6)),
453+
};
454+
assert_eq!(
455+
a.merge(b),
456+
Details {
457+
type_def: TypeDef::any(),
458+
value: None
459+
}
460+
)
461+
}
462+
}

lib/vrl/stdlib/src/append.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ impl Expression for AppendFn {
7676
fn type_def(&self, state: (&state::LocalEnv, &state::ExternalEnv)) -> TypeDef {
7777
self.value
7878
.type_def(state)
79-
.merge_append(self.items.type_def(state))
79+
.restrict_array()
80+
.merge_append(self.items.type_def(state).restrict_array())
8081
}
8182
}
8283

lib/vrl/stdlib/src/merge.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ impl Expression for MergeFn {
8989
fn type_def(&self, state: (&state::LocalEnv, &state::ExternalEnv)) -> TypeDef {
9090
self.to
9191
.type_def(state)
92-
.merge_shallow(self.from.type_def(state))
92+
.restrict_object()
93+
.merge_shallow(self.from.type_def(state).restrict_object())
9394
}
9495
}
9596

lib/vrl/stdlib/src/push.rs

+5-2
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,11 @@ impl Expression for PushFn {
8484
0.into(),
8585
self.item.type_def(state).into(),
8686
)]));
87-
88-
self.value.type_def(state).merge_append(item).infallible()
87+
self.value
88+
.type_def(state)
89+
.restrict_array()
90+
.merge_append(item)
91+
.infallible()
8992
}
9093
}
9194

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# result: true
2+
3+
.a = 5
4+
if true {
5+
.a = 0.0
6+
} else {
7+
.a = "string"
8+
}
9+
assert!(.a == 0.0)
10+
assert!(type_def(.a) == {"float": true, "bytes": true})
11+
12+
.a = 5
13+
if false {
14+
.a = 0.0
15+
} else {
16+
.a = "string"
17+
}
18+
assert!(.a == "string")
19+
assert!(type_def(.a) == {"float": true, "bytes": true})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# result: true
2+
3+
a = 5
4+
if true {
5+
a = 0.0
6+
} else {
7+
a = "string"
8+
}
9+
assert!(a == 0.0)
10+
assert!(type_def(a) == {"float": true, "bytes": true})
11+
12+
a = 5
13+
if false {
14+
a = 0.0
15+
} else {
16+
a = "string"
17+
}
18+
assert!(a == "string")
19+
assert!(type_def(a) == {"float": true, "bytes": true})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# result: true
2+
3+
result = if true {
4+
"string"
5+
}
6+
assert!(result == "string")
7+
assert!(type_def(result) == {"bytes": true, "null": true})
8+
9+
10+
result = if false {
11+
"string"
12+
}
13+
assert!(result == null)
14+
assert!(type_def(result) == {"bytes": true, "null": true})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# result: true
2+
3+
.a = 5
4+
if false {
5+
.a = "string"
6+
}
7+
assert!(.a == 5)
8+
assert!(type_def(.a) == {"integer": true, "bytes": true})
9+
10+
.a = 5
11+
if true {
12+
.a = "string"
13+
}
14+
assert!(.a == "string")
15+
assert!(type_def(.a) == {"integer": true, "bytes": true})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# result: true
2+
3+
a = 5
4+
if false {
5+
a = "string"
6+
}
7+
assert!(a == 5)
8+
assert!(type_def(a) == {"integer": true, "bytes": true})
9+
10+
a = 5
11+
if true {
12+
a = "string"
13+
}
14+
assert!(a == "string")
15+
assert!(type_def(a) == {"integer": true, "bytes": true})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# result: {"any": true}
2+
3+
assert!(type_def(.a) == {"any": true})
4+
if false {
5+
.a = "string"
6+
}
7+
type_def(.a)
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
# result: null
1+
# result: true
22

3-
if false {
3+
result = if false {
44
"yes"
55
}
6+
assert!(result == null)
7+
assert!(type_def(result) == {"bytes": true, "null": true})
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
# result: "yes"
1+
# result: true
22

3-
if true {
3+
result = if true {
44
"yes"
55
}
6+
assert!(result == "yes")
7+
assert!(type_def(result) == {"bytes": true, "null": true})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# result: null
2+
3+
if .platform == "Apache2" || .platform == "Nginx" {
4+
apache2 = del(.apache2)
5+
if is_null(apache2) { apache2 = {} }
6+
.http = merge(object!(apache2), {})
7+
}

scripts/test-vrl.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,5 @@ set -euo pipefail
1010
(
1111
cd "$(dirname "${BASH_SOURCE[0]}")/../lib/vrl/tests"
1212

13-
cargo run -- --runtime=ast && cargo run -- --runtime=vm
13+
cargo run -- --runtime=ast
1414
)

0 commit comments

Comments
 (0)