Skip to content

Commit 7d4231b

Browse files
AJenboAJenbo
committed
Update roadmap
1 parent fe04dc7 commit 7d4231b

6 files changed

Lines changed: 171 additions & 211 deletions

File tree

‎docs/ARCHITECTURE.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -869,6 +869,12 @@ The `symbol_maps` store is keyed by file URI, matching `ast_map`. A typical PHP
869869

870870
Files that are not open (vendor files loaded via PSR-4 on demand) do not get a symbol map — they use the stored byte offsets from Tier 2 (which live on `ClassInfo` / `MethodInfo` / etc. in `ast_map`).
871871

872+
## Diagnostic Worker Architecture
873+
874+
Diagnostics run in a background `tokio::spawn` task so they never block completion, hover, or signature help. The worker is created during `initialized` via `clone_for_diagnostic_worker`, which builds a shallow clone of the `Backend`. All `Arc`-wrapped fields (maps, caches, the notify/pending slot) are shared by `Arc::clone`, so the worker sees every mutation the main `Backend` makes.
875+
876+
Non-`Arc` fields are snapshotted at spawn time: `php_version`, `vendor_uri_prefixes`, `vendor_dir_paths`, and `config`. These fields are only written during `initialized` (before the worker is spawned) and never change afterwards. If a future feature adds hot-reloading of `.phpantom.toml` or runtime PHP version changes, the worker would need to be notified or re-cloned. This invariant ("init-time fields are write-once") should be verified before adding any post-init mutation to these fields.
877+
872878
## Name Resolution
873879

874880
PHP class names go through resolution at parse time (`resolve_parent_class_names`):

‎docs/CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5454

5555
### Fixed
5656

57+
- **Type alias array shape diagnostics no longer fire on object values.** When a method returns a `@phpstan-type` alias that expands to an array shape containing object values (e.g. `array{pen: Pen}`), accessing a method on the object value no longer triggers a false diagnostic. Type aliases are now expanded before walking array shape segments in the resolution pipeline.
58+
- **Inline array-element function calls resolve correctly in diagnostics.** `end($obj->items)->method()` no longer produces a false "unknown member" diagnostic. Previously the argument text for property access expressions was lost during symbol map extraction, causing the resolver to fall back to the native `mixed|false` return type instead of extracting the element type from the array's generic annotation.
59+
- **Eloquent Builder scope chain diagnostics no longer flicker.** Scope methods on Builder chains (e.g. `Bakery::where(...)->fresh()->get()`) no longer produce false diagnostics that flip on and off between edits. The diagnostic now checks the pre-resolved class returned by the completion pipeline before re-resolving through the cache, preventing stale cache entries from hiding context-specific virtual members.
5760
- **Diagnostics refresh across open files when a class signature changes.** Editing a class (adding a method, changing a relation, modifying `$casts`, renaming a property) now triggers a diagnostic re-check in all other open files. Previously only the edited file was re-diagnosed, so stale "unknown member" or "unknown class" warnings in other tabs persisted until those files were manually re-opened or edited.
5861
- **Unknown member diagnostics on property chains and method return chains.** `$obj->prop->member` and `$obj->getX()->member` now flag unknown members on the resolved intermediate type. Previously the diagnostic was silently skipped because the subject resolver could not handle chained expressions. Virtual properties from `@property` annotations are also resolved through the chain. When the intermediate type is scalar (e.g. `$model->boolProp->value`), a "Cannot access property on type 'bool'" diagnostic is emitted instead of silently skipping.
5962
- **Variable type resolves through ternary, elvis, null-coalesce, and match assignments.** When a variable is reassigned via `$x = $x ?: Foo::bar()`, `$x = $a ?? $b`, `$x = $cond ? $a : $b`, or `$x = match(...) { ... }`, hover and foreach iteration now show the resolved type from the assignment branches. Previously these expressions returned unknown because the raw type inference path did not recurse into conditional expressions.

‎docs/todo.md‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,12 @@ with each step.
3434

3535
## Sprint 2 — Stabilise for 0.5.0 release
3636

