diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index d1165bcd1..7ff826124 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -521,6 +521,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Laravel +- **Scopes keep custom Eloquent builders.** Chaining a local scope preserves the custom builder and its model type, including with ide-helper docblocks, so its methods remain available and valid returns are no longer reported as type mismatches. Contributed by @shuvroroy. - **Route groups in `bootstrap/app.php`.** Routes registered from the `then:` or `using:` callback of `withRouting()`, including route files grouped there under a name prefix, are now known by their full names and are no longer reported as unknown routes. - **Imported `#[Table]` and `#[Connection]` attributes.** A model's table and connection attributes are now read when the attribute is imported with `use`, not only when written fully qualified, in files open in the editor and in `analyze`. Contributed by @shuvroroy. - **Nullable columns stay nullable through a cast.** A model attribute cast to `boolean`, `integer`, a date, an enum or another cast that hands `null` back untouched is now nullable when its column is, and so is a nullable `updated_at` or other timestamp column. A null check on a nullable date column, the soft-delete column included, leaves the date class with all its members. diff --git a/examples/laravel/app/Demo.php b/examples/laravel/app/Demo.php index 4b048d853..53bac34d8 100644 --- a/examples/laravel/app/Demo.php +++ b/examples/laravel/app/Demo.php @@ -15,6 +15,7 @@ use App\Http\Requests\StoreBakeryRequest; use App\Http\Requests\UpdateBakeryRequest; use App\Models\Baker; +use App\Models\BakerBuilder; use App\Models\Bakery; use App\Models\BakeryOrder; use App\Models\BlogAuthor; @@ -186,6 +187,14 @@ public function classCasts(CastSample $sample): void // ── Eloquent Query Builder ────────────────────────────────────────────── + /** @return BakerBuilder */ + public function scopedBakers(): BakerBuilder + { + // Try: hover employed() and available(), then complete after either. + // Both scopes keep BakerBuilder, including its active() method. + return Baker::query()->employed()->available()->active(); + } + public function eloquentQuery(): void { // Builder-as-static forwarding diff --git a/examples/laravel/app/Models/Baker.php b/examples/laravel/app/Models/Baker.php index 8408f1930..b365a6335 100644 --- a/examples/laravel/app/Models/Baker.php +++ b/examples/laravel/app/Models/Baker.php @@ -3,10 +3,24 @@ namespace App\Models; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Attributes\Scope; use Illuminate\Database\Eloquent\Attributes\UseEloquentBuilder; +/** @method static BakerBuilder|Baker employed() */ #[UseEloquentBuilder(BakerBuilder::class)] class Baker extends Model { public function getName(): string { return ''; } + + protected function scopeEmployed(Builder $query): Builder + { + return $query->whereNotNull('hired_at'); + } + + #[Scope] + protected function available(Builder $query): void + { + $query->where('available', true); + } } diff --git a/examples/laravel/app/Models/BakerBuilder.php b/examples/laravel/app/Models/BakerBuilder.php index dfc967a03..e1c16eeb9 100644 --- a/examples/laravel/app/Models/BakerBuilder.php +++ b/examples/laravel/app/Models/BakerBuilder.php @@ -7,6 +7,7 @@ /** * @template TModel of \App\Models\Baker * @extends Builder + * @mixin Baker */ class BakerBuilder extends Builder { diff --git a/examples/laravel/assertions.php b/examples/laravel/assertions.php index 2c0f7ea04..81ba52d52 100644 --- a/examples/laravel/assertions.php +++ b/examples/laravel/assertions.php @@ -376,6 +376,20 @@ function assertMethodReturnType(string $class, string $method, string $expected) 'BakerBuilder holds the Baker model it queries', $result->getModel() instanceof \App\Models\Baker ); +check( + 'Convention and attribute scopes preserve BakerBuilder and its model', + $result->employed()->available()->active() === $result + && $result->getModel() instanceof \App\Models\Baker +); +check( + 'Static scope forwarding preserves BakerBuilder', + \App\Models\Baker::employed()->available()->active() instanceof \App\Models\BakerBuilder +); +check( + 'The scoped builder demo returns BakerBuilder', + ($scopedBakers = (new \App\Demo())->scopedBakers()) instanceof \App\Models\BakerBuilder + && $scopedBakers->getModel() instanceof \App\Models\Baker +); check( 'Inherited where/orderBy calls preserve the LoafBuilder instance', diff --git a/src/type_engine/variable/closure_resolution.rs b/src/type_engine/variable/closure_resolution.rs index eb165bb16..d27267463 100644 --- a/src/type_engine/variable/closure_resolution.rs +++ b/src/type_engine/variable/closure_resolution.rs @@ -880,14 +880,15 @@ fn extract_generic_args_from_methods(class: &ClassInfo, class_fqn: &str) -> Opti None } -/// Extract the model type from a resolved `Builder` class by -/// scanning its method return types for `Builder` and returning `X`. +/// Recover the model from a resolved builder's fluent return types, +/// including scopes returning the custom builder itself. pub(in crate::type_engine) fn extract_model_from_builder(builder: &ClassInfo) -> Option { + let builder_fqn = builder.fqn(); for method in &builder.methods { if let Some(ref ret) = method.return_type && let TypeKind::Generic(g) = ret.kind() && !g.args.is_empty() - && (g.name == ELOQUENT_BUILDER_FQN || g.name == "Builder") + && (g.name == ELOQUENT_BUILDER_FQN || g.name == "Builder" || g.name == builder_fqn) { // Skip unsubstituted template params like "TModel". if !g.args[0].is_empty() && !g.args[0].is_named("TModel") { @@ -897,3 +898,25 @@ pub(in crate::type_engine) fn extract_model_from_builder(builder: &ClassInfo) -> } None } + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_fixtures::{make_class, make_method}; + + #[test] + fn recover_model_from_custom_builder_uses_its_exact_class() { + let mut builder = make_class("App\\Builders\\Query"); + for (name, ret) in [ + ("other", "Other\\Builders\\Query"), + ("unbound", "App\\Builders\\Query"), + ("scoped", "App\\Builders\\Query"), + ] { + builder.methods.push(Arc::new(make_method(name, Some(ret)))); + } + assert_eq!( + extract_model_from_builder(&builder), + Some(PhpType::named(atom("App\\Models\\Post"))), + ); + } +} diff --git a/src/virtual_members/laravel/builder_injection.rs b/src/virtual_members/laravel/builder_injection.rs index 5d8bc1dbe..0a5b3bf09 100644 --- a/src/virtual_members/laravel/builder_injection.rs +++ b/src/virtual_members/laravel/builder_injection.rs @@ -279,11 +279,20 @@ fn inject_scopes_and_model_methods( // 1. Inject scope methods. let scope_methods = build_scope_methods_for_builder(model_arg, class_loader); for method in scope_methods { - let already_exists = result + let existing = result .methods .iter() - .any(|m| m.name == method.name && m.is_static == method.is_static); - if !already_exists { + .position(|m| m.name.eq_ignore_ascii_case(&method.name)); + if let Some(index) = existing { + // Builder::__call invokes the model's scope ahead of any + // @method or @mixin description of that scope. Real builder + // methods and registered macros still win at runtime. + let previous = &result.methods[index]; + if previous.is_virtual && !previous.is_macro { + result.methods.make_mut()[index] = + forward_builder_return(&method, forwarded, class_loader); + } + } else { result .methods .push(forward_builder_return(&method, forwarded, class_loader)); diff --git a/src/virtual_members/laravel/model_provider.rs b/src/virtual_members/laravel/model_provider.rs index a461510aa..32117d6f5 100644 --- a/src/virtual_members/laravel/model_provider.rs +++ b/src/virtual_members/laravel/model_provider.rs @@ -27,7 +27,7 @@ use super::relationships::{ RelationshipKind, build_property_type, classify_relationship_typed, count_property_name, extract_pivot_accessor_typed, extract_related_type_typed, }; -use super::scopes::{build_scope_methods, is_scope_method}; +use super::scopes::{build_scope_methods, is_scope_method, rebind_scope_builder_return}; use super::table_names::model_schema_table; use super::where_property::{build_where_property_methods_with_properties, lowercase_method_names}; use super::{ @@ -463,6 +463,7 @@ impl<'a> ModelMembers<'a> { /// relationship method loads. fn add_method_members(&mut self) { let class = self.class; + let mut builder_type = None; for method in &class.methods { if self.is_base_model_method(method) { continue; @@ -473,7 +474,11 @@ impl<'a> ModelMembers<'a> { // A method that is both `#[Scope]`-attributed and named // `scopeX` keeps the attribute's name as-is; the prefix is // only stripped for convention-based scope methods. - let [instance_method, static_method] = build_scope_methods(method); + let builder = builder_type + .get_or_insert_with(|| super::model_builder_type(class, self.class_loader)); + let [mut instance_method, mut static_method] = build_scope_methods(method); + rebind_scope_builder_return(&mut instance_method, builder); + rebind_scope_builder_return(&mut static_method, builder); self.methods.push(Arc::new(instance_method)); self.methods.push(Arc::new(static_method)); continue; diff --git a/src/virtual_members/laravel/scopes.rs b/src/virtual_members/laravel/scopes.rs index 830cc18b1..c95a7492e 100644 --- a/src/virtual_members/laravel/scopes.rs +++ b/src/virtual_members/laravel/scopes.rs @@ -12,7 +12,6 @@ use std::sync::Arc; use crate::php_type::{PhpType, TypeKind}; use crate::types::{MethodExtras, MethodInfo}; -use crate::util::short_name; use super::ELOQUENT_BUILDER_FQN; use super::helpers::extends_eloquent_model; @@ -94,6 +93,29 @@ pub(super) fn scope_return_type(method: &MethodInfo) -> PhpType { } } +/// Keep a fluent scope on the model's builder, while preserving a return +/// that explicitly queries a different model or names another result type. +pub(super) fn rebind_scope_builder_return(method: &mut MethodInfo, builder: &PhpType) { + let Some(ret) = method.return_type.as_ref() else { + return; + }; + let should_rebind = match ret.kind() { + TypeKind::Named(name) => Fqn::new(name).as_str() == ELOQUENT_BUILDER_FQN, + TypeKind::Generic(g) => { + Fqn::new(&g.name).as_str() == ELOQUENT_BUILDER_FQN + && builder.base_name() != Some(ELOQUENT_BUILDER_FQN) + && g.args.first().is_some_and(|model| { + model.is_self_ref() + || matches!(builder.kind(), TypeKind::Generic(b) if b.args.first() == Some(model)) + }) + } + _ => false, + }; + if should_rebind { + method.return_type = Some(builder.clone()); + } +} + /// Build virtual methods for a scope method. /// /// Returns two `MethodInfo` values: one static and one instance. Both @@ -189,6 +211,7 @@ pub fn build_scope_methods_for_builder( let subs = super::self_ref_subs(model_type.clone()); let mut methods = Vec::new(); + let mut builder_type = None; // The rewrite below depends only on the model, so the produced // copies are interned and shared by every Builder and relation @@ -203,6 +226,8 @@ pub fn build_scope_methods_for_builder( if !is_scope_method(method) { continue; } + let builder = builder_type + .get_or_insert_with(|| super::model_builder_type(&model_class, class_loader)); let transformed = crate::virtual_members::intern_transformed_method(method, fp, || { // Build an instance method (scopes are called as instance @@ -235,21 +260,8 @@ pub fn build_scope_methods_for_builder( if let Some(ref mut ret) = m.return_type { *ret = ret.substitute(&subs); - - // When a scope method declares a bare `Builder` return type - // (without generic args), the chain loses track of the - // concrete model. Subsequent calls on the returned Builder - // would not find model-specific scope methods because - // `type_hint_to_classes_typed` only injects scopes when - // generic args are present. Wrap bare Builder return types - // as `Builder` to preserve the chain. - if is_bare_builder_type(ret) { - *ret = PhpType::generic( - ELOQUENT_BUILDER_FQN, - vec![PhpType::named(atom(model_name))], - ); - } } + rebind_scope_builder_return(&mut m, builder); m }); @@ -259,19 +271,6 @@ pub fn build_scope_methods_for_builder( methods } -/// Check whether a `PhpType` is a bare Eloquent Builder reference -/// without generic arguments. -/// -/// Matches both the FQN (`Illuminate\Database\Eloquent\Builder`) and -/// the short name (`Builder`) since scope methods in user code -/// typically use the imported short name. -fn is_bare_builder_type(ty: &PhpType) -> bool { - match ty.kind() { - TypeKind::Named(name) => name == ELOQUENT_BUILDER_FQN || short_name(name) == "Builder", - _ => false, - } -} - #[cfg(test)] #[path = "scopes_tests.rs"] mod tests; diff --git a/src/virtual_members/laravel/scopes_tests.rs b/src/virtual_members/laravel/scopes_tests.rs index 9004dbfc2..3d562785c 100644 --- a/src/virtual_members/laravel/scopes_tests.rs +++ b/src/virtual_members/laravel/scopes_tests.rs @@ -154,6 +154,63 @@ fn scope_return_type_custom() { ); } +#[test] +fn rebind_scope_builder_return_preserves_explicit_results() { + let custom = PhpType::parse("App\\Builders\\PostBuilder"); + let base = PhpType::parse("Illuminate\\Database\\Eloquent\\Builder"); + for (ret, builder, expected) in [ + (None, &custom, None), + (Some("int"), &custom, Some("int")), + (Some("int|string"), &custom, Some("int|string")), + (Some("App\\Builder"), &custom, Some("App\\Builder")), + (Some("Builder"), &custom, Some("Builder")), + ( + Some("Illuminate\\Database\\Eloquent\\Builder"), + &custom, + Some("App\\Builders\\PostBuilder"), + ), + ( + Some("\\Illuminate\\Database\\Eloquent\\Builder"), + &custom, + Some("App\\Builders\\PostBuilder"), + ), + ( + Some("Illuminate\\Database\\Eloquent\\Builder"), + &base, + Some("Illuminate\\Database\\Eloquent\\Builder"), + ), + ( + Some("Illuminate\\Database\\Eloquent\\Builder"), + &custom, + Some("App\\Builders\\PostBuilder"), + ), + ( + Some("Illuminate\\Database\\Eloquent\\Builder"), + &custom, + Some("App\\Builders\\PostBuilder"), + ), + ( + Some("Illuminate\\Database\\Eloquent\\Builder"), + &custom, + Some("Illuminate\\Database\\Eloquent\\Builder"), + ), + ( + Some("App\\Builders\\OtherBuilder"), + &custom, + Some("App\\Builders\\OtherBuilder"), + ), + ( + Some("Illuminate\\Database\\Eloquent\\Builder"), + &base, + Some("Illuminate\\Database\\Eloquent\\Builder"), + ), + ] { + let mut method = make_method("scopePublished", ret); + rebind_scope_builder_return(&mut method, builder); + assert_eq!(method.return_type, expected.map(PhpType::parse), "{ret:?}"); + } +} + // ── build_scope_methods (convention) ───────────────────────────────── #[test] diff --git a/src/virtual_members/laravel/tests.rs b/src/virtual_members/laravel/tests.rs index 0f624fff1..c14b8fd36 100644 --- a/src/virtual_members/laravel/tests.rs +++ b/src/virtual_members/laravel/tests.rs @@ -12,6 +12,59 @@ use std::sync::Arc; // ── applies_to ────────────────────────────────────────────────────── +#[test] +fn builder_scope_injection_respects_declared_methods_and_macros() { + let mut model = make_class("App\\Models\\Post"); + model.parent_class = Some(atom(ELOQUENT_MODEL_FQN)); + model.methods.push(Arc::new(make_method( + "scopePublished", + Some(ELOQUENT_BUILDER_FQN), + ))); + let loader = |fqn: Fqn<'_>| match fqn.as_str() { + "App\\Models\\Post" => Some(Arc::new(model.clone())), + ELOQUENT_MODEL_FQN => Some(Arc::new(make_class(ELOQUENT_MODEL_FQN))), + _ => None, + }; + for (is_virtual, is_macro, is_static, expected) in [ + (false, false, false, "int"), + (true, true, false, "int"), + ( + true, + false, + false, + "Illuminate\\Database\\Eloquent\\Builder", + ), + ( + true, + false, + true, + "Illuminate\\Database\\Eloquent\\Builder", + ), + ] { + let mut builder = make_class(ELOQUENT_BUILDER_FQN); + let raw = builder.clone(); + let mut method = make_method("PUBLISHED", Some("int")); + method.is_virtual = is_virtual; + method.is_macro = is_macro; + method.is_static = is_static; + builder.methods.push(Arc::new(method)); + super::builder_injection::try_inject_builder_scopes( + &mut builder, + &raw, + ELOQUENT_BUILDER_FQN, + &[PhpType::named(model.fqn())], + &loader, + ); + let instance = builder + .methods + .iter() + .find(|m| !m.is_static && m.name.eq_ignore_ascii_case("published")) + .unwrap(); + assert_eq!(instance.return_type_str().as_deref(), Some(expected)); + assert!(!instance.is_static); + } +} + #[test] fn model_builder_type_survives_missing_framework_and_custom_stubs() { let mut model = make_class("App\\Models\\Product"); diff --git a/tests/integration/laravel_custom_builder.rs b/tests/integration/laravel_custom_builder.rs index 22c8cb1d2..eb07c53b7 100644 --- a/tests/integration/laravel_custom_builder.rs +++ b/tests/integration/laravel_custom_builder.rs @@ -2,6 +2,190 @@ use crate::common::create_psr4_workspace; use crate::common::{completion_response, goto_definition_at, open_php}; use tower_lsp_server::gen_lsp_types::*; +#[tokio::test] +async fn test_custom_builder_scopes_preserve_query_type() { + use crate::common::{ + assigned_type, complete_labels_at, hover_on, messages_with_code, position_after, + }; + + for (model_tag, builder_mixin, extends_tag) in [ + (true, true, "@template-extends"), + (false, true, "@template-extends"), + (true, false, "@template-extends"), + (true, true, "@extends"), + (false, false, ""), + ] { + let builder = format!( + r#"") + }, + if builder_mixin { "@mixin Post" } else { "" }, + ); + let model = format!( + r#" */ + protected function scopeGeneric(Builder $query): Builder {{ return $query; }} + protected function scopeCounted(Builder $query): int {{ return 1; }} + protected function scopeOverridden(Builder $query): Builder {{ return $query; }} +}} +class ChildPost extends Post {{}} +"#, + if model_tag { + "@method static PostBuilder|Post published()" + } else { + "" + }, + ); + let (backend, dir) = create_psr4_workspace( + r#"{"autoload":{"psr-4":{"App\\":"app/","Illuminate\\":"vendor/illuminate/"}}}"#, + &[ + ( + "vendor/illuminate/Database/Eloquent/Model.php", + " */ + public function scoped(): PostBuilder { + $plain = Post::query(); + $published = Post::query()->published(); + $recent = Post::query()->recent(); + $featured = Post::query()->featured(); + $generic = Post::query()->generic(); + $inherited = Post::query()->inherited(); + $child = ChildPost::query()->published(); + $static = Post::published(); + $instance = (new Post())->published(); + $chained = Post::query()->published()->recent()->featured()->popular(); + $model = Post::query()->published()->firstOrFail(); + $plainModel = Post::query()->firstOrFail(); + $count = Post::query()->counted(); + $override = Post::query()->overridden(); + return Post::query()->published(); + } + public function wrong(): string { + return Post::query()->published(); + } +} +"#; + open_php(&backend, &uri, content).await; + for variable in [ + "$plain", + "$published", + "$recent", + "$featured", + "$generic", + "$static", + "$instance", + "$chained", + "$inherited", + ] { + assert_eq!( + assigned_type(&backend, uri.as_str(), content, variable), + "PostBuilder", + "{variable}, model tag: {model_tag}, mixin: {builder_mixin}, {extends_tag}", + ); + } + assert_eq!( + assigned_type(&backend, uri.as_str(), content, "$child"), + "PostBuilder", + ); + for (variable, expected) in [ + ("$plainModel", "Post"), + ("$model", "Post"), + ("$count", "int"), + ("$override", "string"), + ] { + assert_eq!( + assigned_type(&backend, uri.as_str(), content, variable), + expected + ); + } + let hover = hover_on(&backend, &uri, content, "$published =").await; + assert!(!hover.contains("PostBuilderpop"); + let labels = complete_labels_at( + &backend, + &uri, + completion, + position.line, + position.character, + ) + .await; + assert!( + labels.iter().any(|label| label.starts_with("popular")), + "{labels:?}" + ); + } +} + #[tokio::test] async fn test_custom_eloquent_builder_attribute() { let (backend, dir) = create_psr4_workspace(