From b8946696e0292340c531d4ac26c6ac1b3bc3c240 Mon Sep 17 00:00:00 2001 From: Caleb White Date: Thu, 18 Jun 2026 06:48:12 -0500 Subject: [PATCH] fix(unused-variables): treat get_defined_vars() as using all variables in scope `get_defined_vars()` (like `compact()`) references variables by name without a direct read. When present in a function/method body, suppress unused-variable diagnostics for all locals to avoid false positives on debug/introspection code. --- src/diagnostics/undefined_variables.rs | 309 +++++++++++++++++++++++++ src/diagnostics/unused_variables.rs | 87 ++++++- 2 files changed, 393 insertions(+), 3 deletions(-) diff --git a/src/diagnostics/undefined_variables.rs b/src/diagnostics/undefined_variables.rs index b3de01e2..58b2fd3b 100644 --- a/src/diagnostics/undefined_variables.rs +++ b/src/diagnostics/undefined_variables.rs @@ -1214,6 +1214,315 @@ fn collect_compact_from_expr(expr: &Expression<'_>, vars: &mut HashSet) } } +// ─── get_defined_vars() detection ─────────────────────────────────────────── + +/// Returns true if the statements contain a call to `get_defined_vars()`. +/// When present in a scope, all variables defined in that scope are +/// considered used (e.g. for debug dumps), so unused-variable diagnostics +/// should be suppressed for them. +pub(crate) fn has_get_defined_vars(statements: &[Statement<'_>]) -> bool { + for stmt in statements { + if stmt_has_get_defined_vars(stmt) { + return true; + } + } + false +} + +fn stmt_has_get_defined_vars(stmt: &Statement<'_>) -> bool { + match stmt { + Statement::Expression(es) => expr_has_get_defined_vars(es.expression), + Statement::Return(ret) => ret.value.is_some_and(|v| expr_has_get_defined_vars(v)), + Statement::Echo(echo) => echo.values.iter().any(|v| expr_has_get_defined_vars(v)), + Statement::If(if_stmt) => { + if expr_has_get_defined_vars(if_stmt.condition) { + return true; + } + match &if_stmt.body { + IfBody::Statement(body) => { + if stmt_has_get_defined_vars(body.statement) { + return true; + } + for clause in body.else_if_clauses.iter() { + if expr_has_get_defined_vars(clause.condition) + || stmt_has_get_defined_vars(clause.statement) + { + return true; + } + } + if let Some(ref el) = body.else_clause + && stmt_has_get_defined_vars(el.statement) + { + return true; + } + } + IfBody::ColonDelimited(body) => { + for s in body.statements.iter() { + if stmt_has_get_defined_vars(s) { + return true; + } + } + for clause in body.else_if_clauses.iter() { + if expr_has_get_defined_vars(clause.condition) { + return true; + } + for s in clause.statements.iter() { + if stmt_has_get_defined_vars(s) { + return true; + } + } + } + if let Some(ref el) = body.else_clause { + for s in el.statements.iter() { + if stmt_has_get_defined_vars(s) { + return true; + } + } + } + } + } + false + } + Statement::Foreach(foreach) => { + expr_has_get_defined_vars(foreach.expression) + || match &foreach.body { + ForeachBody::Statement(s) => stmt_has_get_defined_vars(s), + ForeachBody::ColonDelimited(b) => { + b.statements.iter().any(|s| stmt_has_get_defined_vars(s)) + } + } + } + Statement::While(w) => { + expr_has_get_defined_vars(w.condition) + || match &w.body { + WhileBody::Statement(s) => stmt_has_get_defined_vars(s), + WhileBody::ColonDelimited(b) => { + b.statements.iter().any(|s| stmt_has_get_defined_vars(s)) + } + } + } + Statement::DoWhile(dw) => { + stmt_has_get_defined_vars(dw.statement) || expr_has_get_defined_vars(dw.condition) + } + Statement::For(for_stmt) => { + for_stmt + .initializations + .iter() + .any(|e| expr_has_get_defined_vars(e)) + || for_stmt + .conditions + .iter() + .any(|e| expr_has_get_defined_vars(e)) + || for_stmt + .increments + .iter() + .any(|e| expr_has_get_defined_vars(e)) + || match &for_stmt.body { + ForBody::Statement(s) => stmt_has_get_defined_vars(s), + ForBody::ColonDelimited(b) => { + b.statements.iter().any(|s| stmt_has_get_defined_vars(s)) + } + } + } + Statement::Switch(sw) => { + expr_has_get_defined_vars(sw.expression) + || sw.body.cases().iter().any(|c| match c { + SwitchCase::Expression(sc) => { + expr_has_get_defined_vars(sc.expression) + || sc.statements.iter().any(|s| stmt_has_get_defined_vars(s)) + } + SwitchCase::Default(dc) => { + dc.statements.iter().any(|s| stmt_has_get_defined_vars(s)) + } + }) + } + Statement::Try(try_stmt) => { + try_stmt + .block + .statements + .iter() + .any(|s| stmt_has_get_defined_vars(s)) + || try_stmt.catch_clauses.iter().any(|c| { + c.block + .statements + .iter() + .any(|s| stmt_has_get_defined_vars(s)) + }) + || try_stmt.finally_clause.as_ref().is_some_and(|f| { + f.block + .statements + .iter() + .any(|s| stmt_has_get_defined_vars(s)) + }) + } + Statement::Block(block) => block + .statements + .iter() + .any(|s| stmt_has_get_defined_vars(s)), + _ => false, + } +} + +fn expr_has_get_defined_vars(expr: &Expression<'_>) -> bool { + match expr { + Expression::Call(Call::Function(fc)) => { + if let Expression::Identifier(ident) = fc.function + && ident.value().eq_ignore_ascii_case(b"get_defined_vars") + { + return true; + } + expr_has_get_defined_vars(fc.function) + // Recurse into arguments for nested calls. + || fc + .argument_list + .arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + } + Expression::Call(Call::Method(mc)) => { + expr_has_get_defined_vars(mc.object) + || mc + .argument_list + .arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + } + Expression::Call(Call::NullSafeMethod(mc)) => { + expr_has_get_defined_vars(mc.object) + || mc + .argument_list + .arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + } + Expression::Call(Call::StaticMethod(sc)) => { + expr_has_get_defined_vars(sc.class) + || sc + .argument_list + .arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + } + Expression::Access(access) => match access { + Access::Property(pa) => expr_has_get_defined_vars(pa.object), + Access::NullSafeProperty(pa) => expr_has_get_defined_vars(pa.object), + Access::StaticProperty(spa) => expr_has_get_defined_vars(spa.class), + Access::ClassConstant(cca) => expr_has_get_defined_vars(cca.class), + }, + Expression::ArrayAccess(aa) => { + expr_has_get_defined_vars(aa.array) || expr_has_get_defined_vars(aa.index) + } + Expression::ArrayAppend(append) => expr_has_get_defined_vars(append.array), + Expression::Array(arr) => arr + .elements + .iter() + .any(|e| array_elem_has_get_defined_vars(e)), + Expression::LegacyArray(arr) => arr + .elements + .iter() + .any(|e| array_elem_has_get_defined_vars(e)), + Expression::List(list) => list + .elements + .iter() + .any(|e| array_elem_has_get_defined_vars(e)), + Expression::Assignment(a) => { + expr_has_get_defined_vars(a.lhs) || expr_has_get_defined_vars(a.rhs) + } + Expression::Binary(b) => { + expr_has_get_defined_vars(b.lhs) || expr_has_get_defined_vars(b.rhs) + } + Expression::UnaryPrefix(u) => expr_has_get_defined_vars(u.operand), + Expression::UnaryPostfix(u) => expr_has_get_defined_vars(u.operand), + Expression::Parenthesized(p) => expr_has_get_defined_vars(p.expression), + Expression::Conditional(c) => { + expr_has_get_defined_vars(c.condition) + || c.then.is_some_and(|t| expr_has_get_defined_vars(t)) + || expr_has_get_defined_vars(c.r#else) + } + Expression::Instantiation(inst) => { + expr_has_get_defined_vars(inst.class) + || inst.argument_list.as_ref().is_some_and(|al| { + al.arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + }) + } + Expression::Throw(t) => expr_has_get_defined_vars(t.exception), + Expression::Clone(c) => expr_has_get_defined_vars(c.object), + Expression::Yield(yield_expr) => match yield_expr { + Yield::Value(yv) => yv.value.is_some_and(expr_has_get_defined_vars), + Yield::Pair(yp) => { + expr_has_get_defined_vars(yp.key) || expr_has_get_defined_vars(yp.value) + } + Yield::From(yf) => expr_has_get_defined_vars(yf.iterator), + }, + Expression::Match(m) => { + expr_has_get_defined_vars(m.expression) + || m.arms.iter().any(|arm| match arm { + MatchArm::Expression(ea) => { + ea.conditions.iter().any(|c| expr_has_get_defined_vars(c)) + || expr_has_get_defined_vars(ea.expression) + } + MatchArm::Default(da) => expr_has_get_defined_vars(da.expression), + }) + } + Expression::Construct(construct) => match construct { + Construct::Isset(isset) => isset.values.iter().any(|v| expr_has_get_defined_vars(v)), + Construct::Empty(empty) => expr_has_get_defined_vars(empty.value), + Construct::Eval(eval) => expr_has_get_defined_vars(eval.value), + Construct::Include(inc) => expr_has_get_defined_vars(inc.value), + Construct::IncludeOnce(inc) => expr_has_get_defined_vars(inc.value), + Construct::Require(req) => expr_has_get_defined_vars(req.value), + Construct::RequireOnce(req) => expr_has_get_defined_vars(req.value), + Construct::Print(print) => expr_has_get_defined_vars(print.value), + Construct::Exit(exit) => exit.arguments.as_ref().is_some_and(|args| { + args.arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + }), + Construct::Die(die) => die.arguments.as_ref().is_some_and(|args| { + args.arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + }), + }, + Expression::CompositeString(composite) => composite.parts().iter().any(|part| match part { + StringPart::Expression(inner_expr) => expr_has_get_defined_vars(inner_expr), + StringPart::BracedExpression(braced) => expr_has_get_defined_vars(braced.expression), + StringPart::Literal(_) => false, + }), + Expression::Pipe(pipe) => { + expr_has_get_defined_vars(pipe.input) || expr_has_get_defined_vars(pipe.callable) + } + Expression::PartialApplication(partial) => match partial { + PartialApplication::Function(func_pa) => expr_has_get_defined_vars(func_pa.function), + PartialApplication::Method(method_pa) => expr_has_get_defined_vars(method_pa.object), + PartialApplication::StaticMethod(static_pa) => { + expr_has_get_defined_vars(static_pa.class) + } + }, + Expression::AnonymousClass(anon) => anon.argument_list.as_ref().is_some_and(|args| { + args.arguments + .iter() + .any(|a| expr_has_get_defined_vars(a.value())) + }), + // Don't recurse into closures/arrow functions. + Expression::Closure(_) | Expression::ArrowFunction(_) => false, + _ => false, + } +} + +fn array_elem_has_get_defined_vars(elem: &ArrayElement<'_>) -> bool { + match elem { + ArrayElement::KeyValue(kv) => { + expr_has_get_defined_vars(kv.key) || expr_has_get_defined_vars(kv.value) + } + ArrayElement::Value(v) => expr_has_get_defined_vars(v.value), + ArrayElement::Variadic(s) => expr_has_get_defined_vars(s.value), + ArrayElement::Missing(_) => false, + } +} + // ─── @var annotation collection ───────────────────────────────────────────── /// Scan the source text for `/** @var Type $varName */` inline diff --git a/src/diagnostics/unused_variables.rs b/src/diagnostics/unused_variables.rs index d38a74d5..f73594dc 100644 --- a/src/diagnostics/unused_variables.rs +++ b/src/diagnostics/unused_variables.rs @@ -18,7 +18,7 @@ use tower_lsp::lsp_types::*; use crate::Backend; use crate::atom::bytes_to_str; -use crate::diagnostics::undefined_variables::collect_compact_vars; +use crate::diagnostics::undefined_variables::{collect_compact_vars, has_get_defined_vars}; use crate::parser::with_parsed_program; use crate::scope_collector::{ AccessKind, FrameKind, ScopeMap, collect_function_scope_with_kind, @@ -96,6 +96,7 @@ fn collect_from_statement(stmt: &Statement<'_>, ctx: &mut DiagnosticCtx<'_>) { let body_start = func.body.left_brace.start.offset; let body_end = func.body.right_brace.end.offset; let compact_vars = collect_compact_vars(func.body.statements.as_slice()); + let has_get_defined = has_get_defined_vars(func.body.statements.as_slice()); let scope = collect_function_scope_with_resolver( &func.parameter_list, func.body.statements.as_slice(), @@ -103,7 +104,7 @@ fn collect_from_statement(stmt: &Statement<'_>, ctx: &mut DiagnosticCtx<'_>) { body_end, None, ); - check_scope(&scope, ctx, None, &compact_vars); + check_scope(&scope, ctx, None, &compact_vars, has_get_defined); } Statement::Class(class) => { collect_from_class_members(class.members.as_slice(), ctx); @@ -141,6 +142,7 @@ fn collect_from_class_members(members: &[ClassLikeMember<'_>], ctx: &mut Diagnos let promoted_params = collect_promoted_params(&method.parameter_list); let compact_vars = collect_compact_vars(block.statements.as_slice()); + let has_get_defined = has_get_defined_vars(block.statements.as_slice()); let scope = collect_function_scope_with_kind( &method.parameter_list, block.statements.as_slice(), @@ -149,7 +151,13 @@ fn collect_from_class_members(members: &[ClassLikeMember<'_>], ctx: &mut Diagnos FrameKind::Method, ); - check_scope(&scope, ctx, Some(&promoted_params), &compact_vars); + check_scope( + &scope, + ctx, + Some(&promoted_params), + &compact_vars, + has_get_defined, + ); } } } @@ -178,6 +186,7 @@ fn check_scope( ctx: &mut DiagnosticCtx<'_>, promoted_params: Option<&HashSet>, compact_vars: &HashSet, + has_get_defined_vars: bool, ) { if scope.frames.is_empty() { return; @@ -203,6 +212,13 @@ fn check_scope( continue; } + // `get_defined_vars()` only consumes variables from the enclosing + // function/method scope. Nested closures, arrow functions, and catch + // blocks still need their own unused-variable analysis. + if has_get_defined_vars && matches!(frame.kind, FrameKind::Function | FrameKind::Method) { + continue; + } + // For catch frames, we only check the catch variable (which is // in frame.parameters). We don't re-check variables inherited // from the parent — those are the parent frame's responsibility. @@ -1213,6 +1229,71 @@ class Ctrl { return view('page', compact('brand', 'series')); } } +"#, + ); + assert_eq!(diags.len(), 0); + } + + #[test] + fn get_defined_vars_suppresses_all_unused_in_function() { + let diags = collect( + r#" get_defined_vars()]; +} "#, ); assert_eq!(diags.len(), 0);