Implements DCA warnings for function parameters. (FuelLabs#4406)

## Description With these changes we will have DCA warnings for function parameters. This PR also fixes tests that have unused function parameters. ## Checklist - [ ] 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. - [x] 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. --------- Co-authored-by: Mohammad Fawaz <[email protected]>
aliali1215 · Apr 12, 2023 · dcb7917 · dcb7917
1 parent cd936a5
commit dcb7917
Show file tree

Hide file tree

Showing 30 changed files with 231 additions and 83 deletions.
diff --git a/sway-core/src/control_flow_analysis/dead_code_analysis.rs b/sway-core/src/control_flow_analysis/dead_code_analysis.rs
@@ -41,26 +41,6 @@ impl<'cfg> ControlFlowGraph<'cfg> {
         }
 
         let is_dead_check = |n: &NodeIndex| {
-            if let ControlFlowGraphNode::ProgramNode {
-                node:
-                    ty::TyAstNode {
-                        content: ty::TyAstNodeContent::Declaration(ty::TyDecl::VariableDecl { .. }),
-                        ..
-                    },
-                ..
-            } = &self.graph[*n]
-            {
-                // Consider variables declarations dead when count is not greater than 1
-                connections_count
-                    .get(n)
-                    .cloned()
-                    .map_or(true, |count| count <= 1)
-            } else {
-                false
-            }
-        };
-
-        let is_alive_check = |n: &NodeIndex| {
             match &self.graph[*n] {
                 ControlFlowGraphNode::ProgramNode {
                     node:
@@ -71,11 +51,64 @@ impl<'cfg> ControlFlowGraph<'cfg> {
                         },
                     ..
                 } => {
-                    // Consider variables declarations alive when count is greater than 1
+                    // Consider variables declarations dead when count is not greater than 1
                     connections_count
                         .get(n)
                         .cloned()
-                        .map_or(false, |count| count > 1)
+                        .map_or(true, |count| count <= 1)
+                }
+                ControlFlowGraphNode::FunctionParameter { .. } => {
+                    // Consider variables declarations dead when count is not greater than 1
+                    // Function param always has the function pointing to them
+                    connections_count
+                        .get(n)
+                        .cloned()
+                        .map_or(true, |count| count <= 1)
+                }
+                _ => false,
+            }
+        };
+
+        let is_alive_check = |n: &NodeIndex| {
+            match &self.graph[*n] {
+                ControlFlowGraphNode::ProgramNode {
+                    node:
+                        ty::TyAstNode {
+                            content:
+                                ty::TyAstNodeContent::Declaration(ty::TyDecl::VariableDecl(decl)),
+                            ..
+                        },
+                    ..
+                } => {
+                    if decl.name.as_str().starts_with('_') {
+                        true
+                    } else {
+                        // Consider variables declarations alive when count is greater than 1
+                        // This is explicilty required because the variable may be considered dead
+                        // when it is not connected from an entry point, while it may still be used by other dead code.
+                        connections_count
+                            .get(n)
+                            .cloned()
+                            .map_or(false, |count| count > 1)
+                    }
+                }
+                ControlFlowGraphNode::FunctionParameter {
+                    param_name,
+                    is_self,
+                    ..
+                } => {
+                    if *is_self || param_name.as_str().starts_with('_') {
+                        // self type parameter is always alive
+                        true
+                    } else {
+                        // Consider param alive when count is greater than 1
+                        // This is explicilty required because the param may be considered dead
+                        // when it is not connected from an entry point, while it may still be used by other dead code.
+                        connections_count
+                            .get(n)
+                            .cloned()
+                            .map_or(true, |count| count > 1)
+                    }
                 }
                 ControlFlowGraphNode::ProgramNode {
                     node:
@@ -189,6 +222,12 @@ impl<'cfg> ControlFlowGraph<'cfg> {
                             })
                         }
                         ControlFlowGraphNode::OrganizationalDominator(..) => None,
+                        ControlFlowGraphNode::FunctionParameter { param_name, .. } => {
+                            Some(CompileWarning {
+                                span: param_name.span(),
+                                warning_content: Warning::DeadDeclaration,
+                            })
+                        }
                     }
                 }
             })
@@ -851,6 +890,26 @@ fn connect_typed_fn_decl<'eng: 'cfg, 'cfg>(
     options: NodeConnectionOptions,
 ) -> Result<(), CompileError> {
     let type_engine = engines.te();
+
+    graph.namespace.push_code_block();
+    for fn_param in fn_decl.parameters.iter() {
+        let fn_param_node = graph.add_node(ControlFlowGraphNode::FunctionParameter {
+            param_name: fn_param.name.clone(),
+            is_self: matches!(
+                type_engine.get(fn_param.type_argument.initial_type_id),
+                TypeInfo::SelfType
+            ),
+        });
+        graph.add_edge(entry_node, fn_param_node, "".into());
+
+        graph.namespace.insert_variable(
+            fn_param.name.clone(),
+            VariableNamespaceEntry {
+                variable_decl_ix: fn_param_node,
+            },
+        )
+    }
+
     let fn_exit_node = graph.add_node(format!("\"{}\" fn exit", fn_decl.name.as_str()).into());
     let (_exit_nodes, _exit_node) = depth_first_insertion_code_block(
         engines,
@@ -864,6 +923,8 @@ fn connect_typed_fn_decl<'eng: 'cfg, 'cfg>(
             parent_node: Some(entry_node),
         },
     )?;
