From 17e434f17c2f879838ac0da0078338db53787d23 Mon Sep 17 00:00:00 2001 From: Christoph Burgdorf Date: Fri, 6 Aug 2021 13:49:29 +0200 Subject: [PATCH] Reject certain code that should return but doesn't Fixes #497 --- crates/analyzer/src/traversal/functions.rs | 2 +- crates/analyzer/tests/errors.rs | 1 + .../snapshots/errors__missing_return_after_if.snap | 14 ++++++++++++++ .../snapshots/errors__missing_return_in_else.snap | 6 ------ .../compile_errors/missing_return_after_if.fe | 4 ++++ .../compile_errors/missing_return_in_else.fe | 2 +- crates/test-files/src/lib.rs | 4 ++-- newsfragments/497.bugfix.md | 14 ++++++++++++++ 8 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 crates/analyzer/tests/snapshots/errors__missing_return_after_if.snap create mode 100644 crates/test-files/fixtures/compile_errors/missing_return_after_if.fe create mode 100644 newsfragments/497.bugfix.md diff --git a/crates/analyzer/src/traversal/functions.rs b/crates/analyzer/src/traversal/functions.rs index 821bc310f2..5744cc5ce2 100644 --- a/crates/analyzer/src/traversal/functions.rs +++ b/crates/analyzer/src/traversal/functions.rs @@ -174,7 +174,7 @@ fn all_paths_return_or_revert(block: &[Node]) -> bool { or_else, } => { let body_returns = all_paths_return_or_revert(body); - let or_else_returns = or_else.is_empty() || all_paths_return_or_revert(or_else); + let or_else_returns = !or_else.is_empty() && all_paths_return_or_revert(or_else); if body_returns && or_else_returns { return true; } diff --git a/crates/analyzer/tests/errors.rs b/crates/analyzer/tests/errors.rs index e26735421e..c45aa62bb9 100644 --- a/crates/analyzer/tests/errors.rs +++ b/crates/analyzer/tests/errors.rs @@ -180,6 +180,7 @@ test_file! { issue_451 } test_file! { mismatch_return_type } test_file! { missing_return } test_file! { missing_return_in_else } +test_file! { missing_return_after_if } test_file! { needs_mem_copy } test_file! { not_callable } test_file! { not_in_scope } diff --git a/crates/analyzer/tests/snapshots/errors__missing_return_after_if.snap b/crates/analyzer/tests/snapshots/errors__missing_return_after_if.snap new file mode 100644 index 0000000000..5d2273b096 --- /dev/null +++ b/crates/analyzer/tests/snapshots/errors__missing_return_after_if.snap @@ -0,0 +1,14 @@ +--- +source: crates/analyzer/tests/errors.rs +expression: "error_string(&path, &src)" + +--- +error: function body is missing a return or revert statement + ┌─ compile_errors/missing_return_after_if.fe:2:13 + │ +2 │ pub def bar(val: u256) -> u256: + │ ^^^ ---- expected function to return `u256` + │ │ + │ all paths of this function must `return` or `revert` + + diff --git a/crates/analyzer/tests/snapshots/errors__missing_return_in_else.snap b/crates/analyzer/tests/snapshots/errors__missing_return_in_else.snap index 6abaec2a86..b78dcefc56 100644 --- a/crates/analyzer/tests/snapshots/errors__missing_return_in_else.snap +++ b/crates/analyzer/tests/snapshots/errors__missing_return_in_else.snap @@ -11,10 +11,4 @@ error: function body is missing a return or revert statement │ │ │ all paths of this function must `return` or `revert` -error: cannot find value `x` in this scope - ┌─ compile_errors/missing_return_in_else.fe:6:13 - │ -6 │ x = 1 - │ ^ undefined - diff --git a/crates/test-files/fixtures/compile_errors/missing_return_after_if.fe b/crates/test-files/fixtures/compile_errors/missing_return_after_if.fe new file mode 100644 index 0000000000..fa43304f50 --- /dev/null +++ b/crates/test-files/fixtures/compile_errors/missing_return_after_if.fe @@ -0,0 +1,4 @@ +contract Foo: + pub def bar(val: u256) -> u256: + if val > 1: + return 5 diff --git a/crates/test-files/fixtures/compile_errors/missing_return_in_else.fe b/crates/test-files/fixtures/compile_errors/missing_return_in_else.fe index 392f794d98..53809c2638 100644 --- a/crates/test-files/fixtures/compile_errors/missing_return_in_else.fe +++ b/crates/test-files/fixtures/compile_errors/missing_return_in_else.fe @@ -3,4 +3,4 @@ contract Foo: if val > 1: return 5 else: - x = 1 + x:u256 = 1 diff --git a/crates/test-files/src/lib.rs b/crates/test-files/src/lib.rs index 817a07d874..8697870910 100644 --- a/crates/test-files/src/lib.rs +++ b/crates/test-files/src/lib.rs @@ -5,7 +5,7 @@ const FIXTURES: Dir = include_dir!("fixtures"); pub fn fixture(path: &str) -> &'static str { FIXTURES .get_file(path) - .expect("bad fixture file path") + .unwrap_or_else(|| panic!("bad fixture file path {}", path)) .contents_utf8() .expect("fixture file isn't utf8") } @@ -13,6 +13,6 @@ pub fn fixture(path: &str) -> &'static str { pub fn fixture_bytes(path: &str) -> &'static [u8] { FIXTURES .get_file(path) - .expect("bad fixture file path") + .unwrap_or_else(|| panic!("bad fixture file path {}", path)) .contents() } diff --git a/newsfragments/497.bugfix.md b/newsfragments/497.bugfix.md new file mode 100644 index 0000000000..64507e05dd --- /dev/null +++ b/newsfragments/497.bugfix.md @@ -0,0 +1,14 @@ +Fixed an issue with a missing return statement not properly detected. + +Previous to this fix, the following code compiles but it should not: + +``` +contract Foo: + pub def bar(val: u256) -> u256: + if val > 1: + return 5 +``` + +With this change, the compiler rightfully detects that the code is missing +a `return` or `revert` statement after the `if` statement since it is not +guaranteed that the path of execution always follows the arm of the `if` statement.