From 3b2175d0dae409a29c6bed43816a7ffd979dbc6a Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Mon, 6 May 2024 13:21:02 -0400 Subject: [PATCH] Fix some funky unused code checks --- src/analyzer/expr/assertion_finder.rs | 32 ++++----- src/analyzer/expr/call/arguments_analyzer.rs | 2 +- .../existing_atomic_method_call_analyzer.rs | 21 +++--- .../expr/call/expression_call_analyzer.rs | 14 +++- src/analyzer/expr/call/new_analyzer.rs | 9 +-- src/analyzer/expr/call_analyzer.rs | 10 ++- src/analyzer/expr/expression_identifier.rs | 60 ++++++++--------- src/analyzer/stmt_analyzer.rs | 63 +++++------------- src/code_info/functionlike_info.rs | 4 +- src/code_info/lib.rs | 18 +++-- src/code_info_builder/functionlike_scanner.rs | 2 +- src/code_info_builder/lib.rs | 66 ++++++++++++++----- src/str/build.rs | 4 ++ .../reflectionClassCallDoesStuff/input.hack | 3 + .../throwInClosureUsed/input.hack | 6 ++ 15 files changed, 169 insertions(+), 145 deletions(-) create mode 100644 tests/unused/UnusedExpression/reflectionClassCallDoesStuff/input.hack create mode 100644 tests/unused/UnusedExpression/throwInClosureUsed/input.hack diff --git a/src/analyzer/expr/assertion_finder.rs b/src/analyzer/expr/assertion_finder.rs index 1216e1b3..b9dc33de 100644 --- a/src/analyzer/expr/assertion_finder.rs +++ b/src/analyzer/expr/assertion_finder.rs @@ -58,15 +58,15 @@ pub(crate) fn scrape_assertions( ); } aast::Expr_::Call(call) => { - let functionlike_id = get_static_functionlike_id_from_call( - call, - if let Some((_, interner)) = assertion_context.codebase { - Some(interner) - } else { - None - }, - assertion_context.resolved_names, - ); + let functionlike_id = if let Some((_, interner)) = assertion_context.codebase { + get_static_functionlike_id_from_call( + call, + interner, + assertion_context.resolved_names, + ) + } else { + None + }; if let Some(FunctionLikeIdentifier::Function(name)) = functionlike_id { return scrape_function_assertions( @@ -348,15 +348,11 @@ fn scrape_shapes_isset( negated: bool, ) { if let aast::Expr_::Call(call) = &var_expr.2 { - let functionlike_id = get_static_functionlike_id_from_call( - call, - if let Some((_, interner)) = assertion_context.codebase { - Some(interner) - } else { - None - }, - assertion_context.resolved_names, - ); + let functionlike_id = if let Some((_, interner)) = assertion_context.codebase { + get_static_functionlike_id_from_call(call, interner, assertion_context.resolved_names) + } else { + None + }; if let Some(FunctionLikeIdentifier::Method(class_name, member_name)) = functionlike_id { if let Some((codebase, interner)) = assertion_context.codebase { diff --git a/src/analyzer/expr/call/arguments_analyzer.rs b/src/analyzer/expr/call/arguments_analyzer.rs index 1e21d21b..4db9dcac 100644 --- a/src/analyzer/expr/call/arguments_analyzer.rs +++ b/src/analyzer/expr/call/arguments_analyzer.rs @@ -181,7 +181,7 @@ pub(crate) fn check_arguments_match( if matches!(functionlike_info.effects, FnEffect::Some(_)) || matches!(functionlike_info.effects, FnEffect::Arg(_)) - || functionlike_info.pure_can_throw + || functionlike_info.has_throw || functionlike_info.user_defined || functionlike_info.method_info.is_some() { diff --git a/src/analyzer/expr/call/existing_atomic_method_call_analyzer.rs b/src/analyzer/expr/call/existing_atomic_method_call_analyzer.rs index 2acc32a7..1b4191b1 100644 --- a/src/analyzer/expr/call/existing_atomic_method_call_analyzer.rs +++ b/src/analyzer/expr/call/existing_atomic_method_call_analyzer.rs @@ -9,7 +9,7 @@ use hakana_reflection_info::{ t_atomic::{DictKey, TAtomic}, t_union::TUnion, }; -use hakana_reflection_info::{GenericParent, VarId, EFFECT_WRITE_LOCAL, EFFECT_WRITE_PROPS}; +use hakana_reflection_info::{GenericParent, VarId, EFFECT_WRITE_LOCAL}; use hakana_str::StrId; use hakana_type::get_null; use hakana_type::template::standin_type_replacer; @@ -164,14 +164,6 @@ pub(crate) fn analyze( } } - // .hhi for NumberFormatter was incorrect - if classlike_name == StrId::NUMBER_FORMATTER { - analysis_data.expr_effects.insert( - (pos.start_offset() as u32, pos.end_offset() as u32), - EFFECT_WRITE_PROPS, - ); - } - check_method_args( statements_analyzer, analysis_data, @@ -185,6 +177,17 @@ pub(crate) fn analyze( method_name_pos, )?; + // .hhi for NumberFormatter was incorrect + // or if we're calling parent::__construct, make sure we set correct write props effect + if method_id.0 == StrId::NUMBER_FORMATTER || method_id.1 == StrId::CONSTRUCT { + if let Some(effects) = analysis_data + .expr_effects + .get_mut(&(pos.start_offset() as u32, pos.end_offset() as u32)) + { + *effects |= EFFECT_WRITE_LOCAL; + } + } + if functionlike_storage.ignore_taints_if_true { analysis_data.if_true_assertions.insert( (pos.start_offset() as u32, pos.end_offset() as u32), diff --git a/src/analyzer/expr/call/expression_call_analyzer.rs b/src/analyzer/expr/call/expression_call_analyzer.rs index 95758225..0ebcd03b 100644 --- a/src/analyzer/expr/call/expression_call_analyzer.rs +++ b/src/analyzer/expr/call/expression_call_analyzer.rs @@ -68,7 +68,16 @@ pub(crate) fn analyze( start_line: 0, }, ); - lambda_storage.user_defined = true; + let existing_storage = codebase + .functionlike_infos + .get(&(closure_id.0 .0, StrId(closure_id.1))); + + if let Some(existing_storage) = existing_storage { + lambda_storage.has_throw = existing_storage.has_throw; + } + + lambda_storage.effects = FnEffect::from_u8(effects); + lambda_storage.params = closure_params .iter() .map(|fn_param| { @@ -84,7 +93,8 @@ pub(crate) fn analyze( }) .collect(); lambda_storage.return_type = closure_return_type.clone().map(|t| (*t).clone()); - lambda_storage.effects = FnEffect::from_u8(effects); + + lambda_storage.user_defined = true; let functionlike_id = FunctionLikeIdentifier::Closure(closure_id.0, closure_id.1); diff --git a/src/analyzer/expr/call/new_analyzer.rs b/src/analyzer/expr/call/new_analyzer.rs index 4d3efcf0..391efb70 100644 --- a/src/analyzer/expr/call/new_analyzer.rs +++ b/src/analyzer/expr/call/new_analyzer.rs @@ -227,8 +227,8 @@ fn analyze_atomic( } }; - match statements_analyzer.get_interner().lookup(&classlike_name) { - "ReflectionClass" | "ReflectionTypeAlias" => { + match classlike_name { + StrId::REFLECTION_CLASS | StrId::REFLECTION_FUNCTION | StrId::REFLECTION_TYPE_ALIAS => { analysis_data.expr_effects.insert( (pos.start_offset() as u32, pos.end_offset() as u32), EFFECT_WRITE_GLOBALS, @@ -317,10 +317,7 @@ fn analyze_named_constructor( let mut generic_type_params = None; - let method_name = statements_analyzer - .get_interner() - .get("__construct") - .unwrap(); + let method_name = StrId::CONSTRUCT; let method_id = MethodIdentifier(classlike_name, method_name); let declaring_method_id = codebase.get_declaring_method_id(&method_id); diff --git a/src/analyzer/expr/call_analyzer.rs b/src/analyzer/expr/call_analyzer.rs index f92a7e38..670a8b9b 100644 --- a/src/analyzer/expr/call_analyzer.rs +++ b/src/analyzer/expr/call_analyzer.rs @@ -2,7 +2,9 @@ use std::sync::Arc; use hakana_reflection_info::code_location::HPos; use hakana_reflection_info::issue::{Issue, IssueKind}; -use hakana_reflection_info::{GenericParent, EFFECT_IMPURE, EFFECT_PURE, EFFECT_WRITE_PROPS}; +use hakana_reflection_info::{ + GenericParent, EFFECT_CAN_THROW, EFFECT_IMPURE, EFFECT_PURE, EFFECT_WRITE_PROPS, +}; use hakana_str::StrId; use hakana_type::template::standin_type_replacer::get_relevant_bounds; use hakana_type::type_comparator::type_comparison_result::TypeComparisonResult; @@ -506,7 +508,11 @@ pub(crate) fn apply_effects( FnEffect::Pure => { analysis_data.expr_effects.insert( (pos.start_offset() as u32, pos.end_offset() as u32), - EFFECT_PURE, + if function_storage.has_throw { + EFFECT_CAN_THROW + } else { + EFFECT_PURE + }, ); } FnEffect::Unknown => { diff --git a/src/analyzer/expr/expression_identifier.rs b/src/analyzer/expr/expression_identifier.rs index df4ac89c..da9f4e14 100644 --- a/src/analyzer/expr/expression_identifier.rs +++ b/src/analyzer/expr/expression_identifier.rs @@ -167,58 +167,50 @@ pub fn get_functionlike_id_from_call( resolved_names: &FxHashMap, expr_types: &FxHashMap<(u32, u32), Rc>, ) -> Option { - get_static_functionlike_id_from_call(call_expr, Some(interner), resolved_names) + get_static_functionlike_id_from_call(call_expr, interner, resolved_names) .or_else(|| get_method_id_from_call(call_expr, interner, expr_types)) } pub fn get_static_functionlike_id_from_call( call: &oxidized::ast::CallExpr, - interner: Option<&Interner>, + interner: &Interner, resolved_names: &FxHashMap, ) -> Option { match &call.func.2 { aast::Expr_::Id(boxed_id) => { - if let Some(interner) = interner { - let name = if boxed_id.1 == "isset" { - StrId::ISSET - } else if boxed_id.1 == "\\in_array" { - interner.get("in_array").unwrap() - } else if let Some(resolved_name) = - resolved_names.get(&(boxed_id.0.start_offset() as u32)) - { - *resolved_name - } else { - return None; - }; - - Some(FunctionLikeIdentifier::Function(name)) + let name = if boxed_id.1 == "isset" { + StrId::ISSET + } else if boxed_id.1 == "\\in_array" { + interner.get("in_array").unwrap() + } else if let Some(resolved_name) = + resolved_names.get(&(boxed_id.0.start_offset() as u32)) + { + *resolved_name } else { - None - } + return None; + }; + + Some(FunctionLikeIdentifier::Function(name)) } aast::Expr_::ClassConst(boxed) => { - if let Some(interner) = interner { - let (class_id, rhs_expr) = (&boxed.0, &boxed.1); + let (class_id, rhs_expr) = (&boxed.0, &boxed.1); - match &class_id.2 { - aast::ClassId_::CIexpr(lhs_expr) => { - if let aast::Expr_::Id(id) = &lhs_expr.2 { - if let (Some(class_name), Some(method_name)) = ( - resolved_names.get(&(id.0.start_offset() as u32)), - interner.get(&rhs_expr.1), - ) { - Some(FunctionLikeIdentifier::Method(*class_name, method_name)) - } else { - None - } + match &class_id.2 { + aast::ClassId_::CIexpr(lhs_expr) => { + if let aast::Expr_::Id(id) = &lhs_expr.2 { + if let (Some(class_name), Some(method_name)) = ( + resolved_names.get(&(id.0.start_offset() as u32)), + interner.get(&rhs_expr.1), + ) { + Some(FunctionLikeIdentifier::Method(*class_name, method_name)) } else { None } + } else { + None } - _ => None, } - } else { - None + _ => None, } } _ => None, diff --git a/src/analyzer/stmt_analyzer.rs b/src/analyzer/stmt_analyzer.rs index add7bcca..6b18b234 100644 --- a/src/analyzer/stmt_analyzer.rs +++ b/src/analyzer/stmt_analyzer.rs @@ -1,5 +1,6 @@ use hakana_reflection_info::code_location::{HPos, StmtStart}; use hakana_reflection_info::functionlike_identifier::FunctionLikeIdentifier; +use hakana_reflection_info::functionlike_info::FnEffect; use hakana_reflection_info::EFFECT_PURE; use hakana_str::StrId; use hakana_type::get_arrayish_params; @@ -8,7 +9,9 @@ use rustc_hash::FxHashSet; use crate::custom_hook::AfterStmtAnalysisData; use crate::expr::binop::assignment_analyzer; -use crate::expr::expression_identifier::get_functionlike_id_from_call; +use crate::expr::expression_identifier::{ + get_functionlike_id_from_call, get_static_functionlike_id_from_call, +}; use crate::expression_analyzer; use crate::function_analysis_data::FunctionAnalysisData; use crate::scope_analyzer::ScopeAnalyzer; @@ -286,51 +289,12 @@ fn detect_unused_statement_expressions( ); } - let functionlike_id = if let aast::Expr_::Call(boxed_call) = &boxed.2 { - get_functionlike_id_from_call( - boxed_call, - statements_analyzer.get_interner(), - statements_analyzer.get_file_analyzer().resolved_names, - &analysis_data.expr_types, - ) - } else { - None - }; - if let Some(effect) = analysis_data.expr_effects.get(&( boxed.pos().start_offset() as u32, boxed.pos().end_offset() as u32, )) { if effect == &EFFECT_PURE { - let mut is_constructor_call = false; - let mut fn_can_throw = false; - - if let Some(functionlike_id) = functionlike_id { - match functionlike_id { - FunctionLikeIdentifier::Function(function_id) => { - let codebase = statements_analyzer.get_codebase(); - - if let Some(functionlike_info) = codebase - .functionlike_infos - .get(&(function_id, StrId::EMPTY)) - { - fn_can_throw = functionlike_info.pure_can_throw - } - } - FunctionLikeIdentifier::Method(_, method_name_id) => { - if method_name_id == StrId::CONSTRUCT { - is_constructor_call = true; - } - - if method_name_id == StrId::ASSERT { - fn_can_throw = true; - } - } - _ => (), - } - }; - - if !is_constructor_call && !fn_can_throw { + if !matches!(boxed.2, aast::Expr_::New(..)) { analysis_data.maybe_add_issue( Issue::new( IssueKind::UnusedStatement, @@ -346,19 +310,24 @@ fn detect_unused_statement_expressions( } } - if let Some(functionlike_id) = functionlike_id { - if let FunctionLikeIdentifier::Function(function_id) = functionlike_id { + if let aast::Expr_::Call(boxed_call) = &boxed.2 { + let functionlike_id = get_static_functionlike_id_from_call( + boxed_call, + statements_analyzer.get_interner(), + statements_analyzer.get_file_analyzer().resolved_names, + ); + + if let Some(FunctionLikeIdentifier::Function(function_id)) = functionlike_id { let codebase = statements_analyzer.get_codebase(); - if let Some(_functionlike_info) = codebase + if let Some(functionlike_info) = codebase .functionlike_infos .get(&(function_id, StrId::EMPTY)) { if let Some(expr_type) = analysis_data.get_rc_expr_type(boxed.pos()).cloned() { let function_name = statements_analyzer.get_interner().lookup(&function_id); - if (function_name.starts_with("HH\\Lib\\Keyset\\") - || function_name.starts_with("HH\\Lib\\Vec\\") - || function_name.starts_with("HH\\Lib\\Dict\\")) + if !functionlike_info.user_defined + && matches!(functionlike_info.effects, FnEffect::Arg(..)) && expr_type.is_single() { let array_types = get_arrayish_params(expr_type.get_single(), codebase); diff --git a/src/code_info/functionlike_info.rs b/src/code_info/functionlike_info.rs index e11091ba..31da99b4 100644 --- a/src/code_info/functionlike_info.rs +++ b/src/code_info/functionlike_info.rs @@ -99,7 +99,7 @@ pub struct FunctionLikeInfo { pub effects: FnEffect, - pub pure_can_throw: bool, + pub has_throw: bool, /** * Whether or not the function output is dependent solely on input - a function can be @@ -191,7 +191,7 @@ impl FunctionLikeInfo { where_constraints: vec![], async_version: None, is_production_code: true, - pure_can_throw: false, + has_throw: false, is_closure: false, overriding: false, } diff --git a/src/code_info/lib.rs b/src/code_info/lib.rs index 86be887a..b9337379 100644 --- a/src/code_info/lib.rs +++ b/src/code_info/lib.rs @@ -47,13 +47,17 @@ pub struct FileSource<'a> { } pub const EFFECT_PURE: u8 = 0b00000000; -pub const EFFECT_WRITE_LOCAL: u8 = 0b00000001; -pub const EFFECT_READ_PROPS: u8 = 0b00000010; -pub const EFFECT_READ_GLOBALS: u8 = 0b00000100; -pub const EFFECT_WRITE_PROPS: u8 = 0b00001000; -pub const EFFECT_WRITE_GLOBALS: u8 = 0b0010000; -pub const EFFECT_IMPURE: u8 = - EFFECT_READ_PROPS | EFFECT_READ_GLOBALS | EFFECT_WRITE_PROPS | EFFECT_WRITE_GLOBALS; +pub const EFFECT_CAN_THROW: u8 = 0b00000010; +pub const EFFECT_WRITE_LOCAL: u8 = 0b00000010; +pub const EFFECT_READ_PROPS: u8 = 0b00000100; +pub const EFFECT_READ_GLOBALS: u8 = 0b00001000; +pub const EFFECT_WRITE_PROPS: u8 = 0b00010000; +pub const EFFECT_WRITE_GLOBALS: u8 = 0b0100000; +pub const EFFECT_IMPURE: u8 = EFFECT_CAN_THROW + | EFFECT_READ_PROPS + | EFFECT_READ_GLOBALS + | EFFECT_WRITE_PROPS + | EFFECT_WRITE_GLOBALS; #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, Serialize, Deserialize, PartialOrd, Ord)] diff --git a/src/code_info_builder/functionlike_scanner.rs b/src/code_info_builder/functionlike_scanner.rs index 95acf124..8d15a71f 100644 --- a/src/code_info_builder/functionlike_scanner.rs +++ b/src/code_info_builder/functionlike_scanner.rs @@ -85,7 +85,7 @@ pub(crate) fn scan_method( || method_name == StrId::ASSERT || method_name == StrId::ASSERT_ALL) { - functionlike_info.pure_can_throw = true; + functionlike_info.has_throw = true; } let mut method_info = MethodInfo::new(); diff --git a/src/code_info_builder/lib.rs b/src/code_info_builder/lib.rs index eee89b8e..5513163c 100644 --- a/src/code_info_builder/lib.rs +++ b/src/code_info_builder/lib.rs @@ -37,6 +37,7 @@ struct Context { function_name: Option, member_name: Option, has_yield: bool, + has_throw: bool, has_asio_join: bool, has_static_field_access: bool, uses_position: Option<(usize, usize)>, @@ -470,30 +471,36 @@ impl<'ast> Visitor<'ast> for Scanner<'_> { c.member_name = None; + let classlike_name = *c.classlike_name.as_ref().unwrap(); + + let functionlike_storage = self + .codebase + .functionlike_infos + .get_mut(&(classlike_name, method_name)) + .unwrap(); + + if classlike_name == StrId::REFLECTION_CLASS || classlike_name == StrId::REFLECTION_FUNCTION + { + functionlike_storage.has_throw = true; + } + if c.has_yield { - self.codebase - .functionlike_infos - .get_mut(&(*c.classlike_name.as_ref().unwrap(), method_name)) - .unwrap() - .has_yield = true; + functionlike_storage.has_yield = true; c.has_yield = false; } + if c.has_throw { + functionlike_storage.has_throw = true; + c.has_throw = false; + } + if c.has_asio_join { - self.codebase - .functionlike_infos - .get_mut(&(*c.classlike_name.as_ref().unwrap(), method_name)) - .unwrap() - .has_asio_join = true; + functionlike_storage.has_asio_join = true; c.has_asio_join = false; } if !c.has_static_field_access && m.static_ { - self.codebase - .functionlike_infos - .get_mut(&(*c.classlike_name.as_ref().unwrap(), method_name)) - .unwrap() - .specialize_call = true; + functionlike_storage.specialize_call = true; c.has_static_field_access = false; } @@ -557,6 +564,16 @@ impl<'ast> Visitor<'ast> for Scanner<'_> { result } + fn visit_stmt_(&mut self, c: &mut Context, p: &aast::Stmt_<(), ()>) -> Result<(), ()> { + let result = p.recurse(c, self); + + if let aast::Stmt_::Throw(..) = &p { + c.has_throw = true; + } + + result + } + fn visit_expr(&mut self, c: &mut Context, p: &aast::Expr<(), ()>) -> Result<(), ()> { let result = p.recurse(c, self); @@ -694,7 +711,7 @@ impl<'a> Scanner<'a> { | StrId::LIB_C_ONLYX ) ) { - functionlike_storage.pure_can_throw = true; + functionlike_storage.has_throw = true; } functionlike_storage.type_resolution_context = Some(type_resolution_context); @@ -704,6 +721,22 @@ impl<'a> Scanner<'a> { fix_function_return_type(name, &mut functionlike_storage); } } + + if c.has_yield { + functionlike_storage.has_yield = true; + c.has_yield = false; + } + + if c.has_throw { + functionlike_storage.has_throw = true; + c.has_throw = false; + } + + if c.has_asio_join { + functionlike_storage.has_asio_join = true; + c.has_asio_join = false; + } + functionlike_storage } } @@ -857,6 +890,7 @@ pub fn collect_info_for_aast( function_name: None, member_name: None, has_yield: false, + has_throw: false, has_asio_join: false, has_static_field_access: false, uses_position: None, diff --git a/src/str/build.rs b/src/str/build.rs index f0dd1e72..e974e8cb 100644 --- a/src/str/build.rs +++ b/src/str/build.rs @@ -18,6 +18,7 @@ fn main() -> Result<()> { "DOMDocument", "DateTime", "DateTimeImmutable", + "Error", "Exception", "HH\\AnyArray", "HH\\Asio\\join", @@ -251,6 +252,9 @@ fn main() -> Result<()> { "Hakana\\SpecialTypes\\LiteralString", "Hakana\\TestOnly", "NumberFormatter", + "ReflectionClass", + "ReflectionFunction", + "ReflectionTypeAlias", "SimpleXMLElement", "XHPChild", "__DIR__", diff --git a/tests/unused/UnusedExpression/reflectionClassCallDoesStuff/input.hack b/tests/unused/UnusedExpression/reflectionClassCallDoesStuff/input.hack new file mode 100644 index 00000000..acbe3116 --- /dev/null +++ b/tests/unused/UnusedExpression/reflectionClassCallDoesStuff/input.hack @@ -0,0 +1,3 @@ +function specifyString(string $className): void{ + new ReflectionClass($className); +} diff --git a/tests/unused/UnusedExpression/throwInClosureUsed/input.hack b/tests/unused/UnusedExpression/throwInClosureUsed/input.hack new file mode 100644 index 00000000..c2ae4952 --- /dev/null +++ b/tests/unused/UnusedExpression/throwInClosureUsed/input.hack @@ -0,0 +1,6 @@ +function foo(): void { + $fn_fail = () ==> { + throw new Exception("bad"); + }; + $fn_fail(); +} \ No newline at end of file