diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 15e490d09..384841a76 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -126,6 +126,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Blade partial variables stay typed during CLI analysis.** `phpantom_lsp analyze` now discovers view and include callers while running without the editor's reference index, and direct variables passed in template data retain nearby `@var` overrides. Contributed by @shuvroroy (#337). +- **A conditional helper keeps a scalar return selected by its arguments.** A helper whose PHPDoc selects between a class and a scalar could choose the scalar correctly and then replace it with the broader native return type when the call was used inline or passed into a generic function. In Laravel, that made `url('/login')` look like a URL generator even though it returns a string. `url()` and `url(null)` still resolve to the generator, while a non-null path now resolves to `string` consistently through direct chains, assignments, and template-bound calls. Contributed by @shuvroroy (#337). - **Reference counts above a declaration update once the initial index finishes.** A file opened before the project's background index completed showed `0 references` above every class, method, property, and function, because the count was computed from an index that was still filling in. Nothing then told the editor to ask again: a refresh was already sent when a `didChange` parse committed a new symbol map and when a queued member reference count finished computing, but not when the initial index itself completed, so the stale zeros sat there until an unrelated edit happened to trigger one. The same refresh is now sent once the initial index finishes. - **Renaming a global constant no longer renames a class constant of the same name.** Find References and rename on a global `const BAR = 1;` matched any class constant declaration named `BAR` too, since the check only compared short names. Renaming the global constant rewrote `Holder::BAR`'s declaration in an unrelated class while leaving every `Holder::BAR` use site alone, producing a file that no longer compiled. A class constant's declaration is no longer treated as a candidate when searching for a global constant's references. - **Find References on a global constant can exclude its declaration.** A request with `includeDeclaration: false` still returned the `const BAR = 1;` line, unlike the same request against a function, method, or property, because a constant's declaration site carried no marker distinguishing it from a use of it. It is now excluded when asked, the same way a function's declaration already is. diff --git a/docs/todo.md b/docs/todo.md index c2990104d..2b6c68611 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -102,6 +102,7 @@ unlikely to move the needle for most users. | T11 | [Nested list destructuring](todo/type-inference.md#t11-nested-list-destructuring) | Low | Medium | | | **[Bugs](todo/bugs.md)** | | | | B1 | [Editing a service provider does not re-scan what it registers](todo/bugs.md#b1-editing-a-service-provider-does-not-re-scan-what-it-registers) | Medium | Medium | +| B2 | [A request accessor written with named arguments loses its key](todo/bugs.md#b2-a-request-accessor-written-with-named-arguments-loses-its-key) | Low | Low | | | **[Diagnostics](todo/diagnostics.md)** | | | | D6 | [Unreachable code diagnostic](todo/diagnostics.md#d6-unreachable-code-diagnostic) | Low-Medium | Medium | | D16 | [`unreachable_match_arm` ignores literal subject types](todo/diagnostics.md#d16-unreachable_match_arm-ignores-literal-subject-types) | Low-Medium | Medium | @@ -162,7 +163,6 @@ unlikely to move the needle for most users. | L46 | [`->can()` on a user model the receiver does not name](todo/laravel.md#l46-can-on-a-user-model-the-receiver-does-not-name) | Medium-High | Medium-High | | L30 | [Eloquent attribute-array key completion](todo/laravel.md#l30-eloquent-attribute-array-key-completion) | Medium | Medium | | L32 | [Config-backed named-resource strings](todo/laravel.md#l32-config-backed-named-resource-strings) (log channels, cache stores, guards, connections, rate limiters) | Medium | Medium | -| L41 | [Conditional return types on `url()` and its sibling helpers](todo/laravel.md#l41-conditional-return-types-on-url-and-its-sibling-helpers) | Medium | Medium | | L49 | [Unguarded Eloquent mass assignment diagnostic](todo/laravel.md#l49-unguarded-eloquent-mass-assignment-diagnostic) | Medium | Medium | | L17 | [Additional string contexts without booting](todo/laravel.md#l17-additional-string-contexts-without-booting) (middleware, assets, validation, Inertia) | Medium | Medium-High | | L25 | [Storage disk name strings](todo/laravel.md#l25-storage-disk-name-strings) | Low-Medium | Low | diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index f6371fb17..4a9b002fb 100644 --- a/docs/todo/bugs.md +++ b/docs/todo/bugs.md @@ -45,6 +45,42 @@ No outstanding items. ## Miscellaneous +#### B2. A request accessor written with named arguments loses its key + +**Impact: Low · Complexity: Low** + +`header()`, `query()`, `cookie()`, `input()`, `post()`, and `file()` +each declare one union spanning every way of calling them, and the +call's own arguments are what pick a member. Both resolution paths read +`$key` and `$default` off the call by position, so a named argument is +read as whichever slot it happens to sit in: + +```php +$photos = $request->file(key: 'photos'); // list|null + // resolves as UploadedFile|null +$all = $request->header(default: 'x'); // the whole HeaderBag + // resolves as a plain string +``` + +The key text is `key: 'photos'`, which no string literal unquotes to, so +the validation rules that decide between one upload and a list are never +consulted, and a call that supplies only a default is read as if that +default were the key. + +The argument texts reaching both sites keep their `name:` labels on +purpose, so that `bind_text_args_to_params` can route them; these two +sites just never call it. The AST counterpart (`bind_args_to_params`) +covers the default's expression the same way. + +**Where to look:** `try_resolve_request_accessor_type` in +`type_engine/variable/rhs_resolution/calls.rs` and +`resolve_request_accessor_at_call` in +`type_engine/call_resolution/return_types.rs`. Binding needs the +accessor's declared parameters, and the `ClassInfo` both sites hold is +the receiver's own class rather than an inheritance-merged one, so +`get_method_ci("file")` on an app's `FormRequest` subclass finds +nothing: the lookup has to walk the parent chain. + #### B1. Editing a service provider does not re-scan what it registers **Impact: Medium · Complexity: Medium** diff --git a/docs/todo/laravel.md b/docs/todo/laravel.md index 1e1d8fa12..7d307a01b 100644 --- a/docs/todo/laravel.md +++ b/docs/todo/laravel.md @@ -702,33 +702,6 @@ always-on diagnostics — dynamic construction (`view("emails.$type")`) makes "unused" inherently a heuristic, so it must read as "no static reference found," never as an error. -#### L41. Conditional return types on `url()` and its sibling helpers - -**Impact: Medium · Complexity: Medium** - -`url()` returns the generator or a string depending on whether it got a -path: - -```php -/** @return ($path is null ? \Illuminate\Contracts\Routing\UrlGenerator : string) */ -function url($path = null, $parameters = [], $secure = null); -``` - -We resolve the call to the contract regardless of the argument, so a -string result is typed as `UrlGenerator`. Real-world fallout is -`assertSame(url('/login'), $response->getTargetUrl())` reporting a -mismatch (four sites in one sample project), though the diagnostic only -fires there because of B4. - -This is the same shape as the `view()` helper, which is now resolved -precisely. `route()`, `secure_url()`, `action()`, and `redirect()` are -worth auditing in the same pass, as is `app()` (`($abstract is null ? -Application : mixed)`). - -**Where to look:** the helper interception next to `view()` in -`type_engine/call_resolution/return_types.rs` and the matching branch in -`type_engine/variable/rhs_resolution/calls.rs`. - #### L44. Sibling resource registrations and degenerate resource names **Impact: Low-Medium · Complexity: Medium** diff --git a/examples/laravel/app/Demo.php b/examples/laravel/app/Demo.php index b5f35afb8..a96dc8eaf 100644 --- a/examples/laravel/app/Demo.php +++ b/examples/laravel/app/Demo.php @@ -1278,4 +1278,16 @@ public function dateHelpers(): \DateTime // because Illuminate\Support\Carbon extends \DateTime. return now()->addHours(1); } + + // ── URL helper conditional return ────────────────────────────────────── + + public function urlHelpers(string $path): string + { + // Without a path, url() returns the generator and its methods resolve. + url()->to($path); // → UrlGenerator + url(null)->to($path); // → UrlGenerator + + // Supplying a path returns the generated URL string. + return url($path); // → string + } } diff --git a/examples/laravel/assertions.php b/examples/laravel/assertions.php index 0e6abb88f..990095cc4 100644 --- a/examples/laravel/assertions.php +++ b/examples/laravel/assertions.php @@ -1362,6 +1362,26 @@ public function toArray(): array ]) ); +// ─── URL helper conditional return ────────────────────────────────────────── + +$previousContainer = \Illuminate\Container\Container::getInstance(); +$helperContainer = new \Illuminate\Container\Container(); +$urlGenerator = new \Illuminate\Routing\UrlGenerator( + new \Illuminate\Routing\RouteCollection(), + \Illuminate\Http\Request::create('https://example.test') +); +$helperContainer->instance( + \Illuminate\Contracts\Routing\UrlGenerator::class, + $urlGenerator +); +\Illuminate\Container\Container::setInstance($helperContainer); + +check('url() returns the URL generator', url() === $urlGenerator); +check('url(null) returns the URL generator', url(null) === $urlGenerator); +check("url('/login') returns a string", is_string(url('/login'))); + +\Illuminate\Container\Container::setInstance($previousContainer); + // ─── Summary ──────────────────────────────────────────────────────────────── echo "\n"; diff --git a/examples/php/scaffolding/scaffolding.php b/examples/php/scaffolding/scaffolding.php index 9fe96b6ef..fb57105db 100644 --- a/examples/php/scaffolding/scaffolding.php +++ b/examples/php/scaffolding/scaffolding.php @@ -1037,7 +1037,7 @@ public function __construct(private array $items) {} public function offsetExists(mixed $offset): bool { return isset($this->items[$offset]); } /** @return T */ - public function offsetGet(mixed $offset) { return $this->items[$offset]; } + public function offsetGet(mixed $offset): mixed { return $this->items[$offset]; } public function offsetSet(mixed $offset, mixed $value): void { $this->items[$offset] = $value; } public function offsetUnset(mixed $offset): void { unset($this->items[$offset]); } } diff --git a/src/blade/call_site_inference.rs b/src/blade/call_site_inference.rs index 919983dea..35b0e69f5 100644 --- a/src/blade/call_site_inference.rs +++ b/src/blade/call_site_inference.rs @@ -107,10 +107,15 @@ pub(crate) struct BladeScope { pub components: Vec<(String, crate::blade::preprocessor::ComponentTarget)>, } -/// User files that render Blade views, with their symbol maps. Shared -/// across a whole refresh pass so the workspace is walked once, not once -/// per template. -pub(crate) type ViewCallerSnapshot = Vec<(String, Arc)>; +/// User files that render Blade views, with their symbol maps. Shared across +/// a whole refresh pass so the workspace is walked once, not once per +/// template. Headless analysis also carries a compact per-view candidate +/// index because it deliberately does not build the workspace reference +/// index used by the editor. +pub(crate) struct ViewCallerSnapshot { + files: Vec<(String, Arc)>, + local_candidates: Option>>, +} /// Every Blade file's raw source, snapshotted once per refresh pass. /// @@ -248,7 +253,7 @@ impl Backend { .collect(); let own_snapshot; let snapshot = match shared { - Some(shared) => shared.as_slice(), + Some(shared) => shared.files.as_slice(), None => { // Never trigger (or wait on) workspace indexing from here: // this runs while a Blade file is being opened or a @@ -261,21 +266,32 @@ impl Backend { } }; // A shared snapshot holds every file in the workspace that renders - // any view, so the ones that name *this* template are picked out of - // it the same way the per-template snapshot was built: by asking the - // reference index. Without it, every template would read every - // other template's spans. `None` while the index is still being - // built, which falls back to reading the snapshot whole. - let candidates = shared.and_then(|_| self.reference_candidate_uris_for_keys(&keys)); + // any view. Use its compact local index during headless analysis and + // the workspace reference index in the editor, so each template only + // reads callers that can name it. Before editor indexing completes, + // `None` conservatively reads the parsed snapshot whole. + let local_candidates = shared.and_then(|snapshot| snapshot.local_candidates.as_ref()); + let candidates = shared + .filter(|_| local_candidates.is_none()) + .and_then(|_| self.reference_candidate_uris_for_keys(&keys)); // Union the variables from every call site, per name. let mut merged: HashMap> = HashMap::new(); - for (file_uri, snapshot_map) in snapshot { + for (snapshot_index, (file_uri, snapshot_map)) in snapshot.iter().enumerate() { // A template must not feed itself: a recursive `@include` names // the template the spans it would be read from belong to. if file_uri == uri { continue; } + if let Some(local_candidates) = local_candidates + && !view_names.iter().any(|name| { + local_candidates + .get(name) + .is_some_and(|indices| indices.binary_search(&snapshot_index).is_ok()) + }) + { + continue; + } if let Some(candidates) = &candidates && !candidates.contains(file_uri.as_str()) { @@ -587,7 +603,7 @@ impl Backend { fn view_caller_snapshot(&self) -> ViewCallerSnapshot { let vendor_prefixes = self.workspace.vendor_uri_prefixes.lock().clone(); let maps = self.symbol_maps.read(); - let mut snapshot: ViewCallerSnapshot = maps + let mut files: Vec<(String, Arc)> = maps .iter() .filter(|(uri, map)| { !uri.starts_with("phpantom-stub://") @@ -609,8 +625,37 @@ impl Backend { // Deterministic order so the whole inference pass (and its // per-name type unions) is reproducible across runs, not just // the caller's `HashMap` iteration order. - snapshot.sort_by(|(a, _), (b, _)| a.cmp(b)); - snapshot + files.sort_by(|(a, _), (b, _)| a.cmp(b)); + + let local_candidates = self.skip_reference_index.then(|| { + let mut candidates: HashMap> = HashMap::new(); + for (index, (_, map)) in files.iter().enumerate() { + let mut names: Vec<&str> = map + .spans + .iter() + .filter_map(|span| match &span.kind { + SymbolKind::LaravelStringKey { + kind: LaravelStringKind::View, + key, + .. + } => Some(key.as_str()), + _ => None, + }) + .chain(map.view_receiver_sites.iter().map(|site| site.key.as_str())) + .collect(); + names.sort_unstable(); + names.dedup(); + for name in names { + candidates.entry(name.to_string()).or_default().push(index); + } + } + candidates + }); + + ViewCallerSnapshot { + files, + local_candidates, + } } /// Every known Blade file's raw source, for the component-tag scan in @@ -953,9 +998,25 @@ impl Backend { expr, } => { let span = expr.span(); - let ty = crate::type_engine::variable::foreach_resolution::resolve_expression_type( - expr, &var_ctx, - ) + // A bare variable needs the public variable + // resolver's `@var` precedence. The generic RHS + // resolver deliberately skips that outer layer + // because it is also used inside the forward walk. + let ty = match expr { + Expression::Variable(Variable::Direct(dv)) => crate::type_engine::variable::resolution::resolve_variable_php_type( + bytes_to_str(dv.name), + content, + site.offset, + Some(current_class), + &file_ctx.classes, + &class_loader, + Some(self), + Loaders::with_function(Some(&function_loader_cl)), + ), + _ => crate::type_engine::variable::foreach_resolution::resolve_expression_type( + expr, &var_ctx, + ), + } .unwrap_or_else(PhpType::mixed); (name, key_range, (span.start.offset, span.end.offset), ty) } @@ -2004,8 +2065,10 @@ fn is_variable_name(value: &str) -> bool { #[cfg(test)] mod tests { use super::join_call_site_types; + use crate::Backend; use crate::atom::atom; use crate::php_type::PhpType; + use tower_lsp::lsp_types::Url; /// The joined union's member order must not depend on the order the /// call sites were visited in, since that order comes from a @@ -2023,4 +2086,77 @@ mod tests { "joining the same types in reverse order must produce the same union string" ); } + + #[test] + fn headless_view_snapshot_builds_local_candidates() { + let mut backend = Backend::new_test(); + backend.skip_reference_index = true; + let uri = "file:///project/app/Controller.php"; + backend.update_ast(uri, " new Item()]);\n"); + + let snapshot = backend.view_caller_snapshot(); + let candidates = snapshot + .local_candidates + .as_ref() + .and_then(|by_view| by_view.get("shop")) + .expect("headless refresh should index the view caller locally"); + + assert_eq!(candidates.len(), 1); + assert_eq!(snapshot.files[candidates[0]].0, uri); + } + + #[test] + fn headless_inference_skips_non_candidate_callers() { + let dir = tempfile::tempdir().expect("failed to create test workspace"); + let root = dir + .path() + .canonicalize() + .expect("test workspace should canonicalize"); + let views = root.join("resources/views"); + let app = root.join("app"); + std::fs::create_dir_all(&views).expect("failed to create view directory"); + std::fs::create_dir_all(&app).expect("failed to create app directory"); + + let target = views.join("shop.blade.php"); + let matching = app.join("MatchingController.php"); + let unrelated = app.join("UnrelatedController.php"); + let matching_source = " 1]);\n"; + let unrelated_source = " 2]);\n"; + std::fs::write(&target, "").expect("failed to write target view"); + std::fs::write(&matching, matching_source).expect("failed to write matching caller"); + std::fs::write(&unrelated, unrelated_source).expect("failed to write unrelated caller"); + + let mut backend = Backend::new_test_with_workspace(root, Vec::new()); + backend.skip_reference_index = true; + for (path, source) in [(&matching, matching_source), (&unrelated, unrelated_source)] { + let uri = Url::from_file_path(path).expect("caller path should become a file URI"); + backend.update_ast(uri.as_str(), source); + } + + let snapshot = backend.view_caller_snapshot(); + let by_view = snapshot + .local_candidates + .as_ref() + .expect("headless snapshot should carry local candidates"); + assert_eq!(snapshot.files.len(), 2); + assert_eq!(by_view.get("shop").map(Vec::len), Some(1)); + assert_eq!(by_view.get("other").map(Vec::len), Some(1)); + + let target_uri = Url::from_file_path(target).expect("target path should become a file URI"); + assert_eq!( + backend.view_names_for_blade_uri(target_uri.as_str()), + vec!["shop"] + ); + let scope = + backend.compute_blade_injected_vars(target_uri.as_str(), "", Some(&snapshot), None); + + assert!( + scope.vars.iter().any(|(name, _)| name == "kept"), + "matching caller should contribute its data: {scope:?}" + ); + assert!( + scope.vars.iter().all(|(name, _)| name != "excluded"), + "non-candidate caller must be skipped: {scope:?}" + ); + } } diff --git a/src/call_args.rs b/src/call_args.rs index 1f597bd9e..132948a3e 100644 --- a/src/call_args.rs +++ b/src/call_args.rs @@ -141,6 +141,11 @@ fn split_named_text_arg(arg: &str) -> (Option<&str>, &str) { (None, trimmed) } +/// Return an argument's value text with any leading `name:` label removed. +pub(crate) fn text_arg_value(arg: &str) -> &str { + split_named_text_arg(arg).1 +} + /// Bind already-split textual arguments to parameters by PHP's rules. /// /// This is the text-source counterpart of [`bind_args_to_params`], used by @@ -238,6 +243,18 @@ mod tests { } } + #[test] + fn text_arg_value_strips_only_named_argument_labels() { + assert_eq!(text_arg_value("path: null"), "null"); + assert_eq!(text_arg_value("array: ['x:y']"), "['x:y']"); + assert_eq!(text_arg_value("Foo::class"), "Foo::class"); + assert_eq!( + text_arg_value("$flag ? 'a:b' : 'c:d'"), + "$flag ? 'a:b' : 'c:d'" + ); + assert_eq!(text_arg_value("'key: value'"), "'key: value'"); + } + #[test] fn named_arg_filling_optional_leaves_required_missing() { // function f(int $a, int $b = 0, int $c = 0) called as f(c: 3). diff --git a/src/reference_index.rs b/src/reference_index.rs index 43d6f5b25..387806380 100644 --- a/src/reference_index.rs +++ b/src/reference_index.rs @@ -122,7 +122,7 @@ impl Backend { &self, keys: &[ReferenceIndexKey], ) -> Option>> { - if !self.workspace_indexed.load(Ordering::Acquire) { + if self.skip_reference_index || !self.workspace_indexed.load(Ordering::Acquire) { return None; } @@ -604,6 +604,18 @@ mod tests { assert!(candidates.is_none()); } + #[test] + fn candidate_lookup_is_disabled_when_reference_indexing_is_skipped() { + let mut backend = Backend::new_test(); + backend.skip_reference_index = true; + backend.workspace_indexed.store(true, Ordering::Release); + + let candidates = backend + .reference_candidate_uris_for_keys(&[ReferenceIndexKey::Class("App\\Foo".to_string())]); + + assert!(candidates.is_none()); + } + #[test] fn reference_index_populates_candidates_from_parsed_file() { let backend = Backend::new_test(); diff --git a/src/symbol_map/extraction/subject_text.rs b/src/symbol_map/extraction/subject_text.rs index 344f80f40..929bf8252 100644 --- a/src/symbol_map/extraction/subject_text.rs +++ b/src/symbol_map/extraction/subject_text.rs @@ -316,11 +316,14 @@ fn lower_method_call( pub(super) fn format_all_call_args(args: &TokenSeparatedSequence<'_, Argument<'_>>) -> String { let mut parts = Vec::new(); for arg in args.iter() { - let arg_expr = match arg { - Argument::Positional(pos) => pos.value, - Argument::Named(named) => named.value, + let text = match arg { + Argument::Positional(pos) => format_arg_expr(pos.value), + Argument::Named(named) => format!( + "{}: {}", + bytes_to_str(named.name.value), + format_arg_expr(named.value) + ), }; - let text = format_arg_expr(arg_expr); parts.push(text); } // Trim trailing `...` placeholders beyond the first argument so diff --git a/src/type_engine/call_resolution/arg_type_resolution.rs b/src/type_engine/call_resolution/arg_type_resolution.rs index 6b8df61b6..8aec2d98f 100644 --- a/src/type_engine/call_resolution/arg_type_resolution.rs +++ b/src/type_engine/call_resolution/arg_type_resolution.rs @@ -74,30 +74,9 @@ impl<'a, 'ctx> TextArrayFuncArgs<'a, 'ctx> { /// The nth argument's value text, with any `name:` prefix removed /// so that a named argument reads the same as a positional one. fn arg_text(&self, index: usize) -> Option<&'a str> { - self.args.get(index).map(|arg| strip_arg_name(arg.trim())) - } -} - -/// Strip a leading `name:` label from a named argument's source text. -/// -/// Returns the text unchanged when there is no label, taking care not -/// to mistake `Foo::BAR`, `$a ? b : c`, or a colon inside a string -/// literal for one. -fn strip_arg_name(arg: &str) -> &str { - let Some(colon) = arg.find(':') else { - return arg; - }; - if arg[colon + 1..].starts_with(':') { - return arg; - } - let name = arg[..colon].trim_end(); - let is_identifier = !name.is_empty() - && !name.starts_with(|c: char| c.is_ascii_digit()) - && name.chars().all(|c| c.is_alphanumeric() || c == '_'); - if is_identifier { - arg[colon + 1..].trim_start() - } else { - arg + self.args + .get(index) + .map(|arg| crate::call_args::text_arg_value(arg)) } } @@ -179,7 +158,7 @@ impl Backend { ',' if depth == 0 => { let arg = trimmed[..i].trim(); if !arg.is_empty() { - return Some(arg.to_string()); + return Some(crate::call_args::text_arg_value(arg).to_string()); } return None; } @@ -189,7 +168,7 @@ impl Backend { // No top-level comma: the whole text is a single argument. let arg = trimmed.trim(); if !arg.is_empty() { - Some(arg.to_string()) + Some(crate::call_args::text_arg_value(arg).to_string()) } else { None } diff --git a/src/type_engine/call_resolution/return_types.rs b/src/type_engine/call_resolution/return_types.rs index e93279831..18550c6e4 100644 --- a/src/type_engine/call_resolution/return_types.rs +++ b/src/type_engine/call_resolution/return_types.rs @@ -206,11 +206,15 @@ fn resolve_validated_shape_at_call( let call = validated_shape::shape_bearing_method(method_name)?; let receiver = owners.iter().find_map(|rt| rt.class_info.as_ref())?; + let mut args = split_text_args(text_args); + for arg in &mut args { + *arg = crate::call_args::text_arg_value(arg); + } validated_shape::resolve_shape_at_call( receiver, call, - &split_text_args(text_args), + &args, &|| validated_shape::safe_source_class(base, ctx), ctx.content, ctx.cursor_offset, @@ -262,7 +266,8 @@ fn auth_guard_name(base: &SubjectExpr, user_args: &str) -> Option { /// not a single-quoted or double-quoted string literal. fn first_string_literal_arg(args_text: &str) -> Option { let first = split_text_args(args_text).into_iter().next()?; - crate::text_scan::unquote_php_string(first.trim()).map(str::to_string) + crate::text_scan::unquote_php_string(crate::call_args::text_arg_value(first)) + .map(str::to_string) } fn replace_support_carbon_return(ty: &PhpType, configured_class: &str) -> Option { @@ -1011,14 +1016,21 @@ impl Backend { ); // The collapsed conditional is the call's real // type; report it even when it names no class - // (`list`, an array shape), or callers - // that need the type string rather than a + // (`string`, `list`, an array shape), or + // callers that need the type string rather than a // `ClassInfo` fall back to the declared // `array`/`mixed` below. - if let Some(ref mut hint_out) = return_type_hint_out { - **hint_out = Some(parsed_ty); - } - if !classes.is_empty() { + // + // A branch that collapsed to bare `mixed` decided + // nothing, though. The function-level `@template` + // substitution below still binds the parameter's + // default, which is what makes an argument-less + // `app()` its `Application::class` default rather + // than `mixed`, so leave the answer to it. + if !classes.is_empty() || !parsed_ty.is_mixed() { + if let Some(ref mut hint_out) = return_type_hint_out { + **hint_out = Some(parsed_ty); + } return classes; } } @@ -1241,12 +1253,13 @@ impl Backend { if let Some(ctor) = ctor_ref && !ctor.template_bindings.is_empty() { - let arg_texts: Vec = - crate::type_engine::conditional_resolution::split_text_args(text_args) - .into_iter() - .map(|s| s.to_string()) - .collect(); + let arg_texts = + crate::type_engine::conditional_resolution::split_text_args(text_args); if !arg_texts.is_empty() { + let bound_args = crate::call_args::bind_text_args_to_params( + &ctor.parameters, + &arg_texts, + ); let mut subs = std::collections::HashMap::new(); for (tpl_name, param_name) in &ctor.template_bindings { let param_idx = match ctor @@ -1257,10 +1270,11 @@ impl Backend { Some(idx) => idx, None => continue, }; - let arg_text = match arg_texts.get(param_idx) { - Some(text) => text.trim(), - None => continue, - }; + let arg_text = + match bound_args.get(param_idx).and_then(Option::as_deref) { + Some(text) => text, + None => continue, + }; let param_hint = ctor .parameters .get(param_idx) @@ -2623,12 +2637,17 @@ mod cast_tests { #[cfg(test)] mod auth_guard_tests { - use super::{auth_guard_name, first_string_literal_arg, replace_support_carbon_return}; + use super::{ + auth_guard_name, first_string_literal_arg, replace_support_carbon_return, + resolve_validated_shape_at_call, + }; use crate::Backend; use crate::atom::atom; use crate::php_type::PhpType; use crate::test_fixtures::{make_class, make_method}; + use crate::type_engine::resolver::ResolutionCtx; use crate::type_engine::subject_expr::SubjectExpr; + use crate::types::ResolvedType; use std::sync::Arc; #[test] @@ -2655,6 +2674,45 @@ mod auth_guard_tests { assert_eq!(first_string_literal_arg("GUARD_NAME"), None); } + #[test] + fn named_validate_rules_are_normalized_before_shape_resolution() { + let mut request = make_class("Request"); + request.file_namespace = Some(atom("Illuminate\\Http")); + let request = Arc::new(request); + let classes = vec![Arc::clone(&request)]; + let class_loader = |name: &str| { + (name.trim_start_matches('\\') == "Illuminate\\Http\\Request") + .then(|| Arc::clone(&request)) + }; + assert!(class_loader("\\Illuminate\\Http\\Request").is_some()); + let ctx = ResolutionCtx { + current_class: None, + all_classes: &classes, + content: "", + cursor_offset: 0, + class_loader: &class_loader, + backend: None, + laravel_macro_this_resolver: None, + resolved_class_cache: None, + function_loader: None, + scope_var_resolver: None, + is_in_static_method: false, + preserve_static: false, + }; + let owners = vec![ResolvedType::from_arc(Arc::clone(&request))]; + + let shape = resolve_validated_shape_at_call( + &SubjectExpr::parse("$request"), + "validate", + "rules: ['title' => 'required|string']", + &owners, + &ctx, + ) + .expect("the named rules argument should produce a validated shape"); + + assert_eq!(shape.to_string(), "array{title: string}"); + } + #[test] fn replaces_support_carbon_inside_nullable_union() { assert_eq!( diff --git a/src/type_engine/resolver/mod.rs b/src/type_engine/resolver/mod.rs index 2b4966682..890a695f3 100644 --- a/src/type_engine/resolver/mod.rs +++ b/src/type_engine/resolver/mod.rs @@ -607,14 +607,12 @@ fn resolve_target_classes_expr_inner( if classes.iter().any(|c| !c.template_params.is_empty()) || hint_names_a_class { return ResolvedType::from_classes_with_hint(classes, h); } - // `object`/`?object` is the "any object" escape hatch: no - // concrete class matches it, but the base chain and method - // lookup above have already determined that. Surface it as - // a type-string-only candidate instead of dropping it, so - // callers that need to detect a bare `object` return (e.g. - // `resolve_subject_outcome`'s stdClass synthesis) don't have - // to re-resolve the callee's base chain a second time. - if classes.is_empty() && h.is_object() { + // A scalar conditional branch (and the `object`/`?object` + // escape hatch) has no concrete class to carry it. Surface + // it as a type-string-only candidate instead of dropping it, + // so every consumer sees the branch selected at this call + // site rather than re-reading the callable's broader union. + if classes.is_empty() && (h.is_object() || h.all_members_primitive_scalar()) { return vec![ResolvedType::from_type_string(h)]; } } diff --git a/src/virtual_members/laravel/factory_count.rs b/src/virtual_members/laravel/factory_count.rs index ce365498f..7505265dd 100644 --- a/src/virtual_members/laravel/factory_count.rs +++ b/src/virtual_members/laravel/factory_count.rs @@ -68,9 +68,10 @@ pub(crate) fn chain_count(receiver: &SubjectExpr) -> FactoryCount { } else { None }; - let first_arg = args - .as_ref() - .and_then(|args| args.first().map(|arg| arg.trim())); + let first_arg = args.as_ref().and_then(|args| { + args.first() + .map(|arg| crate::call_args::text_arg_value(arg)) + }); if let Some(state) = instance_count_state(method, first_arg, &|| None) { return state; } @@ -84,9 +85,10 @@ pub(crate) fn chain_count(receiver: &SubjectExpr) -> FactoryCount { } else { None }; - let first_arg = args - .as_ref() - .and_then(|args| args.first().map(|arg| arg.trim())); + let first_arg = args.as_ref().and_then(|args| { + args.first() + .map(|arg| crate::call_args::text_arg_value(arg)) + }); return static_count_state(method, first_arg, &|| None); } _ => return FactoryCount::Unknown, diff --git a/tests/integration/completion_generics.rs b/tests/integration/completion_generics.rs index 441d377c3..d6bceb98a 100644 --- a/tests/integration/completion_generics.rs +++ b/tests/integration/completion_generics.rs @@ -9080,6 +9080,82 @@ async fn test_chained_new_expression_propagates_constructor_generics() { } } +#[tokio::test] +async fn test_named_constructor_argument_skips_an_omitted_template_binding() { + let backend = create_test_backend(); + let uri = Url::parse("file:///named_constructor_generic.php").unwrap(); + let marked = r#"get()->§ +} +"#; + let cursor = marked.find('§').expect("test source needs a cursor"); + let before = &marked[..cursor]; + let position = Position { + line: before.matches('\n').count() as u32, + character: before.rsplit('\n').next().unwrap_or("").chars().count() as u32, + }; + let text = marked.replace('§', ""); + + backend + .did_open(DidOpenTextDocumentParams { + text_document: TextDocumentItem { + uri: uri.clone(), + language_id: "php".to_string(), + version: 1, + text, + }, + }) + .await; + + let result = backend + .completion(CompletionParams { + text_document_position: TextDocumentPositionParams { + text_document: TextDocumentIdentifier { uri }, + position, + }, + work_done_progress_params: WorkDoneProgressParams::default(), + partial_result_params: PartialResultParams::default(), + context: None, + }) + .await + .unwrap(); + let items = match result { + Some(CompletionResponse::Array(items)) => items, + Some(CompletionResponse::List(list)) => list.items, + None => Vec::new(), + }; + let method_names: Vec<&str> = items + .iter() + .filter(|item| item.kind == Some(CompletionItemKind::METHOD)) + .map(|item| item.filter_text.as_deref().unwrap_or(&item.label)) + .collect(); + + assert!( + method_names.contains(&"productOnly"), + "the named value argument should bind TValue despite the omitted first parameter, got: {method_names:?}" + ); +} + /// Chained instantiation with a string literal: `(new Box("hello"))->get()` /// should infer T=string so that `get()` returns `string`. #[tokio::test] diff --git a/tests/integration/diagnostics_return_type_errors.rs b/tests/integration/diagnostics_return_type_errors.rs index 00771afbc..2eedf721a 100644 --- a/tests/integration/diagnostics_return_type_errors.rs +++ b/tests/integration/diagnostics_return_type_errors.rs @@ -3347,6 +3347,38 @@ fn argument_less_view_helper_still_resolves_to_the_factory() { ); } +#[test] +fn scalar_conditional_function_return_survives_template_binding() { + // A templated wrapper binds its return from the shared text-based call + // resolver. The selected `string` branch must not be replaced by the + // loadable class from the helper's broader native union. + let php = r#"current(); + url(null)->current(); + url(path: null)->current(); + url(parameters: [])->current(); + url(secure: true, parameters: [])->current(); + + // Every non-null path form returns a string, so member access is invalid. + url('/login')->current(); + url(path: '/login')->current(); + url(parameters: [], path: '/login')->current(); + url('/users/' . $id)->current(); + identity(url('/login'))->current(); + + // The assignment path uses the AST/RHS resolver and must agree. + $login = url('/login'); + $login->current(); +} +"#; + let diags = unknown_member_diagnostics(&backend, uri, php); + let scalar_diags: Vec<_> = diags + .iter() + .filter(|diag| { + diag.code.as_ref().is_some_and( + |code| matches!(code, NumberOrString::String(s) if s == "scalar_member_access"), + ) + }) + .collect(); + assert_eq!( + scalar_diags.len(), + 6, + "only non-null path results should be strings, got: {:?}", + diags + ); + assert!( + scalar_diags + .iter() + .all(|diag| diag.message.contains("string") && diag.message.contains("current")), + "each invalid access should identify the selected string branch, got: {:?}", + scalar_diags + ); + assert_eq!( + diags.len(), + scalar_diags.len(), + "generator results should resolve without unknown or unresolved member diagnostics, got: {diags:?}" + ); +} + +/// A `class-string` parameter with a class name for its default states the +/// return type of the argument-less call, and the conditional such a +/// signature reads as answers `mixed` for that call. The default is the more +/// precise of the two, so a branch that collapses to bare `mixed` must not +/// stand in its way. +#[test] +fn a_class_string_helpers_default_types_its_argument_less_call() { + let backend = create_test_backend(); + { + let mut cfg = backend.config(); + cfg.diagnostics.unresolved_member_access = Some(true); + backend.set_config(cfg); + } + let uri = "file:///test.php"; + let php = r#" $name + * @return T + */ +function app(string $name = Application::class): object {} + +function test(): void { + app()->basePath('/tmp'); + app(Mailer::class)->send(); +} +"#; + let diags = unknown_member_diagnostics(&backend, uri, php); + assert!( + diags.is_empty(), + "the parameter default and an explicit class-string should both resolve, got: {diags:?}" + ); +} + // ─── Issue #168: instanceof narrowing must not leak into elseif body ──────── #[test]