From 8e80e81c019f77ff1f8415aebd28dff42d5a6105 Mon Sep 17 00:00:00 2001 From: Caleb White Date: Sat, 18 Jul 2026 02:54:36 +0200 Subject: [PATCH 1/6] feat: add return type and property type mismatch diagnostics Add two new diagnostic collectors to the type mismatch family: - type_mismatch_return: checks return statements against declared return types. Flags incompatible types, void functions returning values, and bare return; in non-void functions. Skips generators, abstract methods, mixed/untyped returns. - type_mismatch_property: checks $this->prop and self::$prop assignments against declared property types. Only plain = assignments; compound operators are skipped. Both reuse the existing is_type_compatible() policy layer and resolve_expression_type() infrastructure. A unified collect_type_mismatch_diagnostics() entry point groups all three collectors (argument, return, property). Closes #201 --- docs/CHANGELOG.md | 2 + src/analyse.rs | 8 +- src/diagnostics/mod.rs | 25 +- src/diagnostics/property_type_errors.rs | 483 +++ src/diagnostics/return_type_errors.rs | 807 +++++ src/diagnostics/type_errors.rs | 6 +- .../diagnostics_property_type_errors.rs | 2203 +++++++++++++ .../diagnostics_return_type_errors.rs | 2730 +++++++++++++++++ tests/integration/diagnostics_type_errors.rs | 14 +- tests/integration/main.rs | 2 + 10 files changed, 6267 insertions(+), 13 deletions(-) create mode 100644 src/diagnostics/property_type_errors.rs create mode 100644 src/diagnostics/return_type_errors.rs create mode 100644 tests/integration/diagnostics_property_type_errors.rs create mode 100644 tests/integration/diagnostics_return_type_errors.rs diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 34d9a2b7..d7f02c71 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Macro hover shows origin and inferred return types.** Hovering on a macro method call now displays a "macro" indicator instead of the generic "virtual" label, distinguishing `::macro()` registrations from `@method`/`@mixin` synthesized members. When the closure has no explicit return type hint, the return type is inferred from the closure body and shown with an "(inferred)" annotation. Bare `$this` / `self` / `static` returns preserve their keyword form, and method chains like `$this->transform(...)` use the last method's declared return type directly — preserving `$this`, `static`, and generic parameters that the general resolver would flatten to a bare class name. Regular (non-macro) methods with inferred return types also show the "(inferred)" annotation on hover. A new `preserve_static` flag on the resolution context controls whether self-references resolve to their keyword form or the concrete class name. Contributed by @calebdw. +- **Return type mismatch diagnostics (`type_mismatch_return`).** Functions and methods with a declared return type are now checked against their `return` statements. Incompatible return values are flagged as errors. Void functions returning a value and bare `return;` in non-void functions are also flagged. Generators (functions using `yield`) are skipped. Uses the same conservative `is_type_compatible` policy as argument type checking to avoid false positives. Contributed by @calebdw. +- **Property type assignment diagnostics (`type_mismatch_property`).** Assignments to typed properties (`$this->prop = expr` and `self::$prop = expr`) are checked against the declared property type. Incompatible values are flagged as errors. Only plain `=` assignments are checked; compound operators (`+=`, `.=`, etc.) are skipped. Untyped and `mixed` properties are not flagged. Contributed by @calebdw. - **PSR-4 mismatch diagnostics and rename-based moves.** Files now warn when the declared namespace or primary class name does not match the PSR-4 path or filename, with quick fixes to correct them. Renaming a class from its declaration now opens the full FQCN so you can move it between namespaces in one step, and renaming a namespace can rewrite multiple segments at once while moving PSR-4 directories and updating references across the project. Contributed by @calebdw. - **Case-sensitive autoloading diagnostic.** A class reference whose casing differs from the class's actual declaration is now flagged, with a quick fix to correct it. This catches the bug where code loads on a case-insensitive filesystem (macOS, Windows) but fails with a class-not-found error on Linux, because PSR-4 maps the name to a file path and path lookups are case-sensitive there. It covers `use` imports and inline references to autoloaded classes; built-in classes and same-file references, which never reach the autoloader, are left alone. - **Completion candidates ranked by dependency provenance.** Class, function, and constant completions are now sorted by origin tier: project code first, then core/stub symbols, then explicit Composer dependencies (`require` / `require-dev`), then transitive vendor dependencies last. The provenance is inferred from `composer.json` and `installed.json` during indexing. Contributed by @calebdw. diff --git a/src/analyse.rs b/src/analyse.rs index 4f3b15c8..8e7953e9 100644 --- a/src/analyse.rs +++ b/src/analyse.rs @@ -424,7 +424,13 @@ pub async fn run(options: AnalyseOptions) -> i32 { b.collect_argument_count_diagnostics(u, c, o) }), ("type_mismatch_argument", &|b, u, c, o| { - b.collect_type_error_diagnostics(u, c, o) + b.collect_argument_type_diagnostics(u, c, o) + }), + ("type_mismatch_return", &|b, u, c, o| { + b.collect_return_type_diagnostics(u, c, o) + }), + ("type_mismatch_property", &|b, u, c, o| { + b.collect_property_type_diagnostics(u, c, o) }), ("missing_implementation", &|b, u, c, o| { b.collect_implementation_error_diagnostics(u, c, o) diff --git a/src/diagnostics/mod.rs b/src/diagnostics/mod.rs index 63ab6d96..62d6fe51 100644 --- a/src/diagnostics/mod.rs +++ b/src/diagnostics/mod.rs @@ -14,7 +14,9 @@ //! 3. **`unknown_*`** — symbol could not be resolved (class, function, //! member, variable). //! 4. **`unused_*`** — symbol is defined/imported but never referenced. -//! 5. **`type_mismatch_*`** — a value's type doesn't satisfy a constraint. +//! 5. **`type_mismatch_*`** — a value's type doesn't satisfy a constraint +//! (`type_mismatch_argument`, `type_mismatch_return`, +//! `type_mismatch_property`). //! 6. **`missing_*`** — a required declaration is absent (e.g. //! `missing_implementation` for unimplemented interface methods). //! 7. **`invalid_*`** — a structural/syntactic violation (e.g. @@ -175,6 +177,8 @@ pub(crate) mod ignore_rules; mod implementation_errors; mod invalid_class_kind; pub(crate) mod namespace_mismatch; +mod property_type_errors; +mod return_type_errors; mod syntax_errors; mod type_errors; pub(crate) mod undefined_variables; @@ -286,12 +290,29 @@ impl Backend { // inside collect_unknown_member_diagnostics (in the Untyped arm) // to avoid a second full walk with duplicate type resolution. self.collect_argument_count_diagnostics(uri_str, content, out); - self.collect_type_error_diagnostics(uri_str, content, out); + self.collect_type_mismatch_diagnostics(uri_str, content, out); self.collect_implementation_error_diagnostics(uri_str, content, out); self.collect_deprecated_diagnostics(uri_str, content, out); self.collect_undefined_variable_diagnostics(uri_str, content, out); self.collect_invalid_class_kind_diagnostics(uri_str, content, out); } + + /// Collect all type mismatch diagnostics: argument types, return + /// types, and property assignment types. + /// + /// This is a convenience entry point that groups the three + /// `type_mismatch_*` collectors. The individual methods remain + /// available for selective use (e.g. in `analyse.rs`). + pub fn collect_type_mismatch_diagnostics( + &self, + uri_str: &str, + content: &str, + out: &mut Vec, + ) { + self.collect_argument_type_diagnostics(uri_str, content, out); + self.collect_return_type_diagnostics(uri_str, content, out); + self.collect_property_type_diagnostics(uri_str, content, out); + } } /// Check whether a cached PHPStan diagnostic is stale given the current diff --git a/src/diagnostics/property_type_errors.rs b/src/diagnostics/property_type_errors.rs new file mode 100644 index 00000000..d466fa92 --- /dev/null +++ b/src/diagnostics/property_type_errors.rs @@ -0,0 +1,483 @@ +//! Property type assignment mismatch diagnostics. +//! +//! Walk assignment expressions in the file and flag every assignment +//! to a typed property where the assigned value's resolved type is +//! incompatible with the declared property type. +//! +//! Handles both instance properties (`$this->prop = expr`) and static +//! properties (`ClassName::$prop = expr`). +//! +//! Uses the same conservative approach as argument type checking: +//! when in doubt (unresolved types, `mixed`, complex generics), +//! the diagnostic is suppressed to avoid false positives. + +use std::collections::HashMap; +use std::sync::Arc; + +use mago_span::HasSpan; +use mago_syntax::ast::access::Access; +use mago_syntax::ast::class_like::member::ClassLikeMemberSelector; +use mago_syntax::ast::expression::Expression; +use mago_syntax::ast::statement::Statement; +use mago_syntax::ast::variable::Variable; + +use tower_lsp::lsp_types::*; + +use crate::Backend; +use crate::atom::bytes_to_str; +use crate::completion::resolver::{Loaders, VarResolutionCtx}; +use crate::completion::variable::foreach_resolution::resolve_expression_type; +use crate::parser::{with_parse_cache, with_parsed_program}; +use crate::php_type::PhpType; +use crate::types::ClassInfo; + +use super::helpers::{find_innermost_enclosing_class, make_diagnostic}; +use super::type_errors::{has_strict_types, is_type_compatible}; + +/// Diagnostic code used for property type mismatch diagnostics. +pub(crate) const TYPE_MISMATCH_PROPERTY_CODE: &str = "type_mismatch_property"; + +// ── Collected assignment site info ────────────────────────────────────────── + +/// A single property assignment's resolved type plus the byte range +/// of the RHS expression in source. +struct ResolvedPropertyAssignment { + /// The resolved type of the RHS expression. + rhs_type: PhpType, + /// The declared type of the property. + declared_type: PhpType, + /// Byte offset of the RHS expression start (inclusive). + start: usize, + /// Byte offset of the RHS expression end (exclusive). + end: usize, + /// The property name (for diagnostic messages). + property_name: String, +} + +// ── Context struct ────────────────────────────────────────────────────────── + +/// Bundles the read-only context and output accumulator threaded through +/// every walker function, eliminating `clippy::too_many_arguments`. +struct PropertyCheckCtx<'a> { + content: &'a str, + file_ctx: &'a crate::types::FileContext, + class_loader: &'a dyn Fn(&str) -> Option>, + function_loader: &'a dyn Fn(&str) -> Option, + constant_loader: &'a dyn Fn(&str) -> Option>, + backend: &'a Backend, + out: &'a mut Vec, +} + +// ── AST walkers ───────────────────────────────────────────────────────────── + +/// Walk a top-level statement collecting property assignments. +fn collect_from_statement(stmt: &Statement<'_>, ctx: &mut PropertyCheckCtx<'_>) { + match stmt { + Statement::Namespace(ns) => { + for inner in ns.statements().iter() { + collect_from_statement(inner, ctx); + } + } + Statement::Class(class) => { + for member in class.members.iter() { + collect_from_class_member(member, ctx); + } + } + Statement::Interface(iface) => { + for member in iface.members.iter() { + collect_from_class_member(member, ctx); + } + } + Statement::Trait(trait_def) => { + for member in trait_def.members.iter() { + collect_from_class_member(member, ctx); + } + } + Statement::Enum(enum_def) => { + for member in enum_def.members.iter() { + collect_from_class_member(member, ctx); + } + } + Statement::Function(func) => { + for s in func.body.statements.iter() { + collect_from_statement(s, ctx); + } + } + Statement::Expression(expr_stmt) => { + check_expression_for_property_assignment(expr_stmt.expression, ctx); + } + Statement::Return(ret) => { + if let Some(val) = ret.value { + check_expression_for_property_assignment(val, ctx); + } + } + Statement::If(if_stmt) => { + collect_from_if_body(&if_stmt.body, ctx); + } + Statement::While(w) => { + for s in w.body.statements() { + collect_from_statement(s, ctx); + } + } + Statement::DoWhile(dw) => { + collect_from_statement(dw.statement, ctx); + } + Statement::For(f) => { + for s in f.body.statements() { + collect_from_statement(s, ctx); + } + } + Statement::Foreach(fe) => { + for s in fe.body.statements() { + collect_from_statement(s, ctx); + } + } + Statement::Switch(sw) => { + collect_from_switch_body(&sw.body, ctx); + } + Statement::Try(t) => { + for s in t.block.statements.iter() { + collect_from_statement(s, ctx); + } + for catch in t.catch_clauses.iter() { + for s in catch.block.statements.iter() { + collect_from_statement(s, ctx); + } + } + if let Some(ref finally) = t.finally_clause { + for s in finally.block.statements.iter() { + collect_from_statement(s, ctx); + } + } + } + Statement::Block(block) => { + for s in block.statements.iter() { + collect_from_statement(s, ctx); + } + } + Statement::Declare(declare) => { + use mago_syntax::ast::declare::DeclareBody; + match &declare.body { + DeclareBody::Statement(inner) => { + collect_from_statement(inner, ctx); + } + DeclareBody::ColonDelimited(body) => { + for s in body.statements.iter() { + collect_from_statement(s, ctx); + } + } + } + } + _ => {} + } +} + +fn collect_from_class_member( + member: &mago_syntax::ast::class_like::member::ClassLikeMember<'_>, + ctx: &mut PropertyCheckCtx<'_>, +) { + use mago_syntax::ast::class_like::member::ClassLikeMember; + use mago_syntax::ast::class_like::method::MethodBody; + + if let ClassLikeMember::Method(method) = member + && let MethodBody::Concrete(block) = &method.body + { + for s in block.statements.iter() { + collect_from_statement(s, ctx); + } + } +} + +fn collect_from_if_body( + body: &mago_syntax::ast::control_flow::r#if::IfBody<'_>, + ctx: &mut PropertyCheckCtx<'_>, +) { + use mago_syntax::ast::control_flow::r#if::IfBody; + match body { + IfBody::Statement(inner) => { + collect_from_statement(inner.statement, ctx); + for c in inner.else_if_clauses.iter() { + collect_from_statement(c.statement, ctx); + } + if let Some(ref c) = inner.else_clause { + collect_from_statement(c.statement, ctx); + } + } + IfBody::ColonDelimited(body) => { + for s in body.statements.iter() { + collect_from_statement(s, ctx); + } + for c in body.else_if_clauses.iter() { + for s in c.statements.iter() { + collect_from_statement(s, ctx); + } + } + if let Some(ref c) = body.else_clause { + for s in c.statements.iter() { + collect_from_statement(s, ctx); + } + } + } + } +} + +fn collect_from_switch_body( + body: &mago_syntax::ast::control_flow::switch::SwitchBody<'_>, + ctx: &mut PropertyCheckCtx<'_>, +) { + use mago_syntax::ast::control_flow::switch::SwitchBody; + match body { + SwitchBody::BraceDelimited(b) => { + for case in b.cases.iter() { + for s in case.statements().iter() { + collect_from_statement(s, ctx); + } + } + } + SwitchBody::ColonDelimited(b) => { + for case in b.cases.iter() { + for s in case.statements().iter() { + collect_from_statement(s, ctx); + } + } + } + } +} + +/// Check an expression for property assignments (`$this->prop = expr`). +fn check_expression_for_property_assignment(expr: &Expression<'_>, ctx: &mut PropertyCheckCtx<'_>) { + // Only handle plain `=` assignments, not compound (`+=`, `.=`, etc.). + let assign = match expr { + Expression::Assignment(a) if a.operator.is_assign() => a, + _ => return, + }; + + // Check if the LHS is a property access. + let (prop_name, prop_class) = match assign.lhs { + // Instance property: `$this->propName = expr` + Expression::Access(Access::Property(pa)) => { + let is_this = matches!( + pa.object, + Expression::Variable(Variable::Direct(dv)) if dv.name == b"$this" + ); + if !is_this { + return; + } + let name = match &pa.property { + ClassLikeMemberSelector::Identifier(ident) => bytes_to_str(ident.value).to_string(), + _ => return, // Dynamic property name -- skip + }; + + let offset = pa.object.span().start.offset; + let enclosing = find_innermost_enclosing_class(&ctx.file_ctx.classes, offset); + match enclosing { + Some(cls) => (name, cls), + None => return, + } + } + // Static property: `self::$propName = expr` or `static::$propName = expr` + Expression::Access(Access::StaticProperty(spa)) => { + let class_name = match spa.class { + Expression::Identifier(ident) => { + let raw = bytes_to_str(ident.value()); + let lower = raw.to_ascii_lowercase(); + if lower != "self" && lower != "static" { + return; // Only handle self/static for now + } + raw.to_string() + } + _ => return, + }; + let _ = class_name; // We use offset-based class lookup + + let prop_name = match &spa.property { + Variable::Direct(dv) => { + let raw = bytes_to_str(dv.name).to_string(); + raw.strip_prefix('$').unwrap_or(&raw).to_string() + } + _ => return, // Dynamic variable name -- skip + }; + + let offset = spa.class.span().start.offset; + let enclosing = find_innermost_enclosing_class(&ctx.file_ctx.classes, offset); + match enclosing { + Some(cls) => (prop_name, cls), + None => return, + } + } + _ => return, + }; + + // Look up the property's declared type. + let declared_type = prop_class + .properties + .iter() + .find(|p| p.name == prop_name) + .and_then(|p| p.type_hint.clone()); + + let declared_type = match declared_type { + Some(t) if !t.is_untyped() && !t.is_mixed() => t, + _ => return, + }; + + // Resolve `self`/`static`/`parent` in the declared property type. + let declared_type = declared_type.resolve_names(&|name: &str| { + let lower = name.to_ascii_lowercase(); + match lower.as_str() { + "self" | "static" | "$this" => prop_class.fqn().to_string(), + "parent" => prop_class + .parent_class + .as_ref() + .map(|p| p.to_string()) + .unwrap_or_else(|| name.to_string()), + _ => { + if name.contains("__anonymous@") { + return name.to_string(); + } + if let Some(cls) = (ctx.class_loader)(name) { + cls.fqn().to_string() + } else { + name.to_string() + } + } + } + }); + + // Resolve the RHS expression type. + let rhs_span = assign.rhs.span(); + let rhs_start = rhs_span.start.offset as usize; + let rhs_end = rhs_span.end.offset as usize; + + let loaders = Loaders { + function_loader: Some(ctx.function_loader), + constant_loader: Some(ctx.constant_loader), + }; + + let var_ctx = VarResolutionCtx { + var_name: "", + top_level_scope: None, + current_class: prop_class, + all_classes: &ctx.file_ctx.classes, + content: ctx.content, + cursor_offset: rhs_start as u32, + class_loader: ctx.class_loader, + loaders, + resolved_class_cache: Some(&ctx.backend.resolved_class_cache), + enclosing_return_type: None, + branch_aware: true, + match_arm_narrowing: HashMap::new(), + scope_var_resolver: None, + }; + + let rhs_type = resolve_expression_type(assign.rhs, &var_ctx).unwrap_or_else(PhpType::untyped); + + // Skip unresolved types. + if rhs_type.is_untyped() + || rhs_type.is_empty() + || matches!(&rhs_type, PhpType::Raw(s) if s.is_empty()) + { + return; + } + + // Resolve short class names to FQN. + let rhs_type = rhs_type.resolve_names(&|name: &str| { + if name.contains("__anonymous@") { + return name.to_string(); + } + if let Some(cls) = (ctx.class_loader)(name) { + cls.fqn().to_string() + } else { + name.to_string() + } + }); + + ctx.out.push(ResolvedPropertyAssignment { + rhs_type, + declared_type, + start: rhs_start, + end: rhs_end, + property_name: prop_name, + }); +} + +// ── Main diagnostic collection ────────────────────────────────────────────── + +impl Backend { + /// Collect property type assignment mismatch diagnostics for a single file. + /// + /// Appends diagnostics to `out`. The caller is responsible for + /// publishing them via `textDocument/publishDiagnostics`. + pub fn collect_property_type_diagnostics( + &self, + uri: &str, + content: &str, + out: &mut Vec, + ) { + let file_ctx = self.file_context(uri); + + let _parse_guard = with_parse_cache(content); + + let class_loader = self.class_loader(&file_ctx); + let function_loader_cl = self.function_loader(&file_ctx); + let constant_loader_cl = self.constant_loader(); + + let results: Vec = + with_parsed_program(content, "property_type_diagnostics", |program, _content| { + let mut resolved: Vec = Vec::new(); + + let mut ctx = PropertyCheckCtx { + content, + file_ctx: &file_ctx, + class_loader: &class_loader, + function_loader: &function_loader_cl, + constant_loader: &constant_loader_cl, + backend: self, + out: &mut resolved, + }; + + for stmt in program.statements.iter() { + collect_from_statement(stmt, &mut ctx); + } + + resolved + }); + + // Emit diagnostics for incompatible property assignments. + let strict_types = with_parsed_program(content, "property_strict", |program, _| { + has_strict_types(program) + }); + + for assignment in &results { + if is_type_compatible( + &assignment.rhs_type, + &assignment.declared_type, + &class_loader, + strict_types, + ) { + continue; + } + + let range = match self.offset_range_to_lsp_range( + uri, + content, + assignment.start, + assignment.end, + ) { + Some(r) => r, + None => continue, + }; + + let message = format!( + "Property ${} expects {}, got {}", + assignment.property_name, assignment.declared_type, assignment.rhs_type, + ); + + out.push(make_diagnostic( + range, + DiagnosticSeverity::ERROR, + TYPE_MISMATCH_PROPERTY_CODE, + message, + )); + } + } +} diff --git a/src/diagnostics/return_type_errors.rs b/src/diagnostics/return_type_errors.rs new file mode 100644 index 00000000..a1bb470e --- /dev/null +++ b/src/diagnostics/return_type_errors.rs @@ -0,0 +1,807 @@ +//! Return type mismatch diagnostics. +//! +//! Walk methods and functions in the file and flag every `return` +//! statement where the returned expression's resolved type is +//! incompatible with the declared return type. +//! +//! Uses the same conservative approach as argument type checking: +//! when in doubt (unresolved types, `mixed`, complex generics), +//! the diagnostic is suppressed to avoid false positives. + +use std::collections::HashMap; +use std::sync::Arc; + +use mago_span::HasSpan; +use mago_syntax::ast::expression::Expression; +use mago_syntax::ast::statement::Statement; + +use tower_lsp::lsp_types::*; + +use crate::Backend; +use crate::atom::bytes_to_str; +use crate::completion::resolver::{Loaders, VarResolutionCtx}; +use crate::completion::variable::foreach_resolution::resolve_expression_type; +use crate::parser::{with_parse_cache, with_parsed_program}; +use crate::php_type::PhpType; +use crate::types::ClassInfo; + +use super::helpers::{find_innermost_enclosing_class, make_diagnostic}; +use super::type_errors::{has_strict_types, is_type_compatible}; + +/// Diagnostic code used for return type mismatch diagnostics. +pub(crate) const TYPE_MISMATCH_RETURN_CODE: &str = "type_mismatch_return"; + +// ── Collected return site info ────────────────────────────────────────────── + +/// A single return statement's resolved type plus the byte range of +/// the return expression in source. +struct ResolvedReturn { + /// The resolved type of the return expression, or `None` for bare `return;`. + ty: Option, + /// Byte offset of the return expression (or `return` keyword for bare returns) start (inclusive). + start: usize, + /// Byte offset of the return expression (or `return` keyword for bare returns) end (exclusive). + end: usize, + /// The declared return type of the enclosing function/method. + declared_type: PhpType, +} + +// ── AST walkers ───────────────────────────────────────────────────────────── + +/// Check whether a statement list contains any `yield` expression +/// (indicating a generator function whose return type semantics differ). +fn body_contains_yield(stmts: &mago_syntax::ast::sequence::Sequence<'_, Statement<'_>>) -> bool { + for stmt in stmts.iter() { + if stmt_contains_yield(stmt) { + return true; + } + } + false +} + +fn stmt_contains_yield(stmt: &Statement<'_>) -> bool { + match stmt { + Statement::Expression(expr_stmt) => expr_contains_yield(expr_stmt.expression), + Statement::Return(ret) => ret.value.is_some_and(|v| expr_contains_yield(v)), + Statement::Echo(echo) => echo.values.iter().any(|e| expr_contains_yield(e)), + Statement::If(if_stmt) => { + expr_contains_yield(if_stmt.condition) || if_body_contains_yield(&if_stmt.body) + } + Statement::While(w) => { + expr_contains_yield(w.condition) + || w.body.statements().iter().any(|s| stmt_contains_yield(s)) + } + Statement::DoWhile(dw) => { + stmt_contains_yield(dw.statement) || expr_contains_yield(dw.condition) + } + Statement::For(f) => { + f.initializations.iter().any(|e| expr_contains_yield(e)) + || f.conditions.iter().any(|e| expr_contains_yield(e)) + || f.increments.iter().any(|e| expr_contains_yield(e)) + || f.body.statements().iter().any(|s| stmt_contains_yield(s)) + } + Statement::Foreach(fe) => { + expr_contains_yield(fe.expression) + || fe.body.statements().iter().any(|s| stmt_contains_yield(s)) + } + Statement::Switch(sw) => { + expr_contains_yield(sw.expression) || switch_body_contains_yield(&sw.body) + } + Statement::Try(t) => { + t.block.statements.iter().any(|s| stmt_contains_yield(s)) + || t.catch_clauses + .iter() + .any(|c| c.block.statements.iter().any(|s| stmt_contains_yield(s))) + || t.finally_clause + .as_ref() + .is_some_and(|f| f.block.statements.iter().any(|s| stmt_contains_yield(s))) + } + Statement::Block(b) => b.statements.iter().any(|s| stmt_contains_yield(s)), + // Don't recurse into nested functions/closures — their yields + // don't make the *enclosing* function a generator. + _ => false, + } +} + +fn if_body_contains_yield(body: &mago_syntax::ast::control_flow::r#if::IfBody<'_>) -> bool { + use mago_syntax::ast::control_flow::r#if::IfBody; + match body { + IfBody::Statement(inner) => { + stmt_contains_yield(inner.statement) + || inner + .else_if_clauses + .iter() + .any(|c| stmt_contains_yield(c.statement)) + || inner + .else_clause + .as_ref() + .is_some_and(|c| stmt_contains_yield(c.statement)) + } + IfBody::ColonDelimited(body) => { + body.statements.iter().any(|s| stmt_contains_yield(s)) + || body + .else_if_clauses + .iter() + .any(|c| c.statements.iter().any(|s| stmt_contains_yield(s))) + || body + .else_clause + .as_ref() + .is_some_and(|c| c.statements.iter().any(|s| stmt_contains_yield(s))) + } + } +} + +fn switch_body_contains_yield( + body: &mago_syntax::ast::control_flow::switch::SwitchBody<'_>, +) -> bool { + use mago_syntax::ast::control_flow::switch::SwitchBody; + match body { + SwitchBody::BraceDelimited(b) => b + .cases + .iter() + .any(|c| c.statements().iter().any(|s| stmt_contains_yield(s))), + SwitchBody::ColonDelimited(b) => b + .cases + .iter() + .any(|c| c.statements().iter().any(|s| stmt_contains_yield(s))), + } +} + +fn expr_contains_yield(expr: &Expression<'_>) -> bool { + matches!(expr, Expression::Yield(_)) +} + +/// Collect return expressions from a function/method body. +/// +/// Recurses into control flow blocks (if/else, for, while, try/catch, +/// etc.) but does NOT recurse into closures, arrow functions, or nested +/// function declarations — those have their own return types. +fn collect_returns_from_body<'a>( + stmts: &mago_syntax::ast::sequence::Sequence<'a, Statement<'a>>, + returns: &mut Vec<(Option<&'a Expression<'a>>, usize, usize)>, +) { + for stmt in stmts.iter() { + collect_returns_from_stmt(stmt, returns); + } +} + +fn collect_returns_from_stmt<'a>( + stmt: &Statement<'a>, + returns: &mut Vec<(Option<&'a Expression<'a>>, usize, usize)>, +) { + match stmt { + Statement::Return(ret) => { + if let Some(val) = ret.value { + let span = val.span(); + returns.push(( + Some(val), + span.start.offset as usize, + span.end.offset as usize, + )); + } else { + // Bare `return;` — use the `return` keyword span. + let kw_span = ret.r#return.span; + returns.push(( + None, + kw_span.start.offset as usize, + kw_span.end.offset as usize, + )); + } + } + Statement::Namespace(ns) => { + for inner in ns.statements().iter() { + collect_returns_from_stmt(inner, returns); + } + } + Statement::If(if_stmt) => { + collect_returns_from_if_body(&if_stmt.body, returns); + } + Statement::While(w) => { + for s in w.body.statements() { + collect_returns_from_stmt(s, returns); + } + } + Statement::DoWhile(dw) => { + collect_returns_from_stmt(dw.statement, returns); + } + Statement::For(f) => { + for s in f.body.statements() { + collect_returns_from_stmt(s, returns); + } + } + Statement::Foreach(fe) => { + for s in fe.body.statements() { + collect_returns_from_stmt(s, returns); + } + } + Statement::Switch(sw) => { + collect_returns_from_switch_body(&sw.body, returns); + } + Statement::Try(t) => { + for s in t.block.statements.iter() { + collect_returns_from_stmt(s, returns); + } + for catch in t.catch_clauses.iter() { + for s in catch.block.statements.iter() { + collect_returns_from_stmt(s, returns); + } + } + if let Some(ref finally) = t.finally_clause { + for s in finally.block.statements.iter() { + collect_returns_from_stmt(s, returns); + } + } + } + Statement::Block(block) => { + for s in block.statements.iter() { + collect_returns_from_stmt(s, returns); + } + } + Statement::Declare(declare) => { + use mago_syntax::ast::declare::DeclareBody; + match &declare.body { + DeclareBody::Statement(inner) => { + collect_returns_from_stmt(inner, returns); + } + DeclareBody::ColonDelimited(body) => { + for s in body.statements.iter() { + collect_returns_from_stmt(s, returns); + } + } + } + } + // Do NOT recurse into closures, arrow functions, or nested + // functions — they have their own return types. + Statement::Function(_) => {} + Statement::Class(_) + | Statement::Interface(_) + | Statement::Trait(_) + | Statement::Enum(_) => {} + _ => {} + } +} + +fn collect_returns_from_if_body<'a>( + body: &mago_syntax::ast::control_flow::r#if::IfBody<'a>, + returns: &mut Vec<(Option<&'a Expression<'a>>, usize, usize)>, +) { + use mago_syntax::ast::control_flow::r#if::IfBody; + match body { + IfBody::Statement(inner) => { + collect_returns_from_stmt(inner.statement, returns); + for c in inner.else_if_clauses.iter() { + collect_returns_from_stmt(c.statement, returns); + } + if let Some(ref c) = inner.else_clause { + collect_returns_from_stmt(c.statement, returns); + } + } + IfBody::ColonDelimited(body) => { + for s in body.statements.iter() { + collect_returns_from_stmt(s, returns); + } + for c in body.else_if_clauses.iter() { + for s in c.statements.iter() { + collect_returns_from_stmt(s, returns); + } + } + if let Some(ref c) = body.else_clause { + for s in c.statements.iter() { + collect_returns_from_stmt(s, returns); + } + } + } + } +} + +fn collect_returns_from_switch_body<'a>( + body: &mago_syntax::ast::control_flow::switch::SwitchBody<'a>, + returns: &mut Vec<(Option<&'a Expression<'a>>, usize, usize)>, +) { + use mago_syntax::ast::control_flow::switch::SwitchBody; + match body { + SwitchBody::BraceDelimited(b) => { + for case in b.cases.iter() { + for s in case.statements().iter() { + collect_returns_from_stmt(s, returns); + } + } + } + SwitchBody::ColonDelimited(b) => { + for case in b.cases.iter() { + for s in case.statements().iter() { + collect_returns_from_stmt(s, returns); + } + } + } + } +} + +// ── Main diagnostic collection ────────────────────────────────────────────── + +impl Backend { + /// Collect return type mismatch diagnostics for a single file. + /// + /// Appends diagnostics to `out`. The caller is responsible for + /// publishing them via `textDocument/publishDiagnostics`. + pub fn collect_return_type_diagnostics( + &self, + uri: &str, + content: &str, + out: &mut Vec, + ) { + let file_ctx = self.file_context(uri); + + let _parse_guard = with_parse_cache(content); + + let class_loader = self.class_loader(&file_ctx); + let function_loader_cl = self.function_loader(&file_ctx); + let constant_loader_cl = self.constant_loader(); + let default_class = ClassInfo::default(); + + // Walk the AST, find return statements in method/function + // bodies, resolve their types, and pair them with the declared + // return type. + let results: Vec = + with_parsed_program(content, "return_type_diagnostics", |program, _content| { + let strict_types = has_strict_types(program); + let _ = strict_types; // used below in VarResolutionCtx + let mut resolved_returns: Vec = Vec::new(); + + for stmt in program.statements.iter() { + process_top_level_statement( + stmt, + content, + &file_ctx, + &class_loader, + &function_loader_cl, + &constant_loader_cl, + &default_class, + self, + &mut resolved_returns, + ); + } + + resolved_returns + }); + + // Emit diagnostics for incompatible returns. + let strict_types_for_check = with_parsed_program(content, "return_strict", |program, _| { + has_strict_types(program) + }); + + for ret in &results { + let range = match self.offset_range_to_lsp_range(uri, content, ret.start, ret.end) { + Some(r) => r, + None => continue, + }; + + let message = match &ret.ty { + // Bare `return;` in a void function — OK. + None if ret.declared_type.is_void() => continue, + // Bare `return;` in a non-void function — error. + None => format!( + "Function with return type {} must not return without a value", + ret.declared_type, + ), + // `return $expr;` in a void function — error. + Some(_) if ret.declared_type.is_void() => { + "Void function must not return a value".to_string() + } + // `return $expr;` with a compatible type — OK. + Some(ty) + if is_type_compatible( + ty, + &ret.declared_type, + &class_loader, + strict_types_for_check, + ) => + { + continue; + } + // `return $expr;` with an incompatible type — error. + Some(ty) => format!( + "Return type {} is incompatible with declared return type {}", + ty, ret.declared_type, + ), + }; + + out.push(make_diagnostic( + range, + DiagnosticSeverity::ERROR, + TYPE_MISMATCH_RETURN_CODE, + message, + )); + } + } +} + +#[allow(clippy::too_many_arguments)] +/// Resolve the type of a return expression and push a `ResolvedReturn`. +/// +/// For bare `return;` statements (`maybe_expr` is `None`), pushes with +/// `ty: None` — the diagnostic emission handles these specially. +/// For `return $expr;`, resolves the expression type and pushes with +/// `ty: Some(resolved_type)`. +fn resolve_return_and_push( + maybe_expr: Option<&Expression<'_>>, + start: usize, + end: usize, + declared_return: &PhpType, + current_class: &ClassInfo, + content: &str, + all_classes: &[Arc], + class_loader: &dyn Fn(&str) -> Option>, + loaders: Loaders<'_>, + backend: &Backend, + out: &mut Vec, +) { + match maybe_expr { + None => { + // Bare `return;` — push with ty: None for the emission logic. + out.push(ResolvedReturn { + ty: None, + start, + end, + declared_type: declared_return.clone(), + }); + } + Some(expr) => { + // `return $expr;` — skip void-declared functions here; + // they'll be flagged regardless of the expression type. + if declared_return.is_void() { + out.push(ResolvedReturn { + ty: Some(PhpType::untyped()), // placeholder; message ignores it + start, + end, + declared_type: declared_return.clone(), + }); + return; + } + + let var_ctx = VarResolutionCtx { + var_name: "", + top_level_scope: None, + current_class, + all_classes, + content, + cursor_offset: start as u32, + class_loader, + loaders, + resolved_class_cache: Some(&backend.resolved_class_cache), + enclosing_return_type: None, + branch_aware: true, + match_arm_narrowing: HashMap::new(), + scope_var_resolver: None, + }; + + let ty = resolve_expression_type(expr, &var_ctx).unwrap_or_else(PhpType::untyped); + + // Skip unresolved types. + if ty.is_untyped() || ty.is_empty() || matches!(&ty, PhpType::Raw(s) if s.is_empty()) { + return; + } + + // Resolve short class names to FQN. + let ty = ty.resolve_names(&|name: &str| { + if name.contains("__anonymous@") { + return name.to_string(); + } + if let Some(cls) = class_loader(name) { + cls.fqn().to_string() + } else { + name.to_string() + } + }); + + out.push(ResolvedReturn { + ty: Some(ty), + start, + end, + declared_type: declared_return.clone(), + }); + } + } +} + +#[allow(clippy::too_many_arguments)] +/// Walk a top-level statement looking for function/class declarations. +fn process_top_level_statement( + stmt: &Statement<'_>, + content: &str, + file_ctx: &crate::types::FileContext, + class_loader: &dyn Fn(&str) -> Option>, + function_loader: &dyn Fn(&str) -> Option, + constant_loader: &dyn Fn(&str) -> Option>, + default_class: &ClassInfo, + backend: &Backend, + out: &mut Vec, +) { + match stmt { + Statement::Namespace(ns) => { + for inner in ns.statements().iter() { + process_top_level_statement( + inner, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + } + Statement::Class(class) => { + for member in class.members.iter() { + process_class_member( + member, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + } + Statement::Interface(iface) => { + for member in iface.members.iter() { + process_class_member( + member, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + } + Statement::Trait(trait_def) => { + for member in trait_def.members.iter() { + process_class_member( + member, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + } + Statement::Enum(enum_def) => { + for member in enum_def.members.iter() { + process_class_member( + member, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + } + Statement::Function(func) => { + let func_name = bytes_to_str(func.name.value); + let func_offset = func.name.span.start.offset; + + // Extract the declared return type. Prefer the AST's native + // return type hint (always available for the current file), + // then fall back to the global function index (which may + // carry a richer docblock-enriched type). + let declared_return = func + .return_type_hint + .as_ref() + .map(|rth| crate::parser::extract_hint_type(&rth.hint)) + .or_else(|| { + let fqn = file_ctx.resolve_name_at(func_name, func_offset); + backend + .global_functions() + .read() + .get(&fqn) + .and_then(|(_, fi)| fi.return_type.clone()) + .or_else(|| { + backend + .global_functions() + .read() + .get(func_name) + .and_then(|(_, fi)| fi.return_type.clone()) + }) + }); + + let declared_return = match declared_return { + Some(t) if !t.is_untyped() && !t.is_mixed() => t, + _ => return, + }; + + // Skip generators. + if body_contains_yield(&func.body.statements) { + return; + } + + // Collect return statements (both bare and with values). + let mut returns: Vec<(Option<&Expression<'_>>, usize, usize)> = Vec::new(); + collect_returns_from_body(&func.body.statements, &mut returns); + + if returns.is_empty() { + return; + } + + // Resolve types and check. + let enclosing = find_innermost_enclosing_class(&file_ctx.classes, func_offset); + let current_class = enclosing.unwrap_or(default_class); + + let loaders = Loaders { + function_loader: Some(function_loader), + constant_loader: Some(constant_loader), + }; + + for (maybe_expr, start, end) in returns { + resolve_return_and_push( + maybe_expr, + start, + end, + &declared_return, + current_class, + content, + &file_ctx.classes, + class_loader, + loaders, + backend, + out, + ); + } + } + Statement::Declare(declare) => { + use mago_syntax::ast::declare::DeclareBody; + match &declare.body { + DeclareBody::Statement(inner) => { + process_top_level_statement( + inner, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + DeclareBody::ColonDelimited(body) => { + for s in body.statements.iter() { + process_top_level_statement( + s, + content, + file_ctx, + class_loader, + function_loader, + constant_loader, + default_class, + backend, + out, + ); + } + } + } + } + _ => {} + } +} + +#[allow(clippy::too_many_arguments)] +/// Process a class member (looking for methods with return types). +fn process_class_member( + member: &mago_syntax::ast::class_like::member::ClassLikeMember<'_>, + content: &str, + file_ctx: &crate::types::FileContext, + class_loader: &dyn Fn(&str) -> Option>, + function_loader: &dyn Fn(&str) -> Option, + constant_loader: &dyn Fn(&str) -> Option>, + _default_class: &ClassInfo, + backend: &Backend, + out: &mut Vec, +) { + use mago_syntax::ast::class_like::member::ClassLikeMember; + use mago_syntax::ast::class_like::method::MethodBody; + + let method = match member { + ClassLikeMember::Method(m) => m, + _ => return, + }; + + let body = match &method.body { + MethodBody::Concrete(block) => &block.statements, + MethodBody::Abstract(_) => return, + }; + + let method_name = bytes_to_str(method.name.value); + let method_offset = method.name.span.start.offset; + + // Find the enclosing class to look up the method's declared return type. + let enclosing = find_innermost_enclosing_class(&file_ctx.classes, method_offset); + let current_class = match enclosing { + Some(cls) => cls, + None => return, + }; + + // Look up the method's declared return type from the parsed MethodInfo. + let declared_return = current_class + .get_method(method_name) + .and_then(|mi| mi.return_type.clone()); + + let declared_return = match declared_return { + Some(t) if !t.is_untyped() && !t.is_mixed() => t, + _ => return, + }; + + // Skip generators. + if body_contains_yield(body) { + return; + } + + // Collect return statements (both bare and with values). + let mut returns: Vec<(Option<&Expression<'_>>, usize, usize)> = Vec::new(); + collect_returns_from_body(body, &mut returns); + + if returns.is_empty() { + return; + } + + // Resolve the declared return type's `self`/`static`/`parent` to + // concrete class names for accurate comparison. + let declared_return = declared_return.resolve_names(&|name: &str| { + let lower = name.to_ascii_lowercase(); + match lower.as_str() { + "self" | "static" | "$this" => current_class.fqn().to_string(), + "parent" => current_class + .parent_class + .as_ref() + .map(|p| p.to_string()) + .unwrap_or_else(|| name.to_string()), + _ => { + if name.contains("__anonymous@") { + return name.to_string(); + } + if let Some(cls) = class_loader(name) { + cls.fqn().to_string() + } else { + name.to_string() + } + } + } + }); + + let loaders = Loaders { + function_loader: Some(function_loader), + constant_loader: Some(constant_loader), + }; + + for (maybe_expr, start, end) in returns { + resolve_return_and_push( + maybe_expr, + start, + end, + &declared_return, + current_class, + content, + &file_ctx.classes, + class_loader, + loaders, + backend, + out, + ); + } +} diff --git a/src/diagnostics/type_errors.rs b/src/diagnostics/type_errors.rs index 586894b2..6ce184f4 100644 --- a/src/diagnostics/type_errors.rs +++ b/src/diagnostics/type_errors.rs @@ -68,7 +68,7 @@ fn is_bare_array(ty: &PhpType) -> bool { /// Returns `true` if the argument type can be passed to the parameter /// without a type error. Conservative: returns `true` (compatible) /// when in doubt. -fn is_type_compatible( +pub(super) fn is_type_compatible( arg_type: &PhpType, param_type: &PhpType, class_loader: &dyn Fn(&str) -> Option>, @@ -776,7 +776,7 @@ fn is_refined_scalar_pair(arg: &PhpType, param: &PhpType) -> bool { /// `declare(strict_types=1)` directive. In PHP this must appear as /// the very first statement (after `) -> bool { +pub(super) fn has_strict_types(program: &Program<'_>) -> bool { for stmt in program.statements.iter() { if let Statement::Declare(declare) = stmt { for item in declare.items.iter() { @@ -1433,7 +1433,7 @@ impl Backend { /// /// Appends diagnostics to `out`. The caller is responsible for /// publishing them via `textDocument/publishDiagnostics`. - pub fn collect_type_error_diagnostics( + pub fn collect_argument_type_diagnostics( &self, uri: &str, content: &str, diff --git a/tests/integration/diagnostics_property_type_errors.rs b/tests/integration/diagnostics_property_type_errors.rs new file mode 100644 index 00000000..ab32d78c --- /dev/null +++ b/tests/integration/diagnostics_property_type_errors.rs @@ -0,0 +1,2203 @@ +use crate::common::create_test_backend; +use tower_lsp::lsp_types::*; + +// ─── Helpers ──────────────────────────────────────────────────────────────── + +fn collect(php: &str) -> Vec { + let backend = create_test_backend(); + let uri = "file:///test.php"; + backend.update_ast(uri, php); + let mut out = Vec::new(); + backend.collect_property_type_diagnostics(uri, php, &mut out); + out +} + +fn has_property_error(diags: &[Diagnostic]) -> bool { + diags.iter().any(|d| { + d.code.as_ref().is_some_and( + |c| matches!(c, NumberOrString::String(s) if s == "type_mismatch_property"), + ) + }) +} + +fn property_error_messages(diags: &[Diagnostic]) -> Vec { + diags + .iter() + .filter(|d| { + d.code.as_ref().is_some_and( + |c| matches!(c, NumberOrString::String(s) if s == "type_mismatch_property"), + ) + }) + .map(|d| d.message.clone()) + .collect() +} + +// ─── Basic: assign wrong type to property ─────────────────────────────────── + +#[test] +fn flags_string_assigned_to_int_property() { + let php = r#"count = "hello"; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error for string assigned to int, got: {diags:?}" + ); + let msgs = property_error_messages(&diags); + assert!( + msgs.iter().any(|m| m.contains("count")), + "Expected message mentioning property name, got: {msgs:?}" + ); +} + +// ─── Correct assignment — no diagnostic ──────────────────────────────────── + +#[test] +fn no_diagnostic_for_correct_property_assignment() { + let php = r#"name = "hello"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct property assignment, got: {diags:?}" + ); +} + +// ─── Assign null to non-nullable property ────────────────────────────────── + +#[test] +fn flags_null_assigned_to_non_nullable_property() { + let php = r#"name = null; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error for null assigned to string, got: {diags:?}" + ); +} + +// ─── Assign null to nullable property — OK ───────────────────────────────── + +#[test] +fn no_diagnostic_for_null_to_nullable_property() { + let php = r#"name = null; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag null assigned to ?string, got: {diags:?}" + ); +} + +// ─── Assign int to string property ───────────────────────────────────────── + +#[test] +fn flags_array_assigned_to_string_property_basic() { + let php = r#"label = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error for array assigned to string, got: {diags:?}" + ); +} + +// ─── Assign bool to int property ─────────────────────────────────────────── + +#[test] +fn flags_bool_assigned_to_int_property() { + let php = r#"value = true; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error for bool assigned to int, got: {diags:?}" + ); +} + +// ─── Untyped property — no diagnostic ────────────────────────────────────── + +#[test] +fn no_diagnostic_for_untyped_property() { + let php = r#"anything = 42; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag untyped property, got: {diags:?}" + ); +} + +// ─── Mixed property — no diagnostic ──────────────────────────────────────── + +#[test] +fn no_diagnostic_for_mixed_property() { + let php = r#"data = "hello"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag mixed property, got: {diags:?}" + ); +} + +// ─── Union property type — compatible ────────────────────────────────────── + +#[test] +fn no_diagnostic_for_union_property_correct() { + let php = r#"value = 42; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag int assigned to string|int, got: {diags:?}" + ); +} + +// ─── Union property type — incompatible ──────────────────────────────────── + +#[test] +fn flags_bool_assigned_to_string_or_int_property() { + let php = r#"value = true; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error for bool assigned to string|int, got: {diags:?}" + ); +} + +// ─── Array assigned to string property ───────────────────────────────────── + +#[test] +fn flags_array_assigned_to_string_property() { + let php = r#"name = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error for array assigned to string, got: {diags:?}" + ); +} + +// ─── Compound assignment (+=) — not flagged ──────────────────────────────── + +#[test] +fn no_diagnostic_for_compound_assignment() { + let php = r#"count += 1; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag compound assignment (+=), got: {diags:?}" + ); +} + +// ─── Assignment inside if/else ───────────────────────────────────────────── + +#[test] +fn flags_wrong_assignment_in_if_branch() { + let php = r#"count = "wrong"; + } else { + $this->count = 42; + } + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + assert_eq!( + msgs.len(), + 1, + "Expected exactly one property error (in if branch), got: {msgs:?}" + ); +} + +// ─── Assignment in constructor ───────────────────────────────────────────── + +#[test] +fn flags_wrong_assignment_in_constructor() { + let php = r#"name = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error in constructor, got: {diags:?}" + ); +} + +// ─── Correct constructor assignment — no diagnostic ──────────────────────── + +#[test] +fn no_diagnostic_for_correct_constructor_assignment() { + let php = r#"name = $name; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct constructor assignment, got: {diags:?}" + ); +} + +// ─── Assignment in try/catch ─────────────────────────────────────────────── + +#[test] +fn flags_wrong_assignment_in_try() { + let php = r#"result = false; + } catch (\Exception $e) { + $this->result = "error"; + } + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + assert_eq!( + msgs.len(), + 1, + "Expected one property error (in try block), got: {msgs:?}" + ); +} + +// ─── Multiple wrong assignments ──────────────────────────────────────────── + +#[test] +fn flags_multiple_wrong_assignments() { + let php = r#"a = "wrong"; + $this->b = []; + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + assert_eq!(msgs.len(), 2, "Expected two property errors, got: {msgs:?}"); +} + +// ─── Assignment inside foreach ───────────────────────────────────────────── + +#[test] +fn flags_wrong_assignment_in_foreach() { + let php = r#"total = "wrong"; + } + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error in foreach, got: {diags:?}" + ); +} + +// ─── Assignment inside while ─────────────────────────────────────────────── + +#[test] +fn flags_wrong_assignment_in_while() { + let php = r#"running = "yes"; + } + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected property type error in while loop, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: type juggling (non-strict mode) +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_int_assigned_to_string_property_non_strict() { + // PHP coerces int to string in non-strict mode. + let php = r#"text = 42; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag int assigned to string property (type juggling), got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_float_assigned_to_string_property_non_strict() { + let php = r#"value = 3.14; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag float assigned to string property (type juggling), got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_int_assigned_to_float_property() { + // int is always widened to float in PHP. + let php = r#"value = 42; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag int assigned to float property (widening), got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: strict_types interactions +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn strict_types_flags_int_assigned_to_string_property() { + let php = r#"name = 42; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for int assigned to string property under strict_types=1, got: {diags:?}" + ); +} + +#[test] +fn strict_types_still_allows_null_for_nullable_property() { + let php = r#"name = null; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "strict_types should not affect nullable null assignment, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: class hierarchy (subclass / interface) +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_subclass_assigned_to_parent_property() { + let php = r#"animal = new Cat(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag subclass Cat assigned to Animal property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_interface_implementor_assigned_to_interface_property() { + let php = r#"logger = new FileLogger(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag interface implementor assigned to interface property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_deep_inheritance_assigned_to_base_property() { + let php = r#"item = new Leaf(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag deep subclass assigned to base-typed property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_object_property_with_class_instance() { + let php = r#"item = new Foo(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag class instance assigned to object property, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_class_assigned_to_typed_property() { + let php = r#"pet = new Cat(); + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for Cat assigned to Dog property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: Stringable objects assigned to string property +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_stringable_assigned_to_string_property() { + let php = r#"content = new HtmlString(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag Stringable object assigned to string property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: static property assignments +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_static_property_self() { + let php = r#"email = "test@example.com"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag string assigned to ?string property, got: {diags:?}" + ); +} + +#[test] +fn flags_array_assigned_to_nullable_string_property() { + let php = r#"email = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to ?string property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: union property types +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_string_assigned_to_union_string_int() { + let php = r#"value = "hello"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag string assigned to string|int property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_null_assigned_to_union_with_null() { + let php = r#"value = null; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag null assigned to string|int|null property, got: {diags:?}" + ); +} + +#[test] +fn flags_bool_assigned_to_union_string_int_null() { + let php = r#"value = true; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for bool assigned to string|int|null property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: iterable / callable / array property types +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_array_assigned_to_iterable_property() { + let php = r#"items = [1, 2, 3]; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag array assigned to iterable property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_array_literal_assigned_to_array_property() { + let php = r#"items = [1, 2, 3]; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag array literal assigned to array property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: trait properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_trait_property_assignment() { + let php = r#"name = "hello"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment in trait method, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_trait_property_assignment() { + let php = r#"name = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to string trait property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: constructor promoted properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_promoted_property_assignment() { + let php = r#"name = $newName; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment to promoted property, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_promoted_property_assignment() { + let php = r#"age = "not a number"; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for string assigned to promoted int property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: readonly properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_readonly_property_assignment() { + let php = r#"dsn = $dsn; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment to readonly property, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_readonly_property_assignment() { + let php = r#"dsn = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to readonly string property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: bool / true / false property types +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_true_assigned_to_bool_property() { + let php = r#"active = true; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag true assigned to bool property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_false_assigned_to_bool_property() { + let php = r#"active = false; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag false assigned to bool property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: namespaced classes +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_namespaced_property_assignment() { + let php = r#"name = "Alice"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment in namespaced class, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_namespaced_property_assignment() { + let php = r#"name = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to string property in namespaced class, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: multiple classes in same file +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_across_multiple_classes_correct() { + let php = r#"name = "foo"; + } +} + +class Bar { + public int $count; + + public function set(): void { + $this->count = 42; + } +} + +class Baz { + public bool $flag; + + public function set(): void { + $this->flag = true; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag any correct assignments across multiple classes, got: {diags:?}" + ); +} + +#[test] +fn only_wrong_class_flagged_among_multiple_property() { + let php = r#"name = "good"; + } +} + +class Bad { + public int $count; + + public function set(): void { + $this->count = "not a number"; + } +} + +class AlsoGood { + public bool $flag; + + public function set(): void { + $this->flag = true; + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + assert_eq!( + msgs.len(), + 1, + "Expected exactly one property error (in Bad class), got: {msgs:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: multiple properties, only wrong ones flagged +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn flags_only_wrong_properties_in_same_method() { + let php = r#"title = "hello"; + $this->priority = "wrong"; + $this->active = false; + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + assert_eq!( + msgs.len(), + 1, + "Expected exactly one property error (priority), got: {msgs:?}" + ); + assert!( + msgs[0].contains("priority"), + "Expected error to mention 'priority', got: {msgs:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: assignment in switch +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_assignment_in_switch() { + let php = r#"result = "one"; + break; + case 2: + $this->result = "two"; + break; + default: + $this->result = "unknown"; + } + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignments in switch, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_assignment_in_switch_case() { + let php = r#"result = "one"; + break; + default: + $this->result = []; + } + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + assert_eq!( + msgs.len(), + 1, + "Expected one property error (default case), got: {msgs:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: assignment in deeply nested control flow +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_deeply_nested_assignment() { + let php = r#" 0) { + if ($b > 0) { + $this->status = "both positive"; + } else { + $this->status = "a positive"; + } + } else { + $this->status = "a non-positive"; + } + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignments in deeply nested if, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: assignment in for/do-while +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_assignment_in_for() { + let php = r#"result = ""; + for ($i = 0; $i < 10; $i++) { + $this->result = "built"; + } + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment in for loop, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_correct_assignment_in_do_while() { + let php = r#"attempts = 0; + do { + $this->attempts = 1; + } while (false); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment in do-while, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: string concat / arithmetic expressions +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_string_concat_assigned_to_string_property() { + let php = r#"message = "Hello, " . $name; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag string concat assigned to string property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: assignment in declare block +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_assignment_in_declare_block() { + let php = r#"name = "hello"; + } + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct assignment inside declare block, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: method return value assigned to property +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_method_return_assigned_to_matching_property() { + let php = r#"text = $h->getText(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag method return matching property type, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: self/static typed properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_self_assigned_to_self_property() { + let php = r#"next = $other; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag self assigned to ?self property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_new_self_assigned_to_self_property() { + let php = r#"head = new self(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag new self() assigned to ?self property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_null_assigned_to_nullable_self_property() { + let php = r#"next = null; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag null assigned to ?self property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Genuine errors: various real mismatches that SHOULD be flagged +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn flags_object_assigned_to_int_property() { + let php = r#"count = new Foo(); + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for object assigned to int property, got: {diags:?}" + ); +} + +#[test] +fn flags_null_to_non_nullable_int_property() { + let php = r#"value = null; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for null assigned to int property, got: {diags:?}" + ); +} + +#[test] +fn flags_bool_assigned_to_string_property() { + let php = r#"text = false; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for bool assigned to string property, got: {diags:?}" + ); +} + +#[test] +fn flags_string_assigned_to_bool_property() { + let php = r#"on = "yes"; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for string assigned to bool property, got: {diags:?}" + ); +} + +#[test] +fn flags_array_assigned_to_int_property() { + let php = r#"total = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to int property, got: {diags:?}" + ); +} + +#[test] +fn flags_array_assigned_to_bool_property() { + let php = r#"enabled = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to bool property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Edge case: compound assignment operators should NOT be flagged +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_concat_assign() { + let php = r#"buffer .= "more"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag .= compound assignment, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_minus_assign() { + let php = r#"count -= 1; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag -= compound assignment, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_multiply_assign() { + let php = r#"value *= 2; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag *= compound assignment, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_null_coalescing_assign() { + let php = r#"name ??= "default"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag ??= compound assignment, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Edge case: dynamic property name — should be skipped +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_dynamic_property_name() { + let php = r#"$prop = "anything"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag dynamic property name assignment, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Edge case: non-$this property access — should be skipped +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_non_this_property_access() { + // We only check $this->prop and self::$prop, not $other->prop + let php = r#"x = "wrong but we skip non-$this"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag assignment to non-$this property (not checked), got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: nullable class-typed properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_class_assigned_to_nullable_class_property() { + let php = r#"currentUser = new User(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag User assigned to ?User property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_null_assigned_to_nullable_class_property() { + let php = r#"currentUser = null; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag null assigned to ?User property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_subclass_assigned_to_nullable_parent_property() { + let php = r#"parked = new Car(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag subclass Car assigned to ?Vehicle property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: complex union property types with classes +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_class_in_multi_class_union_property() { + let php = r#"result = new Pending(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag Pending assigned to Success|Failure|Pending property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_false_assigned_to_string_or_false_property() { + let php = r#"value = false; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag false assigned to string|false property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_string_assigned_to_string_or_false_property() { + let php = r#"value = "cached"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag string assigned to string|false property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: intersection typed properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_implementing_class_assigned_to_intersection_property() { + let php = r#"collection = new SmartCollection(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag class implementing both interfaces for intersection property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: property assigned from typed parameter +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_typed_param_assigned_to_matching_property() { + let php = r#"dsn = $dsn; + $this->timeout = $timeout; + $this->debug = $debug; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag typed params assigned to matching properties, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: property assigned from cast expressions +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_int_cast_assigned_to_int_property() { + let php = r#"value = (int) $s; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag (int) cast assigned to int property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_string_cast_assigned_to_string_property() { + let php = r#"output = (string) $v; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag (string) cast assigned to string property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_array_cast_assigned_to_array_property() { + let php = r#"data = (array) $o; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag (array) cast assigned to array property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: real-world patterns — builder, entity, DTO +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_complex_entity_class() { + let php = r#"paid = true; + $this->status = "paid"; + } + + public function addNote(string $note): void { + $this->notes = $note; + } + + public function clearNote(): void { + $this->notes = null; + } + + public function setTotal(float $amount): void { + $this->total = $amount; + } + + public function setItems(array $items): void { + $this->items = $items; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag any correct assignments in complex entity, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_dto_with_promoted_properties() { + let php = r#"phone = $phone; + } + + public function withAge(int $age): void { + $this->age = $age; + } + + public function deactivate(): void { + $this->active = false; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag any correct assignments in DTO with promoted props, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: assignment from ternary / null coalescing +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_ternary_assigned_to_string_property() { + let php = r#"mode = $flag ? "verbose" : "quiet"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag ternary returning strings assigned to string property, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_null_coalescing_assigned_to_string_property() { + let php = r#"name = $input ?? "default"; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag null coalescing assigned to string property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: private/protected properties +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_private_property_assignment() { + let php = r#"secret = "hidden"; + $this->guarded = 42; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag correct private/protected property assignments, got: {diags:?}" + ); +} + +#[test] +fn flags_wrong_private_property_assignment() { + let php = r#"secret = []; + } +} +"#; + let diags = collect(php); + assert!( + has_property_error(&diags), + "Expected error for array assigned to private string property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: multiple assignment targets in same statement line +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn flags_only_mismatched_in_sequential_assignments() { + let php = r#"a = "ok"; + $this->b = []; + $this->c = true; + $this->d = "wrong"; + } +} +"#; + let diags = collect(php); + let msgs = property_error_messages(&diags); + // $b gets array (wrong), $d gets string (wrong in strict; but float + // may or may not be juggled from string — depends on strict mode). + // At minimum $b should be flagged. + assert!( + msgs.iter().any(|m| m.contains("$b") || m.contains("b")), + "Expected at least $b to be flagged, got: {msgs:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: exception hierarchy property +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_exception_subclass_assigned_to_exception_property() { + let php = r#"lastError = new ValidationException("bad"); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag deep exception subclass assigned to RuntimeException property, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: enum property assignment +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_backed_enum_value_in_string_context() { + // Backed enum ->value is a string, assigning to string property + // is valid but we don't necessarily track ->value type. Ensure + // we at least don't false-positive here. + let php = r#"primary = Color::Red->value; + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag backed enum ->value assigned to string property, got: {diags:?}" + ); +} diff --git a/tests/integration/diagnostics_return_type_errors.rs b/tests/integration/diagnostics_return_type_errors.rs new file mode 100644 index 00000000..5dfebefd --- /dev/null +++ b/tests/integration/diagnostics_return_type_errors.rs @@ -0,0 +1,2730 @@ +use crate::common::create_test_backend; +use tower_lsp::lsp_types::*; + +// ─── Helpers ──────────────────────────────────────────────────────────────── + +fn collect(php: &str) -> Vec { + let backend = create_test_backend(); + let uri = "file:///test.php"; + backend.update_ast(uri, php); + let mut out = Vec::new(); + backend.collect_return_type_diagnostics(uri, php, &mut out); + out +} + +fn has_return_error(diags: &[Diagnostic]) -> bool { + diags.iter().any(|d| { + d.code + .as_ref() + .is_some_and(|c| matches!(c, NumberOrString::String(s) if s == "type_mismatch_return")) + }) +} + +fn return_error_messages(diags: &[Diagnostic]) -> Vec { + diags + .iter() + .filter(|d| { + d.code.as_ref().is_some_and( + |c| matches!(c, NumberOrString::String(s) if s == "type_mismatch_return"), + ) + }) + .map(|d| d.message.clone()) + .collect() +} + +// ─── Basic: return wrong type from function ───────────────────────────────── + +#[test] +fn flags_array_returned_from_string_function_basic() { + let php = r#" $x * 2; + return "done"; +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Arrow function return should not affect outer function, got: {diags:?}" + ); +} + +#[test] +fn nested_closure_with_array_map_does_not_leak() { + let php = r#" 0) { + if ($b > 0) { + if ($c > 0) { + return "deep"; + } else { + return "c-neg"; + } + } else { + return "b-neg"; + } + } else { + return "a-neg"; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag correct returns in deeply nested if, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_correct_return_in_do_while() { + let php = r#" 100) { + return "big"; + } elseif ($x > 50) { + return "medium"; + } elseif ($x > 0) { + return []; + } else { + return "negative"; + } +} +"#; + let diags = collect(php); + let msgs = return_error_messages(&diags); + assert_eq!( + msgs.len(), + 1, + "Expected exactly one return error (the array branch), got: {msgs:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: declare(strict_types=1) interactions +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn strict_types_flags_int_returned_from_string() { + let php = r#"name = 'default'; + return; + } + $this->name = $name; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag bare return in constructor, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// False-positive tests: returning typed parameters directly +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_returning_typed_parameter() { + let php = r#" 1000) { return "huge"; } + if ($n > 100) { return "large"; } + if ($n > 10) { return "medium"; } + if ($n > 0) { return "small"; } + if ($n === 0) { return "zero"; } + return "negative"; +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag any of the many correct string returns, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Edge case: function with no return statement and non-void type +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_no_return_statement() { + // Functions that throw or loop forever might have no return. + // We only check explicit return statements, not missing returns. + let php = r#" */ + public function getCounts(): array { + return ['a' => 1, 'b' => 2]; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag array literal from method with @return array, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_phpdoc_collection_return() { + let php = r#" */ + public function getUsers(): Collection { + return new Collection(); + } +} + +class User {} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag Collection returned from Collection-typed method, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: generic / typed array return types +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_array_literal_from_typed_array_return() { + // The function returns array but the PHPDoc says int[]. + // An empty array literal should be fine. + let php = r#" */ +function get_names(): array { + return []; +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag empty array from list return type, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_array_from_generic_array_return() { + let php = r#" */ +function get_config(): array { + return ['key' => 'value']; +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag array literal from array, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: nullable union with class types +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_class_returned_from_nullable_class() { + let php = r#" 'Alice', 'age' => 30]; +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag matching array shape return, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: real-world patterns with generics and inheritance +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_builder_pattern_fluent_returns() { + let php = r#"r * $this->r; } +} + +class Square extends Shape { + public function __construct(private float $s) {} + public function area(): float { return $this->s * $this->s; } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag factory methods returning subclasses of self, got: {diags:?}" + ); +} + +#[test] +fn no_diagnostic_for_repository_pattern_nullable_return() { + let php = r#" 'Active', + self::Inactive => 'Inactive', + self::Pending => 'Pending', + }; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag match expression returning strings from string method, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: method with multiple nullable/union return paths +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_complex_conditional_nullable_returns() { + let php = r#"helper->getName(); + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag method call return matching declared type, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: returning from private/protected methods +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_correct_private_method_return() { + let php = r#" $s !== ''; + return []; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag method with internal closures returning correct type, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: PHP 8.1+ enum backed values +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_backed_enum_method_correct_return() { + let php = r#" 'red', + self::Clubs, self::Spades => 'black', + }; + } + + public function isRed(): bool { + return $this === self::Hearts || $this === self::Diamonds; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag correct returns in backed enum methods, got: {diags:?}" + ); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Advanced: never return type (should have no returns at all) +// ═══════════════════════════════════════════════════════════════════════════ + +#[test] +fn no_diagnostic_for_never_function_that_throws() { + let php = r#" $id, 'total' => 100]; + } + + public function getStatus(int $id): string|int { + if ($id <= 0) { + return "unknown"; + } + return $id; + } + + public function process(int $id): bool { + if ($id <= 0) { + return false; + } + return true; + } + + public function getTotal(int $id): float { + return 99.99; + } + + public function cancel(int $id): void { + return; + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "Should not flag any returns in complex service class, got: {diags:?}" + ); +} diff --git a/tests/integration/diagnostics_type_errors.rs b/tests/integration/diagnostics_type_errors.rs index 13a47c04..3614dc90 100644 --- a/tests/integration/diagnostics_type_errors.rs +++ b/tests/integration/diagnostics_type_errors.rs @@ -11,7 +11,7 @@ fn collect(php: &str) -> Vec { let uri = "file:///test.php"; backend.update_ast(uri, php); let mut out = Vec::new(); - backend.collect_type_error_diagnostics(uri, php, &mut out); + backend.collect_argument_type_diagnostics(uri, php, &mut out); out } @@ -20,7 +20,7 @@ fn collect_with_stubs(php: &str) -> Vec { let uri = "file:///test.php"; backend.update_ast(uri, php); let mut out = Vec::new(); - backend.collect_type_error_diagnostics(uri, php, &mut out); + backend.collect_argument_type_diagnostics(uri, php, &mut out); out } @@ -29,7 +29,7 @@ fn collect_with_full_stubs(php: &str) -> Vec { let uri = "file:///test.php"; backend.update_ast(uri, php); let mut out = Vec::new(); - backend.collect_type_error_diagnostics(uri, php, &mut out); + backend.collect_argument_type_diagnostics(uri, php, &mut out); out } @@ -1707,7 +1707,7 @@ class ExplorerController { ); let content = files[2].1; let mut diags = Vec::new(); - backend.collect_type_error_diagnostics(&uri, content, &mut diags); + backend.collect_argument_type_diagnostics(&uri, content, &mut diags); assert!( !has_type_error(&diags), "Should not flag a union of class-strings satisfying a template bound, got: {}", @@ -4268,7 +4268,7 @@ class TestCase { let uri = format!("file://{}/src/TestCase.php", dir.path().display()); let content = files[3].1; let mut diags = Vec::new(); - backend.collect_type_error_diagnostics(&uri, content, &mut diags); + backend.collect_argument_type_diagnostics(&uri, content, &mut diags); let msgs = type_error_messages(&diags); // The argument to ClassNode::__construct is the return value of // getNodeForCallingTestCase which returns ASTNode. The diagnostic @@ -4368,7 +4368,7 @@ class MyException extends NativeException {} backend.update_ast("file:///test.php", php); let mut out = Vec::new(); - backend.collect_type_error_diagnostics("file:///test.php", php, &mut out); + backend.collect_argument_type_diagnostics("file:///test.php", php, &mut out); let msgs = type_error_messages(&out); assert!( msgs.is_empty(), @@ -5971,7 +5971,7 @@ class Caller { let uri = format!("file://{}/src/Caller.php", dir.path().display()); let content = files[2].1; let mut diags = Vec::new(); - backend.collect_type_error_diagnostics(&uri, content, &mut diags); + backend.collect_argument_type_diagnostics(&uri, content, &mut diags); assert!( !has_type_error(&diags), "Imported type alias parameter must not be treated as a class, got: {}", diff --git a/tests/integration/main.rs b/tests/integration/main.rs index 88d711f3..6f83bac7 100644 --- a/tests/integration/main.rs +++ b/tests/integration/main.rs @@ -99,7 +99,9 @@ mod diag_forward_walk_complex_expr; mod diag_timing; mod diagnostics_compound_narrowing; mod diagnostics_deprecated; +mod diagnostics_property_type_errors; mod diagnostics_psr4_mismatch; +mod diagnostics_return_type_errors; mod diagnostics_type_errors; mod diagnostics_undefined_variables; mod diagnostics_unknown_members; From fc6d8eea200a55ea5c6d4b9da8ca512361fe4edd Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Sat, 18 Jul 2026 04:54:10 +0200 Subject: [PATCH 2/6] Fix a few false positives --- src/diagnostics/property_type_errors.rs | 38 +++++----- src/diagnostics/return_type_errors.rs | 39 +++++----- src/diagnostics/type_errors.rs | 30 ++++++++ src/php_type.rs | 74 +++++++++++++++++++ .../diagnostics_property_type_errors.rs | 49 ++++++++++++ .../diagnostics_return_type_errors.rs | 64 ++++++++++++++++ 6 files changed, 251 insertions(+), 43 deletions(-) diff --git a/src/diagnostics/property_type_errors.rs b/src/diagnostics/property_type_errors.rs index d466fa92..4f0067b9 100644 --- a/src/diagnostics/property_type_errors.rs +++ b/src/diagnostics/property_type_errors.rs @@ -320,28 +320,24 @@ fn check_expression_for_property_assignment(expr: &Expression<'_>, ctx: &mut Pro _ => return, }; - // Resolve `self`/`static`/`parent` in the declared property type. - let declared_type = declared_type.resolve_names(&|name: &str| { - let lower = name.to_ascii_lowercase(); - match lower.as_str() { - "self" | "static" | "$this" => prop_class.fqn().to_string(), - "parent" => prop_class - .parent_class - .as_ref() - .map(|p| p.to_string()) - .unwrap_or_else(|| name.to_string()), - _ => { - if name.contains("__anonymous@") { - return name.to_string(); - } - if let Some(cls) = (ctx.class_loader)(name) { - cls.fqn().to_string() - } else { - name.to_string() - } + // Resolve `self`/`static`/`parent`/`$this` in the declared property + // type, then expand any remaining short class names to their + // fully-qualified form. + let declared_type = declared_type + .resolve_self_refs( + prop_class.fqn().as_str(), + prop_class.parent_class.as_deref(), + ) + .resolve_names(&|name: &str| { + if name.contains("__anonymous@") { + return name.to_string(); } - } - }); + if let Some(cls) = (ctx.class_loader)(name) { + cls.fqn().to_string() + } else { + name.to_string() + } + }); // Resolve the RHS expression type. let rhs_span = assign.rhs.span(); diff --git a/src/diagnostics/return_type_errors.rs b/src/diagnostics/return_type_errors.rs index a1bb470e..855805b2 100644 --- a/src/diagnostics/return_type_errors.rs +++ b/src/diagnostics/return_type_errors.rs @@ -760,29 +760,24 @@ fn process_class_member( return; } - // Resolve the declared return type's `self`/`static`/`parent` to - // concrete class names for accurate comparison. - let declared_return = declared_return.resolve_names(&|name: &str| { - let lower = name.to_ascii_lowercase(); - match lower.as_str() { - "self" | "static" | "$this" => current_class.fqn().to_string(), - "parent" => current_class - .parent_class - .as_ref() - .map(|p| p.to_string()) - .unwrap_or_else(|| name.to_string()), - _ => { - if name.contains("__anonymous@") { - return name.to_string(); - } - if let Some(cls) = class_loader(name) { - cls.fqn().to_string() - } else { - name.to_string() - } + // Resolve the declared return type's `self`/`static`/`parent`/`$this` + // to concrete class names for accurate comparison, then expand any + // remaining short class names to their fully-qualified form. + let declared_return = declared_return + .resolve_self_refs( + current_class.fqn().as_str(), + current_class.parent_class.as_deref(), + ) + .resolve_names(&|name: &str| { + if name.contains("__anonymous@") { + return name.to_string(); } - } - }); + if let Some(cls) = class_loader(name) { + cls.fqn().to_string() + } else { + name.to_string() + } + }); let loaders = Loaders { function_loader: Some(function_loader), diff --git a/src/diagnostics/type_errors.rs b/src/diagnostics/type_errors.rs index 6ce184f4..654a089e 100644 --- a/src/diagnostics/type_errors.rs +++ b/src/diagnostics/type_errors.rs @@ -236,6 +236,21 @@ pub(super) fn is_type_compatible( return true; } + // ── Intersection argument handling ─────────────────────────── + // A value of intersection type `A&B` satisfies *every* member, so + // it is compatible with the param when *any* member is. This is + // the standard subtyping rule for intersections and covers common + // cases like PHPUnit's `MockObject&Foo` (a mock that is also a Foo) + // being returned where `Foo` (or a union containing `Foo`) is + // expected. + if let PhpType::Intersection(members) = arg_type + && members + .iter() + .any(|m| is_type_compatible(m, param_type, class_loader, strict_types)) + { + return true; + } + // ── Conservative union parameter handling ──────────────────── // When the param is a union, accept if the arg is compatible // with *any* member. This extends the structural check to use @@ -249,6 +264,21 @@ pub(super) fn is_type_compatible( return true; } + // ── Conservative intersection parameter handling ───────────── + // When the param is an intersection `A&B`, the value must satisfy + // *every* member. Stay silent unless the arg is definitely + // incompatible with at least one member — i.e. accept when the arg + // is compatible with all members we can check. Combined with the + // conservative rules above, this avoids false positives on mock + // types like `MethodNode&MockObject`. + if let PhpType::Intersection(members) = param_type + && members + .iter() + .all(|m| is_type_compatible(arg_type, m, class_loader, strict_types)) + { + return true; + } + // ── Bare Closure/callable ↔ callable specification: MAYBE ─── // When the param is a callable specification like // `Closure(Builder): mixed` and the arg is a bare `Closure` diff --git a/src/php_type.rs b/src/php_type.rs index 95726fb6..81864488 100644 --- a/src/php_type.rs +++ b/src/php_type.rs @@ -1894,6 +1894,32 @@ impl PhpType { self.replace_self_with_type(&PhpType::Named(class_name.to_string())) } + /// Resolve relative class-reference keywords to concrete class names, + /// walking the entire type tree (including array elements and generic + /// arguments). + /// + /// `self`, `static`, and `$this` become `class_name`; `parent` becomes + /// `parent_class` when it is `Some`. Unlike [`resolve_names`], which + /// treats these keywords as non-class types and leaves them untouched, + /// this resolves them so a declared type can be compared against a + /// resolved value type. + /// + /// [`resolve_names`]: PhpType::resolve_names + pub fn resolve_self_refs(&self, class_name: &str, parent_class: Option<&str>) -> PhpType { + // self / static / $this — case-insensitive, whole-tree walk. + let replaced = self.replace_self(class_name); + match parent_class { + Some(parent) => { + let subs = std::collections::HashMap::from([( + "parent".to_string(), + PhpType::Named(parent.to_string()), + )]); + replaced.substitute(&subs) + } + None => replaced, + } + } + /// Replace only the `self` keyword (not `static` or `$this`) with a /// concrete class name. Used during inheritance merging so that /// inherited methods carry the declaring class's identity for `self` @@ -6540,6 +6566,54 @@ mod tests { assert_eq!(replaced.to_string(), "App\\User&JsonSerializable"); } + // ── resolve_self_refs ─────────────────────────────────────── + + #[test] + fn resolve_self_refs_bare() { + let ty = PhpType::parse("self"); + assert_eq!( + ty.resolve_self_refs("App\\Cat", None).to_string(), + "App\\Cat" + ); + } + + #[test] + fn resolve_self_refs_in_array_element() { + // `self[]` inside an array must be resolved to the concrete class. + // Regression: `resolve_names` treats `self` as a keyword and skips + // it, so array/generic element `self` was left unresolved. + let ty = PhpType::parse("self[]"); + let resolved = ty.resolve_self_refs("App\\Cat", None); + assert_eq!(resolved, PhpType::parse("App\\Cat[]")); + } + + #[test] + fn resolve_self_refs_static_in_generic() { + let ty = PhpType::parse("array"); + let resolved = ty.resolve_self_refs("App\\Cat", None); + assert_eq!(resolved, PhpType::parse("array")); + } + + #[test] + fn resolve_self_refs_parent() { + let ty = PhpType::parse("parent[]"); + let resolved = ty.resolve_self_refs("App\\Cat", Some("App\\Animal")); + assert_eq!(resolved, PhpType::parse("App\\Animal[]")); + } + + #[test] + fn resolve_self_refs_parent_without_parent_class_unchanged() { + let ty = PhpType::parse("parent"); + assert_eq!(ty.resolve_self_refs("App\\Cat", None).to_string(), "parent"); + } + + #[test] + fn resolve_self_refs_leaves_other_classes_alone() { + let ty = PhpType::parse("Other[]"); + let resolved = ty.resolve_self_refs("App\\Cat", None); + assert_eq!(resolved, PhpType::parse("Other[]")); + } + // ── extract_class_names (recursive) ───────────────────────── #[test] diff --git a/tests/integration/diagnostics_property_type_errors.rs b/tests/integration/diagnostics_property_type_errors.rs index ab32d78c..0ac3dc11 100644 --- a/tests/integration/diagnostics_property_type_errors.rs +++ b/tests/integration/diagnostics_property_type_errors.rs @@ -2201,3 +2201,52 @@ class Palette { "Should not flag backed enum ->value assigned to string property, got: {diags:?}" ); } + +#[test] +fn no_diagnostic_for_self_typed_property_assignment() { + // A property typed `self` (or an array of `self`) must resolve to the + // enclosing class so an assignment of the concrete type compares equal. + let php = r#"next = new Node(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag Node assigned to self-typed property, got: {:?}", + property_error_messages(&diags) + ); +} + +#[test] +fn no_diagnostic_for_intersection_assigned_to_property() { + // An intersection value `A&B` satisfies each member, so assigning it to + // a property typed `A` is compatible. + let php = r#"service = $this->mock(); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Should not flag Service&Countable assigned to Service property, got: {:?}", + property_error_messages(&diags) + ); +} diff --git a/tests/integration/diagnostics_return_type_errors.rs b/tests/integration/diagnostics_return_type_errors.rs index 5dfebefd..a9eaa9a8 100644 --- a/tests/integration/diagnostics_return_type_errors.rs +++ b/tests/integration/diagnostics_return_type_errors.rs @@ -968,6 +968,70 @@ class Builder { ); } +#[test] +fn no_diagnostic_for_self_array_return() { + // `@return self[]` returning an array of the enclosing class. `self` + // inside the array element must resolve to the concrete class so the + // element types compare equal. + let php = r#" Date: Sat, 18 Jul 2026 05:25:58 +0200 Subject: [PATCH 3/6] A conditional `@return` type is evaluated at the call site even when it is declared on an interface --- docs/CHANGELOG.md | 1 + docs/todo/bugs.md | 14 +- src/completion/variable/rhs_resolution.rs | 345 +++++++++++------- .../diagnostics_property_type_errors.rs | 52 +++ .../diagnostics_return_type_errors.rs | 87 +++++ 5 files changed, 358 insertions(+), 141 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index d7f02c71..ed4cc5a7 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A conditional `@return` type is evaluated at the call site even when it is declared on an interface.** A method whose PHPStan conditional return type narrows a literal-array argument (as in Spatie LaravelData's `Data::collect([...])`, which yields `array` for an array) now resolves to that narrowed branch instead of the method's broad declared union. This holds when the conditional is declared on an interface while the concrete method comes from a trait, and the narrowed branch now supersedes the union rather than being discarded (which previously also dropped the union's `array` member). Member access, hover, and the return- and property-type mismatch diagnostics all see the precise type, eliminating false "incompatible with declared type `array`" reports. - **A property assigned from a function of itself no longer crashes analysis.** A self-referencing assignment such as `$this->items = array_unique(array_merge($this->items, $more))`, where the property is read on its own right-hand side, previously sent type resolution into unbounded recursion that overflowed the stack and aborted the whole `analyze` run. The property now resolves to its declared type instead. - **Method-call chains no longer report a spurious unresolved type on one branch but not an adjacent identical one.** A variable assigned from a call whose return type is inferred from the callee's body (for example an Eloquent query builder returned by an un-annotated `query()` method) kept its type across the whole method, so every `$query->whereBetween(...)->pluck(...)` chain resolves consistently. Previously the receiver's type could be dropped for one branch while an identically shaped adjacent branch resolved cleanly, producing an intermittent "type could not be resolved" warning. - **A variable destructured from an untyped array can be narrowed by a later assertion.** When list-destructuring pulls variables out of a value whose type is unknown (`[$type, $variable] = $declarations[0]` where `$declarations` is a bare `array`), a following `assertInstanceOf(Wanted::class, $type)` now narrows `$type` to the asserted class, so member access on it resolves instead of reporting the type as unresolvable. A plain assignment from the same untyped value already worked; only the destructuring form left the variables unnarrowable. diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index 0566a919..ebd5e1d7 100644 --- a/docs/todo/bugs.md +++ b/docs/todo/bugs.md @@ -7,4 +7,16 @@ pipeline so it produces correct data. Downstream consumers (diagnostics, hover, completion, definition) should never need to second-guess upstream output. -No outstanding items. +## `ReflectionClass::newInstanceArgs()` returns the class-string, not the instance + +`new ReflectionClass($class)` where `$class` is +`class-string` binds the +`ReflectionClass` template parameter to the *class-string* rather than +unwrapping it to the object type `T`. As a result `$reflection->newInstanceArgs(...)` +(and `newInstance()`) resolve to `class-string<...>|null` instead of the +instantiated object type. Surfaced by the return-type mismatch diagnostic +on `pdepend`'s `ASTNodeTestCase::createNodeInstance()`, which returns +`$reflection->newInstanceArgs(...)` from a method declared +`: AbstractASTNode|ASTAnonymousClass`. The fix is to unwrap `class-string` +to `X` when binding the `ReflectionClass` constructor argument to the class +template parameter. diff --git a/src/completion/variable/rhs_resolution.rs b/src/completion/variable/rhs_resolution.rs index 8ccf4421..0ea3b84b 100644 --- a/src/completion/variable/rhs_resolution.rs +++ b/src/completion/variable/rhs_resolution.rs @@ -3375,7 +3375,7 @@ fn resolve_rhs_method_call_inner<'b>( merged.get_method_ci("__call") } }; - let ret_type_string = method_ref.and_then(|m| m.return_type.as_ref()).map(|ret| { + let native_ret_type_string = method_ref.and_then(|m| m.return_type.as_ref()).map(|ret| { let substituted = if !template_subs.is_empty() { ret.substitute(&template_subs).simplified() } else { @@ -3413,64 +3413,35 @@ fn resolve_rhs_method_call_inner<'b>( // (a method-call chain, property access, …) rather than a literal. let arg_ty_resolver = |t: &str| Backend::resolve_arg_text_to_type(t, &rctx); - // When the method declares a PHPStan conditional return type, - // evaluate it against the call-site arguments and prefer the - // resolved type. This carries mode-dependent shapes (e.g. - // `list<\stdClass>` or `array|false` from - // `PDOStatement::fetch`/`fetchAll`) into consumers that read the - // type string (hover, foreach element extraction) rather than the - // vague native return type (`array`/`mixed`). - let ret_type_string = match method_ref.and_then(|m| m.conditional_return.as_ref()) { - Some(cond) => { - let params = method_ref.map(|m| m.parameters.as_slice()).unwrap_or(&[]); - let tpl = crate::completion::conditional_resolution::TemplateContext { - defaults: None, - params: method_ref - .map(|m| m.template_params.as_slice()) - .unwrap_or(&[]), - arg_type_resolver: Some(&arg_ty_resolver), - }; - crate::completion::conditional_resolution::resolve_conditional_with_text_args_and_defaults( - cond, - params, - &text_args, - Some(&var_resolver), - Some(&ctx.current_class.name), - ctx.class_loader, - &tpl, - ) - .map(|resolved| { - let substituted = if template_subs.is_empty() { - resolved - } else { - resolved.substitute(&template_subs) - }; - // Replace `static`/`self`/`$this` in the resolved branch - // just as the native return-type path does, so a branch - // like `static>` becomes - // `Collection>` instead of - // carrying the unresolvable `static` keyword downstream. - if substituted.contains_self_ref() { - match receiver_type_for_owner(&receiver_resolved, &owner.name) { - Some(rt) => substituted.replace_self_with_type(&rt), - None => substituted.replace_self(&owner.fqn()), - } - } else { - substituted - } - }) - .or(ret_type_string) - } - None => ret_type_string, - }; + // Resolve the PHPStan conditional return type against the call-site + // arguments, if the method declares one. When it yields an + // informative type it is *authoritative*: the branch it selects + // (e.g. `list<\stdClass>` from `PDOStatement::fetchAll`, or + // `array` for a literal-array argument) supersedes the + // method's broad native union return type. Resolving classes from + // the native union instead would both ignore the call-site narrowing + // and silently drop scalar or `array` members the union carries. + let conditional_ret = resolve_conditional_return_for_call( + method_ref, + &text_args, + Some(&var_resolver), + &ctx.current_class.name, + ctx.class_loader, + &template_subs, + Some(&arg_ty_resolver), + |ty| match receiver_type_for_owner(&receiver_resolved, &owner.name) { + Some(rt) => ty.replace_self_with_type(&rt), + None => ty.replace_self(&owner.fqn()), + }, + ); // Collapse any conditionals nested inside the (template-substituted) - // return type against the call arguments, so a generic wrapper like - // `Collection<($groupBy is array|string ? array-key : …), …>` yields - // a concrete key type instead of carrying a raw conditional that - // later gets compared against — and printed in — an argument-type - // diagnostic. - let ret_type_string = ret_type_string.map(|ty| { + // native return type against the call arguments, so a generic + // wrapper like `Collection<($groupBy is array|string ? array-key : + // …), …>` yields a concrete key type instead of carrying a raw + // conditional that later gets compared against — and printed in — an + // argument-type diagnostic. + let native_ret_type_string = native_ret_type_string.map(|ty| { if ty.contains_conditional() { let params = method_ref.map(|m| m.parameters.as_slice()).unwrap_or(&[]); let tpl = crate::completion::conditional_resolution::TemplateContext { @@ -3494,6 +3465,24 @@ fn resolve_rhs_method_call_inner<'b>( } }); + // When the conditional resolved to a definite, informative type, it + // wins — resolve the result classes from it directly. + if let Some(cond_ty) = conditional_ret { + let owner_results = resolve_from_authoritative_type( + cond_ty, + &ctx.current_class.name, + ctx.all_classes, + ctx.class_loader, + ); + if !is_union { + return owner_results; + } + ResolvedType::extend_unique(&mut union_results, owner_results); + continue; + } + + let ret_type_string = native_ret_type_string; + let results = Backend::resolve_method_return_types_with_args( owner, &method_name, @@ -3502,24 +3491,7 @@ fn resolve_rhs_method_call_inner<'b>( ); if !results.is_empty() { let classes: Vec> = results; - // When the method has a conditional return type, - // `ret_type_string` already holds the branch resolved - // against the call-site arguments (generics and self/static - // substituted), so it is the correct hint — not the - // declared return type. Keep it so generic arguments like - // `Collection>` survive; drop - // it only when the resolved branch is uninformative (e.g. a - // bare `mixed` else-branch), letting the resolved class - // names speak for themselves. - let has_conditional = merged - .get_method_ci(&method_name) - .is_some_and(|m| m.conditional_return.is_some()); - let effective_hint = if has_conditional { - ret_type_string.filter(|t| !t.is_uninformative_return()) - } else { - ret_type_string - }; - let owner_results = match effective_hint { + let owner_results = match ret_type_string { Some(hint) => ResolvedType::from_classes_with_hint(classes, hint), None => ResolvedType::from_classes(classes), }; @@ -3724,6 +3696,117 @@ fn receiver_type_for_owner( None } +/// Resolve a method's PHPStan conditional return type against the call-site +/// arguments, returning the winning branch's type when it is definite and +/// informative. +/// +/// The returned type has template substitutions applied, `self`/`static`/ +/// `$this` replaced (via the `replace_self` closure, which differs between the +/// instance and static call paths), and any conditionals nested inside the +/// winning branch collapsed. Returns `None` when the method has no +/// conditional return type, the condition cannot be decided from the +/// arguments, or the winning branch is uninformative (a bare `mixed`/`array` +/// else-branch) — in which case the caller falls back to the native return +/// type so the full union (including scalar/`array` members) is preserved. +#[allow(clippy::too_many_arguments)] +fn resolve_conditional_return_for_call( + method_ref: Option<&crate::types::MethodInfo>, + text_args: &str, + var_resolver: crate::completion::conditional_resolution::VarClassStringResolver<'_>, + calling_class_name: &str, + class_loader: &dyn Fn(&str) -> Option>, + template_subs: &HashMap, + arg_type_resolver: crate::completion::conditional_resolution::ArgTypeResolver<'_>, + replace_self: impl Fn(&PhpType) -> PhpType, +) -> Option { + let method = method_ref?; + let cond = method.conditional_return.as_ref()?; + let params = method.parameters.as_slice(); + let tpl = crate::completion::conditional_resolution::TemplateContext { + defaults: None, + params: method.template_params.as_slice(), + arg_type_resolver, + }; + let resolved = + crate::completion::conditional_resolution::resolve_conditional_with_text_args_and_defaults( + cond, + params, + text_args, + var_resolver, + Some(calling_class_name), + class_loader, + &tpl, + )?; + let substituted = if template_subs.is_empty() { + resolved + } else { + resolved.substitute(template_subs) + }; + let substituted = if substituted.contains_self_ref() { + replace_self(&substituted) + } else { + substituted + }; + // Collapse any conditionals nested inside the winning branch. + let collapsed = if substituted.contains_conditional() { + let tpl2 = crate::completion::conditional_resolution::TemplateContext { + defaults: Some(template_subs), + params: method.template_params.as_slice(), + arg_type_resolver, + }; + crate::completion::conditional_resolution::evaluate_nested_conditionals_text( + &substituted, + params, + text_args, + var_resolver, + Some(calling_class_name), + class_loader, + &tpl2, + ) + } else { + substituted + }; + if collapsed.is_uninformative_return() { + None + } else { + Some(collapsed) + } +} + +/// Resolve an authoritative return type (e.g. a call-site-narrowed +/// conditional branch) to `ResolvedType` values. +/// +/// Prefers class-backed results when the type names concrete classes, keeping +/// the full type string as the hint (so generics like `Collection` +/// survive). When the type names no class (a bare `array<…>`, `list<…>`, +/// scalar, or shape) a type-string-only entry is returned so consumers that +/// read `.type_string` still see it. `void` collapses to `null`. +fn resolve_from_authoritative_type( + ty: PhpType, + current_class_name: &str, + all_classes: &[Arc], + class_loader: &dyn Fn(&str) -> Option>, +) -> Vec { + let classes = crate::completion::type_resolution::type_hint_to_classes_typed( + &ty, + current_class_name, + all_classes, + class_loader, + ); + if !classes.is_empty() { + return ResolvedType::from_classes_with_hint(classes, ty); + } + if ty == PhpType::void() { + return vec![ResolvedType::from_type_string(PhpType::null())]; + } + vec![resolved_type_with_lookup( + ty, + current_class_name, + all_classes, + class_loader, + )] +} + /// Resolve a static method call: `ClassName::method()`, `self::method()`, /// `static::method()`. fn resolve_rhs_static_call( @@ -3939,14 +4022,15 @@ fn resolve_rhs_static_call( let method_ref = owner .get_method_ci(&method_name) .or_else(|| merged.get_method_ci(&method_name)); - let ret_type_string = method_ref.and_then(|m| m.return_type.as_ref()).map(|ret| { - let substituted = if !template_subs.is_empty() { - ret.substitute(&template_subs) - } else { - ret.clone() - }; - substituted.replace_self(&owner.fqn()) - }); + let native_ret_type_string = + method_ref.and_then(|m| m.return_type.as_ref()).map(|ret| { + let substituted = if !template_subs.is_empty() { + ret.substitute(&template_subs) + } else { + ret.clone() + }; + substituted.replace_self(&owner.fqn()) + }); // Resolver from an argument's source text to its type, used to // evaluate `is ` conditions whose argument is an expression @@ -3954,51 +4038,31 @@ fn resolve_rhs_static_call( // literal, so the correct branch is chosen. let arg_ty_resolver = |t: &str| Backend::resolve_arg_text_to_type(t, &rctx); - // Prefer the conditional return type resolved against the - // call-site arguments (see the instance-call path above). - let ret_type_string = match method_ref.and_then(|m| m.conditional_return.as_ref()) { - Some(cond) => { - let params = method_ref.map(|m| m.parameters.as_slice()).unwrap_or(&[]); - let tpl = crate::completion::conditional_resolution::TemplateContext { - defaults: None, - params: method_ref - .map(|m| m.template_params.as_slice()) - .unwrap_or(&[]), - arg_type_resolver: Some(&arg_ty_resolver), - }; - crate::completion::conditional_resolution::resolve_conditional_with_text_args_and_defaults( - cond, - params, - &text_args, - Some(&var_resolver), - Some(&ctx.current_class.name), - ctx.class_loader, - &tpl, - ) - .map(|resolved| { - let substituted = if template_subs.is_empty() { - resolved - } else { - resolved.substitute(&template_subs) - }; - // Replace `static`/`self`/`$this` in the resolved - // branch just as the native return-type path does, - // so generic branches keep a resolvable base class. - if substituted.contains_self_ref() { - substituted.replace_self(&owner.fqn()) - } else { - substituted - } - }) - .or(ret_type_string) - } - None => ret_type_string, - }; + // Resolve the PHPStan conditional return type against the + // call-site arguments, if the method declares one. When this + // yields an informative type it is *authoritative*: the branch + // it selects (e.g. `array` for a literal-array + // argument) supersedes the method's broad native union return + // type. Resolving classes from the native union instead would + // both ignore the call-site narrowing and silently drop scalar + // or `array` members that the union carries. + let conditional_ret = resolve_conditional_return_for_call( + method_ref, + &text_args, + Some(&var_resolver), + &ctx.current_class.name, + ctx.class_loader, + &template_subs, + Some(&arg_ty_resolver), + |ty| ty.replace_self(&owner.fqn()), + ); - // Collapse conditionals nested inside the return type (e.g. a - // static factory returning `Collection<($k is array|string ? - // array-key : …), …>`) against the call arguments. - let ret_type_string = ret_type_string.map(|ty| { + // Collapse conditionals nested inside the native return type + // (e.g. a static factory returning `Collection<($k is + // array|string ? array-key : …), …>`) against the call + // arguments. Only applies when there is no top-level + // conditional (that path is handled above). + let native_ret_type_string = native_ret_type_string.map(|ty| { if ty.contains_conditional() { let params = method_ref.map(|m| m.parameters.as_slice()).unwrap_or(&[]); let tpl = crate::completion::conditional_resolution::TemplateContext { @@ -4022,6 +4086,19 @@ fn resolve_rhs_static_call( } }); + // When the conditional resolved to a definite, informative type, + // it wins — resolve the result classes from it directly. + if let Some(cond_ty) = conditional_ret { + return resolve_from_authoritative_type( + cond_ty, + current_class_name, + ctx.all_classes, + ctx.class_loader, + ); + } + + let ret_type_string = native_ret_type_string; + let results = Backend::resolve_method_return_types_with_args( owner, &method_name, @@ -4030,19 +4107,7 @@ fn resolve_rhs_static_call( ); if !results.is_empty() { let classes: Vec> = results; - // `ret_type_string` already holds the conditional branch - // resolved against the call-site arguments (with generics - // and self/static substituted), so it is the correct hint. - // Drop it only when the resolved branch is uninformative. - let has_conditional = merged - .get_method_ci(&method_name) - .is_some_and(|m| m.conditional_return.is_some()); - let effective_hint = if has_conditional { - ret_type_string.filter(|t| !t.is_uninformative_return()) - } else { - ret_type_string - }; - return match effective_hint { + return match ret_type_string { Some(hint) => ResolvedType::from_classes_with_hint(classes, hint), None => ResolvedType::from_classes(classes), }; diff --git a/tests/integration/diagnostics_property_type_errors.rs b/tests/integration/diagnostics_property_type_errors.rs index 0ac3dc11..56c93095 100644 --- a/tests/integration/diagnostics_property_type_errors.rs +++ b/tests/integration/diagnostics_property_type_errors.rs @@ -2250,3 +2250,55 @@ class Test { property_error_messages(&diags) ); } + +// ─── Conditional return type narrows over a broad native union ────────────── + +#[test] +fn conditional_return_over_union_assigned_to_array_property_no_error() { + // Assigning `X::collect([...])` to a `@var array` property should + // resolve through the method's conditional `@return`, narrowing the + // literal-array argument to `array` rather than the method's + // broad native union return type. Mirrors Spatie LaravelData usage. + let php = r#" : ($items is array ? array : DataCollection)) + */ + public static function collect(mixed $items, ?string $into = null): array|DataCollection|Enumerable|Collection; +} + +trait BaseDataTrait { + public static function collect(mixed $items, ?string $into = null): array|DataCollection|Enumerable|Collection { + return []; + } +} + +class Data implements BaseDataContract { + use BaseDataTrait; +} + +class AccordionData extends Data {} + +class DataCollection {} +class Enumerable {} +class Collection {} + +class Component { + /** @var array */ + public array $items; + + public function __construct() { + $this->items = AccordionData::collect([1, 2, 3]); + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "collect([...]) narrowed via conditional should satisfy array property, got: {}", + property_error_messages(&diags).join("; ") + ); +} diff --git a/tests/integration/diagnostics_return_type_errors.rs b/tests/integration/diagnostics_return_type_errors.rs index a9eaa9a8..788d3dc8 100644 --- a/tests/integration/diagnostics_return_type_errors.rs +++ b/tests/integration/diagnostics_return_type_errors.rs @@ -2792,3 +2792,90 @@ class OrderService { "Should not flag any returns in complex service class, got: {diags:?}" ); } + +// ─── Conditional return type narrows over a broad native union ────────────── + +#[test] +fn conditional_return_over_union_narrows_array_literal_no_error() { + // A method whose PHPStan conditional `@return` narrows a literal-array + // argument to `array` should be evaluated at the call site, and + // the narrowed branch must supersede the method's broad native union + // return type (which would otherwise drop the `array` member). Mirrors + // Spatie LaravelData's `Data::collect()`, where the conditional lives on + // an interface while the concrete method comes from a trait. + let php = r#" : ($items is array ? array : DataCollection)) + */ + public static function collect(mixed $items, ?string $into = null): array|DataCollection|Enumerable|Collection; +} + +trait BaseDataTrait { + public static function collect(mixed $items, ?string $into = null): array|DataCollection|Enumerable|Collection { + return []; + } +} + +class Data implements BaseDataContract { + use BaseDataTrait; +} + +class AccordionData extends Data {} + +class DataCollection {} +class Enumerable {} +class Collection {} + +class Factory { + /** @return array */ + public static function make(): array { + return AccordionData::collect([1, 2, 3]); + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "collect([...]) narrowed via conditional should satisfy array, got: {}", + return_error_messages(&diags).join("; ") + ); +} + +#[test] +fn conditional_return_over_union_on_instance_call_no_error() { + // The same authoritative-conditional behaviour must apply to instance + // method calls, not just static ones: `$factory->build([...])` whose + // conditional narrows a literal array to `list` should supersede + // the method's broad native union return type. + let php = r#" : WidgetCollection) + */ + public function build(mixed $items): array|WidgetCollection|WidgetBag { + return []; + } +} + +class Consumer { + /** @return list */ + public function run(Factory $factory): array { + return $factory->build([1, 2, 3]); + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "instance build([...]) narrowed via conditional should satisfy list, got: {}", + return_error_messages(&diags).join("; ") + ); +} From 45beb95a7832581a4bde4eff0fb492a5629c07a4 Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Sat, 18 Jul 2026 05:48:35 +0200 Subject: [PATCH 4/6] `new ReflectionClass($class)` resolves instances to the reflected type --- docs/CHANGELOG.md | 1 + docs/todo/bugs.md | 14 +---- examples/demo.php | 33 ++++++++++++ src/docblock/tags.rs | 18 ++++--- tests/integration/diagnostics_type_errors.rs | 57 ++++++++++++++++++++ tests/unit/docblock_parsing.rs | 11 ++++ 6 files changed, 115 insertions(+), 19 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index ed4cc5a7..55425c59 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -54,6 +54,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **A conditional `@return` type is evaluated at the call site even when it is declared on an interface.** A method whose PHPStan conditional return type narrows a literal-array argument (as in Spatie LaravelData's `Data::collect([...])`, which yields `array` for an array) now resolves to that narrowed branch instead of the method's broad declared union. This holds when the conditional is declared on an interface while the concrete method comes from a trait, and the narrowed branch now supersedes the union rather than being discarded (which previously also dropped the union's `array` member). Member access, hover, and the return- and property-type mismatch diagnostics all see the precise type, eliminating false "incompatible with declared type `array`" reports. +- **`new ReflectionClass($class)` resolves instances to the reflected type.** When the argument is a `class-string`, `newInstance()` and `newInstanceArgs()` now resolve to the object type `T` (nullable for `newInstanceArgs`) instead of the class-string, so returning `$reflection->newInstanceArgs(...)` from a method declared to return that object type no longer reports a false return-type mismatch. More generally, a docblock type now refines a native union that mixes `object` with a scalar (such as the `object|string` hint many reflection stubs carry), where it was previously discarded. - **A property assigned from a function of itself no longer crashes analysis.** A self-referencing assignment such as `$this->items = array_unique(array_merge($this->items, $more))`, where the property is read on its own right-hand side, previously sent type resolution into unbounded recursion that overflowed the stack and aborted the whole `analyze` run. The property now resolves to its declared type instead. - **Method-call chains no longer report a spurious unresolved type on one branch but not an adjacent identical one.** A variable assigned from a call whose return type is inferred from the callee's body (for example an Eloquent query builder returned by an un-annotated `query()` method) kept its type across the whole method, so every `$query->whereBetween(...)->pluck(...)` chain resolves consistently. Previously the receiver's type could be dropped for one branch while an identically shaped adjacent branch resolved cleanly, producing an intermittent "type could not be resolved" warning. - **A variable destructured from an untyped array can be narrowed by a later assertion.** When list-destructuring pulls variables out of a value whose type is unknown (`[$type, $variable] = $declarations[0]` where `$declarations` is a bare `array`), a following `assertInstanceOf(Wanted::class, $type)` now narrows `$type` to the asserted class, so member access on it resolves instead of reporting the type as unresolvable. A plain assignment from the same untyped value already worked; only the destructuring form left the variables unnarrowable. diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index ebd5e1d7..0566a919 100644 --- a/docs/todo/bugs.md +++ b/docs/todo/bugs.md @@ -7,16 +7,4 @@ pipeline so it produces correct data. Downstream consumers (diagnostics, hover, completion, definition) should never need to second-guess upstream output. -## `ReflectionClass::newInstanceArgs()` returns the class-string, not the instance - -`new ReflectionClass($class)` where `$class` is -`class-string` binds the -`ReflectionClass` template parameter to the *class-string* rather than -unwrapping it to the object type `T`. As a result `$reflection->newInstanceArgs(...)` -(and `newInstance()`) resolve to `class-string<...>|null` instead of the -instantiated object type. Surfaced by the return-type mismatch diagnostic -on `pdepend`'s `ASTNodeTestCase::createNodeInstance()`, which returns -`$reflection->newInstanceArgs(...)` from a method declared -`: AbstractASTNode|ASTAnonymousClass`. The fix is to unwrap `class-string` -to `X` when binding the `ReflectionClass` constructor argument to the class -template parameter. +No outstanding items. diff --git a/examples/demo.php b/examples/demo.php index 9220bc83..eea83da0 100644 --- a/examples/demo.php +++ b/examples/demo.php @@ -4267,9 +4267,38 @@ public function mockPath(): string } } +// ── ReflectionClass instantiation ──────────────────────────────────────── +// `new ReflectionClass($classString)` binds the reflected type from a +// `class-string`. `newInstance()` returns `T` and `newInstanceArgs()` +// returns `T|null`, even though the constructor's native hint is the broad +// `object|string`. + +class ReflectionInstantiationDemo +{ + /** + * @param class-string $class + */ + public function build(string $class): ReflectedWidget + { + // Fully-qualified `\ReflectionClass` so the unused-import demo above + // keeps its `use ReflectionClass;` dimmed. + $reflection = new \ReflectionClass($class); + + // newInstance() → ReflectedWidget (not class-string) + return $reflection->newInstance(); + } +} + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ // ┃ SCAFFOLDING — Supporting definitions below this line. ┃ +// ── ReflectionClass instantiation scaffolding ──────────────────────────── + +class ReflectedWidget +{ + public function label(): string { return 'widget'; } +} + // ── @phpstan-require-extends scaffolding ──────────────────────────────────── class Mock @@ -6648,6 +6677,10 @@ function runDemoAssertions(): void assert($cfd instanceof Model, 'ClassFilteringDemo must extend Model'); assert($cfd instanceof Renderable, 'ClassFilteringDemo must implement Renderable'); + // ── ReflectionClass instantiation ──────────────────────────────── + $widget = (new ReflectionInstantiationDemo())->build(ReflectedWidget::class); + assert($widget instanceof ReflectedWidget, 'ReflectionClass::newInstance() must be ReflectedWidget'); + // ── Inline new chaining ───────────────────────────────────────────── $fromNew = (new Canvas())->getBrush(); assert($fromNew instanceof Brush, '(new Canvas())->getBrush() must be Brush'); diff --git a/src/docblock/tags.rs b/src/docblock/tags.rs index c86d94fe..2dbb4411 100644 --- a/src/docblock/tags.rs +++ b/src/docblock/tags.rs @@ -1384,15 +1384,21 @@ pub fn should_override_type_typed(docblock_type: &PhpType, native_type: &PhpType // If the native type is a union or intersection, check each component. // A member is refinable when it is non-scalar (a class the docblock can - // parameterise) or a broad container (`array`, `iterable`, `callable`) - // that a docblock commonly refines. `is_scalar()` counts bare `array` - // as scalar, so without the container check a native union like - // `array|false` would wrongly reject a `array|false` docblock - // and drop the element type. + // parameterise) or a broad type (`array`, `iterable`, `callable`, + // `object`) that a docblock commonly refines. `is_scalar()` counts bare + // `array` and `object` as scalar, so without the explicit container and + // object checks a native union like `array|false` would wrongly reject a + // `array|false` docblock (dropping the element type), and + // `object|string` would reject a `class-string|T` docblock (as with + // `new ReflectionClass($classString)`, dropping the template binding). match native_inner { PhpType::Union(members) | PhpType::Intersection(members) => { return members.iter().any(|m| { - !m.is_scalar() || m.is_bare_array() || m.is_iterable() || m.is_callable() + !m.is_scalar() + || m.is_bare_array() + || m.is_iterable() + || m.is_callable() + || m.is_object() }); } _ => {} diff --git a/tests/integration/diagnostics_type_errors.rs b/tests/integration/diagnostics_type_errors.rs index 3614dc90..27f094a3 100644 --- a/tests/integration/diagnostics_type_errors.rs +++ b/tests/integration/diagnostics_type_errors.rs @@ -5978,3 +5978,60 @@ class Caller { type_error_messages(&diags).join(", ") ); } + +/// A `new ReflectionClass($classString)` where the argument is a +/// `class-string` binds the class template parameter `T` to the object +/// type, not the class-string itself. The phpstorm-stubs constructor is +/// annotated `@param class-string|T $objectOrClass` but its native hint +/// comes from a `#[LanguageLevelTypeAware]` attribute resolving to +/// `object|string`. The docblock type must still refine that native union +/// so `$reflection->newInstanceArgs(...)` resolves to `T|null` (the +/// instance), not `class-string|null`. +#[test] +fn no_false_positive_for_reflection_class_new_instance_args() { + const REFLECTION_STUB: &str = r#"|T $objectOrClass */ + public function __construct(#[LanguageLevelTypeAware(['8.0' => 'object|string'], default: '')] $objectOrClass) {} + /** @return T|null */ + public function newInstanceArgs(array $args = []): ?object {} +} +"#; + let mut class_stubs: std::collections::HashMap<&'static str, &'static str> = + std::collections::HashMap::new(); + class_stubs.insert("ReflectionClass", REFLECTION_STUB); + let backend = phpantom_lsp::Backend::new_test_with_all_stubs( + class_stubs, + std::collections::HashMap::new(), + std::collections::HashMap::new(), + ); + let uri = "file:///reflection.php"; + let content = r#" */ + $class = substr(static::class, 0, -4); + + $reflection = new ReflectionClass($class); + + return $reflection->newInstanceArgs([__METHOD__]); + } +} +"#; + + backend.update_ast(uri, content); + let mut out = Vec::new(); + backend.collect_return_type_diagnostics(uri, content, &mut out); + assert!( + out.is_empty(), + "newInstanceArgs on ReflectionClass should resolve to the instance type, \ + not a class-string, got: {out:?}" + ); +} diff --git a/tests/unit/docblock_parsing.rs b/tests/unit/docblock_parsing.rs index c7b0d72b..33ee4b9c 100644 --- a/tests/unit/docblock_parsing.rs +++ b/tests/unit/docblock_parsing.rs @@ -698,6 +698,17 @@ fn override_mixed_with_class() { )); } +#[test] +fn override_object_string_union_with_class_string_union() { + // The phpstorm-stubs `ReflectionClass::__construct` native hint resolves + // to `object|string`; its docblock `@param class-string|T` must still + // refine that union so the class template `T` binds to the object type. + assert!(should_override_type_typed( + &PhpType::parse("class-string|T"), + &PhpType::parse("object|string") + )); +} + #[test] fn override_class_with_subclass() { assert!(should_override_type_typed( From c5abedfd9ef69a3622d0d4b443d244fb76aa2134 Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Sat, 18 Jul 2026 07:00:37 +0200 Subject: [PATCH 5/6] Laravel's `now()` and `today()` resolve to the concrete Carbon class --- docs/CHANGELOG.md | 1 + docs/todo.md | 2 + docs/todo/bugs.md | 53 ++++++++++++++- docs/todo/laravel.md | 49 ++++++++++++++ docs/todo/lsp-features.md | 14 ++-- docs/todo/performance.md | 6 +- docs/todo/type-inference.md | 16 ++--- examples/laravel/app/Demo.php | 13 ++++ src/code_actions/import_class.rs | 4 +- src/code_actions/phpstan/remove_throws.rs | 12 ++-- src/completion/call_resolution.rs | 23 +++++++ src/completion/context/class_completion.rs | 4 +- .../context/class_completion_tests.rs | 64 +++++++++---------- .../context/namespace_completion.rs | 56 ++++++++-------- src/completion/variable/rhs_resolution.rs | 24 +++++++ src/diagnostics/type_errors.rs | 10 ++- src/diagnostics/unknown_members/tests.rs | 6 +- src/parser/ast_update.rs | 4 +- src/virtual_members/laravel/mod.rs | 4 ++ tests/integration/completion_class_names.rs | 50 +++++++-------- .../completion_closure_param_inference.rs | 20 +++--- tests/integration/crash_sandbox.rs | 10 +-- tests/integration/diag_timing.rs | 11 ++-- tests/integration/diagnostics_deprecated.rs | 4 +- .../diagnostics_return_type_errors.rs | 55 ++++++++++++++++ tests/unit/composer.rs | 8 +-- 26 files changed, 370 insertions(+), 153 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 55425c59..4a9091a0 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Laravel's `now()` and `today()` resolve to the concrete Carbon class.** Both helpers are declared to return the `CarbonInterface` interface, but they hand back Laravel's concrete `Illuminate\Support\Carbon` (which extends `\DateTime`). They now resolve to that concrete class, so a chained call like `now()->addHours(1)` returned from a method declared `: DateTime` no longer reports a false return-type mismatch, and member access on the result resolves precisely. This mirrors the model the Laravel and Carbon ecosystem is written against. - **A conditional `@return` type is evaluated at the call site even when it is declared on an interface.** A method whose PHPStan conditional return type narrows a literal-array argument (as in Spatie LaravelData's `Data::collect([...])`, which yields `array` for an array) now resolves to that narrowed branch instead of the method's broad declared union. This holds when the conditional is declared on an interface while the concrete method comes from a trait, and the narrowed branch now supersedes the union rather than being discarded (which previously also dropped the union's `array` member). Member access, hover, and the return- and property-type mismatch diagnostics all see the precise type, eliminating false "incompatible with declared type `array`" reports. - **`new ReflectionClass($class)` resolves instances to the reflected type.** When the argument is a `class-string`, `newInstance()` and `newInstanceArgs()` now resolve to the object type `T` (nullable for `newInstanceArgs`) instead of the class-string, so returning `$reflection->newInstanceArgs(...)` from a method declared to return that object type no longer reports a false return-type mismatch. More generally, a docblock type now refines a native union that mixes `object` with a scalar (such as the `object|string` hint many reflection stubs carry), where it was previously discarded. - **A property assigned from a function of itself no longer crashes analysis.** A self-referencing assignment such as `$this->items = array_unique(array_merge($this->items, $more))`, where the property is read on its own right-hand side, previously sent type resolution into unbounded recursion that overflowed the stack and aborted the whole `analyze` run. The property now resolves to its declared type instead. diff --git a/docs/todo.md b/docs/todo.md index 5eb603cf..7aa72d65 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -25,6 +25,8 @@ within the same impact tier. | # | Item | Impact | Effort | | --- | --------------------------------------------------------------------------------------------------------------------- | ---------- | ------ | +| L21 | [Tighten the supertype-where-subtype comparison escape hatch (blocked on resolver precision)](todo/laravel.md#l21-tighten-the-supertype-where-subtype-comparison-escape-hatch-blocked-on-resolver-precision) | Medium | High | +| B114 | [`Mockery::mock()` / `$this->mock()` drop the intersection with the mocked class](todo/bugs.md#b114-mockerymock--thismock-drop-the-intersection-with-the-mocked-class) | Medium | Low-Medium | | L18 | [`Storage::disk()` return type — resolve from config, don't guess](todo/laravel.md#l18-storagedisk-return-type-is-genuinely-polymorphic--resolve-from-config-dont-guess) | Medium-High | Low-Medium | | L19 | [Redis client (phpredis/predis) — resolve from config, don't guess](todo/laravel.md#l19-redis-connection-client-is-chosen-by-config-phpredis-vs-predis--resolve-from-config-dont-guess) | Medium | Low-Medium | | X4 | [Full background indexing](todo/indexing.md#x4-full-background-indexing) (workspace symbols, fast find-references) | Medium | High | diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index 0566a919..d2534fce 100644 --- a/docs/todo/bugs.md +++ b/docs/todo/bugs.md @@ -7,4 +7,55 @@ pipeline so it produces correct data. Downstream consumers (diagnostics, hover, completion, definition) should never need to second-guess upstream output. -No outstanding items. +## B114. `Mockery::mock()` / `$this->mock()` drop the intersection with the mocked class + +**Severity: Medium (false positives wherever a mock is assigned to a +property or return type typed as the concrete mocked class) · Confirmed +against real projects (a production Laravel codebase and a legacy PHP +app); PHPStan/Larastan report zero errors on the same lines** + +```php +/** @var EpaymentService */ +private $epaymentService; + +protected function setUp(): void +{ + $mock = Mockery::mock(EpaymentService::class); + $this->epaymentService = $mock; // "Property expects EpaymentService, got + // Mockery\MockInterface|Mockery\LegacyMockInterface" +} +``` + +```php +private function mockHelloRetailClient(): Client&MockInterface +{ + $mock = $this->mock(Client::class); + // ... + return $mock; // "Return type MockInterface is incompatible with + // declared return type Client&MockInterface" +} +``` + +Mockery generates a dynamic subclass of the class passed to +`Mockery::mock()` (and Laravel's `TestCase::mock()` / +`partialMock()` / `spy()`, which delegate to it) when that class is +not `final`, so the returned object genuinely satisfies both the +concrete class and `MockInterface`. PHPantom currently resolves the +call's return type from `Mockery::mock()`'s own docblock +(`@return \Mockery\MockInterface`), dropping the class-string +argument entirely. PHPStan gets this right via +`phpstan-mockery`'s `MockDynamicReturnTypeExtension` (for +`Mockery::mock()`/`spy()`) and Larastan's `TestCaseExtension` (for +`$this->mock()`/`partialMock()`/`spy()`) — both are +argument-dependent return type extensions that intersect +`MockInterface` with the type of the class-string argument. + +PHPantom has no equivalent mechanism for a handful of well-known +argument-dependent return types outside our own conditional +`@return` support. Where to add this: `completion/call_resolution.rs` +(or the return-type resolution path it feeds) should special-case +`Mockery::mock()`, `Mockery::spy()`, and, in Laravel projects, the +`TestCase` trait's `mock()`/`partialMock()`/`spy()` methods, and +intersect `MockInterface` with the resolved type of the first +argument when it is a `::class` reference (skip anything else, same +scope restriction as our other literal-argument-only resolvers). diff --git a/docs/todo/laravel.md b/docs/todo/laravel.md index 0980abbc..1567fed3 100644 --- a/docs/todo/laravel.md +++ b/docs/todo/laravel.md @@ -697,3 +697,52 @@ recoverable signature). Larastan reference: it boots the app and reflects the runtime `$macros` static, so it gets `mixin()` for free; we recover the common literal shape from source instead. + +#### L21. Tighten the supertype-where-subtype comparison escape hatch (blocked on resolver precision) + +**Impact: Medium (a whole class of genuine mismatches goes unreported) +· Effort: High (blocked on two precision gaps below)** + +The argument/return compatibility check has a deliberate "reverse +direction MAYBE" escape hatch: when the resolved value is a *broader* +supertype and the declared type is a *narrower* subtype (a downcast), +we accept it rather than flag a mismatch. This is intentionally +over-generous. It hides real errors such as returning a base type +where a specific subclass is declared. + +We would like to drop this escape hatch to catch those genuine +mismatches. We cannot yet, because doing so surfaces ~250 diagnostics +across real Laravel codebases. These are *not* classic false +positives: the code is correct against the model Larastan presents, +and Larastan hacks around Carbon and Eloquent rather than modelling +them precisely. Because the entire ecosystem (and therefore +application code) is written against that looser model, tightening the +check drowns real projects in mismatches that only PHPantom would +report. PHPStan/Larastan report zero on the same lines. + +Two resolver-precision gaps produce the bulk of them; both must be +closed before the escape hatch can go: + +1. **Eloquent custom collection classes.** A relation or query typed + as the base `Illuminate\Database\Eloquent\Collection` + where the method declares a custom `ModelCollection` subclass. We + do not yet resolve a model's `$collectionClass` / `newCollection()` + override, so the base type leaks and looks like a downcast. This is + the largest chunk and is not Carbon-specific. + +2. **Carbon parent/child typing.** A value typed `Carbon\Carbon` where + `Illuminate\Support\Carbon` is declared (for example a datetime-cast + property chained through a fluent method). The runtime value is the + concrete Laravel subclass, but our cast/property typing lands on the + parent. Related to the `now()`/`today()` handling already in place, + which maps those helpers to `Illuminate\Support\Carbon`. + +Note the `now()`/`today()` mapping itself is part of the same looser +model: it is not strictly sound (the helpers' declared return type is +the interface) but mirrors Larastan so real code does not drown in +mismatches. Any tightening here has to preserve that. + +Where to look: the reverse-direction hierarchy block in the argument +type compatibility layer, and the two resolution paths named above. +Reproducible in real projects (a production Laravel codebase surfaces +all ~250 when the escape hatch is removed). diff --git a/docs/todo/lsp-features.md b/docs/todo/lsp-features.md index cdba5ec1..42618787 100644 --- a/docs/todo/lsp-features.md +++ b/docs/todo/lsp-features.md @@ -181,12 +181,12 @@ Consider implementing after X4 (full background indexing) ships, or accept the same scan-based latency that Find References currently has. **References:** -- php-lsp: `references/php-lsp/src/navigation/call_hierarchy.rs` — a +- php-lsp: `src/navigation/call_hierarchy.rs` in its own repo — a working Rust implementation (prepare/incoming/outgoing, cross-file) built on the same "wrap Find References + walk the body" shape - described above. Their wire-protocol tests - (`references/php-lsp/tests/`, call-hierarchy cases in the Symfony - suite) show the expected item/range semantics editors rely on. + described above. Their wire-protocol tests (`tests/`, call-hierarchy + cases in the Symfony suite) show the expected item/range semantics + editors rely on. - Phpactor: call hierarchy via its references index. ## F7. Evaluatable expression support (DAP integration) @@ -560,9 +560,9 @@ F18's namespace computation). **References:** - Phpactor: `MoveClass` refactoring in the class-mover package. - php-lsp: `handle_will_rename_files` in - `references/php-lsp/src/backend/handlers/workspace.rs` (updates - `use` imports workspace-wide on file rename) and their - `willCreateFiles` PSR-4 stub insertion. + `src/backend/handlers/workspace.rs` in its own repo (updates `use` + imports workspace-wide on file rename) and their `willCreateFiles` + PSR-4 stub insertion. ## F18. Fix namespace/class name from PSR-4 diff --git a/docs/todo/performance.md b/docs/todo/performance.md index 627eea9f..55aefe28 100644 --- a/docs/todo/performance.md +++ b/docs/todo/performance.md @@ -553,7 +553,7 @@ isn't needed. **References:** - Psalm: `ClassLikeStorageCacheProvider` in - `references/psalm/src/Psalm/Internal/Provider/ClassLikeStorageCacheProvider.php` + `Psalm\Internal\Provider\ClassLikeStorageCacheProvider` - Psalm: `FileStorageCacheProvider` for the content-hash invalidation pattern - php-lsp: persists `FileIndex` entries under `~/.cache/php-lsp/` @@ -563,7 +563,7 @@ isn't needed. content hashing. Confirms the content-hash-as-authority choice above; never trust mtime for correctness, at most as a cheap pre-filter to skip hashing unchanged files. -- php-lsp: `references/php-lsp/docs/salsa-migration.md` (Phases K1-K4) +- php-lsp: `docs/salsa-migration.md` in its own repo (Phases K1-K4) documents their cache-size cap, reset-on-overflow, and LRU-by-mtime eviction plans — useful pitfall list if P20 ships. @@ -609,7 +609,7 @@ diagnostic layer. **References:** - Psalm: `FileDiffer` and `FileStatementsDiffer` in - `references/psalm/src/Psalm/Internal/Diff/` + `Psalm\Internal\Diff` - Psalm: `Analyzer::shiftFileOffsets()` for the offset-shifting logic --- diff --git a/docs/todo/type-inference.md b/docs/todo/type-inference.md index 5c36da22..3d43b63e 100644 --- a/docs/todo/type-inference.md +++ b/docs/todo/type-inference.md @@ -403,9 +403,8 @@ Key design decisions to adopt: (assertions + types → narrowed types). Each is independently testable. -See `references/psalm/src/Psalm/Internal/Algebra.php`, -`references/psalm/src/Psalm/Internal/Clause.php`, and -`references/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php`. +See Psalm's `Psalm\Internal\Algebra`, `Psalm\Internal\Clause`, and +`Psalm\Internal\Analyzer\Statements\Expression\AssertionFinder`. **Depends on:** The structured type representation (`PhpType`) has landed, which makes reconciliation much simpler than working with @@ -563,8 +562,7 @@ This eliminates the exponential re-resolution that causes performance issues on deeply chained expressions (P20 class of problems). **References:** -- Psalm: `NodeTypeProvider` interface in - `references/psalm/src/Psalm/NodeTypeProvider.php` +- Psalm: `NodeTypeProvider` interface (`Psalm\NodeTypeProvider`) - Mago: per-node type caching via `spl_object_id` equivalent --- @@ -611,8 +609,7 @@ When selecting the final type for a template param: **References:** - Psalm: `TemplateBound` with `appearance_depth` and - `getMostSpecificTypeFromBounds` in - `references/psalm/src/Psalm/Internal/Type/TemplateResult.php` + `getMostSpecificTypeFromBounds` in `Psalm\Internal\Type\TemplateResult` --- @@ -658,7 +655,7 @@ intentional). **References:** - Psalm: `Context::$vars_in_scope` and `Context::$vars_possibly_in_scope` - in `references/psalm/src/Psalm/Context.php` + (`Psalm\Context`) --- @@ -682,8 +679,7 @@ that normal code never hits it, but low enough to prevent pathological blowup. **References:** -- Psalm: literal limit in `Type::combineUnionTypes()` at - `references/psalm/src/Psalm/Type.php` +- Psalm: literal limit in `Type::combineUnionTypes()` (`Psalm\Type`) diff --git a/examples/laravel/app/Demo.php b/examples/laravel/app/Demo.php index ed130224..b4ddd617 100644 --- a/examples/laravel/app/Demo.php +++ b/examples/laravel/app/Demo.php @@ -418,4 +418,17 @@ public function globalRedisOverImport(): void // The bare `Redis` short name still resolves to the imported facade. Redis::connection(); // Facades\Redis static method } + + public function dateHelpers(): \DateTime + { + // now()/today() are declared to return CarbonInterface, but they + // instantiate the concrete Illuminate\Support\Carbon (a \DateTime), + // so member access resolves and a chained fluent call stays concrete. + now()->addHours(1); // → Illuminate\Support\Carbon + today()->startOfDay(); // → Illuminate\Support\Carbon + + // Returning the chain from a :DateTime method is not a mismatch, + // because Illuminate\Support\Carbon extends \DateTime. + return now()->addHours(1); + } } diff --git a/src/code_actions/import_class.rs b/src/code_actions/import_class.rs index e27f27eb..02cbe40c 100644 --- a/src/code_actions/import_class.rs +++ b/src/code_actions/import_class.rs @@ -1254,8 +1254,8 @@ mod tests { = HashMap::from([( - "Decimal".to_string(), - "Luxplus\\Decimal\\Decimal".to_string(), - )]); + let use_map: HashMap = + HashMap::from([("Decimal".to_string(), "Acme\\Decimal\\Decimal".to_string())]); let ns: Option = None; let db = find_docblock_above_line(php, 7).unwrap(); let edit = - build_remove_throws_edit(php, &db, "Luxplus\\Decimal\\Decimal", &use_map, &ns).unwrap(); + build_remove_throws_edit(php, &db, "Acme\\Decimal\\Decimal", &use_map, &ns).unwrap(); assert!( !edit.new_text.contains("@throws"), "should remove @throws: {:?}", diff --git a/src/completion/call_resolution.rs b/src/completion/call_resolution.rs index 0174b52f..d65d9d59 100644 --- a/src/completion/call_resolution.rs +++ b/src/completion/call_resolution.rs @@ -1461,6 +1461,29 @@ impl Backend { return vec![cls]; } + // ── now() / today() → Illuminate\Support\Carbon ───── + // The global `now()`/`today()` helpers are declared to + // return `CarbonInterface`, but they actually instantiate + // the concrete `Illuminate\Support\Carbon` (which extends + // `\DateTime`). Resolving to the interface loses the + // concrete type and produces spurious mismatches when a + // chained call is assigned to a `DateTime`/`DateTimeImmutable` + // declaration. Map both to the concrete class. Only applies + // when the class is loadable (i.e. inside a Laravel project). + // + // Not strictly sound (the declared type is the interface), + // but it mirrors Larastan's `NowAndTodayExtension`, which the + // ecosystem is written against. See the matching note in + // `rhs_resolution.rs`. + if matches!( + normalized_func, + "now" | "today" | "Illuminate\\Support\\now" | "Illuminate\\Support\\today" + ) && let Some(cls) = + (ctx.class_loader)(crate::virtual_members::laravel::SUPPORT_CARBON_FQN) + { + return vec![cls]; + } + // Check for array element/preserving functions first. let is_array_element_func = ARRAY_ELEMENT_FUNCS .iter() diff --git a/src/completion/context/class_completion.rs b/src/completion/context/class_completion.rs index d5d2918b..70757db6 100644 --- a/src/completion/context/class_completion.rs +++ b/src/completion/context/class_completion.rs @@ -1212,7 +1212,7 @@ impl ClassItemCtx<'_> { /// /// ```text /// Order App\Models - /// Order Luxplus\Database\Model\Orders + /// Order Acme\Database\Model\Orders /// ``` /// /// The `filter_text` is also the short name so the editor's fuzzy @@ -1280,7 +1280,7 @@ impl ClassItemCtx<'_> { // This gives a clean two-column layout in the editor popup: // // Order App\Models - // Order Luxplus\Database\Model\Orders + // Order Acme\Database\Model\Orders // // so users can distinguish same-named classes without the // label being a long FQN that the editor truncates. diff --git a/src/completion/context/class_completion_tests.rs b/src/completion/context/class_completion_tests.rs index fadd7392..e6850720 100644 --- a/src/completion/context/class_completion_tests.rs +++ b/src/completion/context/class_completion_tests.rs @@ -674,16 +674,16 @@ fn test_affinity_table_single_import() { let mut use_map = HashMap::new(); use_map.insert( "Brand".to_string(), - "Luxplus\\Database\\Model\\Brands\\Brand".to_string(), + "Acme\\Database\\Model\\Brands\\Brand".to_string(), ); let ns: Option = None; let table = build_affinity_table(&use_map, &ns); - assert_eq!(table.get("Luxplus"), Some(&1)); - assert_eq!(table.get("Luxplus\\Database"), Some(&1)); - assert_eq!(table.get("Luxplus\\Database\\Model"), Some(&1)); - assert_eq!(table.get("Luxplus\\Database\\Model\\Brands"), Some(&1)); + assert_eq!(table.get("Acme"), Some(&1)); + assert_eq!(table.get("Acme\\Database"), Some(&1)); + assert_eq!(table.get("Acme\\Database\\Model"), Some(&1)); + assert_eq!(table.get("Acme\\Database\\Model\\Brands"), Some(&1)); assert_eq!( - table.get("Luxplus\\Database\\Model\\Brands\\Brand"), + table.get("Acme\\Database\\Model\\Brands\\Brand"), None, "Class name itself is not a prefix" ); @@ -708,7 +708,7 @@ fn test_affinity_table_file_namespace_plus_imports() { ); use_map.insert( "Brand".to_string(), - "Luxplus\\Database\\Model\\Brands\\Brand".to_string(), + "Acme\\Database\\Model\\Brands\\Brand".to_string(), ); let ns = Some("App\\Http\\Controllers".to_string()); let table = build_affinity_table(&use_map, &ns); @@ -720,8 +720,8 @@ fn test_affinity_table_file_namespace_plus_imports() { assert_eq!(table.get("App\\Http\\Controllers"), Some(&1)); // "App\\Http\\Requests" from import only assert_eq!(table.get("App\\Http\\Requests"), Some(&1)); - // "Luxplus" from import only - assert_eq!(table.get("Luxplus"), Some(&1)); + // "Acme" from import only + assert_eq!(table.get("Acme"), Some(&1)); } #[test] @@ -746,12 +746,12 @@ fn test_affinity_table_global_namespace_import() { #[test] fn test_affinity_score_known_prefix() { let mut table = HashMap::new(); - table.insert("Luxplus".to_string(), 11); - table.insert("Luxplus\\Database".to_string(), 6); - table.insert("Luxplus\\Database\\Model".to_string(), 6); - let score = affinity_score("Luxplus\\Database\\Model\\Orders\\Order", &table); - // Luxplus(11) + Luxplus\Database(6) + Luxplus\Database\Model(6) = 23 - // Luxplus\Database\Model\Orders is not in the table → 0 + table.insert("Acme".to_string(), 11); + table.insert("Acme\\Database".to_string(), 6); + table.insert("Acme\\Database\\Model".to_string(), 6); + let score = affinity_score("Acme\\Database\\Model\\Orders\\Order", &table); + // Acme(11) + Acme\Database(6) + Acme\Database\Model(6) = 23 + // Acme\Database\Model\Orders is not in the table → 0 assert_eq!(score, 23); } @@ -775,7 +775,7 @@ fn test_affinity_score_global_namespace_candidate() { #[test] fn test_affinity_score_empty_table() { let table = HashMap::new(); - let score = affinity_score("Luxplus\\Database\\Model\\Orders\\Order", &table); + let score = affinity_score("Acme\\Database\\Model\\Orders\\Order", &table); assert_eq!(score, 0); } @@ -880,10 +880,10 @@ fn test_class_sort_text_quality_beats_tier() { #[test] fn test_class_sort_text_tier_beats_affinity() { let mut table = HashMap::new(); - table.insert("Luxplus".to_string(), 50); + table.insert("Acme".to_string(), 50); // Same match quality, but tier 1 should beat tier 2 even with lower affinity. let tier1_low = class_sort_text("Order", "App\\Order", "Order", '1', '0', false, &table); - let tier2_high = class_sort_text("Order", "Luxplus\\Order", "Order", '2', '0', false, &table); + let tier2_high = class_sort_text("Order", "Acme\\Order", "Order", '2', '0', false, &table); assert!( tier1_low < tier2_high, "Tier 1 should sort before tier 2 regardless of affinity: tier1={tier1_low}, tier2={tier2_high}" @@ -893,14 +893,14 @@ fn test_class_sort_text_tier_beats_affinity() { #[test] fn test_class_sort_text_affinity_within_same_tier() { let mut table = HashMap::new(); - table.insert("Luxplus".to_string(), 11); - table.insert("Luxplus\\Database".to_string(), 6); - table.insert("Luxplus\\Database\\Model".to_string(), 6); + table.insert("Acme".to_string(), 11); + table.insert("Acme\\Database".to_string(), 6); + table.insert("Acme\\Database\\Model".to_string(), 6); table.insert("App".to_string(), 4); // Both tier 2, both exact, but different affinity. let high = class_sort_text( "Order", - "Luxplus\\Database\\Model\\Orders\\Order", + "Acme\\Database\\Model\\Orders\\Order", "Order", '2', '0', @@ -960,17 +960,17 @@ fn test_class_sort_text_alphabetical_tiebreak() { fn test_class_sort_text_gap_within_same_affinity() { let mut table = HashMap::new(); // Both classes share the same namespace and thus the same affinity score. - table.insert("Luxplus".to_string(), 11); - table.insert("Luxplus\\Core".to_string(), 6); - table.insert("Luxplus\\Core\\Database".to_string(), 6); - table.insert("Luxplus\\Core\\Database\\Model".to_string(), 6); - table.insert("Luxplus\\Core\\Database\\Model\\Products".to_string(), 1); + table.insert("Acme".to_string(), 11); + table.insert("Acme\\Core".to_string(), 6); + table.insert("Acme\\Core\\Database".to_string(), 6); + table.insert("Acme\\Core\\Database\\Model".to_string(), 6); + table.insert("Acme\\Core\\Database\\Model\\Products".to_string(), 1); // "Product" (len 7, gap 7-3=4) should sort before "ProductFilterTerm" (len 17, gap 17-3=14) // when both have the same affinity (same namespace). let short = class_sort_text( "Product", - "Luxplus\\Core\\Database\\Model\\Products\\Product", + "Acme\\Core\\Database\\Model\\Products\\Product", "Pro", '2', '0', @@ -979,7 +979,7 @@ fn test_class_sort_text_gap_within_same_affinity() { ); let long = class_sort_text( "ProductFilterTerm", - "Luxplus\\Core\\Database\\Model\\Products\\Filters\\ProductFilterTerm", + "Acme\\Core\\Database\\Model\\Products\\Filters\\ProductFilterTerm", "Pro", '2', '0', @@ -995,15 +995,15 @@ fn test_class_sort_text_gap_within_same_affinity() { #[test] fn test_class_sort_text_affinity_beats_gap() { let mut table = HashMap::new(); - table.insert("Luxplus".to_string(), 11); - table.insert("Luxplus\\Database".to_string(), 6); + table.insert("Acme".to_string(), 11); + table.insert("Acme\\Database".to_string(), 6); // "Proxy" has a tiny gap (5-3=2) but zero affinity. // "Product" has a larger gap (7-3=4) but high affinity. // Affinity should win because it comes before gap in the sort key. let high_affinity = class_sort_text( "Product", - "Luxplus\\Database\\Product", + "Acme\\Database\\Product", "Pro", '2', '0', diff --git a/src/completion/context/namespace_completion.rs b/src/completion/context/namespace_completion.rs index 81360dd2..a3c5c6a1 100644 --- a/src/completion/context/namespace_completion.rs +++ b/src/completion/context/namespace_completion.rs @@ -31,9 +31,9 @@ struct InferredNamespace { /// /// Returns all matching mappings ordered by specificity (longest match /// first). For a file at `src/core/Brands/Services/Fred.php` with -/// mappings `"Luxplus\\Core\\" => "src/core/"` and -/// `"Luxplus\\Core\\Tasks\\" => "src/tasks/"`, only the first mapping -/// matches, producing `Luxplus\Core\Brands\Services`. +/// mappings `"Acme\\Core\\" => "src/core/"` and +/// `"Acme\\Core\\Tasks\\" => "src/tasks/"`, only the first mapping +/// matches, producing `Acme\Core\Brands\Services`. fn infer_namespaces_from_path( file_path: &Path, workspace_root: &Path, @@ -376,14 +376,14 @@ mod tests { fn multiple_mappings_longest_match_first() { let root = PathBuf::from("/project"); let mappings = vec![ - mapping("Luxplus\\Core\\", "src/core/"), - mapping("Luxplus\\Core\\Database\\", "src/database/"), + mapping("Acme\\Core\\", "src/core/"), + mapping("Acme\\Core\\Database\\", "src/database/"), ]; let file = PathBuf::from("/project/src/core/Brands/Services/Fred.php"); let result = infer_namespaces_from_path(&file, &root, &mappings); assert_eq!(result.len(), 1); - assert_eq!(result[0].namespace, "Luxplus\\Core\\Brands\\Services"); + assert_eq!(result[0].namespace, "Acme\\Core\\Brands\\Services"); } #[test] @@ -393,7 +393,7 @@ mod tests { let root = PathBuf::from("/project"); let mappings = vec![ mapping( - "Database\\Factories\\Luxplus\\Core\\Database\\", + "Database\\Factories\\Acme\\Core\\Database\\", "database/factories/", ), mapping("Database\\Seeders\\", "database/seeders/"), @@ -404,25 +404,25 @@ mod tests { assert_eq!(result.len(), 1); assert_eq!( result[0].namespace, - "Database\\Factories\\Luxplus\\Core\\Database" + "Database\\Factories\\Acme\\Core\\Database" ); } #[test] fn file_in_subdirectory_of_two_matching_bases() { // `src/core/Database/Foo.php` matches both: - // - `Luxplus\Core\` => `src/core/` → `Luxplus\Core\Database` - // - `Luxplus\Core\Database\` => `src/database/` → NO (path doesn't start with `src/database/`) + // - `Acme\Core\` => `src/core/` → `Acme\Core\Database` + // - `Acme\Core\Database\` => `src/database/` → NO (path doesn't start with `src/database/`) let root = PathBuf::from("/project"); let mappings = vec![ - mapping("Luxplus\\Core\\", "src/core/"), - mapping("Luxplus\\Core\\Database\\", "src/database/"), + mapping("Acme\\Core\\", "src/core/"), + mapping("Acme\\Core\\Database\\", "src/database/"), ]; let file = PathBuf::from("/project/src/core/Database/Foo.php"); let result = infer_namespaces_from_path(&file, &root, &mappings); assert_eq!(result.len(), 1); - assert_eq!(result[0].namespace, "Luxplus\\Core\\Database"); + assert_eq!(result[0].namespace, "Acme\\Core\\Database"); } #[test] @@ -456,25 +456,25 @@ mod tests { } #[test] - fn real_world_luxplus_example() { + fn real_world_multi_mapping_example() { let root = PathBuf::from("/project"); let mappings = vec![ - mapping("Luxplus\\Core\\", "src/core/"), - mapping("Luxplus\\Decimal\\", "src/decimal/"), + mapping("Acme\\Core\\", "src/core/"), + mapping("Acme\\Decimal\\", "src/decimal/"), mapping( - "Database\\Factories\\Luxplus\\Core\\Database\\", + "Database\\Factories\\Acme\\Core\\Database\\", "database/factories/", ), mapping("Database\\Seeders\\", "database/seeders/"), - mapping("EchoEcho\\Coolrunner\\", "src/coolrunner-client/"), - mapping("EchoEcho\\ElasticClient\\", "src/elasticsearch-client/"), - mapping("EchoEcho\\Klaviyo\\", "src/klaviyo-client/"), - mapping("EchoEcho\\Shared\\Common\\", "src/common/"), - mapping("Luxplus\\Core\\Database\\", "src/database/"), - mapping("Luxplus\\Core\\Elasticsearch\\", "src/elasticsearch/"), - mapping("Luxplus\\Core\\Enums\\", "src/enums/"), - mapping("Luxplus\\Core\\Tasks\\", "src/tasks/"), - mapping("Luxplus\\Web\\", "src/web/"), + mapping("Vendor\\Coolrunner\\", "src/coolrunner-client/"), + mapping("Vendor\\ElasticClient\\", "src/elasticsearch-client/"), + mapping("Vendor\\Klaviyo\\", "src/klaviyo-client/"), + mapping("Vendor\\Shared\\Common\\", "src/common/"), + mapping("Acme\\Core\\Database\\", "src/database/"), + mapping("Acme\\Core\\Elasticsearch\\", "src/elasticsearch/"), + mapping("Acme\\Core\\Enums\\", "src/enums/"), + mapping("Acme\\Core\\Tasks\\", "src/tasks/"), + mapping("Acme\\Web\\", "src/web/"), mapping("Tests\\Support\\", "tests/Support/"), mapping("Tests\\", "tests/"), ]; @@ -482,12 +482,12 @@ mod tests { let file = PathBuf::from("/project/src/core/Brands/Services/Fred.php"); let result = infer_namespaces_from_path(&file, &root, &mappings); assert_eq!(result.len(), 1); - assert_eq!(result[0].namespace, "Luxplus\\Core\\Brands\\Services"); + assert_eq!(result[0].namespace, "Acme\\Core\\Brands\\Services"); let file2 = PathBuf::from("/project/src/tasks/Import/RunImport.php"); let result2 = infer_namespaces_from_path(&file2, &root, &mappings); assert_eq!(result2.len(), 1); - assert_eq!(result2[0].namespace, "Luxplus\\Core\\Tasks\\Import"); + assert_eq!(result2[0].namespace, "Acme\\Core\\Tasks\\Import"); let file3 = PathBuf::from("/project/tests/Support/Helpers/TestHelper.php"); let result3 = infer_namespaces_from_path(&file3, &root, &mappings); diff --git a/src/completion/variable/rhs_resolution.rs b/src/completion/variable/rhs_resolution.rs index 0ea3b84b..ea579916 100644 --- a/src/completion/variable/rhs_resolution.rs +++ b/src/completion/variable/rhs_resolution.rs @@ -2820,6 +2820,30 @@ fn resolve_rhs_function_call<'b>( return ResolvedType::from_classes(vec![cls]); } } + + // ── now() / today() → Illuminate\Support\Carbon ───── + // Laravel's `now()`/`today()` helpers are declared to return + // `CarbonInterface`, but they instantiate the concrete + // `Illuminate\Support\Carbon` (which extends `\DateTime`). + // Resolving to the interface loses the concrete type and + // produces spurious mismatches when the value flows into a + // `DateTime`/`DateTimeImmutable` declaration. Map both to the + // concrete class. + // + // This is not strictly sound (the helpers' declared type is the + // interface), but it mirrors Larastan's `NowAndTodayExtension`. + // The Laravel/Carbon ecosystem is written against that model, so + // real codebases assume the concrete type; matching it avoids a + // flood of mismatches that only exist because the declared types + // are looser than reality. + if matches!( + normalized_func, + "now" | "today" | "Illuminate\\Support\\now" | "Illuminate\\Support\\today" + ) && let Some(cls) = + (ctx.class_loader)(crate::virtual_members::laravel::SUPPORT_CARBON_FQN) + { + return ResolvedType::from_classes(vec![cls]); + } } // ── Known array functions ──────────────────────── diff --git a/src/diagnostics/type_errors.rs b/src/diagnostics/type_errors.rs index 654a089e..86f737cb 100644 --- a/src/diagnostics/type_errors.rs +++ b/src/diagnostics/type_errors.rs @@ -602,12 +602,16 @@ pub(super) fn is_type_compatible( // is a strict YES handled by the `is_subtype_of_typed` fallback // at the end of this function. No need to duplicate it here. // - // Direction 2 (param <: arg, e.g. `CarbonInterface` passed to - // `Carbon` where `Carbon implements CarbonInterface`) is a MAYBE: + // Direction 2 (param <: arg, e.g. `Carbon\Carbon` passed where a + // `Illuminate\Support\Carbon` subclass is expected) is a MAYBE: // the argument is a *broader* type but the value *might* be the // narrower concrete at runtime (the developer may have checked // with instanceof, or the API always returns the concrete type - // despite being typed as the interface). + // despite being typed as the parent). This also covers cases the + // resolver still under-narrows, such as an Eloquent relation typed + // as the base `Collection` where a custom collection subclass is + // declared — dropping this rule turns those into false positives + // that PHPStan does not report. // // However, if the arg's class is **final**, the value cannot be // any subtype — it is exactly that class. So `final class Jack` diff --git a/src/diagnostics/unknown_members/tests.rs b/src/diagnostics/unknown_members/tests.rs index f411813f..67eb6d2a 100644 --- a/src/diagnostics/unknown_members/tests.rs +++ b/src/diagnostics/unknown_members/tests.rs @@ -4320,7 +4320,7 @@ fn no_false_positive_when_variable_reassigned_inside_try_block() { // after the reassignment (still inside the try) should resolve // against the new type, not the original. let php = r#"getOrCreateCustomer($customer); diff --git a/src/parser/ast_update.rs b/src/parser/ast_update.rs index 2f9ea124..198fe97f 100644 --- a/src/parser/ast_update.rs +++ b/src/parser/ast_update.rs @@ -1101,8 +1101,8 @@ impl Backend { // Resolve class-like names in method return types and property // type hints so that cross-file resolution works correctly. // For example, if a method returns `Country` and the file has - // `use Luxplus\Core\Enums\Country`, the return type becomes - // the FQN `Luxplus\Core\Enums\Country`. + // `use Acme\Core\Enums\Country`, the return type becomes + // the FQN `Acme\Core\Enums\Country`. // // Template params and type alias names are excluded to avoid // mangling generic types and locally-defined type aliases. diff --git a/src/virtual_members/laravel/mod.rs b/src/virtual_members/laravel/mod.rs index 36457420..b450bbaf 100644 --- a/src/virtual_members/laravel/mod.rs +++ b/src/virtual_members/laravel/mod.rs @@ -262,6 +262,10 @@ pub(crate) const ELOQUENT_MODEL_FQN: &str = "Illuminate\\Database\\Eloquent\\Mod /// The fully-qualified name of the Eloquent Builder class. pub const ELOQUENT_BUILDER_FQN: &str = "Illuminate\\Database\\Eloquent\\Builder"; +/// The fully-qualified name of Laravel's concrete Carbon subclass, which +/// the `now()` and `today()` helpers actually instantiate. +pub const SUPPORT_CARBON_FQN: &str = "Illuminate\\Support\\Carbon"; + /// Build a substitution map that replaces `static`, `$this`, and `self` /// with the given type. /// diff --git a/tests/integration/completion_class_names.rs b/tests/integration/completion_class_names.rs index 256dac31..90ea7b71 100644 --- a/tests/integration/completion_class_names.rs +++ b/tests/integration/completion_class_names.rs @@ -3491,7 +3491,7 @@ async fn test_use_import_context_does_not_shorten() { } /// A `use` statement that imports a namespace (not a class) should NOT -/// produce a phantom class completion item. E.g. `use Luxplus\Core\Enums as LCE;` +/// produce a phantom class completion item. E.g. `use Acme\Core\Enums as LCE;` /// where `Enums` is a namespace containing enum classes, not a class itself. #[tokio::test] async fn test_namespace_alias_import_not_shown_as_class() { @@ -3501,27 +3501,27 @@ async fn test_namespace_alias_import_not_shown_as_class() { { let mut idx = backend.fqn_uri_index().write(); idx.insert( - "Luxplus\\Core\\Enums\\Status".to_string(), - "file:///vendor/luxplus/enums/Status.php".to_string(), + "Acme\\Core\\Enums\\Status".to_string(), + "file:///vendor/acme/enums/Status.php".to_string(), ); idx.insert( - "Luxplus\\Core\\Enums\\Color".to_string(), - "file:///vendor/luxplus/enums/Color.php".to_string(), + "Acme\\Core\\Enums\\Color".to_string(), + "file:///vendor/acme/enums/Color.php".to_string(), ); } // Use prefix "LCE" which matches the alias for the namespace import. let uri = Url::parse("file:///ns_alias.php").unwrap(); - let text = concat!("where(function ($q): void { /// $q->where('domain', 'like', '%b%'); // $q should resolve @@ -2005,7 +2005,7 @@ async fn test_cross_file_laravel_eloquent_builder_closure_param_inference() { "autoload": { "psr-4": { "App\\": "src/", - "Luxplus\\Core\\Database\\Model\\": "src/Models/", + "Acme\\Core\\Database\\Model\\": "src/Models/", "Illuminate\\Database\\Eloquent\\": "vendor/laravel/framework/src/Illuminate/Database/Eloquent/", "Illuminate\\Database\\Query\\": "vendor/laravel/framework/src/Illuminate/Database/Query/" } @@ -2062,7 +2062,7 @@ async fn test_cross_file_laravel_eloquent_builder_closure_param_inference() { // User's model class let email_generator_file = concat!( "where(function ($q): void { // Line 5: $q-> @@ -2081,7 +2081,7 @@ async fn test_cross_file_laravel_eloquent_builder_closure_param_inference() { let script_file = concat!( "where(function ($q): void {\n", " $q->\n", diff --git a/tests/integration/crash_sandbox.rs b/tests/integration/crash_sandbox.rs index 44ebb65a..cd7c832a 100644 --- a/tests/integration/crash_sandbox.rs +++ b/tests/integration/crash_sandbox.rs @@ -27,15 +27,15 @@ fn sandbox_exact_content_update_ast_does_not_crash() { namespace App\Http\Controllers\Economy; use App\Http\Controllers\Controller; -use EchoEcho\Shared\Common\Convert; -use EchoEcho\Shared\Common\ConvertException; +use Vendor\Shared\Common\Convert; +use Vendor\Shared\Common\ConvertException; use Exception; use Illuminate\Contracts\View\View; use Illuminate\Database\Query\Builder; use Illuminate\Support\Facades\DB; -use Luxplus\Core\Enums\Country; -use Luxplus\Core\Enums\OrderStatus; -use Luxplus\Decimal\Decimal; +use Acme\Core\Enums\Country; +use Acme\Core\Enums\OrderStatus; +use Acme\Decimal\Decimal; use stdClass; final class ExtractionToolController extends Controller diff --git a/tests/integration/diag_timing.rs b/tests/integration/diag_timing.rs index d936c2f8..91d17530 100644 --- a/tests/integration/diag_timing.rs +++ b/tests/integration/diag_timing.rs @@ -651,11 +651,12 @@ function describe(Base $item): string { /// Verify that `assert($data instanceof stdClass)` narrowing survives /// through multiple intervening `if` blocks with early returns. /// -/// Regression test for Luxplus Shared `RefundCallback`: after the assert -/// on line 33, several `if (!$refund)` / `if (!$payment) { return; }` -/// blocks follow. The forward walker must preserve the `stdClass` -/// narrowing across all of them so that `$data->status` on a later line -/// does not produce an `unresolved_member_access` diagnostic. +/// Regression test seen in a real project's `RefundCallback` handler: +/// after the assert on line 33, several `if (!$refund)` / +/// `if (!$payment) { return; }` blocks follow. The forward walker must +/// preserve the `stdClass` narrowing across all of them so that +/// `$data->status` on a later line does not produce an +/// `unresolved_member_access` diagnostic. #[test] fn assert_instanceof_survives_intervening_if_blocks() { let php = r#" */ - public array $luxplusSubscriptions, + public array $subscriptions, ) {} } "#; diff --git a/tests/integration/diagnostics_return_type_errors.rs b/tests/integration/diagnostics_return_type_errors.rs index 788d3dc8..9763002b 100644 --- a/tests/integration/diagnostics_return_type_errors.rs +++ b/tests/integration/diagnostics_return_type_errors.rs @@ -2879,3 +2879,58 @@ class Consumer { return_error_messages(&diags).join("; ") ); } + +// ─── Laravel now()/today() resolve to the concrete Carbon class ───────────── + +/// Shared Carbon-family scaffolding: the concrete `Illuminate\Support\Carbon` +/// extends `Carbon\Carbon`, which extends the built-in `\DateTime`, plus the +/// `now()`/`today()` helpers declared to return the looser `CarbonInterface`. +const CARBON_SCAFFOLD: &str = r#" +namespace { + interface DateTimeInterface {} + class DateTime implements DateTimeInterface {} +} +namespace Carbon { + interface CarbonInterface extends \DateTimeInterface {} + class Carbon extends \DateTime implements CarbonInterface { + public function addHours(int $value = 1): static { return $this; } + } +} +namespace Illuminate\Support { + class Carbon extends \Carbon\Carbon {} +} +namespace { + function now($tz = null): \Carbon\CarbonInterface { return new \Illuminate\Support\Carbon(); } + function today($tz = null): \Carbon\CarbonInterface { return new \Illuminate\Support\Carbon(); } +} +"#; + +#[test] +fn now_and_today_satisfy_datetime_return() { + let php = format!( + "addHours(1)` is still a `\DateTime`. + let php = format!( + "addHours(1); }}\n }}\n}}\n" + ); + let diags = collect(&php); + assert!( + !has_return_error(&diags), + "now()->addHours(1) should resolve to the concrete Carbon (a \\DateTime), got: {}", + return_error_messages(&diags).join("; ") + ); +} diff --git a/tests/unit/composer.rs b/tests/unit/composer.rs index f583bb17..3b9a5cec 100644 --- a/tests/unit/composer.rs +++ b/tests/unit/composer.rs @@ -984,15 +984,15 @@ fn test_path_repo_psr4_skips_package_inside_vendor() { // is itself in vendor — is an ordinary vendored dependency, not project // code. It must not be added to the PSR-4 mappings, or the diagnostics // pass would walk and analyse the entire dependency as user source. - ws.create_php_file("vendor/luxplus/shared/src/.gitkeep", ""); + ws.create_php_file("vendor/acme/shared/src/.gitkeep", ""); write_installed_json( &ws, r#"{ - "name": "luxplus/shared", + "name": "acme/shared", "dist": { "type": "path" }, "transport-options": { "symlink": true }, - "install-path": "../luxplus/shared", - "autoload": { "psr-4": { "Luxplus\\Shared\\": "src/" } } + "install-path": "../acme/shared", + "autoload": { "psr-4": { "Acme\\Shared\\": "src/" } } }"#, ); From 4ea9e82c8ed362e8882d9da17f01a3eb46a87ffa Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Sat, 18 Jul 2026 08:03:53 +0200 Subject: [PATCH 6/6] Conditional return types keep an intersection with the matched class --- docs/CHANGELOG.md | 1 + docs/todo.md | 1 - docs/todo/bugs.md | 53 +--------- src/completion/types/conditional.rs | 47 ++++++++- .../diagnostics_property_type_errors.rs | 99 +++++++++++++++++++ .../diagnostics_return_type_errors.rs | 51 ++++++++++ 6 files changed, 194 insertions(+), 58 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4a9091a0..0f5ba5e0 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Macro hover shows origin and inferred return types.** Hovering on a macro method call now displays a "macro" indicator instead of the generic "virtual" label, distinguishing `::macro()` registrations from `@method`/`@mixin` synthesized members. When the closure has no explicit return type hint, the return type is inferred from the closure body and shown with an "(inferred)" annotation. Bare `$this` / `self` / `static` returns preserve their keyword form, and method chains like `$this->transform(...)` use the last method's declared return type directly — preserving `$this`, `static`, and generic parameters that the general resolver would flatten to a bare class name. Regular (non-macro) methods with inferred return types also show the "(inferred)" annotation on hover. A new `preserve_static` flag on the resolution context controls whether self-references resolve to their keyword form or the concrete class name. Contributed by @calebdw. - **Return type mismatch diagnostics (`type_mismatch_return`).** Functions and methods with a declared return type are now checked against their `return` statements. Incompatible return values are flagged as errors. Void functions returning a value and bare `return;` in non-void functions are also flagged. Generators (functions using `yield`) are skipped. Uses the same conservative `is_type_compatible` policy as argument type checking to avoid false positives. Contributed by @calebdw. - **Property type assignment diagnostics (`type_mismatch_property`).** Assignments to typed properties (`$this->prop = expr` and `self::$prop = expr`) are checked against the declared property type. Incompatible values are flagged as errors. Only plain `=` assignments are checked; compound operators (`+=`, `.=`, etc.) are skipped. Untyped and `mixed` properties are not flagged. Contributed by @calebdw. +- **Conditional return types keep an intersection with the matched class.** A `@return ($x is class-string ? T&SomeInterface : SomeInterface)` annotation now resolves the matched branch to the concrete class intersected with the interface, instead of collapsing it to the bare class. Mock factories such as Mockery's `mock(Foo::class)` and Laravel's `$this->mock(Foo::class)` therefore resolve to `Foo&MockInterface`, so their members complete and assigning the result to a `Foo`-typed property or returning it from a `Foo&MockInterface` method no longer reports a spurious type mismatch. - **PSR-4 mismatch diagnostics and rename-based moves.** Files now warn when the declared namespace or primary class name does not match the PSR-4 path or filename, with quick fixes to correct them. Renaming a class from its declaration now opens the full FQCN so you can move it between namespaces in one step, and renaming a namespace can rewrite multiple segments at once while moving PSR-4 directories and updating references across the project. Contributed by @calebdw. - **Case-sensitive autoloading diagnostic.** A class reference whose casing differs from the class's actual declaration is now flagged, with a quick fix to correct it. This catches the bug where code loads on a case-insensitive filesystem (macOS, Windows) but fails with a class-not-found error on Linux, because PSR-4 maps the name to a file path and path lookups are case-sensitive there. It covers `use` imports and inline references to autoloaded classes; built-in classes and same-file references, which never reach the autoloader, are left alone. - **Completion candidates ranked by dependency provenance.** Class, function, and constant completions are now sorted by origin tier: project code first, then core/stub symbols, then explicit Composer dependencies (`require` / `require-dev`), then transitive vendor dependencies last. The provenance is inferred from `composer.json` and `installed.json` during indexing. Contributed by @calebdw. diff --git a/docs/todo.md b/docs/todo.md index 7aa72d65..f05b66aa 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -26,7 +26,6 @@ within the same impact tier. | # | Item | Impact | Effort | | --- | --------------------------------------------------------------------------------------------------------------------- | ---------- | ------ | | L21 | [Tighten the supertype-where-subtype comparison escape hatch (blocked on resolver precision)](todo/laravel.md#l21-tighten-the-supertype-where-subtype-comparison-escape-hatch-blocked-on-resolver-precision) | Medium | High | -| B114 | [`Mockery::mock()` / `$this->mock()` drop the intersection with the mocked class](todo/bugs.md#b114-mockerymock--thismock-drop-the-intersection-with-the-mocked-class) | Medium | Low-Medium | | L18 | [`Storage::disk()` return type — resolve from config, don't guess](todo/laravel.md#l18-storagedisk-return-type-is-genuinely-polymorphic--resolve-from-config-dont-guess) | Medium-High | Low-Medium | | L19 | [Redis client (phpredis/predis) — resolve from config, don't guess](todo/laravel.md#l19-redis-connection-client-is-chosen-by-config-phpredis-vs-predis--resolve-from-config-dont-guess) | Medium | Low-Medium | | X4 | [Full background indexing](todo/indexing.md#x4-full-background-indexing) (workspace symbols, fast find-references) | Medium | High | diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index d2534fce..0566a919 100644 --- a/docs/todo/bugs.md +++ b/docs/todo/bugs.md @@ -7,55 +7,4 @@ pipeline so it produces correct data. Downstream consumers (diagnostics, hover, completion, definition) should never need to second-guess upstream output. -## B114. `Mockery::mock()` / `$this->mock()` drop the intersection with the mocked class - -**Severity: Medium (false positives wherever a mock is assigned to a -property or return type typed as the concrete mocked class) · Confirmed -against real projects (a production Laravel codebase and a legacy PHP -app); PHPStan/Larastan report zero errors on the same lines** - -```php -/** @var EpaymentService */ -private $epaymentService; - -protected function setUp(): void -{ - $mock = Mockery::mock(EpaymentService::class); - $this->epaymentService = $mock; // "Property expects EpaymentService, got - // Mockery\MockInterface|Mockery\LegacyMockInterface" -} -``` - -```php -private function mockHelloRetailClient(): Client&MockInterface -{ - $mock = $this->mock(Client::class); - // ... - return $mock; // "Return type MockInterface is incompatible with - // declared return type Client&MockInterface" -} -``` - -Mockery generates a dynamic subclass of the class passed to -`Mockery::mock()` (and Laravel's `TestCase::mock()` / -`partialMock()` / `spy()`, which delegate to it) when that class is -not `final`, so the returned object genuinely satisfies both the -concrete class and `MockInterface`. PHPantom currently resolves the -call's return type from `Mockery::mock()`'s own docblock -(`@return \Mockery\MockInterface`), dropping the class-string -argument entirely. PHPStan gets this right via -`phpstan-mockery`'s `MockDynamicReturnTypeExtension` (for -`Mockery::mock()`/`spy()`) and Larastan's `TestCaseExtension` (for -`$this->mock()`/`partialMock()`/`spy()`) — both are -argument-dependent return type extensions that intersect -`MockInterface` with the type of the class-string argument. - -PHPantom has no equivalent mechanism for a handful of well-known -argument-dependent return types outside our own conditional -`@return` support. Where to add this: `completion/call_resolution.rs` -(or the return-type resolution path it feeds) should special-case -`Mockery::mock()`, `Mockery::spy()`, and, in Laravel projects, the -`TestCase` trait's `mock()`/`partialMock()`/`spy()` methods, and -intersect `MockInterface` with the resolved type of the first -argument when it is a `::class` reference (skip anything else, same -scope restriction as our other literal-argument-only resolvers). +No outstanding items. diff --git a/src/completion/types/conditional.rs b/src/completion/types/conditional.rs index 5e1d66f7..52849cd0 100644 --- a/src/completion/types/conditional.rs +++ b/src/completion/types/conditional.rs @@ -282,6 +282,26 @@ pub fn resolve_conditional_with_text_args_and_defaults( ) }; + // Helper: when the class-string bound is itself a template + // parameter (not a concrete class to subtype-check), the + // condition is definitionally satisfied and `then_type` + // must have the template substituted with the resolved + // class(es) rather than being discarded wholesale — this + // preserves surrounding structure like `T&MockInterface` + // instead of collapsing it to bare `T`. + let substitute_bound = |resolved_ty: PhpType| -> PhpType { + match class_string_bound_name { + Some(bound_name) if bound_is_template => { + let subs = std::collections::HashMap::from([( + bound_name.to_string(), + resolved_ty, + )]); + then_type.substitute(&subs) + } + _ => resolved_ty, + } + }; + // For variadic class-string parameters, collect class // names from ALL arguments at and after param_idx and // form a union type (e.g. `A|B` from `A::class, B::class`). @@ -324,7 +344,7 @@ pub fn resolve_conditional_with_text_args_and_defaults( } else { PhpType::Union(class_names.into_iter().map(PhpType::Named).collect()) }; - return Some(ty); + return Some(substitute_bound(ty)); } return resolve_conditional_with_text_args_and_defaults( else_type, @@ -362,7 +382,7 @@ pub fn resolve_conditional_with_text_args_and_defaults( return choose_branch(satisfies_bound(&resolved)); } - return Some(PhpType::Named(resolved)); + return Some(substitute_bound(PhpType::Named(resolved))); } // Check if the argument is a variable holding class-string // value(s) (e.g. from a match expression). @@ -389,7 +409,7 @@ pub fn resolve_conditional_with_text_args_and_defaults( } else { PhpType::Union(names.into_iter().map(PhpType::Named).collect()) }; - return Some(ty); + return Some(substitute_bound(ty)); } } // Argument isn't a ::class literal or resolvable variable → try else branch @@ -1231,6 +1251,23 @@ pub fn resolve_conditional_with_args_and_defaults<'b>( ) }; + // See the matching helper in resolve_conditional_with_text_args_and_defaults: + // a template-parameter bound must have `then_type` substituted rather + // than be replaced outright, so surrounding structure like + // `T&MockInterface` survives. + let substitute_bound = |resolved_ty: PhpType| -> PhpType { + match class_string_bound_name { + Some(bound_name) if bound_is_template => { + let subs = std::collections::HashMap::from([( + bound_name.to_string(), + resolved_ty, + )]); + then_type.substitute(&subs) + } + _ => resolved_ty, + } + }; + // Check if the argument is `X::class`. When the argument is // omitted entirely, fall back to a `Foo::class` parameter // default so `app()` resolves the same as `app(Foo::class)`. @@ -1251,7 +1288,7 @@ pub fn resolve_conditional_with_args_and_defaults<'b>( return choose_branch(satisfies_bound(&resolved)); } - return Some(PhpType::Named(resolved)); + return Some(substitute_bound(PhpType::Named(resolved))); } // Check if the argument is a variable holding class-string // value(s) (e.g. from a match expression). @@ -1275,7 +1312,7 @@ pub fn resolve_conditional_with_args_and_defaults<'b>( } else { PhpType::Union(names.into_iter().map(PhpType::Named).collect()) }; - return Some(ty); + return Some(substitute_bound(ty)); } } // Argument isn't a ::class literal or resolvable variable → try else branch diff --git a/tests/integration/diagnostics_property_type_errors.rs b/tests/integration/diagnostics_property_type_errors.rs index 56c93095..9e70364c 100644 --- a/tests/integration/diagnostics_property_type_errors.rs +++ b/tests/integration/diagnostics_property_type_errors.rs @@ -2302,3 +2302,102 @@ class Component { property_error_messages(&diags).join("; ") ); } + +// ─── Mockery/Laravel mocks assigned to a concretely-typed property ───────── + +#[test] +fn mock_conditional_return_assigned_to_concrete_property() { + // Mirrors Laravel's `InteractsWithContainer::mock()`: assigning + // `$this->mock(Concrete::class)` to a property typed as the concrete + // class must not flag, since the resolved type is `Concrete&MockInterface`. + let php = r#" $abstract + * @return ($abstract is class-string ? TInstance&\Mockery\MockInterface : \Mockery\MockInterface) + */ + protected function mock($abstract) {} + } + + class TestCase + { + use InteractsWithContainer; + } + + class EpaymentTest extends TestCase + { + private EpaymentService $epaymentService; + + protected function setUp(): void + { + $this->epaymentService = $this->mock(EpaymentService::class); + } + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "$this->mock(EpaymentService::class) should satisfy an EpaymentService-typed property, got: {}", + property_error_messages(&diags).join("; ") + ); +} + +#[test] +fn mockery_static_mock_variadic_template_assigned_to_concrete_property() { + // Mirrors Mockery's own `Mockery::mock()`: the template param (`TMock`) + // is inferred from a `class-string` alternative buried inside a + // union nested in a variadic array parameter, and the intersection + // `LegacyMockInterface&MockInterface&TMock` must keep the concrete class. + let php = r#"|TMock|Closure(LegacyMockInterface&MockInterface&TMock):LegacyMockInterface&MockInterface&TMock|array> $args + * + * @return LegacyMockInterface&MockInterface&TMock + */ + public static function mock(...$args) {} + } +} + +namespace App { + class EpaymentService {} + + class EpaymentTest + { + private EpaymentService $epaymentService; + + protected function setUp(): void + { + $mock = \Mockery\Mockery::mock(EpaymentService::class); + $this->epaymentService = $mock; + } + } +} +"#; + let diags = collect(php); + assert!( + !has_property_error(&diags), + "Mockery::mock(EpaymentService::class) should satisfy an EpaymentService-typed property, got: {}", + property_error_messages(&diags).join("; ") + ); +} diff --git a/tests/integration/diagnostics_return_type_errors.rs b/tests/integration/diagnostics_return_type_errors.rs index 9763002b..00a1b291 100644 --- a/tests/integration/diagnostics_return_type_errors.rs +++ b/tests/integration/diagnostics_return_type_errors.rs @@ -2934,3 +2934,54 @@ fn now_chain_narrows_to_concrete_carbon() { return_error_messages(&diags).join("; ") ); } + +// ─── Conditional return type intersecting a template param with a fixed +// interface (Laravel's `TestCase::mock()`/`partialMock()`/`spy()`) ────── + +#[test] +fn mock_conditional_return_preserves_interface_intersection() { + // Mirrors Laravel's `InteractsWithContainer::mock()`: + // `@return ($abstract is class-string ? TInstance&\Mockery\MockInterface : \Mockery\MockInterface)`. + // The template param (`TInstance`) must be substituted with the + // resolved class while keeping the `&MockInterface` intersection, + // not collapsed down to the template alone. + let php = r#" $abstract + * @return ($abstract is class-string ? TInstance&\Mockery\MockInterface : \Mockery\MockInterface) + */ + protected function mock($abstract) {} + } + + class TestCase + { + use InteractsWithContainer; + } + + class MyTest extends TestCase + { + private function mockClient(): Client&\Mockery\MockInterface + { + return $this->mock(Client::class); + } + } +} +"#; + let diags = collect(php); + assert!( + !has_return_error(&diags), + "$this->mock(Client::class) should resolve to Client&MockInterface, got: {}", + return_error_messages(&diags).join("; ") + ); +}