Do better analysis for unused code after return/throw

slackhq · May 10, 2024 · 01b3fdc · 01b3fdc
1 parent 25d650e
commit 01b3fdc
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 86 deletions.
diff --git a/src/analyzer/expr/call/function_call_analyzer.rs b/src/analyzer/expr/call/function_call_analyzer.rs
@@ -203,40 +203,20 @@ pub(crate) fn analyze(
     );
 
     if !function_storage.is_production_code && function_storage.user_defined {
-        if let Some(calling_context) = context.function_context.calling_functionlike_id {
-            let is_caller_production = match calling_context {
-                FunctionLikeIdentifier::Function(function_id) => {
-                    codebase
-                        .functionlike_infos
-                        .get(&(function_id, StrId::EMPTY))
-                        .unwrap()
-                        .is_production_code
-                }
-                FunctionLikeIdentifier::Method(classlike_name, method_name) => {
-                    codebase
-                        .functionlike_infos
-                        .get(&(classlike_name, method_name))
-                        .unwrap()
-                        .is_production_code
-                }
-                _ => false,
-            };
-
-            if is_caller_production {
-                analysis_data.maybe_add_issue(
-                    Issue::new(
-                        IssueKind::TestOnlyCall,
-                        format!(
-                            "Cannot call test-only function {} from non-test context",
-                            statements_analyzer.get_interner().lookup(&name)
-                        ),
-                        statements_analyzer.get_hpos(pos),
-                        &context.function_context.calling_functionlike_id,
+        if context.function_context.is_production(codebase) {
+            analysis_data.maybe_add_issue(
+                Issue::new(
+                    IssueKind::TestOnlyCall,
+                    format!(
+                        "Cannot call test-only function {} from non-test context",
+                        statements_analyzer.get_interner().lookup(&name)
                     ),
-                    statements_analyzer.get_config(),
-                    statements_analyzer.get_file_path_actual(),
-                )
-            }
+                    statements_analyzer.get_hpos(pos),
+                    &context.function_context.calling_functionlike_id,
+                ),
+                statements_analyzer.get_config(),
+                statements_analyzer.get_file_path_actual(),
+            )
         }
     }
 
@@ -253,7 +233,10 @@ pub(crate) fn analyze(
 
     analysis_data.set_expr_type(pos, stmt_type.clone());
 
-    if stmt_type.is_nothing() && !context.inside_loop {
+    if stmt_type.is_nothing()
+        && !context.inside_loop
+        && context.function_context.is_production(codebase)
+    {
         context.has_returned = true;
     }
 

diff --git a/src/analyzer/expr/call/function_call_return_type_fetcher.rs b/src/analyzer/expr/call/function_call_return_type_fetcher.rs
@@ -137,6 +137,10 @@ pub(crate) fn fetch(
             &mut analysis_data.data_flow_graph,
         );
 
+        if function_return_type.is_nothing() && !context.function_context.is_production(codebase) {
+            function_return_type = get_mixed();
+        }
+
         // todo dispatch AfterFunctionCallAnalysisEvent
 
         function_return_type

diff --git a/src/analyzer/expr/call/method_call_return_type_fetcher.rs b/src/analyzer/expr/call/method_call_return_type_fetcher.rs
@@ -3,6 +3,7 @@ use std::rc::Rc;
 use hakana_reflection_info::functionlike_identifier::FunctionLikeIdentifier;
 use hakana_reflection_info::{ExprId, GenericParent, VarId};
 use hakana_str::{Interner, StrId};
+use hakana_type::get_mixed;
 use oxidized::{aast, ast_defs};
 use rustc_hash::FxHashMap;
 
@@ -137,6 +138,10 @@ pub(crate) fn fetch(
         &mut analysis_data.data_flow_graph,
     );
 
+    if return_type_candidate.is_nothing() && !context.function_context.is_production(codebase) {
+        return_type_candidate = get_mixed();
+    }
+
     add_dataflow(
         statements_analyzer,
         return_type_candidate,

diff --git a/src/analyzer/expr/call_analyzer.rs b/src/analyzer/expr/call_analyzer.rs
@@ -417,40 +417,20 @@ pub(crate) fn check_method_args(
     }
 
     if !functionlike_storage.is_production_code && functionlike_storage.user_defined {
-        if let Some(calling_context) = context.function_context.calling_functionlike_id {
-            let is_caller_production = match calling_context {
-                FunctionLikeIdentifier::Function(function_id) => {
-                    codebase
-                        .functionlike_infos
-                        .get(&(function_id, StrId::EMPTY))
-                        .unwrap()
-                        .is_production_code
-                }
-                FunctionLikeIdentifier::Method(classlike_name, method_name) => {
-                    codebase
-                        .functionlike_infos
-                        .get(&(classlike_name, method_name))
-                        .unwrap()
-                        .is_production_code
-                }
-                _ => false,
-            };
-
-            if is_caller_production {
-                analysis_data.maybe_add_issue(
-                    Issue::new(
-                        IssueKind::TestOnlyCall,
-                        format!(
-                            "Cannot call test-only function {} from non-test context",
-                            method_id.to_string(statements_analyzer.get_interner()),
-                        ),
-                        statements_analyzer.get_hpos(pos),
-                        &context.function_context.calling_functionlike_id,
+        if context.function_context.is_production(codebase) {
+            analysis_data.maybe_add_issue(
+                Issue::new(
+                    IssueKind::TestOnlyCall,
+                    format!(
+                        "Cannot call test-only function {} from non-test context",
+                        method_id.to_string(statements_analyzer.get_interner()),
                     ),
-                    statements_analyzer.get_config(),
-                    statements_analyzer.get_file_path_actual(),
-                )
-            }
+                    statements_analyzer.get_hpos(pos),
+                    &context.function_context.calling_functionlike_id,
+                ),
+                statements_analyzer.get_config(),
+                statements_analyzer.get_file_path_actual(),
+            )
         }
     }
 

diff --git a/src/analyzer/statements_analyzer.rs b/src/analyzer/statements_analyzer.rs
@@ -47,23 +47,40 @@ impl<'a> StatementsAnalyzer<'a> {
         loop_scope: &mut Option<LoopScope>,
     ) -> Result<(), AnalysisError> {
         for stmt in stmts {
-            if context.has_returned
-                && self.get_config().allow_issue_kind_in_file(
-                    &IssueKind::UnevaluatedCode,
-                    self.get_file_path_actual(),
-                )
-            {
+            if context.has_returned {
                 if self.get_config().find_unused_expressions {
-                    analysis_data.maybe_add_issue(
-                        Issue::new(
-                            IssueKind::UnevaluatedCode,
-                            "Unused code after return/throw/continue".to_string(),
-                            self.get_hpos(&stmt.0),
-                            &context.function_context.calling_functionlike_id,
-                        ),
-                        self.get_config(),
-                        self.get_file_path_actual(),
-                    );
+                    let is_harmless = match &stmt.1 {
+                        aast::Stmt_::Break => true,
+                        aast::Stmt_::Continue => true,
+                        aast::Stmt_::Return(boxed) => boxed.is_none(),
+                        _ => false,
+                    };
+
+                    if stmt.0.line() > 0 {
+                        if is_harmless {
+                            analysis_data.maybe_add_issue(
+                                Issue::new(
+                                    IssueKind::UselessControlFlow,
+                                    "This control flow is unnecessary".to_string(),
+                                    self.get_hpos(&stmt.0),
+                                    &context.function_context.calling_functionlike_id,
+                                ),
+                                self.get_config(),
+                                self.get_file_path_actual(),
+                            );
+                        } else {
+                            analysis_data.maybe_add_issue(
+                                Issue::new(
+                                    IssueKind::UnevaluatedCode,
+                                    "Unused code after return/throw/continue".to_string(),
+                                    self.get_hpos(&stmt.0),
+                                    &context.function_context.calling_functionlike_id,
+                                ),
+                                self.get_config(),
+                                self.get_file_path_actual(),
+                            );
+                        }
+                    }
                 }
             } else {
                 stmt_analyzer::analyze(self, stmt, analysis_data, context, loop_scope)?;

diff --git a/src/code_info/function_context/mod.rs b/src/code_info/function_context/mod.rs
@@ -1,7 +1,7 @@
 use hakana_str::StrId;
 
 pub use crate::functionlike_identifier::FunctionLikeIdentifier;
-use crate::symbol_references::ReferenceSource;
+use crate::{codebase_info::CodebaseInfo, symbol_references::ReferenceSource};
 
 #[derive(Clone, Debug, Copy)]
 pub struct FunctionContext {
@@ -45,4 +45,24 @@ impl FunctionContext {
             ReferenceSource::Symbol(false, *file_path)
         }
     }
+
+    pub fn is_production(&self, codebase: &CodebaseInfo) -> bool {
+        match self.calling_functionlike_id {
+            Some(FunctionLikeIdentifier::Function(function_id)) => {
+                codebase
+                    .functionlike_infos
+                    .get(&(function_id, StrId::EMPTY))
+                    .unwrap()
+                    .is_production_code
+            }
+            Some(FunctionLikeIdentifier::Method(classlike_name, method_name)) => {
+                codebase
+                    .functionlike_infos
+                    .get(&(classlike_name, method_name))
+                    .unwrap()
+                    .is_production_code
+            }
+            _ => false,
+        }
+    }
 }
diff --git a/src/code_info/issue.rs b/src/code_info/issue.rs
@@ -142,6 +142,7 @@ pub enum IssueKind {
     UnusedTypeDefinition,
     UnusedXhpAttribute,
     UpcastAwaitable,
+    UselessControlFlow,
 }
 
 impl IssueKind {

diff --git a/src/file_scanner_analyzer/scanner.rs b/src/file_scanner_analyzer/scanner.rs
@@ -273,7 +273,7 @@ pub fn scan_files(
 
         let mut path_groups = FxHashMap::default();
 
-        if (files_to_scan.len() / group_size) < 4 {
+        if files_to_scan.len() < 4 * group_size {
             group_size = 1;
         }
 

diff --git a/tests/unused/UnusedExpression/varUsedAfterSwitchWithRedundantBreak/output.txt b/tests/unused/UnusedExpression/varUsedAfterSwitchWithRedundantBreak/output.txt
@@ -1 +1 @@
-ERROR: UnevaluatedCode - input.hack:11:13 - Unused code after return/throw/continue
+ERROR: UselessControlFlow - input.hack:11:13 - This control flow is unnecessary