From f8d22d20837f94a25e7f838c00a2c46b877b8df2 Mon Sep 17 00:00:00 2001 From: Anton Trunov Date: Wed, 23 Nov 2022 19:05:15 +0400 Subject: [PATCH] Improve warnings on unnecessary storage annotations (#3414) Fixes #1183: just trying to improve the error message. This PR also provides a fix for the following issue: the purity analyzer did not report the case where *both* read and write storage annotations were unused, i.e. in this case there were no warnings: ``` contract; abi MyContract { #[storage(read, write)] fn foo() -> u64; } impl MyContract for Contract { #[storage(read, write)] fn foo() -> u64 { 0 } } ``` --- sway-core/src/ir_generation/purity.rs | 8 ++++++-- sway-error/src/warning.rs | 4 ++-- .../storage_annotations_unused_read/Forc.lock | 13 +++++++++++++ .../storage_annotations_unused_read/Forc.toml | 8 ++++++++ .../storage_annotations_unused_read/src/main.sw | 13 +++++++++++++ .../storage_annotations_unused_read/test.toml | 3 +++ .../Forc.lock | 13 +++++++++++++ .../Forc.toml | 8 ++++++++ .../src/main.sw | 13 +++++++++++++ .../test.toml | 3 +++ .../storage_annotations_unused_write/Forc.lock | 13 +++++++++++++ .../storage_annotations_unused_write/Forc.toml | 8 ++++++++ .../storage_annotations_unused_write/src/main.sw | 13 +++++++++++++ .../storage_annotations_unused_write/test.toml | 3 +++ 14 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/test.toml diff --git a/sway-core/src/ir_generation/purity.rs b/sway-core/src/ir_generation/purity.rs index ddd4bb2b4c8..f35be43c9a5 100644 --- a/sway-core/src/ir_generation/purity.rs +++ b/sway-core/src/ir_generation/purity.rs @@ -118,11 +118,15 @@ pub(crate) fn check_function_purity( // Or we have unneeded attributes. (Some(StorageOperation::ReadsWrites), false, true) => warn(span, Reads), (Some(StorageOperation::ReadsWrites), true, false) => warn(span, Writes), + (Some(StorageOperation::ReadsWrites), false, false) => warn(span, ReadsWrites), (Some(StorageOperation::Reads), false, false) => warn(span, Reads), (Some(StorageOperation::Writes), false, false) => warn(span, Writes), - // (Pure, false, false) is OK, as is (ReadsWrites, true, true). - _ => (), + // Attributes and effects are in total agreement + (None, false, false) + | (Some(StorageOperation::Reads), true, false) + | (Some(StorageOperation::Writes), false, true) + | (Some(StorageOperation::ReadsWrites), true, true) => (), }; (reads, writes) diff --git a/sway-error/src/warning.rs b/sway-error/src/warning.rs index 1e7ba4bbc15..b620b5fd034 100644 --- a/sway-error/src/warning.rs +++ b/sway-error/src/warning.rs @@ -217,8 +217,8 @@ impl fmt::Display for Warning { ), DeadStorageDeclarationForFunction { unneeded_attrib } => write!( f, - "The '{unneeded_attrib}' storage declaration for this function is never accessed \ - and can be removed." + "This function's storage attributes declaration does not match its \ + actual storage access pattern: '{unneeded_attrib}' attribute(s) can be removed." ), MatchExpressionUnreachableArm => write!(f, "This match arm is unreachable."), UnrecognizedAttribute {attrib_name} => write!(f, "Unknown attribute: \"{attrib_name}\"."), diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.lock new file mode 100644 index 00000000000..503da21dd99 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'core' +source = 'path+from-root-B415B1E0A318C0B0' + +[[package]] +name = 'std' +source = 'path+from-root-B415B1E0A318C0B0' +dependencies = ['core'] + +[[package]] +name = 'storage_annotations_unused_read' +source = 'member' +dependencies = ['std'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.toml new file mode 100644 index 00000000000..1254e80ca74 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "storage_annotations_unused_read" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/src/main.sw new file mode 100644 index 00000000000..7b1253ab718 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/src/main.sw @@ -0,0 +1,13 @@ +contract; + +abi MyContract { + #[storage(read)] + fn foo() -> u64; +} + +impl MyContract for Contract { + #[storage(read)] + fn foo() -> u64 { + 0 + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/test.toml new file mode 100644 index 00000000000..af4da3306ff --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read/test.toml @@ -0,0 +1,3 @@ +category = "compile" + +# check: $()This function's storage attributes declaration does not match this function's actual storage access pattern: 'read' attribute(s) can be removed. diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.lock new file mode 100644 index 00000000000..76bc0df22c2 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'core' +source = 'path+from-root-F9F255B625A3400C' + +[[package]] +name = 'std' +source = 'path+from-root-F9F255B625A3400C' +dependencies = ['core'] + +[[package]] +name = 'storage_annotations_unused_read_and_write' +source = 'member' +dependencies = ['std'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.toml new file mode 100644 index 00000000000..6cc164b7d69 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "storage_annotations_unused_read_and_write" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/src/main.sw new file mode 100644 index 00000000000..db89a200c2b --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/src/main.sw @@ -0,0 +1,13 @@ +contract; + +abi MyContract { + #[storage(read, write)] + fn foo() -> u64; +} + +impl MyContract for Contract { + #[storage(read, write)] + fn foo() -> u64 { + 0 + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/test.toml new file mode 100644 index 00000000000..cc3c6c881a7 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_read_and_write/test.toml @@ -0,0 +1,3 @@ +category = "compile" + +# check: $()This function's storage attributes declaration does not match this function's actual storage access pattern: 'read, write' attribute(s) can be removed. diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.lock new file mode 100644 index 00000000000..9dd9a2c1565 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'core' +source = 'path+from-root-4A37DED755D546B6' + +[[package]] +name = 'std' +source = 'path+from-root-4A37DED755D546B6' +dependencies = ['core'] + +[[package]] +name = 'storage_annotations_unused_write' +source = 'member' +dependencies = ['std'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.toml new file mode 100644 index 00000000000..c9e09db82e1 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "storage_annotations_unused_write" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/src/main.sw new file mode 100644 index 00000000000..e1b71bee061 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/src/main.sw @@ -0,0 +1,13 @@ +contract; + +abi MyContract { + #[storage(write)] // Or any other storage annotation + fn foo() -> u64; +} + +impl MyContract for Contract { + #[storage(write)] // Or any other storage annotation + fn foo() -> u64 { + 0 + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/test.toml new file mode 100644 index 00000000000..fc9cac58427 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/storage_annotations_unused_write/test.toml @@ -0,0 +1,3 @@ +category = "compile" + +# check: $()This function's storage attributes declaration does not match this function's actual storage access pattern: 'write' attribute(s) can be removed.