Skip to content

Commit d7c76c1

Browse files
committed
Renaming a constructor-promoted property parameter now cascades to
`$this->prop` usages
1 parent c137a81 commit d7c76c1

9 files changed

Lines changed: 146 additions & 64 deletions

File tree

docs/CHANGELOG.md

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

4747
- **Completion works after closing a multi-line closure argument on the same line as the next chain operator.** Typing `->` immediately after a call like `->map(function (...) { ... })->` now resolves the full receiver instead of returning no member suggestions until the operator is moved to a new line. Contributed by @calebdw.
4848
- **Conditional return types recognize interpolated strings as strings.** A function with a conditional return type like `($key is string ? mixed : null)` now correctly resolves the `string` branch when called with an interpolated string argument (e.g. `config("{$prefix}.host")`). Previously the interpolated string was not recognized as a string literal, causing the return type to fall through to the else branch and resolve as `null`. Contributed by @calebdw.
49+
- **Renaming a constructor-promoted property parameter now cascades to `$this->prop` usages.** Renaming `private int $someField` in a constructor's parameter list previously only updated the parameter declaration itself, leaving every `$this->someField` reference elsewhere in the class stale. Rename, find references, document highlight, and linked editing now treat a promoted property parameter the same as an ordinary property declaration.
4950
- **Closure/arrow-function parameters passed after reordering named arguments now infer correctly.** When a call used named arguments to reorder or skip parameters ahead of a closure argument (e.g. `process(class: Product::class, cb: function ($p) {...}, flag: true)`), the closure's own parameter type stopped being inferred from the target function/method's `callable(...)`/`Closure(...)` signature, silently losing completions and hover for the closure's parameters. Argument-to-parameter binding now follows PHP's actual named-argument rules instead of assuming call position matches declared position.
5051
- **Deprecated enum cases are recognized.** Enum cases annotated with `@deprecated` or `#[Deprecated]` now carry deprecation metadata, so usages like `self::Low` can be highlighted as deprecated in contextual semantic-token mode. Contributed by @calebdw.
5152
- **Class constant accesses use constant semantic highlighting.** `self::CONSTANT`, `static::CONSTANT`, `parent::CONSTANT`, and `ClassName::CONSTANT` now emit the `enumMember` semantic token instead of being colored as properties, while `ClassName::$property` still emits `property`. Contributed by @calebdw.

docs/todo/bugs.md

Lines changed: 1 addition & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -7,60 +7,4 @@ pipeline so it produces correct data. Downstream consumers
77
(diagnostics, hover, completion, definition) should never need
88
to second-guess upstream output.
99

10-
## B116. Renaming a constructor-promoted property parameter doesn't cascade to `$this->prop` usages
11-
12-
**Severity: Low-Medium (rename only updates the parameter declaration
13-
itself; every `$this->field` reference elsewhere in the class is left
14-
stale, silently producing a broken rename) · Discovered while
15-
investigating a user-reported GitHub Q&A about rename support for
16-
promoted properties**
17-
18-
```php
19-
final class SomeService {
20-
public function __construct(
21-
private int $someField, // renaming this parameter...
22-
) {}
23-
24-
public function handle(): void {
25-
$this->someField; // ...doesn't rename this usage
26-
}
27-
}
28-
```
29-
30-
The symbol-map extraction for constructor parameters
31-
(`src/symbol_map/extraction/class_like.rs:605-642`) unconditionally
32-
tags every parameter's `VarDefSite`/`SymbolSpan` with
33-
`kind: VarDefKind::Parameter` (line 633), with no check for
34-
`param.is_promoted_property()`. This is the symbol map that
35-
`find_references`/rename consult, and it's a separate structure from
36-
the properties list the type engine builds in
37-
`src/parser/classes.rs:1040-1049`, which *does* special-case
38-
`is_promoted_property()` to synthesize a property for hover/completion/
39-
type-inference of `$this->someField` — but that data isn't wired back
40-
into the symbol map's `VarDefKind`.
41-
42-
Because of this, `lookup_var_def_kind_at` (`src/definition/resolve.rs:130`)
43-
reports `VarDefKind::Parameter` for a promoted property, so
44-
`src/references/dispatch.rs:136-172` routes the rename to
45-
`find_variable_references` (`src/references/variables.rs:22`) instead
46-
of the cross-file, member-access-aware `find_member_references`.
47-
`find_variable_references` is explicitly file-local/scope-local and
48-
never looks at `$this->foo` sites, so it only renames the parameter's
49-
own token(s). The same `VarDefKind::Property` check gates
50-
`is_property_rename` in `src/rename/prepare.rs:302-306`, so the rename
51-
is also misclassified as a plain-variable rename rather than a
52-
property rename, which skips the `$this->foo` prefix-fixup logic
53-
`src/rename/mod.rs` documents for property renames.
54-
55-
Fix by making the symbol-map extraction in `class_like.rs` emit
56-
`VarDefKind::Property` (or an equivalent alias treated like `Property`
57-
everywhere) for parameters where `param.is_promoted_property()` is
58-
true, and register the declaration wherever
59-
`find_member_references`/the declaration-hierarchy resolvers expect
60-
property declarations to live. Audit the other call sites that branch
61-
on `VarDefKind::Parameter` vs `Property` for promoted params for their
62-
own reasons (semantic tokens at `semantic_tokens.rs:757-770`, hover at
63-
`hover/mod.rs:152-158`) so they keep working once the kind changes.
64-
There is no existing test coverage for promoted-property rename or
65-
references (`src/rename/tests.rs` and `tests/integration/references.rs`
66-
have no `__construct`/promoted-property cases).
10+
No outstanding items.

