Skip to content

Commit

Permalink
Fix control flow for while(true) with break
Browse files Browse the repository at this point in the history
  • Loading branch information
muglug committed May 10, 2024
1 parent 01b3fdc commit c11fe2e
Show file tree
Hide file tree
Showing 15 changed files with 175 additions and 134 deletions.
31 changes: 16 additions & 15 deletions src/analyzer/expr/call/function_call_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,22 +202,23 @@ pub(crate) fn analyze(
&functionlike_id,
);

if !function_storage.is_production_code && function_storage.user_defined {
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_hpos(pos),
&context.function_context.calling_functionlike_id,
if !function_storage.is_production_code
&& function_storage.user_defined
&& 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(),
)
}

let stmt_type = function_call_return_type_fetcher::fetch(
Expand Down
31 changes: 16 additions & 15 deletions src/analyzer/expr/call_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,22 +416,23 @@ pub(crate) fn check_method_args(
check_template_result(statements_analyzer, template_result, pos, &functionlike_id);
}

if !functionlike_storage.is_production_code && functionlike_storage.user_defined {
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_hpos(pos),
&context.function_context.calling_functionlike_id,
if !functionlike_storage.is_production_code
&& functionlike_storage.user_defined
&& 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(),
)
}

Ok(())
Expand Down
12 changes: 5 additions & 7 deletions src/analyzer/expr/expression_identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,16 +247,14 @@ pub fn get_method_id_from_call(
};

if let aast::Expr_::Id(method_name_node) = &rhs_expr.2 {
if let Some(method_id) = interner.get(&method_name_node.1) {
return Some(FunctionLikeIdentifier::Method(*class_id, method_id));
} else {
return None;
}
interner
.get(&method_name_node.1)
.map(|method_id| FunctionLikeIdentifier::Method(*class_id, method_id))
} else {
return None;
None
}
}
_ => return None,
_ => None,
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/analyzer/function_analysis_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,10 +362,11 @@ impl FunctionAnalysisData {
}
_ => {}
},
4107 => match &issue_kind {
IssueKind::NonExistentFunction => return true,
_ => {}
},
4107 => {
if let IssueKind::NonExistentFunction = &issue_kind {
return true;
}
}
_ => {}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/analyzer/scope_context/control_action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pub enum ControlAction {
End,
Break,
BreakImmediateLoop,
Continue,
LeaveSwitch,
None,
Expand Down
6 changes: 3 additions & 3 deletions src/analyzer/scope_context/loop_scope.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{collections::BTreeMap, rc::Rc};

use hakana_reflection_info::t_union::TUnion;
use rustc_hash::FxHashMap;
use rustc_hash::{FxHashMap, FxHashSet};

use super::control_action::ControlAction;

Expand All @@ -19,7 +19,7 @@ pub struct LoopScope {

pub possibly_defined_loop_parent_vars: FxHashMap<String, TUnion>,

pub final_actions: Vec<ControlAction>,
pub final_actions: FxHashSet<ControlAction>,
}

impl LoopScope {
Expand All @@ -31,7 +31,7 @@ impl LoopScope {
possibly_redefined_loop_vars: FxHashMap::default(),
possibly_redefined_loop_parent_vars: FxHashMap::default(),
possibly_defined_loop_parent_vars: FxHashMap::default(),
final_actions: Vec::new(),
final_actions: FxHashSet::default(),
}
}
}
12 changes: 4 additions & 8 deletions src/analyzer/stmt/break_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ pub(crate) fn analyze(
} else {
false
} {
loop_scope.final_actions.push(ControlAction::LeaveSwitch);
loop_scope.final_actions.insert(ControlAction::LeaveSwitch);
} else {
leaving_switch = false;
loop_scope.final_actions.push(ControlAction::Break);
loop_scope.final_actions.insert(ControlAction::Break);
}