+    graph.namespace.pop_code_block();
+
     if let Some(exit_node) = exit_node {
         graph.add_edge(fn_exit_node, exit_node, "".into());
     }
@@ -1674,17 +1735,27 @@ fn connect_expression<'eng: 'cfg, 'cfg>(
             }
             Ok(vec![])
         }
-        Reassignment(typed_reassignment) => connect_expression(
-            engines,
-            &typed_reassignment.rhs.expression,
-            graph,
-            leaves,
-            exit_node,
-            "variable reassignment",
-            tree_type,
-            typed_reassignment.rhs.clone().span,
-            options,
-        ),
+        Reassignment(typed_reassignment) => {
+            if let Some(variable_entry) = graph
+                .namespace
+                .get_variable(&typed_reassignment.lhs_base_name)
+            {
+                for leaf in leaves {
+                    graph.add_edge(*leaf, variable_entry.variable_decl_ix, "".into());
+                }
+            }
+            connect_expression(
+                engines,
+                &typed_reassignment.rhs.expression,
+                graph,
+                leaves,
+                exit_node,
+                "variable reassignment",
+                tree_type,
+                typed_reassignment.rhs.clone().span,
+                options,
+            )
+        }
         StorageReassignment(typed_storage_reassignment) => connect_expression(
             engines,
             &typed_storage_reassignment.rhs.expression,
@@ -1907,9 +1978,6 @@ fn construct_dead_code_warning_from_node(
             content: ty::TyAstNodeContent::Declaration(ty::TyDecl::VariableDecl(decl)),
             span,
         } => {
-            if decl.name.as_str().starts_with('_') {
-                return None;
-            }
             // In rare cases, variable declaration spans don't have a path, so we need to check for that
             if decl.name.span().path().is_some() {
                 CompileWarning {
@@ -2087,5 +2155,6 @@ fn allow_dead_code_node(
         }
         ControlFlowGraphNode::StorageField { .. } => false,
         ControlFlowGraphNode::OrganizationalDominator(..) => false,
+        ControlFlowGraphNode::FunctionParameter { .. } => false,
     }
 }
diff --git a/sway-core/src/control_flow_analysis/flow_graph/mod.rs b/sway-core/src/control_flow_analysis/flow_graph/mod.rs
@@ -81,6 +81,10 @@ pub enum ControlFlowGraphNode<'cfg> {
     StorageField {
         field_name: Ident,
     },
+    FunctionParameter {
+        param_name: Ident,
+        is_self: bool,
+    },
 }
 
 impl<'cfg> GetDeclIdent for ControlFlowGraphNode<'cfg> {
@@ -96,6 +100,7 @@ impl<'cfg> GetDeclIdent for ControlFlowGraphNode<'cfg> {
                 struct_field_name, ..
             } => Some(struct_field_name.clone()),
             ControlFlowGraphNode::StorageField { field_name, .. } => Some(field_name.clone()),
+            ControlFlowGraphNode::FunctionParameter { param_name, .. } => Some(param_name.clone()),
         }
     }
 }
@@ -156,6 +161,9 @@ impl<'cfg> DebugWithEngines for ControlFlowGraphNode<'cfg> {
             ControlFlowGraphNode::StorageField { field_name } => {
                 format!("Storage field {}", field_name.as_str())
             }
+            ControlFlowGraphNode::FunctionParameter { param_name, .. } => {
+                format!("Function param {}", param_name.as_str())
+            }
         };
         f.write_str(&text)
     }
@@ -297,6 +305,7 @@ impl<'cfg> ControlFlowGraphNode<'cfg> {
                 struct_field_name, ..
             } => Some(struct_field_name.span()),
             ControlFlowGraphNode::StorageField { field_name } => Some(field_name.span()),
+            ControlFlowGraphNode::FunctionParameter { param_name, .. } => Some(param_name.span()),
         }
     }
 }