src/definition/resolve.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,25 @@ impl Backend {
138138
map.var_def_kind_at(var_name, cursor_offset).cloned()
139139
}
140140

141+
/// Whether `offset` is the declaration site of a constructor-promoted
142+
/// property parameter (`public function __construct(private int $x) {}`).
143+
///
144+
/// Promoted parameters share the same byte offset as their
145+
/// `VarDefKind::Parameter` symbol-map entry, but they declare a real
146+
/// class property, not a local variable — callers that special-case
147+
/// `VarDefKind::Property` (cross-file references, rename, linked
148+
/// editing, document highlight) must treat these the same way.
149+
/// `ClassInfo::properties` already carries promoted parameters (built
150+
/// in `parser/classes.rs`), so this checks membership there instead of
151+
/// teaching the symbol map a new `VarDefKind`.
152+
pub(crate) fn is_promoted_property_param(&self, uri: &str, offset: u32) -> bool {
153+
self.get_classes_for_uri(uri)
154+
.iter()
155+
.flat_map(|classes| classes.iter())
156+
.flat_map(|c| c.properties.iter())
157+
.any(|p| p.name_offset != 0 && p.name_offset == offset)
158+
}
159+
141160
/// If the cursor is on a variable at its assignment definition site,
142161
/// return the `effective_from` offset (end of the assignment statement).
143162
///

