From 1cc07404ec52ae56d9505c145f8a1878b2084d1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Ron=C4=8Devi=C4=87?= Date: Tue, 24 Oct 2023 17:41:22 +0200 Subject: [PATCH] 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. --- .../to_parsed_lang/convert_parse_tree.rs | 128 +++++++++--------- sway-lsp/tests/lib.rs | 3 +- .../if_let_no_side_effects/.gitignore | 2 + .../language/if_let_no_side_effects/Forc.lock | 16 +++ .../language/if_let_no_side_effects/Forc.toml | 9 ++ .../json_abi_oracle.json | 25 ++++ .../if_let_no_side_effects/src/main.sw | 26 ++++ .../language/if_let_no_side_effects/test.toml | 3 + .../ir_generation/tests/enum_struct_string.sw | 7 +- test/src/ir_generation/tests/if_let_simple.sw | 14 +- 10 files changed, 163 insertions(+), 70 deletions(-) create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/.gitignore create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/json_abi_oracle.json create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/test.toml 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 index 8fdd686193b..90ca1747c37 100644 --- 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::>()? }; - 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, + span: Span, +) -> Result { + 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 `::baz`. diff --git a/sway-lsp/tests/lib.rs b/sway-lsp/tests/lib.rs index de50f86e179..3c36338d4d0 100644 --- 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 new file mode 100644 index 00000000000..77d3844f58c --- /dev/null +++ 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 new file mode 100644 index 00000000000..b728e25b5b8 --- /dev/null +++ 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 new file mode 100644 index 00000000000..495e2e8b3cb --- /dev/null +++ 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 "] +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/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/json_abi_oracle.json b/test/src/e2e_vm_tests/test_programs/should_pass/language/if_let_no_side_effects/json_abi_oracle.json new file mode 100644 index 00000000000..ad50b55d54c --- /dev/null +++ b/test/src/e2e_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 + } + ] +} \ No newline at end of file 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 new file mode 100644 index 00000000000..90bfdc1faf1 --- /dev/null +++ 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 +} \ No newline at end of file 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 new file mode 100644 index 00000000000..05519dd9fad --- /dev/null +++ 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 index 933dc180074..ca6b362f0d1 100644 --- 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 index 9d79c83ede9..85d28d10310 100644 --- 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