Fix side effects in if-let expressions (#5215)

## Description This PR fixes the critical issue in `if-let`s: #5173 `if-let`s were desugared to `match` expressions but used their own implementation for desugaring which wasn't storing the result of the matched value into a compiler generated variable but instead re-evaluated it on every access, causing side effects. The new implementation generates exactly the same desugaring as the equivalent `match` expression would have, simply by forwarding the `if-let` desugaring to `match` desugaring. Since the generated IR is different, the IR gen tests needed to be adapted as well. Also, a single assert is commented out in the `go_to_definition_for_matches` LSP test until #5221 is solved. Closes #5173. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
FuelLabs · Oct 24, 2023 · 1cc0740 · 1cc0740
1 parent 9d21152
commit 1cc0740
Show file tree

Hide file tree

Showing 10 changed files with 163 additions and 70 deletions.
diff --git a/sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs b/sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs
@@ -1947,22 +1947,6 @@ fn expr_to_expression(
         Expr::Match {
             value, branches, ..
         } => {
-            let value = expr_to_expression(context, handler, engines, *value)?;
-            let var_decl_span = value.span();
-
-            // Generate a deterministic name for the variable returned by the match expression.
-            let match_return_var_name = format!(
-                "{}{}",
-                MATCH_RETURN_VAR_NAME_PREFIX,
-                context.next_match_expression_return_var_unique_suffix(),
-            );
-            let var_decl_name =
-                Ident::new_with_override(match_return_var_name, var_decl_span.clone());
-
-            let var_decl_exp = Expression {
-                kind: ExpressionKind::Variable(var_decl_name.clone()),
-                span: var_decl_span,
-            };
             let branches = {
                 branches
                     .into_inner()
@@ -1972,44 +1956,8 @@ fn expr_to_expression(
                     })
                     .collect::<Result<_, _>>()?
             };
-            Expression {
-                kind: ExpressionKind::CodeBlock(CodeBlock {
-                    contents: vec![
-                        AstNode {
-                            content: AstNodeContent::Declaration(Declaration::VariableDeclaration(
-                                VariableDeclaration {
-                                    type_ascription: {
-                                        let type_id =
-                                            engines.te().insert(engines, TypeInfo::Unknown);
-                                        TypeArgument {
-                                            type_id,
-                                            initial_type_id: type_id,
-                                            span: var_decl_name.span(),
-                                            call_path_tree: None,
-                                        }
-                                    },
-                                    name: var_decl_name,
-                                    is_mutable: false,
-                                    body: value,
-                                },
-                            )),
-                            span: span.clone(),
-                        },
-                        AstNode {
-                            content: AstNodeContent::ImplicitReturnExpression(Expression {
-                                kind: ExpressionKind::Match(MatchExpression {
-                                    value: Box::new(var_decl_exp),
-                                    branches,
-                                }),
-                                span: span.clone(),
-                            }),
-                            span: span.clone(),
-                        },
-                    ],
-                    whole_block_span: span.clone(),
-                }),
-                span,
-            }
+
+            match_expr_to_expression(context, handler, engines, *value, branches, span)?
         }
         Expr::While {
             condition, block, ..
@@ -2787,18 +2735,76 @@ fn if_expr_to_expression(
                     }
                 }
             });
-            Expression {
-                kind: ExpressionKind::Match(MatchExpression {
-                    value: Box::new(expr_to_expression(context, handler, engines, *rhs)?),
-                    branches,
-                }),
-                span,
-            }
+
+            match_expr_to_expression(context, handler, engines, *rhs, branches, span)?
         }
     };
     Ok(expression)
 }
 