src/highlight/mod.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,14 @@ impl Backend {
4545
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
4646
// Check if this is actually a property declaration — if
4747
// so, highlight member accesses instead of local vars.
48-
if let Some(VarDefKind::Property) = symbol_map.var_def_kind_at(name, span.start) {
48+
// Constructor-promoted properties are tagged
49+
// VarDefKind::Parameter (the token is also a real
50+
// parameter) but still declare a class property.
51+
let is_property = matches!(
52+
symbol_map.var_def_kind_at(name, span.start),
53+
Some(VarDefKind::Property)
54+
) || self.is_promoted_property_param(uri, span.start);
55+
if is_property {
4956
self.highlight_member_name(symbol_map, content, name)
5057
} else {
5158
self.highlight_variable(symbol_map, content, name, span.start)

src/linked_editing.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,15 @@ impl Backend {
5757
match &span.kind {
5858
SymbolKind::Variable { name } => {
5959
// Property declarations should not trigger linked editing —
60-
// renaming a property requires cross-file awareness.
61-
if let Some(VarDefKind::Property) = symbol_map.var_def_kind_at(name, span.start) {
60+
// renaming a property requires cross-file awareness. This
61+
// includes constructor-promoted properties, which are
62+
// tagged VarDefKind::Parameter (the token is also a real
63+
// parameter) but still declare a class property.
64+
let is_property = matches!(
65+
symbol_map.var_def_kind_at(name, span.start),
66+
Some(VarDefKind::Property)
67+
) || self.is_promoted_property_param(uri, span.start);
68+
if is_property {
6269
return None;
6370
}
6471

src/references/dispatch.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,14 @@ impl Backend {
137137
// Property declarations use Variable spans (so GTD can
138138
// jump to the type hint), but Find References should
139139
// search for member accesses, not local variable uses.
140-
if let Some(crate::symbol_map::VarDefKind::Property) =
141-
self.lookup_var_def_kind_at(uri, name, span_start)
142-
{
140+
// Constructor-promoted properties are also Variable spans
141+
// (tagged VarDefKind::Parameter, since the token is also a
142+
// real parameter) but declare a property just the same.
143+
let is_property = matches!(
144+
self.lookup_var_def_kind_at(uri, name, span_start),
145+
Some(crate::symbol_map::VarDefKind::Property)
146+
) || self.is_promoted_property_param(uri, span_start);
147+
if is_property {
143148
// Properties are never static in the Variable span
144149
// context ($this->prop). Static properties use
145150
// MemberAccess spans at their usage sites with

src/rename/prepare.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,9 +300,13 @@ impl Backend {
300300
!is_method && !is_constant
301301
}
302302
SymbolKind::Variable { name } => {
303-
// Variable spans can represent property declarations.
303+
// Variable spans can represent property declarations,
304+
// including constructor-promoted properties (tagged
305+
// VarDefKind::Parameter since the token is also a real
306+
// parameter, but still declares a class property).
304307
self.lookup_var_def_kind_at(uri, name, span.start)
305308
.is_some_and(|k| k == crate::symbol_map::VarDefKind::Property)
309+
|| self.is_promoted_property_param(uri, span.start)
306310
}
307311
SymbolKind::CompactVariable { .. } => false,
308312
_ => false,

src/rename/tests.rs

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,57 @@ async fn rename_property_from_access() {
616616
}
617617
}
618618

619+
#[tokio::test]
620+
async fn rename_promoted_property_from_parameter() {
621+
let backend = Backend::new_test();
622+
let uri = Url::parse("file:///test.php").unwrap();
623+
let text = concat!(
624+
"<?php\n",
625+
"class SomeService {\n",
626+
" public function __construct(\n",
627+
" private int $someField,\n",
628+
" ) {}\n",
629+
"\n",
630+
" public function handle(): int {\n",
631+
" return $this->someField;\n",
632+
" }\n",
633+
"}\n",
634+
);
635+
636+
open_file(&backend, &uri, text).await;
637+
638+
// Rename from the constructor-promoted parameter declaration itself.
639+
let (line, character) = line_char_of(text, "$someField,");
640+
let edit = rename(&backend, &uri, line, character, "otherField").await;
641+
assert!(
642+
edit.is_some(),
643+
"Expected a workspace edit for promoted property rename"
644+
);
645+
646+
let file_edits = edits_for_uri(&edit.unwrap(), &uri);
647+
// Should have edits for: the promoted parameter and $this->someField.
648+
assert!(
649+
file_edits.len() >= 2,
650+
"Expected at least 2 edits for someField, got {}",
651+
file_edits.len()
652+
);
653+
654+
// The declaration site includes `$`, the `$this->` access site doesn't.
655+
for te in &file_edits {
656+
assert!(
657+
te.new_text == "otherField" || te.new_text == "$otherField",
658+
"Unexpected edit text: {}",
659+
te.new_text
660+
);
661+
}
662+
663+
let updated = apply_edits(text, &file_edits);
664+
assert!(
665+
updated.contains("$this->otherField"),
666+
"Expected $this->someField usage to cascade to otherField:\n{updated}"
667+
);
668+
}
669+
619670
// ─── Function Rename ────────────────────────────────────────────────────────
620671

621672
#[tokio::test]

tests/integration/references.rs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,50 @@ class Foo {
389389
);
390390
}
391391

392+
#[test]
393+
fn promoted_property_references_cascade() {
394+
let backend = create_test_backend();
395+
let uri = "file:///tmp/test_refs_promoted_prop.php";
396+
let content = r#"<?php
397+
398+
class SomeService {
399+
public function __construct(
400+
private int $someField,
401+
) {}
402+
403+
public function handle(): int {
404+
return $this->someField;
405+
}
406+
}
407+
"#;
408+
409+
open_file(&backend, uri, content);
410+
411+
// Cursor on `someField` in the promoted constructor parameter (line 4).
412+
let results = backend
413+
.find_references(uri, content, Position::new(4, 22), true)
414+
.expect("should find references");
415+
416+
assert_no_duplicates(&results, "promoted_property_references");
417+
418+
// Expect: 1 declaration (the parameter) + 1 access ($this->someField).
419+
assert_eq!(
420+
results.len(),
421+
2,
422+
"Expected 2 references (1 declaration + 1 access), got {}: {:#?}",
423+
results.len(),
424+
results
425+
);
426+
427+
// The access reference must land on the `$this->someField` usage, not
428+
// just the constructor's own local parameter uses.
429+
let access = results
430+
.iter()
431+
.find(|loc| loc.range.start.line == 8)
432+
.expect("should include the $this->someField access site");
433+
assert_eq!(access.range.start.character, 22);
434+
}
435+
392436
// ─── Variable references ────────────────────────────────────────────────────
393437

394438
#[test]

0 commit comments

Comments
 (0)