From 63d619ae2c67f425bc174c66803f4d3c00ee054d Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Sat, 15 Jun 2024 16:06:58 -0400 Subject: [PATCH] Improve data flow to arrays --- .../expr/binop/assignment_analyzer.rs | 10 +- src/analyzer/expr/collection_analyzer.rs | 55 +++++++- src/analyzer/expr/shape_analyzer.rs | 18 ++- src/analyzer/stmt_analyzer.rs | 127 ++++++++++-------- src/code_info/data_flow/graph.rs | 10 +- 5 files changed, 155 insertions(+), 65 deletions(-) diff --git a/src/analyzer/expr/binop/assignment_analyzer.rs b/src/analyzer/expr/binop/assignment_analyzer.rs index 6e9d9de1..8587ce23 100644 --- a/src/analyzer/expr/binop/assignment_analyzer.rs +++ b/src/analyzer/expr/binop/assignment_analyzer.rs @@ -192,11 +192,11 @@ pub(crate) fn analyze( let mut origin_node_ids = vec![]; for parent_node in &existing_var_type.parent_nodes { - origin_node_ids.extend( - analysis_data - .data_flow_graph - .get_origin_node_ids(&parent_node.id, vec![]), - ); + origin_node_ids.extend(analysis_data.data_flow_graph.get_origin_node_ids( + &parent_node.id, + vec![], + false, + )); } origin_node_ids.retain(|id| { diff --git a/src/analyzer/expr/collection_analyzer.rs b/src/analyzer/expr/collection_analyzer.rs index 971721b4..a2be0f39 100644 --- a/src/analyzer/expr/collection_analyzer.rs +++ b/src/analyzer/expr/collection_analyzer.rs @@ -171,7 +171,23 @@ pub(crate) fn analyze_vals( } }); - new_vec.parent_nodes = array_creation_info.parent_nodes; + if !array_creation_info.parent_nodes.is_empty() { + let vec_node = DataFlowNode::get_for_composition(statements_analyzer.get_hpos(pos)); + + for child_node in array_creation_info.parent_nodes { + analysis_data.data_flow_graph.add_path( + &child_node, + &vec_node, + PathKind::Default, + vec![], + vec![], + ); + } + + analysis_data.data_flow_graph.add_node(vec_node.clone()); + + new_vec.parent_nodes = vec![vec_node]; + } analysis_data.set_expr_type(pos, new_vec); } @@ -184,7 +200,24 @@ pub(crate) fn analyze_vals( let mut keyset = get_keyset(item_value_type); - keyset.parent_nodes = array_creation_info.parent_nodes; + if !array_creation_info.parent_nodes.is_empty() { + let keyset_node = + DataFlowNode::get_for_composition(statements_analyzer.get_hpos(pos)); + + for child_node in array_creation_info.parent_nodes { + analysis_data.data_flow_graph.add_path( + &child_node, + &keyset_node, + PathKind::Default, + vec![], + vec![], + ); + } + + analysis_data.data_flow_graph.add_node(keyset_node.clone()); + + keyset.parent_nodes = vec![keyset_node]; + } analysis_data.set_expr_type(pos, keyset); } @@ -296,7 +329,23 @@ pub(crate) fn analyze_keyvals( shape_name: None, }); - new_dict.parent_nodes = array_creation_info.parent_nodes; + if !array_creation_info.parent_nodes.is_empty() { + let dict_node = DataFlowNode::get_for_composition(statements_analyzer.get_hpos(pos)); + + for child_node in array_creation_info.parent_nodes { + analysis_data.data_flow_graph.add_path( + &child_node, + &dict_node, + PathKind::Default, + vec![], + vec![], + ); + } + + analysis_data.data_flow_graph.add_node(dict_node.clone()); + + new_dict.parent_nodes = vec![dict_node]; + } analysis_data.set_expr_type(pos, new_dict); diff --git a/src/analyzer/expr/shape_analyzer.rs b/src/analyzer/expr/shape_analyzer.rs index 616e3f30..cf2aca99 100644 --- a/src/analyzer/expr/shape_analyzer.rs +++ b/src/analyzer/expr/shape_analyzer.rs @@ -174,7 +174,23 @@ pub(crate) fn analyze( shape_name: None, }); - new_dict.parent_nodes = parent_nodes; + if !parent_nodes.is_empty() { + let dict_node = DataFlowNode::get_for_composition(statements_analyzer.get_hpos(pos)); + + for child_node in parent_nodes { + analysis_data.data_flow_graph.add_path( + &child_node, + &dict_node, + PathKind::Default, + vec![], + vec![], + ); + } + + analysis_data.data_flow_graph.add_node(dict_node.clone()); + + new_dict.parent_nodes = vec![dict_node]; + } analysis_data.set_expr_type(pos, new_dict); diff --git a/src/analyzer/stmt_analyzer.rs b/src/analyzer/stmt_analyzer.rs index 5334fd54..5667d11f 100644 --- a/src/analyzer/stmt_analyzer.rs +++ b/src/analyzer/stmt_analyzer.rs @@ -308,66 +308,87 @@ fn detect_unused_statement_expressions( } } - 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, - ); + match &boxed.2 { + aast::Expr_::Call(boxed_call) => { + 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 - .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 !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); - - if let Some((_, value_type)) = array_types { - if !value_type.is_null() && !value_type.is_void() { - analysis_data.maybe_add_issue( - Issue::new( - IssueKind::UnusedBuiltinReturnValue, - format!( - "The value {} returned from {} should be consumed", - expr_type - .get_id(Some(statements_analyzer.get_interner())), - function_name + if let Some(FunctionLikeIdentifier::Function(function_id)) = functionlike_id { + let codebase = statements_analyzer.get_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 !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); + + if let Some((_, value_type)) = array_types { + if !value_type.is_null() && !value_type.is_void() { + analysis_data.maybe_add_issue( + Issue::new( + IssueKind::UnusedBuiltinReturnValue, + format!( + "The value {} returned from {} should be consumed", + expr_type.get_id(Some( + statements_analyzer.get_interner() + )), + function_name + ), + statements_analyzer.get_hpos(&stmt.0), + &context.function_context.calling_functionlike_id, ), - statements_analyzer.get_hpos(&stmt.0), - &context.function_context.calling_functionlike_id, - ), - statements_analyzer.get_config(), - statements_analyzer.get_file_path_actual(), - ); + statements_analyzer.get_config(), + statements_analyzer.get_file_path_actual(), + ); + } } } } } } - } - if let Some(expr_type) = analysis_data.get_rc_expr_type(boxed.pos()).cloned() { - if expr_type.has_awaitable_types() { - analysis_data.maybe_add_issue( - Issue::new( - IssueKind::UnusedAwaitable, - "This awaitable is never awaited".to_string(), - statements_analyzer.get_hpos(&stmt.0), - &context.function_context.calling_functionlike_id, - ), - statements_analyzer.get_config(), - statements_analyzer.get_file_path_actual(), - ); + if let Some(expr_type) = analysis_data.get_rc_expr_type(boxed.pos()).cloned() { + if expr_type.has_awaitable_types() { + analysis_data.maybe_add_issue( + Issue::new( + IssueKind::UnusedAwaitable, + "This awaitable is never awaited".to_string(), + statements_analyzer.get_hpos(&stmt.0), + &context.function_context.calling_functionlike_id, + ), + statements_analyzer.get_config(), + statements_analyzer.get_file_path_actual(), + ); + } } } + aast::Expr_::Collection(_) + | aast::Expr_::ValCollection(_) + | aast::Expr_::KeyValCollection(_) + | aast::Expr_::ArrayGet(_) + | aast::Expr_::Shape(_) + | aast::Expr_::Tuple(_) => { + analysis_data.maybe_add_issue( + Issue::new( + IssueKind::UnusedStatement, + "This statement includes an expression that has no effect".to_string(), + statements_analyzer.get_hpos(&stmt.0), + &context.function_context.calling_functionlike_id, + ), + statements_analyzer.get_config(), + statements_analyzer.get_file_path_actual(), + ); + } + _ => (), } } @@ -458,11 +479,7 @@ fn analyze_awaitall( let parent_nodes = t.parent_nodes.clone(); if t.is_single() { let inner = t.get_single(); - if let TAtomic::TAwaitable { - value, - .. - } = inner - { + if let TAtomic::TAwaitable { value, .. } = inner { let mut new = (**value).clone(); new.parent_nodes = parent_nodes; diff --git a/src/code_info/data_flow/graph.rs b/src/code_info/data_flow/graph.rs index 489a7227..721453d4 100644 --- a/src/code_info/data_flow/graph.rs +++ b/src/code_info/data_flow/graph.rs @@ -152,6 +152,7 @@ impl DataFlowGraph { &self, assignment_node_id: &DataFlowNodeId, ignore_paths: Vec, + var_ids_only: bool, ) -> Vec { let mut visited_child_ids = FxHashSet::default(); @@ -175,6 +176,13 @@ impl DataFlowGraph { visited_child_ids.insert(child_node_id.clone()); + if var_ids_only { + if let DataFlowNodeId::Var(..) | DataFlowNodeId::Param(..) = child_node_id { + origin_nodes.push(child_node_id); + continue; + } + } + let mut new_parent_nodes = FxHashSet::default(); let mut has_visited_a_parent_already = false; @@ -233,7 +241,7 @@ impl DataFlowGraph { } pub fn add_mixed_data(&mut self, assignment_node: &DataFlowNode, pos: &Pos) { - let origin_node_ids = self.get_origin_node_ids(&assignment_node.id, vec![]); + let origin_node_ids = self.get_origin_node_ids(&assignment_node.id, vec![], false); for origin_node_id in origin_node_ids { if let DataFlowNodeId::CallTo(..) | DataFlowNodeId::SpecializedCallTo(..) =