Skip to content

Commit cb8f842

Browse files
calebdwAJenbo
authored andcommitted
fix(rename): handle variables in dynamic property selectors
Dynamic property selectors like `$message->{$attribute}` were only partially walked. The scope collector skipped selector expressions when tracking local variable reads, which caused false `unused_variable` diagnostics for the selector variable. The symbol map extractor also ignored non-identifier property selectors, so find-references and rename missed variable occurrences inside dynamic property accesses. Renaming `$attribute` updated plain variable uses but not the selector inside `$message->{$attribute}`. Walk selector expressions during scope collection and emit variable symbol spans for dynamic property selectors in the symbol map. Add regression tests covering unused-variable diagnostics, references, and rename behavior for this pattern. Signed-off-by: Anders Jenbo <anders@jenbo.dk>
1 parent a3fb686 commit cb8f842

7 files changed

Lines changed: 194 additions & 27 deletions

File tree

docs/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5353
- **Go-to-definition, rename, and highlight accuracy.** References in `@see` tags to qualified names like `App\Foo::bar()` now land on the correct location, and renaming a property selects the whole `$name` instead of `$nam`.
5454
- **`@phpstan-require-extends` and `@phpstan-require-implements` navigation.** Class and interface names in these trait constraint tags now support go-to-definition and hover, and an import used only by such a tag is no longer flagged as unused. Contributed by @calebdw in https://github.com/PHPantom-dev/phpantom_lsp/pull/172.
5555
- **Renaming variables captured by nested closures and arrow functions.** Renaming or finding references to a variable used inside deeply nested arrow functions (`fn () => fn () => $var`) or closures with `use ($var)` now updates every occurrence, whether the rename is triggered on the declaration or from deep inside the nesting. Contributed by @calebdw in https://github.com/AJenbo/phpantom_lsp/pull/145.
56+
- **Variables inside dynamic property accesses are tracked.** A variable used as a dynamic property selector (`$message->{$attribute}`) now counts as a use, so it is no longer wrongly reported as unused, find-references includes it, and renaming the variable updates the selector along with its other occurrences. Contributed by @calebdw in https://github.com/PHPantom-dev/phpantom_lsp/pull/174.
5657
- **Member rename stays scoped to the declaration it targets.** Renaming a method or property no longer touches same-named members on unrelated classes. A private method rename updates only that method and its real usages, calls on a receiver whose type cannot be resolved are left alone, renaming one implementation of an interface no longer renames sibling implementations, and renaming a child override stays on the child branch. Renaming a parent or interface declaration still updates the inherited overrides and implemented usages. Contributed by @calebdw in https://github.com/PHPantom-dev/phpantom_lsp/pull/160.
5758
- **Find references on a constructor lists every call site.** Finding references to a `__construct` declaration now reports the `new ClassName(...)` instantiations, `#[ClassName(...)]` attribute usages, and explicit delegation calls written as `parent::__construct()`, `self::__construct()`, or `Class::__construct()`, including for subclasses that inherit the constructor (and excluding subclasses that override it). Attribute classes that are never written as `new` are now found. Contributed by @RemcoSmitsDev in https://github.com/AJenbo/phpantom_lsp/pull/155.
5859
- **Positions on lines with multibyte characters.** Signature help, go-to-definition on virtual properties, named-argument completion, unused-import removal, and the `@phpstan-ignore` quickfix placed cursors and edits at the wrong column on lines containing multibyte characters; they now use the correct UTF-16 columns. Type strings containing `*` wildcards or variance annotations are also no longer mangled.

src/diagnostics/unused_variables.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,22 @@ function foo() {
622622
assert!(diags.is_empty());
623623
}
624624

