Testing and fixing CEI pattern analysis (follow up for #3168)

sway-core/src/semantic_analysis/cei_pattern_analysis.rs

@@ -15,7 +15,7 @@ use crate::{
 };
 use std::collections::HashSet;
 use sway_error::warning::{CompileWarning, Warning};
-use sway_types::{Ident, Span};
+use sway_types::{Ident, Span, Spanned};
 
 #[derive(PartialEq, Eq, Hash, Clone)]
 enum Effect {
@@ -196,6 +196,7 @@ fn analyze_expression(
             arguments,
             function_decl_id,
             selector,
+            call_path,
             ..
         } => {
             use crate::declaration_engine::de_get_function;
@@ -215,7 +216,14 @@ fn analyze_expression(
 
             if args_effs.contains(&Effect::Interaction) {
                 // TODO: interaction span has to be more precise and point to an argument which performs interaction
-                warn_after_interaction(&fn_effs, &expr.span, &func.span, block_name, warnings)
+                let last_arg_span = &arguments.last().unwrap().1.span;
+                warn_after_interaction(
+                    &fn_effs,
+                    &call_path.span(),
+                    last_arg_span,
+                    block_name,
+                    warnings,
+                )
             }
 
             let mut result_effs = set_union(fn_effs, args_effs);
@@ -273,27 +281,28 @@ fn analyze_expression(
             set_union(cond_then_effs, cond_else_effs)
         }
         WhileLoop { condition, body } => {
+            // if the loop (condition + body) contains both interaction and storage operations
+            // in _any_ order, we report CEI pattern violation
             let cond_effs = analyze_expression(condition, block_name, warnings);
             let body_effs = analyze_code_block(body, block_name, warnings);
-            if cond_effs.contains(&Effect::Interaction) {
-                warn_after_interaction(
-                    &body_effs,
-                    &condition.span,
-                    &expr.span,
-                    block_name,
-                    warnings,
-                );
+            let res_effs = set_union(cond_effs, body_effs);
+            if res_effs.is_superset(&HashSet::from([Effect::Interaction, Effect::StorageRead])) {
+                warnings.push(CompileWarning {
+                    span: expr.span.clone(),
+                    warning_content: Warning::StorageReadAfterInteraction {
+                        block_name: block_name.clone(),
+                    },
+                });
             };
-            if body_effs.contains(&Effect::Interaction) {
-                warn_after_interaction(
-                    &cond_effs,
-                    &expr.span,
-                    &condition.span,
-                    block_name,
-                    warnings,
-                );
+            if res_effs.is_superset(&HashSet::from([Effect::Interaction, Effect::StorageWrite])) {
+                warnings.push(CompileWarning {
+                    span: expr.span.clone(),
+                    warning_content: Warning::StorageWriteAfterInteraction {
+                        block_name: block_name.clone(),
+                    },
+                });
             };
-            set_union(cond_effs, body_effs)
+            res_effs
         }
         AsmExpression {
             registers, body, ..

test/src/e2e_vm_tests/test_programs/should_pass/language/left_to_right_func_args_evaluation/Forc.lock

@@ -0,0 +1,8 @@
+[[package]]
+name = 'core'
+source = 'path+from-root-88DBE1D69186ED7B'
+
+[[package]]
+name = 'left_to_right_arg_evaluation'
+source = 'member'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/language/left_to_right_func_args_evaluation/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+authors = ["Fuel Labs <contact@fuel.sh>"]
+license = "Apache-2.0"
+name = "left_to_right_arg_evaluation"
+entry = "main.sw"
+
+[dependencies]
+core = { path = "../../../../../../../sway-lib-core" }

test/src/e2e_vm_tests/test_programs/should_pass/language/left_to_right_func_args_evaluation/json_abi_oracle.json

@@ -0,0 +1,22 @@
+{
+  "functions": [
+    {
+      "inputs": [],
+      "name": "main",
+      "output": {
+        "name": "",
+        "type": 0,
+        "typeArguments": null
+      }
+    }
+  ],
+  "loggedTypes": [],
+  "types": [
+    {
+      "components": null,
+      "type": "bool",
+      "typeId": 0,
+      "typeParameters": null
+    }
+  ]
+}

test/src/e2e_vm_tests/test_programs/should_pass/language/left_to_right_func_args_evaluation/src/main.sw

@@ -0,0 +1,75 @@
+script;
+// This tests function, tuple, struct arguments are evaluated from left to right
+
+fn func(a: u64, b: u64, c: u64, d: u64) -> u64 {
+    d
+}
+
+struct MyStruct {
+    a: u64,
+    b: u64,
+    c: u64,
+    d: u64,
+}
+
+fn main() -> bool {
+    let mut x: u64 = 0;
+
+    // function arguments evaluation
+    let func_res =
+        func(
+            {
+                x = 1;
+                0
+            },
+            {
+                x = 2;
+                0
+            },
+            {
+                x = 3;
+                0
+            },
+            x
+        );
+
+    // tuple evaluation
+    x = 0;
+    let tuple_res =
+        (
+            {
+                x = 1;
+                0
+            },
+            {
+                x = 2;
+                1
+            },
+            {
+                x = 3;
+                2
+            },
+            x
+        );
+
+    // struct evaluation
+    x = 0;
+    let struct_res =
+        MyStruct {
+            a: {
+                x = 1;
+                0
+            },
+            b: {
+                x = 2;
+                1
+            },
+            c: {
+                x = 3;
+                2
+            },
+            d: x
+        };
+
+    return (func_res == 3) && (tuple_res.3 == 3) && (struct_res.d == 3);
+}

test/src/e2e_vm_tests/test_programs/should_pass/language/left_to_right_func_args_evaluation/test.toml

@@ -0,0 +1,3 @@
+category = "run"
+expected_result = { action = "return", value = 1 }
+validate_abi = true

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-1/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = 'cei_pattern_violation_in_func_app-1'
+source = 'member'
+dependencies = ['std']
+
+[[package]]
+name = 'core'
+source = 'path+from-root-CC4CE5C392D95953'
+
+[[package]]
+name = 'std'
+source = 'path+from-root-CC4CE5C392D95953'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-1/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+name = "cei_pattern_violation_in_func_app-1"
+authors = ["Fuel Labs <contact@fuel.sh>"]
+entry = "main.sw"
+license = "Apache-2.0"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-1/src/main.sw

@@ -0,0 +1,28 @@
+contract;
+
+use std::storage::store;
+
+abi TestAbi {
+  #[storage(write)]
+  fn deposit(amount: u64);
+}
+
+fn pure_function(x: u64) -> u64 {
+  x
+}
+
+impl TestAbi for Contract {
+  #[storage(write)]
+  fn deposit(amount: u64) {
+    // the function argument is a code block with CEI pattern violation
+    pure_function(
+      {
+        // interaction
+        abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae).deposit(amount);
+        // effect -- therefore violation of CEI where effect should go before interaction
+        store(0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae, ());
+        42
+      }
+    );
+  }
+}

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-2/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = 'cei_pattern_violation_in_func_app-2'
+source = 'member'
+dependencies = ['std']
+
+[[package]]
+name = 'core'
+source = 'path+from-root-3351E955C8744D1C'
+
+[[package]]
+name = 'std'
+source = 'path+from-root-3351E955C8744D1C'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-2/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+name = "cei_pattern_violation_in_func_app-2"
+authors = ["Fuel Labs <contact@fuel.sh>"]
+entry = "main.sw"
+license = "Apache-2.0"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-2/src/main.sw

@@ -0,0 +1,33 @@
+contract;
+
+use std::storage::store;
+
+abi TestAbi {
+  #[storage(write)]
+  fn deposit(amount: u64);
+}
+
+fn pure_function(x: u64, y: u64) -> u64 {
+  x
+}
+
+impl TestAbi for Contract {
+  #[storage(write)]
+  fn deposit(amount: u64) {
+    // 1st function argument is a code block with interaction
+    // 2nd function argument is a code block with effect
+    pure_function(
+      {
+        // interaction
+        abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae).deposit(amount);
+        42
+      },
+      {
+        // effect -- therefore violation of CEI where effect should go before interaction
+        // (assuming left-to-right function argument evaluation)
+        store(0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae, ());
+        43
+      }
+    );
+  }
+}

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-2/test.toml

@@ -0,0 +1,3 @@
+category = "compile"
+
+# check: $()Storage modification after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-3/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = 'cei_pattern_violation_in_func_app-3'
+source = 'member'
+dependencies = ['std']
+
+[[package]]
+name = 'core'
+source = 'path+from-root-EAF483A8C0425562'
+
+[[package]]
+name = 'std'
+source = 'path+from-root-EAF483A8C0425562'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-3/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+name = "cei_pattern_violation_in_func_app-3"
+authors = ["Fuel Labs <contact@fuel.sh>"]
+entry = "main.sw"
+license = "Apache-2.0"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-3/src/main.sw

@@ -0,0 +1,28 @@
+contract;
+
+use std::storage::store;
+
+abi TestAbi {
+  #[storage(write)]
+  fn deposit(amount: u64);
+}
+
+#[storage(write)]
+fn do_something(x: u64) {
+  // effect
+  store(0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae, ());
+}
+
+impl TestAbi for Contract {
+  #[storage(write)]
+  fn deposit(amount: u64) {
+    // function's argument is a code block with interaction, function does storage write
+    do_something(
+      {
+        // interaction
+        abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae).deposit(amount);
+        42
+      },
+    );
+  }
+}

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_func_app-3/test.toml

@@ -0,0 +1,3 @@
+category = "compile"
+
+# check: $()Storage modification after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement-1/test.toml

@@ -0,0 +1,3 @@
+category = "compile"
+
+# check: $()Storage modification after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement-2/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = 'cei_pattern_violation_in_if_statement-2'
+source = 'member'
+dependencies = ['std']
+
+[[package]]
+name = 'core'
+source = 'path+from-root-CFB5A7C521AD2AFD'
+
+[[package]]
+name = 'std'
+source = 'path+from-root-CFB5A7C521AD2AFD'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement-2/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+name = "cei_pattern_violation_in_if_statement-2"
+authors = ["Fuel Labs <contact@fuel.sh>"]
+entry = "main.sw"
+license = "Apache-2.0"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }