Skip to content

Commit 15a0514

Browse files
committed
Fix flaky resolve on shadowed classes
1 parent 9e22d06 commit 15a0514

7 files changed

Lines changed: 142 additions & 6 deletions

File tree

docs/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4242

4343
- **Facade static calls keep concrete method return types.** Static calls on Laravel-style facades now resolve missing methods through `getFacadeAccessor()` and facade `@mixin` targets before falling back to `__callStatic()`, so values like `Driver::details()` keep the concrete provider method return type instead of degrading to the facade's broad magic-call return. Contributed by @calebdw.
4444
- **`class-string<static>` parameters no longer reject sibling subclass constants.** Static helper calls from a shared base class that pass concrete `::class` constants for sibling subclasses no longer report false argument-type mismatches against `class-string<static>`. Contributed by @calebdw.
45+
- **Built-in PHP classes shadowed by vendor polyfills resolve to the real definition.** When an installed package ships a polyfill for a PHP built-in (for example symfony/polyfill-php84's `RoundingMode`), resolution sometimes picked the polyfill's legacy pre-enum declaration instead of the built-in, turning enum cases into plain int constants and reporting false "expects RoundingMode, got int" argument mismatches. Which declaration won could change from one run to the next, making whole-project analysis results nondeterministic. Global names of built-in classes now always resolve to the bundled PHP definition, and classes discovered inside phar archives are indexed in a stable order.
4546
- **Member name positions no longer suggest classes.** Typing a name after `function`, `const`, or enum `case` (for example `protected function getC`) no longer offers unrelated class names from the project. Property names were already safe because they start with `$`. Contributed by @calebdw.
4647
- **Null-initialized variables reassigned in an untyped foreach are not stuck as `null`.** When the iterable has no known element type (for example an untyped parameter), the loop value is now treated as `mixed`, so `$x = $value` after `$x = null` participates in post-loop merge and `is_null` early-return narrowing instead of leaving a false `null` type at later call sites. Contributed by @calebdw.
4748
- **By-reference method out-parameters no longer flag undefined variables.** Passing an undeclared variable into a by-ref parameter (e.g. `new A()->dosmth($y, $foo)` where `$foo` is `&$x`) is valid PHP and now defines the variable for later use, matching free functions like `preg_match(..., $matches)` and `$this->method($out)`. Contributed by @calebdw.

docs/todo.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ unlikely to move the needle for most users.
106106
| D15 | [Unused parameter diagnostic](todo/diagnostics.md#d15-unused-parameter-diagnostic) | Low | Low |
107107
| | **[Bug Fixes](todo/bugs.md)** | | |
108108
| B1 | [`resolve_function_name` guesses a single namespace, missing same-file multi-namespace declarations](todo/bugs.md#b1-resolve_function_name-guesses-a-single-namespace-missing-same-file-multi-namespace-declarations) | Low-Medium | Medium |
109+
| B2 | [`self::`/`static::` inside macro closures resolve to the enclosing class, not the macro target](todo/bugs.md#b2-selfstatic-inside-macro-closures-resolve-to-the-enclosing-class-not-the-macro-target) | Medium | Medium |
109110
| | **[Code Actions](todo/actions.md)** | | |
110111
| A40 | [Generate method from call](todo/actions.md#a40-generate-method-from-call) | Medium-High | Medium |
111112
| A41 | [Create class from non-existing name](todo/actions.md#a41-create-class-from-non-existing-name) | Medium | Medium |

docs/todo/bugs.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,32 @@ Fixing this requires either threading the call expression's byte
4040
offset through to `resolve_function_name` or adding an offset-aware
4141
variant of the loader closure. Size the fix accordingly — this is not
4242
a one-line change.
43+
44+
## B2. `self::`/`static::` inside macro closures resolve to the enclosing class, not the macro target
45+
46+
Inside a closure passed to `Target::macro(...)` (Laravel `Macroable`
47+
or Carbon), `$this` correctly resolves to the macro target class via
48+
`laravel_macro_this_resolver` (`closure_this_from_static_receiver` in
49+
`src/completion/variable/closure_resolution.rs`). But `self::` and
50+
`static::` inside the same closure still resolve to the class that
51+
lexically encloses the registration (e.g. the service provider), so
52+
Carbon's static macro idiom
53+
54+
```php
55+
CarbonImmutable::macro('diffFromYear', function (int $year): string {
56+
return self::this()->diffForHumans(...);
57+
});
58+
```
59+
60+
reports a false `Method 'this' not found on class
61+
'App\Providers\DemoServiceProvider'` (`unknown_member`). At runtime
62+
both `Macroable::__call`/`__callStatic` and Carbon bind the closure
63+
with the target as scope, so `self`/`static` refer to the target and
64+
protected members like `Carbon\Traits\Mixin::this()` are accessible.
65+
66+
Subject resolution for `Self_`/`Static` needs the same "am I inside a
67+
macro registration closure?" awareness that `$this` resolution already
68+
has. The `self::this()` demo in
69+
`examples/laravel/app/Providers/DemoServiceProvider.php` was switched
70+
to the supported `$this->` form when this was discovered; restore the
71+
static idiom there once fixed.

examples/laravel/app/Providers/DemoServiceProvider.php

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@ public function boot(): void
2323
// PHPantom recovers those from CollectionMixin's source.
2424
Collection::mixin(new CollectionMixin());
2525

26-
// Carbon supports the same `macro()` pattern as Laravel's Macroable:
26+
// Carbon supports the same `macro()` pattern as Laravel's Macroable.
27+
// `$this` inside the closure is the CarbonImmutable instance the
28+
// macro is called on:
2729
CarbonImmutable::macro('diffFromYear', function (int $year, bool $absolute = false): string {
28-
return self::this()->diffForHumans(
30+
return $this->diffForHumans(
2931
CarbonImmutable::create($year, 1, 1),
3032
['syntax' => \Carbon\CarbonInterface::DIFF_ABSOLUTE]
3133
);

src/phar.rs

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@ pub(crate) struct PharArchive {
2323
data: Vec<u8>,
2424
/// Map of internal file path → (offset into `data`, uncompressed size).
2525
files: HashMap<String, (usize, usize)>,
26+
/// Internal file paths in manifest order. Iteration must be
27+
/// deterministic: consumers register discovered classes
28+
/// first-wins, so a randomized `HashMap` order would make it
29+
/// nondeterministic which file resolves a class name that is
30+
/// declared in several files of the archive (e.g. the enum and
31+
/// legacy-class variants of a symfony polyfill).
32+
paths: Vec<String>,
2633
}
2734

2835
impl PharArchive {
@@ -115,6 +122,7 @@ impl PharArchive {
115122
let data_area_start = manifest_end;
116123

117124
let mut files = HashMap::with_capacity(file_count);
125+
let mut paths = Vec::with_capacity(file_count);
118126
let mut offset = data_area_start;
119127

120128
for entry in &entries {
@@ -123,12 +131,17 @@ impl PharArchive {
123131
if offset + entry.compressed_size > data.len() {
124132
return None;
125133
}
126-
files.insert(entry.filename.clone(), (offset, entry.uncompressed_size));
134+
if files
135+
.insert(entry.filename.clone(), (offset, entry.uncompressed_size))
136+
.is_none()
137+
{
138+
paths.push(entry.filename.clone());
139+
}
127140
}
128141
offset += entry.compressed_size;
129142
}
130143

131-
Some(Self { data, files })
144+
Some(Self { data, files, paths })
132145
}
133146

134147
/// Extract the content of a file inside the phar.
@@ -137,9 +150,9 @@ impl PharArchive {
137150
self.data.get(offset..offset + size)
138151
}
139152

140-
/// Iterate over all file paths in the archive.
153+
/// Iterate over all file paths in the archive, in manifest order.
141154
pub fn file_paths(&self) -> impl Iterator<Item = &str> {
142-
self.files.keys().map(String::as_str)
155+
self.paths.iter().map(String::as_str)
143156
}
144157
}
145158

@@ -295,6 +308,20 @@ mod tests {
295308
assert_eq!(paths, vec!["a.php", "b.php", "c.php"]);
296309
}
297310

311+
#[test]
312+
fn file_paths_preserves_manifest_order() {
313+
let phar_bytes = build_test_phar(&[
314+
("zeta.php", b"<?php // z"),
315+
("alpha.php", b"<?php // a"),
316+
("mid.php", b"<?php // m"),
317+
]);
318+
319+
let archive = PharArchive::parse(phar_bytes).expect("should parse");
320+
321+
let paths: Vec<&str> = archive.file_paths().collect();
322+
assert_eq!(paths, vec!["zeta.php", "alpha.php", "mid.php"]);
323+
}
324+
298325
#[test]
299326
fn parse_returns_none_for_garbage() {
300327
assert!(PharArchive::parse(vec![0, 1, 2, 3]).is_none());

src/resolution.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,26 @@ impl Backend {
321321
return Some(cls);
322322
}
323323

324+
// ── Phase 0.5: built-in names resolve to the embedded stubs ──
325+
// A global-namespace class name that exists in the embedded
326+
// stub index is a PHP built-in (core or bundled extension).
327+
// Vendor code can only redeclare such a name inside a
328+
// conditional block: a polyfill mirroring the built-in for
329+
// older runtimes. symfony/polyfill-php84, for example, ships
330+
// two `RoundingMode` declarations — an enum and a pre-enum
331+
// class whose "cases" are plain int constants. Resolving the
332+
// name through the classmap (Phase 1) would pick whichever
333+
// polyfill file happened to be indexed first, which is
334+
// nondeterministic when several files declare the name and
335+
// wrong whenever a legacy variant wins (enum cases would
336+
// resolve to int constants). The stub is the authoritative
337+
// definition of a built-in, so it wins over classmap entries.
338+
if expected_ns.is_none()
339+
&& let Some(cls) = self.load_stub_class(last_segment)
340+
{
341+
return Some(cls);
342+
}
343+
324344
// ── Phase 1: Try the fqn_uri_index (FQN → URI) ───────────
325345
// The fqn_uri_index is populated by `scan_autoload_files` (Composer
326346
// `autoload_files.php` entries and their `require_once` chains),

tests/integration/diagnostics_type_errors.rs

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6317,3 +6317,59 @@ function test(): void {
63176317
"A string literal value in a list that is NOT a property should be flagged"
63186318
);
63196319
}
6320+
6321+
// ─── Built-in names shadowed by vendor polyfills ─────────────────────────────
6322+
6323+
/// A vendor polyfill (e.g. symfony/polyfill-php84) may declare a legacy
6324+
/// `final class RoundingMode` whose "cases" are plain int constants,
6325+
/// registered in the classmap alongside the real built-in enum. The
6326+
/// embedded stub must win: `RoundingMode::HalfAwayFromZero` passed to a
6327+
/// `RoundingMode` parameter is an enum case, not an int.
6328+
#[test]
6329+
fn builtin_enum_case_prefers_stub_over_polyfill_classmap_entry() {
6330+
let backend = create_test_backend_with_full_stubs();
6331+
6332+
// Simulate the polyfill's legacy-class variant on disk, registered
6333+
// in the classmap under the built-in's global name.
6334+
let dir = tempfile::tempdir().expect("failed to create temp dir");
6335+
let polyfill = dir.path().join("RoundingMode.php");
6336+
std::fs::write(
6337+
&polyfill,
6338+
concat!(
6339+
"<?php\n",
6340+
"if (\\PHP_VERSION_ID < 80100) {\n",
6341+
" final class RoundingMode\n",
6342+
" {\n",
6343+
" const HalfAwayFromZero = 0;\n",
6344+
" const HalfTowardsZero = 1;\n",
6345+
" const HalfEven = 2;\n",
6346+
" }\n",
6347+
"}\n",
6348+
),
6349+
)
6350+
.expect("failed to write polyfill file");
6351+
{
6352+
let mut idx = backend.fqn_uri_index().write();
6353+
idx.insert(
6354+
"RoundingMode".to_string(),
6355+
Url::from_file_path(&polyfill).unwrap().to_string(),
6356+
);
6357+
}
6358+
6359+
let php = r#"<?php
6360+
function takes_mode(RoundingMode $mode): void {}
6361+
6362+
function test(): void {
6363+
takes_mode(RoundingMode::HalfAwayFromZero);
6364+
}
6365+
"#;
6366+
let uri = "file:///test.php";
6367+
backend.update_ast(uri, php);
6368+
let mut out = Vec::new();
6369+
backend.collect_argument_type_diagnostics(uri, php, &mut out);
6370+
assert!(
6371+
!has_type_error(&out),
6372+
"Enum case of a built-in must resolve to the enum, not the polyfill's int constant: {:?}",
6373+
type_error_messages(&out)
6374+
);
6375+
}

0 commit comments

Comments
 (0)