Improve scope error handling (#251)

Cargo.lock

@@ -13,7 +13,7 @@ dependencies = [
 
 [[package]]
 name = "air"
-version = "0.23.0"
+version = "0.24.0"
 dependencies = [
  "air-execution-info-collector",
  "air-interpreter-data",
@@ -52,7 +52,7 @@ version = "0.1.0"
 
 [[package]]
 name = "air-interpreter"
-version = "0.23.0"
+version = "0.24.0"
 dependencies = [
  "air",
  "air-log-targets",

air/src/execution_step/air/next.rs

@@ -43,8 +43,9 @@ impl<'i> super::ExecutableInstruction<'i> for Next<'i> {
         maybe_meet_iteration_start(self, fold_state, trace_ctx)?;
         exec_ctx.scalars.meet_next_before();
 
-        next_instr.execute(exec_ctx, trace_ctx)?;
+        let result = next_instr.execute(exec_ctx, trace_ctx);
         exec_ctx.scalars.meet_next_after();
+        result?;
 
         // get the same fold state again because of borrow checker
         let fold_state = exec_ctx.scalars.get_iterable_mut(iterator_name)?;

air/src/execution_step/execution_context/scalar_variables.rs

@@ -26,6 +26,9 @@ use std::collections::HashMap;
 use std::collections::HashSet;
 use std::rc::Rc;
 
+/// Depth of a global scope.
+const GLOBAL_DEPTH: usize = 0;
+
 // TODO: move this code snippet to documentation when it's ready
 
 /// There are two scopes for variable scalars in AIR: global and local. A local scope
@@ -65,7 +68,6 @@ use std::rc::Rc;
 ///
 /// Although there could be only one iterable value for a fold block, because of CRDT rules.
 /// This struct is intended to provide abilities to work with scalars as it was described.
-#[derive(Default)]
 pub(crate) struct Scalars<'i> {
     // TODO: use Rc<String> to avoid copying
     /// Terminology used here (mainly to resolve concerns re difference between scalars and values):
@@ -91,7 +93,7 @@ pub(crate) struct Scalars<'i> {
     /// They are needed for careful isolation of scopes produced by iterations in fold blocks,
     /// precisely to limit access of non iterable variables defined on one depths to ones
     /// defined on another.
-    pub(crate) invalidated_depths: HashSet<usize>,
+    pub(crate) allowed_depths: HashSet<usize>,
 
     pub(crate) iterable_variables: HashMap<String, FoldState<'i>>,
 
@@ -120,6 +122,17 @@ impl SparseCell {
 }
 
 impl<'i> Scalars<'i> {
+    pub fn new() -> Self {
+        let allowed_depths = maplit::hashset! { GLOBAL_DEPTH };
+
+        Self {
+            non_iterable_variables: HashMap::new(),
+            allowed_depths,
+            iterable_variables: HashMap::new(),
+            current_depth: GLOBAL_DEPTH,
+        }
+    }
+
     /// Returns true if there was a previous value for the provided key on the same
     /// fold block.
     pub(crate) fn set_value(&mut self, name: impl Into<String>, value: ValueAggregate) -> ExecutionResult<bool> {
@@ -180,9 +193,9 @@ impl<'i> Scalars<'i> {
             .get(name)
             .and_then(|values| {
                 let last_cell = values.last();
-                let value_not_invalidated = !self.invalidated_depths.contains(&last_cell.depth);
+                let depth_allowed = self.allowed_depths.contains(&last_cell.depth);
 
-                if value_not_invalidated {
+                if depth_allowed {
                     Some(last_cell.value.as_ref())
                 } else {
                     None
@@ -212,22 +225,27 @@ impl<'i> Scalars<'i> {
 
     pub(crate) fn meet_fold_start(&mut self) {
         self.current_depth += 1;
+        self.allowed_depths.insert(self.current_depth);
     }
 
     // meet next before recursion
     pub(crate) fn meet_next_before(&mut self) {
-        self.invalidated_depths.insert(self.current_depth);
+        self.allowed_depths.remove(&self.current_depth);
         self.current_depth += 1;
+        self.allowed_depths.insert(self.current_depth);
     }
 
     // meet next after recursion
     pub(crate) fn meet_next_after(&mut self) {
+        self.allowed_depths.remove(&self.current_depth);
         self.current_depth -= 1;
-        self.invalidated_depths.remove(&self.current_depth);
+        self.allowed_depths.insert(self.current_depth);
+
         self.cleanup_obsolete_values();
     }
 
     pub(crate) fn meet_fold_end(&mut self) {
+        self.allowed_depths.remove(&self.current_depth);
         self.current_depth -= 1;
         self.cleanup_obsolete_values();
     }
@@ -314,8 +332,8 @@ impl<'i> Scalars<'i> {
     }
 }
 
-fn is_global_value(current_scope_depth: usize) -> bool {
-    current_scope_depth == 0
+fn is_global_value(value_depth: usize) -> bool {
+    value_depth == GLOBAL_DEPTH
 }
 
 fn is_value_obsolete(value_depth: usize, current_scope_depth: usize) -> bool {
@@ -324,6 +342,12 @@ fn is_value_obsolete(value_depth: usize, current_scope_depth: usize) -> bool {
 
 use std::fmt;
 
+impl Default for Scalars<'_> {
+    fn default() -> Self {
+        Scalars::new()
+    }
+}
+
 impl<'i> fmt::Display for Scalars<'i> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         writeln!(f, "fold_block_id: {}", self.current_depth)?;

air/tests/test_module/features/scopes/mod.rs

@@ -15,3 +15,4 @@
  */
 
 mod scalars_scope;
+mod scopes_behaviour_with_errors;

air/tests/test_module/features/scopes/scopes_behaviour_with_errors.rs

@@ -0,0 +1,116 @@
+/*
+ * Copyright 2022 Fluence Labs Limited
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use air_test_utils::prelude::*;
+
+use fstrings::f;
+use fstrings::format_args_f;
+
+#[test]
+fn check_that_scalar_is_visible_only_inside_fold_block() {
+    let variable_setter_peer_id = "variable_setter_peer_id";
+    let mut variable_setter_vm = create_avm(set_variable_call_service(json!([1, 2, 3])), variable_setter_peer_id);
+
+    let fallible_peer_id = "fallible_peer_id";
+    let mut fallible_peer_vm = create_avm(fallible_call_service("fail"), fallible_peer_id);
+
+    let variable_receiver_peer_id = "variable_receiver_peer_id";
+    let mut variable_receiver_peer_vm = create_avm(echo_call_service(), variable_receiver_peer_id);
+
+    let script = f!(r#"
+        (seq
+            (call "{variable_setter_peer_id}" ("" "") ["iterable_1"] iterable_1)
+            (xor
+                (fold iterable_1 iterator_1
+                     (seq
+                         (call "{variable_setter_peer_id}" ("" "") [] scalar)
+                         (seq
+                             (next iterator_1)
+                             (call "{fallible_peer_id}" ("fail" "") [] scalar)
+                         )
+                     )
+                )
+                (call "{variable_receiver_peer_id}" ("" "") [scalar])
+            )
+        )
+    "#);
+
+    let result = checked_call_vm!(variable_setter_vm, <_>::default(), &script, "", "");
+    let result = checked_call_vm!(fallible_peer_vm, <_>::default(), &script, "", result.data);
+    let result = checked_call_vm!(variable_receiver_peer_vm, <_>::default(), &script, "", result.data);
+    let actual_trace = trace_from_result(&result);
+
+    let expected_trace = vec![
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::service_failed(1, "failed result from fallible_call_service"),
+        executed_state::request_sent_by(fallible_peer_id),
+    ];
+
+    assert_eq!(actual_trace, expected_trace);
+}
+
+#[test]
+fn scopes_check_that_scalar_not_overwritten_by_fold_end() {
+    let variable_setter_peer_id = "variable_setter_peer_id";
+    let mut variable_setter_vm = create_avm(set_variable_call_service(json!([1, 2, 3])), variable_setter_peer_id);
+
+    let fallible_peer_id = "fallible_peer_id";
+    let mut fallible_peer_vm = create_avm(fallible_call_service("fail"), fallible_peer_id);
+
+    let variable_receiver_peer_id = "variable_receiver_peer_id";
+    let mut variable_receiver_peer_vm = create_avm(echo_call_service(), variable_receiver_peer_id);
+
+    let script = f!(r#"
+        (seq
+            (seq
+                (call "{variable_setter_peer_id}" ("" "") ["iterable_1"] iterable_1)
+                (call "{variable_setter_peer_id}" ("" "") ["scalar"] scalar)
+            )
+            (xor
+                (fold iterable_1 iterator_1
+                     (seq
+                         (call "{variable_setter_peer_id}" ("" "") [] scalar)
+                         (seq
+                             (next iterator_1)
+                             (call "{fallible_peer_id}" ("fail" "") [] scalar)
+                         )
+                     )
+                )
+                (call "{variable_receiver_peer_id}" ("" "") [scalar])
+            )
+        )
+    "#);
+
+    let result = checked_call_vm!(variable_setter_vm, <_>::default(), &script, "", "");
+    let result = checked_call_vm!(fallible_peer_vm, <_>::default(), &script, "", result.data);
+    let result = checked_call_vm!(variable_receiver_peer_vm, <_>::default(), &script, "", result.data);
+    let actual_trace = trace_from_result(&result);
+
+    let expected_trace = vec![
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::scalar(json!([1, 2, 3])),
+        executed_state::service_failed(1, "failed result from fallible_call_service"),
+        executed_state::scalar(json!([1, 2, 3])),
+    ];
+
+    assert_eq!(actual_trace, expected_trace);
+}

air/tests/test_module/instructions/new.rs

@@ -224,9 +224,9 @@ fn new_in_fold_with_ap() {
 }
 
 #[test]
-fn new_with_errors() {
-    let faillible_peer_id = "failible_peer_id";
-    let mut faillible_vm = create_avm(fallible_call_service("service_id_1"), faillible_peer_id);
+fn new_with_streams_with_errors() {
+    let fallible_peer_id = "fallible_peer_id";
+    let mut fallible_vm = create_avm(fallible_call_service("service_id_1"), fallible_peer_id);
 
     let local_peer_id = "local_peer_id";
     let mut vm = create_avm(echo_call_service(), local_peer_id);
@@ -239,7 +239,7 @@ fn new_with_errors() {
                         (new $restricted_stream_2
                             (seq
                                 (call "{local_peer_id}" ("" "") [2] $restricted_stream_2) ;; should have generation 1 in a data
-                                (call "{faillible_peer_id}" ("service_id_1" "local_fn_name") [] result)
+                                (call "{fallible_peer_id}" ("service_id_1" "local_fn_name") [] result)
                             )
                         )
                         (call "{local_peer_id}" ("" "") [2] restricted_stream_1) ;; should have generation 0 in a data
@@ -248,7 +248,7 @@ fn new_with_errors() {
             )"#);
 
     let result = checked_call_vm!(vm, <_>::default(), &script, "", "");
-    let result = call_vm!(faillible_vm, <_>::default(), script, "", result.data);
+    let result = call_vm!(fallible_vm, <_>::default(), script, "", result.data);
 
     let actual_trace = trace_from_result(&result);
     let expected_trace = vec![
@@ -278,6 +278,62 @@ fn new_with_errors() {
     assert_eq!(actual_global_streams, expected_global_streams);
 }
 
+#[test]
+fn new_with_scalars_with_errors() {
+    let set_variable_peer_id = "set_variable_peer_id";
+    let variables_mapping = maplit::hashmap! {
+        "global".to_string() => json!(1),
+        "scoped".to_string() => json!(2),
+    };
+    let mut set_variable_vm = create_avm(
+        set_variables_call_service(variables_mapping, VariableOptionSource::Argument(0)),
+        set_variable_peer_id,
+    );
+
+    let variable_receiver_peer_id = "variable_receiver_peer_id";
+    let mut variable_receiver_vm = create_avm(echo_call_service(), variable_receiver_peer_id);
+
+    let fallible_peer_id = "fallible_peer_id";
+    let fallible_service_id = "fallible_service_id";
+    let mut fallible_peer_vm = create_avm(fallible_call_service(fallible_service_id), fallible_peer_id);
+
+    let script = f!(r#"
+            (seq
+                (seq
+                    (call "{set_variable_peer_id}" ("" "") ["global"] scalar)
+                    (xor
+                        (new scalar
+                            (seq
+                                (call "{set_variable_peer_id}" ("" "") ["scoped"] scalar)
+                                (seq
+                                    (call "{variable_receiver_peer_id}" ("" "") [scalar])
+                                    (call "{fallible_peer_id}" ("{fallible_service_id}" "") [])
+                                )
+                            )
+                        )
+                        (call "{variable_receiver_peer_id}" ("" "") [scalar])
+                    )
+                )
+                (call "{variable_receiver_peer_id}" ("" "") [scalar])
+            )"#);
+
+    let result = checked_call_vm!(set_variable_vm, <_>::default(), &script, "", "");
+    let result = checked_call_vm!(variable_receiver_vm, <_>::default(), &script, "", result.data);
+    let result = checked_call_vm!(fallible_peer_vm, <_>::default(), &script, "", result.data);
+    let result = checked_call_vm!(variable_receiver_vm, <_>::default(), &script, "", result.data);
+    let actual_trace = trace_from_result(&result);
+
+    let expected_trace = vec![
+        executed_state::scalar_number(1),
+        executed_state::scalar_number(2),
+        executed_state::scalar_number(2),
+        executed_state::service_failed(1, r#"failed result from fallible_call_service"#),
+        executed_state::scalar_number(1),
+        executed_state::scalar_number(1),
+    ];
+    assert_eq!(actual_trace, expected_trace);
+}
+
 #[test]
 fn new_with_global_scalars() {
     let set_variable_peer_id = "set_variable_peer_id";