625+
#[test]
626+
fn no_diagnostic_when_variable_is_used_in_dynamic_property_access() {
627+
let diags = collect(
628+
r#"<?php
629+
function foo(object $message, string $type) {
630+
$attribute = strtolower($type);
631+
return $message->{$attribute};
632+
}
633+
"#,
634+
);
635+
assert!(
636+
diags.is_empty(),
637+
"dynamic property selector should count as a read"
638+
);
639+
}
640+
625641
#[test]
626642
fn skips_unused_parameter() {
627643
// Parameters are intentionally not flagged until suppression

src/rename/tests.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,36 @@ async fn rename_variable_updates_compact_string() {
185185
assert!(updated.contains("compact('person')"));
186186
}
187187

188+
#[tokio::test]
189+
async fn rename_variable_updates_dynamic_property_selector() {
190+
let backend = Backend::new_test();
191+
let uri = Url::parse("file:///test.php").unwrap();
192+
let text = concat!(
193+
"<?php\n",
194+
"function demo(object $message, string $type): void {\n",
195+
" $attribute = strtolower($type);\n",
196+
" if (empty($message->{$attribute})) {\n",
197+
" return;\n",
198+
" }\n",
199+
" echo $attribute;\n",
200+
"}\n",
201+
);
202+
203+
open_file(&backend, &uri, text).await;
204+
205+
let edit = rename(&backend, &uri, 2, 6, "$field").await;
206+
assert!(
207+
edit.is_some(),
208+
"Expected a workspace edit for variable rename"
209+
);
210+
211+
let file_edits = edits_for_uri(&edit.unwrap(), &uri);
212+
let updated = apply_edits(text, &file_edits);
213+
assert!(updated.contains("$field = strtolower($type);"));
214+
assert!(updated.contains("$message->{$field}"));
215+
assert!(updated.contains("echo $field;"));
216+
}
217+
188218
#[tokio::test]
189219
async fn rename_from_compact_string_updates_variable() {
190220
let backend = Backend::new_test();

src/scope_collector/mod.rs

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1046,10 +1046,25 @@ fn walk_expression(expr: &Expression<'_>, collector: &mut Collector<'_>) {
10461046
Expression::Access(access) => match access {
10471047
Access::Property(pa) => {
10481048
walk_expression(pa.object, collector);
1049-
// property selector is not a variable read
1049+
match &pa.property {
1050+
ClassLikeMemberSelector::Identifier(_)
1051+
| ClassLikeMemberSelector::Missing(_) => {}
1052+
ClassLikeMemberSelector::Variable(var) => walk_variable_read(var, collector),
1053+
ClassLikeMemberSelector::Expression(selector) => {
1054+
walk_expression(selector.expression, collector)
1055+
}
1056+
}
10501057
}
10511058
Access::NullSafeProperty(pa) => {
10521059
walk_expression(pa.object, collector);
1060+
match &pa.property {
1061+
ClassLikeMemberSelector::Identifier(_)
1062+
| ClassLikeMemberSelector::Missing(_) => {}
1063+
ClassLikeMemberSelector::Variable(var) => walk_variable_read(var, collector),
1064+
ClassLikeMemberSelector::Expression(selector) => {
1065+
walk_expression(selector.expression, collector)
1066+
}
1067+
}
10531068
}
10541069
Access::StaticProperty(spa) => {
10551070
walk_expression(spa.class, collector);
@@ -1354,9 +1369,23 @@ fn walk_expression_as_write(expr: &Expression<'_>, collector: &mut Collector<'_>
13541369
Expression::Access(Access::Property(pa)) => {
13551370
// `$obj->prop = …` — $obj is read, prop is written.
13561371
walk_expression(pa.object, collector);
1372+
match &pa.property {
1373+
ClassLikeMemberSelector::Identifier(_) | ClassLikeMemberSelector::Missing(_) => {}
1374+
ClassLikeMemberSelector::Variable(var) => walk_variable_read(var, collector),
1375+
ClassLikeMemberSelector::Expression(selector) => {
1376+
walk_expression(selector.expression, collector)
1377+
}
1378+
}
13571379
}
13581380
Expression::Access(Access::NullSafeProperty(pa)) => {
13591381
walk_expression(pa.object, collector);
1382+
match &pa.property {
1383+
ClassLikeMemberSelector::Identifier(_) | ClassLikeMemberSelector::Missing(_) => {}
1384+
ClassLikeMemberSelector::Variable(var) => walk_variable_read(var, collector),
1385+
ClassLikeMemberSelector::Expression(selector) => {
1386+
walk_expression(selector.expression, collector)
1387+
}
1388+
}
13601389
}
13611390
Expression::Access(Access::StaticProperty(spa)) => {
13621391
walk_expression(spa.class, collector);

src/scope_collector/tests.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -532,6 +532,20 @@ function test() {
532532
assert_eq!(count_kind(&scope_map, "$config", AccessKind::Read), 1);
533533
}
534534

535+
#[test]
536+
fn dynamic_property_selector_reads_variable() {
537+
let php = r#"<?php
538+
function test(object $message, string $type) {
539+
$attribute = strtolower($type);
540+
return $message->{$attribute};
541+
}
542+
"#;
543+
let scope_map = collect_from_function(php);
544+
545+
assert_eq!(count_kind(&scope_map, "$attribute", AccessKind::Write), 1);
546+
assert_eq!(count_kind(&scope_map, "$attribute", AccessKind::Read), 1);
547+
}
548+
535549
// ─── Unset ──────────────────────────────────────────────────────────────────
536550

537551
#[test]

src/symbol_map/extraction.rs

Lines changed: 72 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1767,6 +1767,34 @@ fn extract_from_hint_ctx(hint: &Hint<'_>, spans: &mut Vec<SymbolSpan>, ref_ctx:
17671767

17681768
// ─── Expression extractor ───────────────────────────────────────────────────
17691769

1770+
fn extract_variable_symbol_spans<'a>(
1771+
var: &'a Variable<'a>,
1772+
ctx: &mut ExtractionCtx<'a>,
1773+
scope_start: u32,
1774+
) {
1775+
match var {
1776+
Variable::Direct(dv) => {
1777+
let raw = bytes_to_str(dv.name);
1778+
if raw == "$this" {
1779+
ctx.spans.push(SymbolSpan {
1780+
start: dv.span.start.offset,
1781+
end: dv.span.end.offset,
1782+
kind: SymbolKind::SelfStaticParent(SelfStaticParentKind::This),
1783+
});
1784+
} else {
1785+
let name = raw.strip_prefix('$').unwrap_or(raw).to_string();
1786+
ctx.spans.push(SymbolSpan {
1787+
start: dv.span.start.offset,
1788+
end: dv.span.end.offset,
1789+
kind: SymbolKind::Variable { name },
1790+
});
1791+
}
1792+
}
1793+
Variable::Indirect(iv) => extract_from_expression(iv.expression, ctx, scope_start),
1794+
Variable::Nested(nv) => extract_variable_symbol_spans(nv.variable, ctx, scope_start),
1795+
}
1796+
}
1797+
17701798
fn extract_from_expression<'a>(
17711799
expr: &'a Expression<'a>,
17721800
ctx: &mut ExtractionCtx<'a>,
@@ -2105,38 +2133,56 @@ fn extract_from_expression<'a>(
21052133
let subject_text = expr_to_subject_text(pa.object);
21062134
extract_from_expression(pa.object, ctx, scope_start);
21072135

2108-
if let ClassLikeMemberSelector::Identifier(ident) = &pa.property {
2109-
let member_name = bytes_to_str(ident.value).to_string();
2110-
ctx.spans.push(SymbolSpan {
2111-
start: ident.span.start.offset,
2112-
end: ident.span.end.offset,
2113-
kind: SymbolKind::MemberAccess {
2114-
subject_text,
2115-
member_name,
2116-
is_static: false,
2117-
is_method_call: false,
2118-
is_docblock_reference: false,
2119-
},
2120-
});
2136+
match &pa.property {
2137+
ClassLikeMemberSelector::Identifier(ident) => {
2138+
let member_name = bytes_to_str(ident.value).to_string();
2139+
ctx.spans.push(SymbolSpan {
2140+
start: ident.span.start.offset,
2141+
end: ident.span.end.offset,
2142+
kind: SymbolKind::MemberAccess {
2143+
subject_text,
2144+
member_name,
2145+
is_static: false,
2146+
is_method_call: false,
2147+
is_docblock_reference: false,
2148+
},
2149+
});
2150+
}
2151+
ClassLikeMemberSelector::Variable(var) => {
2152+
extract_variable_symbol_spans(var, ctx, scope_start)
2153+
}
2154+
ClassLikeMemberSelector::Expression(selector) => {
2155+
extract_from_expression(selector.expression, ctx, scope_start)
2156+
}
2157+
ClassLikeMemberSelector::Missing(_) => {}
21212158
}
21222159
}
21232160
Access::NullSafeProperty(pa) => {
21242161
let subject_text = expr_to_subject_text(pa.object);
21252162
extract_from_expression(pa.object, ctx, scope_start);
21262163

2127-
if let ClassLikeMemberSelector::Identifier(ident) = &pa.property {
2128-
let member_name = bytes_to_str(ident.value).to_string();
2129-
ctx.spans.push(SymbolSpan {
2130-
start: ident.span.start.offset,
2131-
end: ident.span.end.offset,
2132-
kind: SymbolKind::MemberAccess {
2133-
subject_text,
2134-
member_name,
2135-
is_static: false,
2136-
is_method_call: false,
2137-
is_docblock_reference: false,
2138-
},
2139-
});
2164+
match &pa.property {
2165+
ClassLikeMemberSelector::Identifier(ident) => {
2166+
let member_name = bytes_to_str(ident.value).to_string();
2167+
ctx.spans.push(SymbolSpan {
2168+
start: ident.span.start.offset,
2169+
end: ident.span.end.offset,
2170+
kind: SymbolKind::MemberAccess {
2171+
subject_text,
2172+
member_name,
2173+
is_static: false,
2174+
is_method_call: false,
2175+
is_docblock_reference: false,
2176+
},
2177+
});
2178+
}
2179+
ClassLikeMemberSelector::Variable(var) => {
2180+
extract_variable_symbol_spans(var, ctx, scope_start)
2181+
}
2182+
ClassLikeMemberSelector::Expression(selector) => {
2183+
extract_from_expression(selector.expression, ctx, scope_start)
2184+
}
2185+
ClassLikeMemberSelector::Missing(_) => {}
21402186
}
21412187
}
21422188
Access::StaticProperty(spa) => {

tests/integration/references.rs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,37 @@ function test(): void {
423423
);
424424
}
425425

426+
#[test]
427+
fn variable_references_include_dynamic_property_selector() {
428+
let backend = create_test_backend();
429+
let uri = "file:///tmp/test_refs_dynamic_selector.php";
430+
let content = r#"<?php
431+
432+
function test(object $message, string $type): void {
433+
$attribute = strtolower($type);
434+
if (empty($message->{$attribute})) {
435+
return;
436+
}
437+
echo $attribute;
438+
}
439+
"#;
440+
441+
open_file(&backend, uri, content);
442+
443+
let results = backend
444+
.find_references(uri, content, Position::new(3, 5), true)
445+
.expect("should find references");
446+
447+
assert_no_duplicates(&results, "variable_references_dynamic_selector");
448+
assert_eq!(
449+
results.len(),
450+
3,
451+
"Expected 3 references (definition + dynamic selector + echo), got {}: {:#?}",
452+
results.len(),
453+
results
454+
);
455+
}
456+
426457
// ─── Static member references ───────────────────────────────────────────────
427458

428459
#[test]

0 commit comments

Comments
 (0)