37-
All Sprint 2+ feature work is done. Before tagging 0.5.0, fix the
38-
remaining bugs that would embarrass us in front of new users. No new
39-
features, no refactors, no "while I'm here" detours. Ship what we
40-
have, ship it correct.
41-
42-
The common thread: our own diagnostics contradict our own demos in
43-
`example.php`. If PHPantom flags working code in its own showcase
44-
file, users will not trust any diagnostic it produces.
45-
4637
| # | Item | Effort | Domain | Doc Link |
4738
|---|---|---|---|---|
48-
| 106 | Diagnostics fire on type alias array shape object values | Low | Bug Fixes | [bugs.md §1](todo/bugs.md#1-diagnostics-fire-on-type-alias-array-shape-object-values) |
49-
| 107 | Inline array-element function calls resolve to native return type in diagnostics | Low | Bug Fixes | [bugs.md §2](todo/bugs.md#2-inline-array-element-function-calls-resolve-to-native-return-type-in-diagnostics) |
50-
| 108 | Flaky `unknown_member` diagnostic on Eloquent Builder scope chains | Medium | Bug Fixes | [bugs.md §3](todo/bugs.md#3-flaky-unknown_member-diagnostic-on-eloquent-builder-scope-chains) |
39+
| 106 | Thread panic in parallel file scanners crashes server | Low | Bug Fixes | [bugs.md §0](todo/bugs.md#0-thread-panic-in-parallel-file-scanners-crashes-the-server) |
40+
| 107 | Analyse sequential write-lock acquisition in `parse_and_cache_content_versioned` | Low | Bug Fixes | [bugs.md §1](todo/bugs.md#1-sequential-write-lock-acquisition-in-parse_and_cache_content_versioned) |
5141

52-
**Release gate:** `cargo test`, `cargo clippy -- -D warnings`,
53-
`cargo clippy --tests -- -D warnings`, `cargo fmt --check`,
54-
`php -l example.php`, and `php -d zend.assertions=1 example.php`
55-
all pass. Zero diagnostics on `example.php` from PHPantom itself.
56-
Tag 0.5.0, write release notes, publish.
42+
**After Sprint 2:** Tag 0.5.0, write release notes, publish.
5743

5844
---
5945

@@ -278,6 +264,9 @@ eventually but don't move the needle.
278264
| 96 | Parallel pre-filter in `find_implementors` | Medium | Performance | [performance.md §9](todo/performance.md#9-parallel-pre-filter-in-find_implementors) |
279265
| 97 | `memmem` for block comment terminator search | Low | Performance | [performance.md §10](todo/performance.md#10-memmem-for-block-comment-terminator-search) |
280266
| 98 | `memmap2` for file reads during scanning | Low | Performance | [performance.md §11](todo/performance.md#11-memmap2-for-file-reads-during-scanning) |
267+
| 108 | O(n²) transitive eviction in `evict_fqn` | Low | Performance | [performance.md §12](todo/performance.md#12-on²-transitive-eviction-in-evict_fqn) |
268+
| 109 | `diag_pending_uris` uses `Vec::contains` for dedup | Low | Performance | [performance.md §13](todo/performance.md#13-diag_pending_uris-uses-veccontains-for-deduplication) |
269+
| 110 | `find_class_in_ast_map` linear fallback scan | Low | Performance | [performance.md §14](todo/performance.md#14-find_class_in_ast_map-linear-fallback-scan) |
281270

282271
### External stubs
283272

‎docs/todo/bugs.md‎

Lines changed: 65 additions & 194 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,71 @@ Items are ordered by **impact** (descending), then **effort** (ascending).
1414

1515
---
1616

17-
## 0. Native type hints not considered in virtual property specificity ranking
17+
## 0. Thread panic in parallel file scanners crashes the server
18+
19+
**Impact: High — Effort: Low**
20+
21+
The three `scan_files_parallel_*` functions in `classmap_scanner.rs`
22+
join spawned threads with `.join().unwrap()`. If any spawned thread
23+
panics (e.g. due to a malformed file encountered during the byte-level
24+
scan), the `unwrap()` propagates the panic to the caller. Because
25+
these functions run during server initialization (outside any
26+
`catch_unwind` guard), a single thread panic kills the entire LSP
27+
process. The user's editor silently loses PHP intelligence with no
28+
error message.
29+
30+
The `scan_content`, `find_classes`, and `find_symbols` functions
31+
process arbitrary user and vendor files from disk. While they are
32+
designed to be robust, a panic in any of them (or in future code
33+
changes) would be fatal.
34+
35+
### Fix
36+
37+
Replace `.join().unwrap()` with `.join().unwrap_or_default()` (or
38+
`.join().ok()` with appropriate flattening) and log an error for
39+
any thread that panicked. The affected scan results are simply
40+
skipped, matching the existing behaviour of `fs::read` failures
41+
which are silently ignored.
42+
43+
Three call sites:
44+
- `scan_files_parallel_classes` (L407)
45+
- `scan_files_parallel_psr4` (L467)
46+
- `scan_files_parallel_full` (L537)
47+
48+
---
49+
50+
## 1. Sequential write-lock acquisition in `parse_and_cache_content_versioned`
51+
52+
**Impact: Low-Medium — Effort: Low**
53+
54+
`parse_and_cache_content_versioned` in `resolution.rs` acquires write
55+
locks on `ast_map`, `use_map`, `namespace_map`, `fqn_index`, and
56+
`resolved_class_cache` in sequence. Each lock is acquired and released
57+
individually (not nested), so there is no deadlock risk with the
58+
current `parking_lot` implementation. However, holding one write lock
59+
while acquiring the next creates brief windows where readers of the
60+
earlier-released lock see updated data but readers of the
61+
not-yet-written lock see stale data.
62+
63+
In practice this is unlikely to cause user-visible issues because
64+
all five writes complete within microseconds and all consumers
65+
re-read the maps on each request. But the pattern is fragile: if a
66+
future change adds a reader that checks two of these maps for
67+
consistency within the same request, it could observe a partial
68+
update.
69+
70+
### Analysis needed
71+
72+
Audit the code paths that read multiple maps in a single request
73+
to confirm no consumer relies on cross-map consistency. If none
74+
does, document the invariant ("these maps are eventually consistent
75+
within a single `update_ast` call but not atomically consistent")
76+
and close the item. If a consumer is found, batch the writes under
77+
a single coordination mechanism.
78+
79+
---
80+
81+
## 2. Native type hints not considered in virtual property specificity ranking
1882

1983
**Impact: Low-Medium — Effort: Medium**
2084

@@ -37,196 +101,3 @@ because the native vs docblock decision happens upstream in the parser.
37101

38102
This is out of scope for the virtual member specificity work but worth
39103
tracking as a separate improvement to `resolve_effective_type`.
40-
41-
---
42-
43-
## 1. Diagnostics fire on type alias array shape object values
44-
45-
**Impact: High — Effort: Low**
46-
47-
**Release blocker for 0.5.0.** Our own `example.php` shows the bug.
48-
49-
When a method returns a `@phpstan-type` alias that expands to an array
50-
shape containing object values (e.g. `array{name: string, pen: Pen}`),
51-
accessing a method on the object value triggers a false
52-
`unresolved_member_access` diagnostic:
53-
54-
```php
55-
/** @phpstan-type UserData array{name: string, email: string, pen: Pen} */
56-
class TypeAliasDemo {
57-
/** @return UserData */
58-
public function getUserData(): array { /* … */ }
59-
60-
public function demo(): void {
61-
$data = $this->getUserData();
62-
$data['pen']->write(); // ← diagnostic: "Cannot resolve type of '$data['pen']'"
63-
}
64-
}
65-
```
66-
67-
Completion works correctly here (offering `Pen` methods after
68-
`$data['pen']->`), so the type resolution pipeline knows the type.
69-
The diagnostic provider is not reaching the same conclusion. The
70-
likely cause is that the diagnostic's subject resolution does not
71-
expand `@phpstan-type` aliases before checking array shape value
72-
types.
73-
74-
**Reproduces in:** `example.php` lines 637–641 (`TypeAliasDemo`),
75-
both `$data['pen']->write()` and `$status['owner']->getEmail()`.
76-
77-
### Where to start
78-
79-
- **Diagnostic entry point:**
80-
`src/diagnostics/unresolved_member_access.rs` →
81-
`collect_unresolved_member_access_diagnostics`. This is the function
82-
that emits the `unresolved_member_access` diagnostic. Trace how it
83-
resolves the subject type for a `$var['key']->method()` expression.
84-
- **Working completion path:**
85-
`src/completion/array_shape.rs` → `build_array_key_completions`
86-
calls `resolve_type_alias` before extracting shape value types.
87-
The diagnostic path likely skips this alias expansion step.
88-
- **Type alias resolution:**
89-
`src/completion/types/resolution.rs` → `resolve_type_alias`.
90-
This is the function that expands `@phpstan-type` names into their
91-
definitions. The fix is probably a matter of calling it in the
92-
diagnostic's subject resolution path before checking array shape
93-
value types.
94-
- **Type alias storage:** `ClassInfo.type_aliases` in `src/types.rs`.
95-
Populated by `src/docblock/tags.rs` during parsing.
96-
97-
---
98-
99-
## 2. Inline array-element function calls resolve to native return type in diagnostics
100-
101-
**Impact: High — Effort: Low**
102-
103-
**Release blocker for 0.5.0.** Our own `example.php` shows the bug.
104-
105-
When an array-element function (`end()`, `current()`, `reset()`, etc.)
106-
is called inline as the subject of a member access, the diagnostic
107-
resolver falls back to the function's native PHP return type
108-
(`mixed|false`) instead of extracting the element type from the
109-
array argument's generic annotation:
110-
111-
```php
112-
$src = new ScaffoldingArrayFunc(); // has members: array<int, Pen>
113-
114-
$cur = current($src->members);
115-
$cur->write(); // ✓ no diagnostic (variable assignment path works)
116-
117-
end($src->members)->write(); // ✗ diagnostic: "subject type 'mixed|false'"
118-
```
119-
120-
The completion pipeline handles this correctly via
121-
`resolve_call_return_types_expr` → `ARRAY_ELEMENT_FUNCS` →
122-
`resolve_inline_arg_raw_type`. The diagnostic sees the same subject
123-
text but apparently resolves the call's return type from the stub
124-
signature rather than the generic element type.
125-
126-
**Reproduces in:** `example.php` line 1060
127-
(`end($src->members)->write()`).
128-
129-
### Where to start
130-
131-
- **Diagnostic entry point:**
132-
`src/diagnostics/unknown_members.rs` →
133-
`collect_unknown_member_diagnostics`. This emits the
134-
`unknown_member` diagnostic. It calls `resolve_target_classes` to
135-
resolve the subject. Add a log or breakpoint here to see what
136-
subject text the diagnostic sees for the `end(…)->write()` span.
137-
- **Subject extraction:**
138-
`src/subject_extraction.rs` extracts the text left of `->`. For
139-
`end($src->members)->write()`, the subject should be
140-
`end($src->members)`. Verify this is what reaches the resolver.
141-
- **Working completion path:**
142-
`src/completion/call_resolution.rs` →
143-
`resolve_call_return_types_expr`, specifically the
144-
`SubjectExpr::FunctionCall` arm around line 407. It checks
145-
`ARRAY_ELEMENT_FUNCS`, calls `resolve_inline_arg_raw_type` to get
146-
the array's generic type, then extracts the element type via
147-
`extract_generic_value_type`. The diagnostic must be taking a
148-
different path that skips this logic and falls through to the
149-
stub's native `mixed|false` return type.
150-
- **Array element function list:**
151-
`src/completion/variable/mod.rs` → `ARRAY_ELEMENT_FUNCS`. This is
152-
the list of functions (`end`, `current`, `reset`, etc.) that should
153-
resolve to the array's element type rather than their native return.
154-
155-
---
156-
157-
## 3. Flaky `unknown_member` diagnostic on Eloquent Builder scope chains
158-
159-
**Impact: High — Effort: Medium**
160-
161-
**Release blocker for 0.5.0.** Our own `example.php` shows the bug.
162-
163-
Eloquent scope methods on Builder chains produce false
164-
`unknown_member` diagnostics that flip on and off based on cache
165-
state:
166-
167-
```php
168-
Bakery::where('open', true)->fresh()->get();
169-
// ← flags: Method 'fresh' not found on 'Illuminate\Database\Eloquent\Builder'
170-
// ← retyping `>` as `>` (a no-op edit) flips the diagnostic away
171-
// ← retyping again brings it back
172-
```
173-
174-
A no-op edit triggering `didChange` is enough to flip the result.
175-
This confirms the issue is cache invalidation, not code proximity.
176-
The resolved class cache serves a differently-merged `ClassInfo` for
177-
`Builder` depending on whether the cache was populated before or
178-
after the edit cycle.
179-
180-
The non-determinism is the critical part. A diagnostic that is
181-
consistently wrong can be investigated and worked around. A diagnostic
182-
that flickers on every keystroke erodes trust in every diagnostic
183-
PHPantom produces. Users will assume all warnings are unreliable.
184-
185-
Possible causes:
186-
- The `resolved_class_cache` returns a `ClassInfo` for `Builder`
187-
that was merged before the Model's scope methods were fully loaded.
188-
After `didChange` invalidates the cache, the next resolution
189-
happens to load in a different order and produces a complete merge.
190-
- Virtual member merging (scope methods are forwarded from the Model
191-
to the Builder via `__call`) is order-dependent: the result differs
192-
based on whether the Model was fully resolved at the time the
193-
Builder entry was cached.
194-
- The cache is keyed by class name but not by the set of local
195-
classes available at resolution time. Two resolution passes with
196-
different `ast_map` contents can produce different results under
197-
the same cache key.
198-
199-
**Reproduces in:** `example.php` line 1440
200-
(`Bakery::where('open', true)->fresh()->get()`).
201-
202-
### Where to start
203-
204-
- **Diagnostic entry point:**
205-
`src/diagnostics/unknown_members.rs` →
206-
`collect_unknown_member_diagnostics`. Same entry point as bug §2.
207-
The diagnostic resolves the subject via `resolve_target_classes`,
208-
gets back a `ClassInfo` for `Builder`, then checks whether `fresh`
209-
exists on it via `member_exists`. Log what methods the resolved
210-
`Builder` class has to see whether the scope methods are present.
211-
- **Resolved class cache:**
212-
`src/virtual_members.rs` → `ResolvedClassCache` (type alias for a
213-
`DashMap` or similar). This is what `resolved_class_cache` points
214-
to. Check how entries are inserted and whether `didChange` in
215-
`src/server.rs` clears or partially clears this cache.
216-
- **Cache invalidation:**
217-
`src/server.rs` → `did_change` handler. After re-parsing the file,
218-
trace what caches are cleared. If `resolved_class_cache` is not
219-
fully cleared (or is cleared but re-populated from stale
220-
`ast_map` data), that would explain the flip-flop.
221-
- **Virtual member merging:**
222-
`src/virtual_members.rs` or `src/inheritance.rs` →
223-
`resolve_full_class` or similar. This is where scope methods from
224-
a Model are merged onto its Builder. The merge depends on loading
225-
the Model class first. If the Model is not yet in `ast_map` when
226-
the Builder is resolved and cached, the cached Builder will be
227-
missing the scope methods.
228-
- **Debugging approach:** Add logging to the cache lookup: on hit,
229-
log the class name and the number of methods on the cached
230-
`ClassInfo`. Compare runs with and without the no-op edit. The
231-
method count difference will tell you exactly which methods are
232-
missing from the incomplete merge.

0 commit comments

Comments
 (0)