+fn match_expr_to_expression(
+    context: &mut Context,
+    handler: &Handler,
+    engines: &Engines,
+    value: Expr,
+    branches: Vec<MatchBranch>,
+    span: Span,
+) -> Result<Expression, ErrorEmitted> {
+    let value = expr_to_expression(context, handler, engines, value)?;
+    let var_decl_span = value.span();
+
+    // Generate a deterministic name for the variable returned by the match expression.
+    let match_return_var_name = format!(
+        "{}{}",
+        MATCH_RETURN_VAR_NAME_PREFIX,
+        context.next_match_expression_return_var_unique_suffix(),
+    );
+    let var_decl_name = Ident::new_with_override(match_return_var_name, var_decl_span.clone());
+
+    let var_decl_exp = Expression {
+        kind: ExpressionKind::Variable(var_decl_name.clone()),
+        span: var_decl_span,
+    };
+
+    Ok(Expression {
+        kind: ExpressionKind::CodeBlock(CodeBlock {
+            contents: vec![
+                AstNode {
+                    content: AstNodeContent::Declaration(Declaration::VariableDeclaration(
+                        VariableDeclaration {
+                            type_ascription: {
+                                let type_id = engines.te().insert(engines, TypeInfo::Unknown);
+                                TypeArgument {
+                                    type_id,
+                                    initial_type_id: type_id,
+                                    span: var_decl_name.span(),
+                                    call_path_tree: None,
+                                }
+                            },
+                            name: var_decl_name,
+                            is_mutable: false,
+                            body: value,
+                        },
+                    )),
+                    span: span.clone(),
+                },
+                AstNode {
+                    content: AstNodeContent::ImplicitReturnExpression(Expression {
+                        kind: ExpressionKind::Match(MatchExpression {
+                            value: Box::new(var_decl_exp),
+                            branches,
+                        }),
+                        span: span.clone(),
+                    }),
+                    span: span.clone(),
+                },
+            ],
+            whole_block_span: span.clone(),
+        }),
+        span,
+    })
+}
+
 /// Determine if the path is in absolute form, e.g., `::foo::bar`.
 ///
 /// Throws an error when given `<Foo as Bar>::baz`.

