diff --git a/src/analyzer/dataflow/program_analyzer.rs b/src/analyzer/dataflow/program_analyzer.rs index f67820e8..9f5dc5c0 100644 --- a/src/analyzer/dataflow/program_analyzer.rs +++ b/src/analyzer/dataflow/program_analyzer.rs @@ -292,7 +292,7 @@ fn get_child_nodes( generated_source.get_trace(interner, &config.root_dir) ); new_issues.push(Issue::new( - IssueKind::TaintedData(t.clone()), + IssueKind::TaintedData(Box::new(t.clone())), message, **generated_source.pos.as_ref().unwrap(), &None, @@ -370,7 +370,7 @@ fn get_child_nodes( generated_source.get_trace(interner, &config.root_dir) ); new_issues.push(Issue::new( - IssueKind::TaintedData(t.clone()), + IssueKind::TaintedData(Box::new(t.clone())), message, **generated_source.pos.as_ref().unwrap(), &None, @@ -430,7 +430,7 @@ fn get_child_nodes( new_destination.get_trace(interner, &config.root_dir) ); new_issues.push(Issue::new( - IssueKind::TaintedData(matching_sink.clone()), + IssueKind::TaintedData(Box::new(matching_sink.clone())), message, **issue_pos, &None, diff --git a/src/analyzer/expr/call/function_call_analyzer.rs b/src/analyzer/expr/call/function_call_analyzer.rs index b4f6bd07..b0f5db19 100644 --- a/src/analyzer/expr/call/function_call_analyzer.rs +++ b/src/analyzer/expr/call/function_call_analyzer.rs @@ -199,6 +199,44 @@ pub(crate) fn analyze( &functionlike_id, ); + if !function_storage.is_production_code { + 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, + ), + statements_analyzer.get_config(), + statements_analyzer.get_file_path_actual(), + ) + } + } + } + let stmt_type = function_call_return_type_fetcher::fetch( statements_analyzer, expr, diff --git a/src/analyzer/expr/call_analyzer.rs b/src/analyzer/expr/call_analyzer.rs index daad29ad..d3b55277 100644 --- a/src/analyzer/expr/call_analyzer.rs +++ b/src/analyzer/expr/call_analyzer.rs @@ -414,6 +414,44 @@ pub(crate) fn check_method_args( check_template_result(statements_analyzer, template_result, pos, &functionlike_id); } + if !functionlike_storage.is_production_code { + 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, + ), + statements_analyzer.get_config(), + statements_analyzer.get_file_path_actual(), + ) + } + } + } + Ok(()) } diff --git a/src/code_info/classlike_info.rs b/src/code_info/classlike_info.rs index d159ac3f..096296f9 100644 --- a/src/code_info/classlike_info.rs +++ b/src/code_info/classlike_info.rs @@ -5,7 +5,7 @@ use rustc_hash::{FxHashMap, FxHashSet}; use crate::{ code_location::HPos, codebase_info::symbols::SymbolKind, functionlike_info::MetaStart, - t_atomic::TAtomic, t_union::TUnion, GenericParent, + issue::IssueKind, t_atomic::TAtomic, t_union::TUnion, GenericParent, }; use indexmap::IndexMap; use serde::{Deserialize, Serialize}; @@ -202,6 +202,8 @@ pub struct ClassLikeInfo { pub namespace_position: Option<(usize, usize)>, pub is_production_code: bool, + + pub suppressed_issues: Vec<(IssueKind, HPos)>, } impl ClassLikeInfo { @@ -270,6 +272,7 @@ impl ClassLikeInfo { namespace_position: None, is_production_code: true, template_readonly: FxHashSet::default(), + suppressed_issues: vec![], } } } diff --git a/src/code_info/functionlike_info.rs b/src/code_info/functionlike_info.rs index ae7b0566..1135ac46 100644 --- a/src/code_info/functionlike_info.rs +++ b/src/code_info/functionlike_info.rs @@ -70,7 +70,7 @@ pub struct FunctionLikeInfo { pub is_populated: bool, - pub suppressed_issues: Option>, + pub suppressed_issues: Vec<(IssueKind, HPos)>, pub deprecated: bool, @@ -163,7 +163,7 @@ impl FunctionLikeInfo { return_type_location: None, is_populated: false, user_defined: false, - suppressed_issues: None, + suppressed_issues: vec![], deprecated: false, template_types: vec![], has_visitor_issues: false, diff --git a/src/code_info/issue.rs b/src/code_info/issue.rs index b8e17900..2a79844d 100644 --- a/src/code_info/issue.rs +++ b/src/code_info/issue.rs @@ -14,7 +14,7 @@ use crate::{ #[derive(Clone, PartialEq, Eq, Hash, Display, Debug, Serialize, Deserialize, EnumString)] pub enum IssueKind { CannotInferGenericParam, - CustomIssue(String), + CustomIssue(Box), DuplicateEnumValue, EmptyBlock, FalsableReturnStatement, @@ -85,6 +85,7 @@ pub enum IssueKind { NullablePropertyAssignment, NullableReturnStatement, NullableReturnValue, + OnlyUsedInTests, ParadoxicalCondition, PossibleMethodCallOnNull, PossiblyFalseArgument, @@ -104,7 +105,8 @@ pub enum IssueKind { RedundantNonnullTypeComparison, RedundantTruthinessCheck, RedundantTypeComparison, - TaintedData(SinkType), + TaintedData(Box), + TestOnlyCall, UndefinedIntArrayOffset, UndefinedStringArrayOffset, UndefinedVariable, @@ -147,7 +149,7 @@ impl IssueKind { } if all_custom_issues.contains(str) { - Ok(IssueKind::CustomIssue(str.to_string())) + Ok(IssueKind::CustomIssue(Box::new(str.to_string()))) } else { Err(format!("Unknown issue {}", str)) } @@ -156,7 +158,7 @@ impl IssueKind { #[allow(clippy::inherent_to_string_shadow_display)] pub fn to_string(&self) -> String { match self { - Self::CustomIssue(str) => str.clone(), + Self::CustomIssue(str) => (**str).clone(), //Self::TaintedData(sink_type) => format!("TaintedData({})", sink_type), _ => format!("{}", self), } diff --git a/src/code_info/symbol_references.rs b/src/code_info/symbol_references.rs index f560a8db..9bbc8bcb 100644 --- a/src/code_info/symbol_references.rs +++ b/src/code_info/symbol_references.rs @@ -336,7 +336,7 @@ impl SymbolReferences { referenced_symbols_and_members } - pub fn back_references(&self) -> FxHashMap<(StrId, StrId), FxHashSet<&(StrId, StrId)>> { + pub fn back_references(&self) -> FxHashMap<(StrId, StrId), FxHashSet<(StrId, StrId)>> { let mut referenced_symbols_and_members = FxHashMap::default(); for (reference, symbol_references_to_symbols) in &self.symbol_references_to_symbols { @@ -344,7 +344,7 @@ impl SymbolReferences { let v = referenced_symbols_and_members .entry(*r) .or_insert_with(FxHashSet::default); - v.insert(reference); + v.insert(*reference); } } @@ -355,7 +355,7 @@ impl SymbolReferences { let v = referenced_symbols_and_members .entry(*r) .or_insert_with(FxHashSet::default); - v.insert(reference); + v.insert(*reference); } } @@ -404,13 +404,18 @@ impl SymbolReferences { referenced_symbols_and_members } - pub fn get_referenced_overridden_class_members(&self) -> FxHashSet<&(StrId, StrId)> { - let mut referenced_class_members = FxHashSet::default(); + pub fn get_referenced_overridden_class_members( + &self, + ) -> FxHashMap<(StrId, StrId), FxHashSet<(StrId, StrId)>> { + let mut referenced_class_members = FxHashMap::default(); - for symbol_references_to_class_members in - self.symbol_references_to_overridden_members.values() - { - referenced_class_members.extend(symbol_references_to_class_members); + for (k, refs) in &self.symbol_references_to_overridden_members { + for r in refs { + let v = referenced_class_members + .entry(*r) + .or_insert_with(FxHashSet::default); + v.insert(*k); + } } referenced_class_members diff --git a/src/code_info_builder/classlike_scanner.rs b/src/code_info_builder/classlike_scanner.rs index 2c927d2b..813ee95a 100644 --- a/src/code_info_builder/classlike_scanner.rs +++ b/src/code_info_builder/classlike_scanner.rs @@ -55,11 +55,13 @@ pub(crate) fn scan( start_column: definition_location.start_column, }; + let mut suppressed_issues = vec![]; + adjust_location_from_comments( comments, &mut meta_start, file_source, - &mut vec![], + &mut suppressed_issues, all_custom_issues, ); @@ -81,7 +83,9 @@ pub(crate) fn scan( storage.uses_position = uses_position; storage.namespace_position = namespace_position; - storage.is_production_code = file_source.is_production_code; + storage.is_production_code &= file_source.is_production_code; + + storage.suppressed_issues = suppressed_issues; if !classlike_node.tparams.is_empty() { let mut type_context = TypeResolutionContext { @@ -582,6 +586,10 @@ pub(crate) fn scan( storage.immutable = true; } + if name == StrId::HAKANA_TEST_ONLY { + storage.is_production_code = false; + } + storage.attributes.push(AttributeInfo { name }); if name == StrId::SEALED { diff --git a/src/code_info_builder/functionlike_scanner.rs b/src/code_info_builder/functionlike_scanner.rs index d8d281c7..578f22ec 100644 --- a/src/code_info_builder/functionlike_scanner.rs +++ b/src/code_info_builder/functionlike_scanner.rs @@ -77,7 +77,7 @@ pub(crate) fn scan_method( user_defined, ); - functionlike_info.is_production_code = file_source.is_production_code; + functionlike_info.is_production_code &= file_source.is_production_code; if classlike_name == StrId::BUILTIN_ENUM && (method_name == StrId::COERCE @@ -362,6 +362,9 @@ pub(crate) fn get_functionlike( StrId::HAKANA_SECURITY_ANALYSIS_IGNORE_PATH => { functionlike_info.ignore_taint_path = true; } + StrId::HAKANA_TEST_ONLY => { + functionlike_info.is_production_code = false; + } StrId::HAKANA_SECURITY_ANALYSIS_IGNORE_PATH_IF_TRUE => { functionlike_info.ignore_taints_if_true = true; } @@ -410,9 +413,7 @@ pub(crate) fn get_functionlike( functionlike_info.name_location = Some(HPos::new(name_pos, file_source.file_path)); } - if !suppressed_issues.is_empty() { - functionlike_info.suppressed_issues = Some(suppressed_issues); - } + functionlike_info.suppressed_issues = suppressed_issues; functionlike_info.is_async = fun_kind.is_async(); functionlike_info.effects = if let Some(contexts) = contexts { diff --git a/src/file_scanner_analyzer/scanner.rs b/src/file_scanner_analyzer/scanner.rs index d9190726..29c975b2 100644 --- a/src/file_scanner_analyzer/scanner.rs +++ b/src/file_scanner_analyzer/scanner.rs @@ -277,11 +277,18 @@ pub fn scan_files( group_size = 1; } - let test_patterns = config - .test_files - .iter() - .map(|ignore_file| glob::Pattern::new(ignore_file).unwrap()) - .collect::>(); + let mut test_patterns = vec![]; + + for ignore_file in &config.test_files { + match glob::Pattern::new(ignore_file) { + Ok(glob) => { + test_patterns.push(glob); + } + Err(error) => { + panic!("Parsing {} resulted in error {}", ignore_file, error); + } + } + } for (i, str_path) in files_to_scan.into_iter().enumerate() { let group = i % group_size; diff --git a/src/file_scanner_analyzer/unused_symbols.rs b/src/file_scanner_analyzer/unused_symbols.rs index f0d7fb68..30a5b5c2 100644 --- a/src/file_scanner_analyzer/unused_symbols.rs +++ b/src/file_scanner_analyzer/unused_symbols.rs @@ -24,13 +24,51 @@ pub(crate) fn find_unused_definitions( return; } - let referenced_symbols_and_members = analysis_result - .symbol_references - .get_referenced_symbols_and_members(); + let referenced_symbols_and_members = analysis_result.symbol_references.back_references(); + let mut test_symbols = codebase + .classlike_infos + .iter() + .filter(|(_, c)| c.user_defined && !c.is_production_code) + .map(|(k, _)| (*k, StrId::EMPTY)) + .collect::>(); + test_symbols.extend( + codebase + .functionlike_infos + .iter() + .filter(|(_, c)| c.user_defined && !c.is_production_code) + .map(|(k, _)| *k), + ); + + let mut referenced_symbols_and_members_in_production = FxHashSet::default(); + + for (k, v) in referenced_symbols_and_members.clone().into_iter() { + if !v.is_subset(&test_symbols) { + referenced_symbols_and_members_in_production.insert(k); + } + } + + let referenced_symbols_and_members = referenced_symbols_and_members + .into_iter() + .map(|(k, _)| k) + .collect::>(); + let referenced_overridden_class_members = analysis_result .symbol_references .get_referenced_overridden_class_members(); + let mut referenced_overridden_class_members_in_production = FxHashSet::default(); + + for (k, v) in referenced_overridden_class_members.clone().into_iter() { + if !v.is_subset(&test_symbols) { + referenced_overridden_class_members_in_production.insert(k); + } + } + + let referenced_overridden_class_members = referenced_overridden_class_members + .into_iter() + .map(|(k, _)| k) + .collect::>(); + 'outer1: for (functionlike_name, functionlike_info) in &codebase.functionlike_infos { if functionlike_name.1 == StrId::EMPTY && functionlike_info.user_defined @@ -49,13 +87,12 @@ pub(crate) fn find_unused_definitions( } if !referenced_symbols_and_members.contains(&functionlike_name) { - if let Some(suppressed_issues) = &functionlike_info.suppressed_issues { - if suppressed_issues - .iter() - .any(|(i, _)| i == &IssueKind::UnusedFunction) - { - continue; - } + if functionlike_info + .suppressed_issues + .iter() + .any(|(i, _)| i == &IssueKind::UnusedFunction) + { + continue; } if !config.allow_issue_kind_in_file(&IssueKind::UnusedFunction, file_path) { @@ -87,6 +124,31 @@ pub(crate) fn find_unused_definitions( &Some(FunctionLikeIdentifier::Function(functionlike_name.0)), ); + if config.can_add_issue(&issue) { + *analysis_result + .issue_counts + .entry(issue.kind.clone()) + .or_insert(0) += 1; + analysis_result + .emitted_definition_issues + .entry(pos.file_path) + .or_default() + .push(issue); + } + } else if functionlike_info.is_production_code + && !referenced_symbols_and_members_in_production.contains(&functionlike_name) + && config.allow_issue_kind_in_file(&IssueKind::OnlyUsedInTests, file_path) + { + let issue = Issue::new( + IssueKind::OnlyUsedInTests, + format!( + "Production-code function {} is only used in tests — if this is deliberate add the <> attribute", + interner.lookup(&functionlike_name.0) + ), + *pos, + &Some(FunctionLikeIdentifier::Function(functionlike_name.0)), + ); + if config.can_add_issue(&issue) { *analysis_result .issue_counts @@ -168,7 +230,40 @@ pub(crate) fn find_unused_definitions( .push(issue); } } else { - 'inner: for method_name_ptr in &classlike_info.methods { + let mut classlike_only_used_in_tests = false; + + if classlike_info.is_production_code + && !referenced_symbols_and_members_in_production + .contains(&(*classlike_name, StrId::EMPTY)) + { + classlike_only_used_in_tests = true; + + if config.allow_issue_kind_in_file(&IssueKind::OnlyUsedInTests, file_path) { + let issue = Issue::new( + IssueKind::OnlyUsedInTests, + format!( + "Production-code class {} is only used in tests — if this is deliberate add the <> attribute", + interner.lookup(&classlike_name) + ), + *pos, + &Some(FunctionLikeIdentifier::Function(*classlike_name)), + ); + + if config.can_add_issue(&issue) { + *analysis_result + .issue_counts + .entry(issue.kind.clone()) + .or_insert(0) += 1; + analysis_result + .emitted_definition_issues + .entry(pos.file_path) + .or_default() + .push(issue); + } + } + } + + for method_name_ptr in &classlike_info.methods { if *method_name_ptr != StrId::EMPTY { let method_name = interner.lookup(method_name_ptr); @@ -182,47 +277,16 @@ pub(crate) fn find_unused_definitions( if !referenced_symbols_and_members.contains(&pair) && !referenced_overridden_class_members.contains(&pair) { - if has_upstream_method_call( - classlike_info, + if is_method_referenced_somewhere_else( + classlike_name, method_name_ptr, + codebase, + classlike_info, &referenced_symbols_and_members, ) { continue; } - for descendant_classlike in codebase.get_all_descendants(classlike_name) { - if let Some(descendant_classlike_storage) = - codebase.classlike_infos.get(&descendant_classlike) - { - for parent_interface in - &descendant_classlike_storage.all_class_interfaces - { - if referenced_symbols_and_members - .contains(&(*parent_interface, *method_name_ptr)) - { - continue 'inner; - } - } - } - } - - for trait_user in get_trait_users( - classlike_name, - &codebase.symbols, - &codebase.all_classlike_descendants, - ) { - if let Some(classlike_info) = codebase.classlike_infos.get(&trait_user) - { - if has_upstream_method_call( - classlike_info, - method_name_ptr, - &referenced_symbols_and_members, - ) { - continue 'inner; - } - } - } - let functionlike_storage = codebase .functionlike_infos .get(&(*classlike_name, *method_name_ptr)) @@ -230,13 +294,12 @@ pub(crate) fn find_unused_definitions( let method_storage = functionlike_storage.method_info.as_ref().unwrap(); - if let Some(suppressed_issues) = &functionlike_storage.suppressed_issues { - if suppressed_issues - .iter() - .any(|(i, _)| i == &IssueKind::UnusedPrivateMethod) - { - continue; - } + if functionlike_storage + .suppressed_issues + .iter() + .any(|(i, _)| i == &IssueKind::UnusedFunction) + { + continue; } // allow one-liner private construct statements that prevent instantiation @@ -299,6 +362,49 @@ pub(crate) fn find_unused_definitions( .or_default() .push(issue); } + } else { + if !classlike_only_used_in_tests + && classlike_info.is_production_code + && config + .allow_issue_kind_in_file(&IssueKind::OnlyUsedInTests, file_path) + && !classlike_info + .suppressed_issues + .iter() + .any(|(issue, _)| matches!(issue, IssueKind::OnlyUsedInTests)) + && !referenced_symbols_and_members_in_production + .contains(&(*classlike_name, *method_name_ptr)) + && !referenced_overridden_class_members_in_production.contains(&pair) + && !is_method_referenced_somewhere_else( + classlike_name, + method_name_ptr, + codebase, + classlike_info, + &referenced_symbols_and_members_in_production, + ) + { + let issue = Issue::new( + IssueKind::OnlyUsedInTests, + format!( + "Production-code method {}::{} is only used in tests — if this is deliberate add the <> attribute", + interner.lookup(&classlike_name), + interner.lookup(&method_name_ptr) + ), + *pos, + &Some(FunctionLikeIdentifier::Method(*classlike_name, *method_name_ptr)), + ); + + if config.can_add_issue(&issue) { + *analysis_result + .issue_counts + .entry(issue.kind.clone()) + .or_insert(0) += 1; + analysis_result + .emitted_definition_issues + .entry(pos.file_path) + .or_default() + .push(issue); + } + } } } @@ -453,10 +559,55 @@ pub(crate) fn find_unused_definitions( } } +fn is_method_referenced_somewhere_else( + classlike_name: &StrId, + method_name_ptr: &StrId, + codebase: &CodebaseInfo, + classlike_info: &ClassLikeInfo, + referenced_symbols_and_members: &FxHashSet<(StrId, StrId)>, +) -> bool { + if has_upstream_method_call( + classlike_info, + method_name_ptr, + referenced_symbols_and_members, + ) { + return true; + } + for descendant_classlike in codebase.get_all_descendants(classlike_name) { + if let Some(descendant_classlike_storage) = + codebase.classlike_infos.get(&descendant_classlike) + { + for parent_interface in &descendant_classlike_storage.all_class_interfaces { + if referenced_symbols_and_members.contains(&(*parent_interface, *method_name_ptr)) { + return true; + } + } + } + } + + for trait_user in get_trait_users( + classlike_name, + &codebase.symbols, + &codebase.all_classlike_descendants, + ) { + if let Some(classlike_info) = codebase.classlike_infos.get(&trait_user) { + if has_upstream_method_call( + classlike_info, + method_name_ptr, + referenced_symbols_and_members, + ) { + return true; + } + } + } + + false +} + fn has_upstream_method_call( classlike_info: &ClassLikeInfo, method_name_ptr: &StrId, - referenced_class_members: &FxHashSet<&(StrId, StrId)>, + referenced_class_members: &FxHashSet<(StrId, StrId)>, ) -> bool { if let Some(parent_elements) = classlike_info.overridden_method_ids.get(method_name_ptr) { for parent_element in parent_elements { diff --git a/src/str/build.rs b/src/str/build.rs index 8d70bffc..688350e0 100644 --- a/src/str/build.rs +++ b/src/str/build.rs @@ -249,6 +249,7 @@ fn main() -> Result<()> { "Hakana\\SecurityAnalysis\\Source", "Hakana\\SecurityAnalysis\\SpecializeCall", "Hakana\\SpecialTypes\\LiteralString", + "Hakana\\TestOnly", "NumberFormatter", "SimpleXMLElement", "XHPChild", diff --git a/src/ttype/type_comparator/atomic_type_comparator.rs b/src/ttype/type_comparator/atomic_type_comparator.rs index 31287bc3..ca353ff4 100644 --- a/src/ttype/type_comparator/atomic_type_comparator.rs +++ b/src/ttype/type_comparator/atomic_type_comparator.rs @@ -734,9 +734,11 @@ pub fn is_contained_by( if let Some(input_class_name) = input_class_name { return input_class_name == container_enum_name; } else { - let classlike_info = - codebase.classlike_infos.get(container_enum_name).unwrap(); - return classlike_info.constants.contains_key(input_member_name); + if let Some(classlike_info) = + codebase.classlike_infos.get(container_enum_name) + { + return classlike_info.constants.contains_key(input_member_name); + } } } else { return false;