for (var_id, var_type) in &context.vars_in_scope {
Expand Down Expand Up @@ -70,12 +70,8 @@ pub(crate) fn analyze(
let mut finally_scope = (*finally_scope).borrow_mut();
for (var_id, var_type) in &context.vars_in_scope {
if let Some(finally_type) = finally_scope.vars_in_scope.get_mut(var_id) {
*finally_type = Rc::new(combine_union_types(
finally_type,
var_type,
codebase,
false,
));
*finally_type =
Rc::new(combine_union_types(finally_type, var_type, codebase, false));
} else {
finally_scope
.vars_in_scope
Expand Down
2 changes: 1 addition & 1 deletion src/analyzer/stmt/continue_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub(crate) fn analyze(
) {
let codebase = statements_analyzer.get_codebase();
if let Some(loop_scope) = loop_scope {
loop_scope.final_actions.push(ControlAction::Continue);
loop_scope.final_actions.insert(ControlAction::Continue);

let mut removed_var_ids = FxHashSet::default();

Expand Down
84 changes: 45 additions & 39 deletions src/analyzer/stmt/control_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,15 @@ pub(crate) fn get_control_actions(
}
}
aast::Stmt_::Break => {
if !break_context.is_empty() {
if let &BreakContext::Switch = break_context.last().unwrap() {
if !control_actions.contains(&ControlAction::LeaveSwitch) {
control_actions.insert(ControlAction::LeaveSwitch);
if let Some(last_context) = break_context.last() {
match last_context {
&BreakContext::Switch => {
if !control_actions.contains(&ControlAction::LeaveSwitch) {
control_actions.insert(ControlAction::LeaveSwitch);
}
}
BreakContext::Loop => {
control_actions.insert(ControlAction::BreakImmediateLoop);
}
}

Expand Down Expand Up @@ -160,12 +165,40 @@ pub(crate) fn get_control_actions(

// check for infinite loop behaviour
if let Some(types) = analysis_data {
if stmt.1.is_while() {
let stmt = stmt.1.as_while().unwrap();
match &stmt.1 {
aast::Stmt_::While(boxed) => {
if let Some(expr_type) = types.get_expr_type(&boxed.0 .1) {
if expr_type.is_always_truthy() {
//infinite while loop that only return don't have an exit path
let loop_only_ends = control_actions
.iter()
.filter(|action| {
*action != &ControlAction::End
&& *action != &ControlAction::Return
})
.count()
== 0;

if loop_only_ends {
return control_actions;
}
}
}
}
aast::Stmt_::For(boxed) => {
let mut is_infinite_loop = true;

if let Some(expr_type) = types.get_expr_type(&stmt.0 .1) {
if expr_type.is_always_truthy() {
//infinite while loop that only return don't have an exit path
if let Some(for_cond) = &boxed.1 {
if let Some(expr_type) = types.get_expr_type(&for_cond.1) {
if !expr_type.is_always_truthy() {
is_infinite_loop = false
}
} else {
is_infinite_loop = false;
}
}

if is_infinite_loop {
let loop_only_ends = control_actions
.iter()
.filter(|action| {
Expand All @@ -180,38 +213,11 @@ pub(crate) fn get_control_actions(
}
}
}
}

if stmt.1.is_for() {
let stmt = stmt.1.as_for().unwrap();
let mut is_infinite_loop = true;

if let Some(for_cond) = stmt.1 {
if let Some(expr_type) = types.get_expr_type(&for_cond.1) {
if !expr_type.is_always_truthy() {
is_infinite_loop = false
}
} else {
is_infinite_loop = false;
}
}

if is_infinite_loop {
let loop_only_ends = control_actions
.iter()
.filter(|action| {
*action != &ControlAction::End
&& *action != &ControlAction::Return
})
.count()
== 0;

if loop_only_ends {
return control_actions;
}
}
_ => {}
}
}

control_actions.retain(|action| action != &ControlAction::BreakImmediateLoop);
}
aast::Stmt_::Switch(_) => {
let mut has_ended = false;
Expand Down
2 changes: 1 addition & 1 deletion src/analyzer/stmt/try_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ pub(crate) fn analyze(

if let Some(loop_scope) = loop_scope {
if !try_leaves_loop && !loop_scope.final_actions.contains(&ControlAction::None) {
loop_scope.final_actions.push(ControlAction::None);
loop_scope.final_actions.insert(ControlAction::None);
}
}

Expand Down
56 changes: 30 additions & 26 deletions src/analyzer/stmt/while_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,35 +53,39 @@ pub(crate) fn analyze(

let can_leave_loop = !while_true || loop_scope.final_actions.contains(&ControlAction::Break);

if always_enters_loop && can_leave_loop {
let has_break_or_continue = loop_scope.final_actions.contains(&ControlAction::Break)
|| loop_scope.final_actions.contains(&ControlAction::Continue);
if always_enters_loop {
if can_leave_loop {
let has_break_or_continue = loop_scope.final_actions.contains(&ControlAction::Break)
|| loop_scope.final_actions.contains(&ControlAction::Continue);

for (var_id, var_type) in inner_loop_context.vars_in_scope {
// if there are break statements in the loop it's not certain
// that the loop has finished executing, so the assertions at the end
// the loop in the while conditional may not hold
if has_break_or_continue {
if let Some(possibly_defined_type) = loop_scope
.clone()
.possibly_defined_loop_parent_vars
.get(&var_id)
{
context.vars_in_scope.insert(
var_id,
Rc::new(hakana_type::combine_union_types(
&var_type,
possibly_defined_type,
codebase,
false,
)),
);
for (var_id, var_type) in inner_loop_context.vars_in_scope {
// if there are break statements in the loop it's not certain
// that the loop has finished executing, so the assertions at the end
// the loop in the while conditional may not hold
if has_break_or_continue {
if let Some(possibly_defined_type) = loop_scope
.clone()
.possibly_defined_loop_parent_vars
.get(&var_id)
{
context.vars_in_scope.insert(
var_id,
Rc::new(hakana_type::combine_union_types(
&var_type,
possibly_defined_type,
codebase,
false,
)),
);
}
} else {
context
.vars_in_scope
.insert(var_id.clone(), var_type.clone());
}
} else {
context
.vars_in_scope
.insert(var_id.clone(), var_type.clone());
}
} else {
context.has_returned = true;
}
}

Expand Down
Loading

0 comments on commit c11fe2e

Please sign in to comment.