diff --git a/sway-lsp/tests/lib.rs b/sway-lsp/tests/lib.rs
@@ -266,7 +266,8 @@ async fn go_to_definition_for_matches() {
     lsp::definition_check(&server, &go_to);
     lsp::definition_check_with_req_offset(&server, &mut go_to, 19, 18);
     lsp::definition_check_with_req_offset(&server, &mut go_to, 22, 18);
-    lsp::definition_check_with_req_offset(&server, &mut go_to, 22, 30);
+    // TODO: Enable the below check once this issue is fixed: https://github.com/FuelLabs/sway/issues/5221
+    // lsp::definition_check_with_req_offset(&server, &mut go_to, 22, 30);
     lsp::definition_check_with_req_offset(&server, &mut go_to, 23, 16);
     lsp::definition_check_with_req_offset(&server, &mut go_to, 28, 38);
 

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/.gitignore b/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/.gitignore
@@ -0,0 +1,2 @@
+out
+target
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/Forc.lock
@@ -0,0 +1,16 @@
+[[package]]
+name = "core"
+source = "path+from-root-AF27191E41C715AD"
+
+[[package]]
+name = "if_let_no_side_effects"
+source = "member"
+dependencies = [
+    "core",
+    "std",
+]
+
+[[package]]
+name = "std"
+source = "path+from-root-AF27191E41C715AD"
+dependencies = ["core"]
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/Forc.toml
@@ -0,0 +1,9 @@
+[project]
+authors = ["Fuel Labs <contact@fuel.sh>"]
+entry = "main.sw"
+license = "Apache-2.0"
+name = "if_let_no_side_effects"
+
+[dependencies]
+core = { path = "../../../../../../../sway-lib-core" }
+std = { path = "../../../../../../../sway-lib-std" }
diff --git a/...e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/json_abi_oracle.json b/...e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/json_abi_oracle.json
@@ -0,0 +1,25 @@
+{
+  "configurables": [],
+  "functions": [
+    {
+      "attributes": null,
+      "inputs": [],
+      "name": "main",
+      "output": {
+        "name": "",
+        "type": 0,
+        "typeArguments": null
+      }
+    }
+  ],
+  "loggedTypes": [],
+  "messagesTypes": [],
+  "types": [
+    {
+      "components": null,
+      "type": "u64",
+      "typeId": 0,
+      "typeParameters": null
+    }
+  ]
+}
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/src/main.sw
@@ -0,0 +1,26 @@
+// Test that proves that this issue is fixed: https://github.com/FuelLabs/sway/issues/5173
+script;
+
+struct Struct {
+  x: u64,
+  y: u64,
+  z: u64
+}
+
+fn test_inc(ref mut i: u64) -> Struct {
+    i = i + 11;
+
+    Struct { x: 1111, y: 2222, z: 333 }
+}
+
+fn main() -> u64 {
+    let mut i = 0;
+
+    if let Struct { x, y, z: 0 } = test_inc(i) {
+        let a = x + y;
+        assert(a == 3333);
+    };
+    assert(i == 11);
+
+    i
+}
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/test.toml
@@ -0,0 +1,3 @@
+category = "run"
+expected_result = { action = "return", value = 11 }
+validate_abi = true
diff --git a/test/src/ir_generation/tests/enum_struct_string.sw b/test/src/ir_generation/tests/enum_struct_string.sw
@@ -27,10 +27,13 @@ fn main() -> u64 {
 // check: local { u64, ( { { string<17>, u64 }, u64, bool } ) } b_val
 
 // check: get_local ptr { u64, ( { { string<17>, u64 }, u64, bool } ) }, b_val
-
 // check: $(b_val_var=$VAL) = get_local ptr { u64, ( { { string<17>, u64 }, u64, bool } ) }, b_val
+
+// check: get_local ptr { u64, ( { { string<17>, u64 }, u64, bool } ) }, __match_return_var_name_1
+// check: $(match_val_var=$VAL) = get_local ptr { u64, ( { { string<17>, u64 }, u64, bool } ) }, __match_return_var_name_1
+
 // check: $(idx_val=$VAL) = const u64 0
-// check: $(tag_ptr=$VAL) = get_elem_ptr $b_val_var, ptr u64, $idx_val
+// check: $(tag_ptr=$VAL) = get_elem_ptr $match_val_var, ptr u64, $idx_val
 // check: $(b_val_tag=$VAL) = load $tag_ptr
 // check: $(zero=$VAL) = const u64 0
 // check: $(tag_matches=$VAL) = call $(eq_fn=$ID)($b_val_tag, $zero)

diff --git a/test/src/ir_generation/tests/if_let_simple.sw b/test/src/ir_generation/tests/if_let_simple.sw
@@ -19,23 +19,25 @@ fn main() -> u64 {
 // check: local { u64, ( bool | u64 ) } thing
 
 // check: get_local ptr { u64, ( bool | u64 ) }, thing
-
 // check: $(thing_var=$VAL) = get_local ptr { u64, ( bool | u64 ) }, thing
 
+// check: get_local ptr { u64, ( bool | u64 ) }, __match_return_var_name_1
+// check: $(match_var=$VAL) = get_local ptr { u64, ( bool | u64 ) }, __match_return_var_name_1
+
 // check: $(idx_val=$VAL) = const u64 0
-// check: $(tag_ptr=$VAL) = get_elem_ptr $thing_var, ptr u64, $idx_val
-// check: $(thing_tag=$VAL) = load $tag_ptr
+// check: $(tag_ptr=$VAL) = get_elem_ptr $match_var, ptr u64, $idx_val
+// check: $(match_tag=$VAL) = load $tag_ptr
 
 // check: $(one=$VAL) = const u64 1
-// check: $(tags_match=$VAL) = call $(eq_fn=$ID)($thing_tag, $one)
+// check: $(tags_match=$VAL) = call $(eq_fn=$ID)($match_tag, $one)
 // check: cbr $tags_match, $(block0=$ID)(), $(block1=$ID)()
 
 // check: $block0():
-// check: $(thing_var=$VAL) = get_local ptr { u64, ( bool | u64 ) }, thing
+// check: $(match_var=$VAL) = get_local ptr { u64, ( bool | u64 ) }, __match_return_var_name_1
 
 // check: $(idx_1_a=$VAL) = const u64 1
 // check: $(idx_1_b=$VAL) = const u64 1
-// check: $(variant_ptr=$VAL) = get_elem_ptr $thing_var, ptr u64, $idx_1_a, $idx_1_b
+// check: $(variant_ptr=$VAL) = get_elem_ptr $match_var, ptr u64, $idx_1_a, $idx_1_b
 // check: $(thing_variant_val=$VAL) = load $variant_ptr
 
 // check: $(n_var=$VAL) = get_local ptr u64, n