diff --git a/sway-lib-core/src/never.sw b/sway-lib-core/src/never.sw
@@ -50,16 +50,16 @@ impl Not for Never {
 }
 
 impl Eq for Never {
-    fn eq(self, other: Self) -> bool {
+    fn eq(self, _other: Self) -> bool {
         self
     }
 }
 
 impl Ord for Never {
-    fn gt(self, other: Self) -> bool {
+    fn gt(self, _other: Self) -> bool {
         self
     }
-    fn lt(self, other: Self) -> bool {
+    fn lt(self, _other: Self) -> bool {
         self
     }
 }
diff --git a/sway-lib-std/src/tx.sw b/sway-lib-std/src/tx.sw
@@ -111,7 +111,7 @@ pub fn tx_witnesses_count() -> u64 {
 
 // Get a pointer to the witness at index `index` for either `tx_type`
 /// (transaction-script or transaction-create).
-pub fn tx_witness_pointer(index: u64) -> u64 {
+pub fn tx_witness_pointer(_index: u64) -> u64 {
     match tx_type() {
         Transaction::Script => __gtf::<u64>(0, GTF_SCRIPT_WITNESS_AT_INDEX),
         Transaction::Create => __gtf::<u64>(0, GTF_CREATE_WITNESS_AT_INDEX),

diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/simple_generics/test.toml b/test/src/e2e_vm_tests/test_programs/should_fail/simple_generics/test.toml
@@ -1,5 +1,9 @@
 category = "fail"
 
+# check: fn generic<T>(input: T) -> T {
+# nextln: $()This declaration is never used.
+# nextln: $()do_it(input)
+
 # check: do_it(input)
 # nextln: $()Mismatched types.
 # nextln: $()expected: u64

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/Forc.lock
@@ -0,0 +1,3 @@
+[[package]]
+name = 'func_param'
+source = 'member'
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/Forc.toml
@@ -0,0 +1,6 @@
+[project]
+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+name = "func_param"
+implicit-std = false
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/src/main.sw
@@ -0,0 +1,38 @@
+script;
+
+
+// Param `i` should have no warnings as `d` is still using it
+fn unused_fn(i: u64) {
+    let d = i;
+}
+
+fn f(i: u64) {
+}
+
+struct A {}
+
+impl A {
+    fn g(i: u64) {
+    }
+
+    fn h(self, i: u64) {
+    }
+}
+
+fn i(_p: u64) {
+}
+
+fn j(ref mut foo: u64) {
+    foo = 42;
+}
+
+fn main() {
+    f(42);
+    A::g(42);
+    let a = A{};
+    a.h(42);
+    i(42);
+
+    let mut foo = 42;
+    j(foo);
+}
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/dca/func_param/test.toml
@@ -0,0 +1,19 @@
+category = "compile"
+
+# check: $()fn unused_fn(i: u64) {
+# nextln: $()This function is never called.
+# nextln: $()let d = i;
+
+# check: $()let d = i;
+# nextln: $()This declaration is never used.
+
+# check: $()fn f(i: u64) {
+# nextln: $()This declaration is never used.
+
+# check: $()fn g(i: u64) {
+# nextln: $()This declaration is never used.
+
+# check: $()fn h(self, i: u64) {
+# nextln: $()This declaration is never used.
+
+expected_warnings = 5
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/generic_fn_trait_contraint/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/dca/generic_fn_trait_contraint/src/main.sw
@@ -5,7 +5,7 @@ pub trait MyEq {
 }
 
 impl MyEq for u64 {
-    fn my_eq(self, other: Self) {
+    fn my_eq(self, _other: Self) {
     }
 }
 

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/main.sw
@@ -8,7 +8,7 @@ struct Bar {
     value: u64
 }
 
-fn internal_fn(s: Bar) {
+fn internal_fn(_s: Bar) {
 
 }
 

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/utils.sw b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/utils.sw
@@ -4,6 +4,6 @@ pub struct Foo {
     value: u64
 }
 
-pub fn external_fn(s: Foo) {
+pub fn external_fn(_s: Foo) {
 
 }
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_trait/src/utils.sw b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_trait/src/utils.sw
@@ -2,6 +2,6 @@ library;
 
 use ::r#trait::Trait;
 
-pub fn uses_trait<T>(a: T) where T: Trait {
+pub fn uses_trait<T>(_a: T) where T: Trait {
 
 }
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/basic_func_decl/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/basic_func_decl/src/main.sw
@@ -41,6 +41,6 @@ struct MyStructWithEnum {
     b: MyEnum,
 }
 
-fn eight_args(a: MyStruct, b: MyEnum, c: MyStructWithEnum, d: str[5], e: bool, f: u64, g: u8, h: b256) {
+fn eight_args(_a: MyStruct, _b: MyEnum, _c: MyStructWithEnum, _d: str[5], _e: bool, _f: u64, _g: u8, _h: b256) {
     return;
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -5,7 +5,7 @@ pub trait MyEq { @@
     }
     impl MyEq for u64 {
-        fn my_eq(self, other: Self) {
+        fn my_eq(self, _other: Self) {
         }
     }
@@ Expand Down @@