Skip to content

feat(rename): link compact strings to local variables #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
calebdw wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(rename): link compact strings to local variables #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/definition/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl Backend {
cursor_offset: u32,
) -> Option<Vec<Location>> {
match kind {
SymbolKind::Variable { name } => {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
// Try the precomputed var_defs map first.
// This avoids re-parsing the file at request time.

Expand All @@ -191,10 +191,14 @@ impl Backend {
let parsed_uri = Url::parse(uri).ok()?;
let start =
crate::util::offset_to_position(content, cursor_offset as usize);
let end = crate::util::offset_to_position(
content,
cursor_offset as usize + 1 + name.len(),
);
let end_offset = match kind {
SymbolKind::Variable { .. } => cursor_offset as usize + 1 + name.len(),
SymbolKind::CompactVariable { .. } => {
cursor_offset as usize + name.len()
}
_ => unreachable!(),
};
let end = crate::util::offset_to_position(content, end_offset);
return Some(vec![Location {
uri: parsed_uri,
range: Range { start, end },
Expand Down
2 changes: 1 addition & 1 deletion src/definition/type_definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl Backend {
let function_loader = self.function_loader(&ctx);

let resolved_types: Vec<PhpType> = match &symbol.kind {
SymbolKind::Variable { name } => {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
let in_static = self
.symbol_maps
.read()
Expand Down
42 changes: 22 additions & 20 deletions src/highlight/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl Backend {
let symbol_map = maps.get(uri)?;

let highlights = match &span.kind {
SymbolKind::Variable { name } => {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
// Check if this is actually a property declaration — if
// so, highlight member accesses instead of local vars.
if let Some(VarDefKind::Property) = symbol_map.var_def_kind_at(name, span.start) {
Expand Down Expand Up @@ -111,27 +111,29 @@ impl Backend {

// Collect from symbol spans.
for span in &symbol_map.spans {
if let SymbolKind::Variable { name } = &span.kind {
if name != var_name {
continue;
}
let span_scope = symbol_map.find_variable_scope(name, span.start);
if span_scope != scope_start {
continue;
}
seen_offsets.insert(span.start);
let name = match &span.kind {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => name,
_ => continue,
};
if name != var_name {
continue;
}
let span_scope = symbol_map.find_variable_scope(name, span.start);
if span_scope != scope_start {
continue;
}
seen_offsets.insert(span.start);

let kind = if symbol_map.var_def_kind_at(name, span.start).is_some() {
DocumentHighlightKind::WRITE
} else {
DocumentHighlightKind::READ
};
let kind = if symbol_map.var_def_kind_at(name, span.start).is_some() {
DocumentHighlightKind::WRITE
} else {
DocumentHighlightKind::READ
};

highlights.push(DocumentHighlight {
range: byte_range_to_lsp_range(content, span.start as usize, span.end as usize),
kind: Some(kind),
});
}
highlights.push(DocumentHighlight {
range: byte_range_to_lsp_range(content, span.start as usize, span.end as usize),
kind: Some(kind),
});
}

// Include var_def sites that may not have a matching Variable span
Expand Down
2 changes: 1 addition & 1 deletion src/hover/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ impl Backend {
let function_loader = self.function_loader(&ctx);

match kind {
SymbolKind::Variable { name } => {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
// Suppress hover when the cursor is on a variable at its
// definition site where the type is already visible in
// the signature (properties, static/global declarations).
Expand Down
46 changes: 25 additions & 21 deletions src/references/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ impl Backend {
include_declaration: bool,
) -> Vec<Location> {
match kind {
SymbolKind::Variable { name } => {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
// Property declarations use Variable spans (so GTD can
// jump to the type hint), but Find References should
// search for member accesses, not local variable uses.
Expand Down Expand Up @@ -330,26 +330,28 @@ impl Backend {
let reachable_scopes = Self::collect_capture_scopes(symbol_map, var_name, scope_start);

for span in &symbol_map.spans {
if let SymbolKind::Variable { name } = &span.kind {
if name != var_name {
continue;
}
// Check that this variable is in a reachable scope.
let span_scope = symbol_map.find_variable_scope(name, span.start);
if !reachable_scopes.contains(&span_scope) {
continue;
}
// Optionally skip declaration sites.
if !include_declaration && symbol_map.var_def_kind_at(name, span.start).is_some() {
continue;
}
let start = offset_to_position(content, span.start as usize);
let end = offset_to_position(content, span.end as usize);
locations.push(Location {
uri: parsed_uri.clone(),
range: Range { start, end },
});
let name = match &span.kind {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => name,
_ => continue,
};
if name != var_name {
continue;
}
// Check that this variable is in a reachable scope.
let span_scope = symbol_map.find_variable_scope(name, span.start);
if !reachable_scopes.contains(&span_scope) {
continue;
}
// Optionally skip declaration sites.
if !include_declaration && symbol_map.var_def_kind_at(name, span.start).is_some() {
continue;
}
let start = offset_to_position(content, span.start as usize);
let end = offset_to_position(content, span.end as usize);
locations.push(Location {
uri: parsed_uri.clone(),
range: Range { start, end },
});
}

// Also include var_def sites if include_declaration is set,
Expand Down Expand Up @@ -412,7 +414,9 @@ impl Backend {
scope_ends: &HashMap<u32, u32>,
) -> bool {
symbol_map.spans.iter().any(|s| {
if let SymbolKind::Variable { name } = &s.kind {
if let SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } =
&s.kind
{
name == var_name
&& scope_ends
.get(&scope_start)
Expand Down
25 changes: 25 additions & 0 deletions src/references/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,31 @@ async fn test_variable_references_exclude_declaration() {
);
}

#[tokio::test]
async fn test_variable_references_include_compact_string() {
let backend = Backend::new_test();
let uri = Url::parse("file:///test.php").unwrap();
let text = concat!(
"<?php\n",
"function demo(): array {\n",
" $user = 'alice';\n",
" return compact('user');\n",
"}\n",
);

open_file(&backend, &uri, text).await;

let locs = find_references(&backend, &uri, 2, 6, true).await;
assert!(
locs.iter().any(|loc| {
loc.range.start.line == 3
&& loc.range.start.character == 20
&& loc.range.end.character == 24
}),
"Expected compact('user') string contents to be included in variable references: {locs:?}"
);
}

// ─── Class References ───────────────────────────────────────────────────────

#[tokio::test]
Expand Down
29 changes: 22 additions & 7 deletions src/rename/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,10 @@ impl Backend {
// special because the `$` prefix is part of the declaration but
// usage sites via `->` or `?->` don't include it.
let is_property = self.is_property_rename(&span.kind, uri, &span);
let is_variable = matches!(&span.kind, SymbolKind::Variable { .. }) && !is_property;
let is_variable = matches!(
&span.kind,
SymbolKind::Variable { .. } | SymbolKind::CompactVariable { .. }
) && !is_property;

// For class renames, delegate to the specialised handler that
// understands `use` statements, aliases, and collisions.
Expand All @@ -163,12 +166,22 @@ impl Backend {
};

let edit_text = if is_variable {
// Variables: the reference range includes the `$`, so
// the new name should also include it.
if new_name.starts_with('$') {
new_name.to_string()
} else {
format!("${}", new_name)
let bare_name = new_name.strip_prefix('$').unwrap_or(new_name);
let loc_symbol = loc_content.as_deref().and_then(|c| {
self.lookup_symbol_at_position(&loc_uri_str, c, location.range.start)
});
match loc_symbol {
Some(crate::symbol_map::SymbolSpan {
kind: SymbolKind::CompactVariable { .. },
..
}) => bare_name.to_string(),
_ => {
if new_name.starts_with('$') {
new_name.to_string()
} else {
format!("${}", new_name)
}
}
}
} else if is_property {
// Properties: the reference may or may not include `$`.
Expand Down Expand Up @@ -539,6 +552,7 @@ impl Backend {
// Include the `$` prefix in the range — the span already does.
Some((format!("${}", name), range))
}
SymbolKind::CompactVariable { name } => Some((name.clone(), range)),
SymbolKind::ClassReference { name, .. } => Some((name.clone(), range)),
SymbolKind::ClassDeclaration { name } => Some((name.clone(), range)),
SymbolKind::MemberAccess { member_name, .. } => Some((member_name.clone(), range)),
Expand Down Expand Up @@ -614,6 +628,7 @@ impl Backend {
self.lookup_var_def_kind_at(uri, name, span.start)
.is_some_and(|k| k == crate::symbol_map::VarDefKind::Property)
}
SymbolKind::CompactVariable { .. } => false,
_ => false,
}
}
Expand Down
79 changes: 79 additions & 0 deletions src/rename/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,58 @@ async fn rename_variable_without_dollar_prefix() {
}
}

#[tokio::test]
async fn rename_variable_updates_compact_string() {
let backend = Backend::new_test();
let uri = Url::parse("file:///test.php").unwrap();
let text = concat!(
"<?php\n",
"function demo(): array {\n",
" $user = 'alice';\n",
" return compact('user');\n",
"}\n",
);

open_file(&backend, &uri, text).await;

let edit = rename(&backend, &uri, 2, 6, "$person").await;
assert!(
edit.is_some(),
"Expected a workspace edit for variable rename"
);

let file_edits = edits_for_uri(&edit.unwrap(), &uri);
let updated = apply_edits(text, &file_edits);
assert!(updated.contains("$person = 'alice';"));
assert!(updated.contains("compact('person')"));
}

#[tokio::test]
async fn rename_from_compact_string_updates_variable() {
let backend = Backend::new_test();
let uri = Url::parse("file:///test.php").unwrap();
let text = concat!(
"<?php\n",
"function demo(): array {\n",
" $user = 'alice';\n",
" return compact('user');\n",
"}\n",
);

open_file(&backend, &uri, text).await;

let edit = rename(&backend, &uri, 3, 21, "person").await;
assert!(
edit.is_some(),
"Expected a workspace edit for compact rename"
);

let file_edits = edits_for_uri(&edit.unwrap(), &uri);
let updated = apply_edits(text, &file_edits);
assert!(updated.contains("$person = 'alice';"));
assert!(updated.contains("compact('person')"));
}

#[tokio::test]
async fn prepare_rename_variable() {
let backend = Backend::new_test();
Expand Down Expand Up @@ -186,6 +238,33 @@ async fn prepare_rename_variable() {
}
}

#[tokio::test]
async fn prepare_rename_compact_string_uses_bare_name() {
let backend = Backend::new_test();
let uri = Url::parse("file:///test.php").unwrap();
let text = concat!(
"<?php\n",
"function demo(): array {\n",
" $user = 'alice';\n",
" return compact('user');\n",
"}\n",
);

open_file(&backend, &uri, text).await;

let response = prepare_rename(&backend, &uri, 3, 21).await;
assert!(
response.is_some(),
"Expected prepare rename on compact string"
);

if let Some(PrepareRenameResponse::RangeWithPlaceholder { placeholder, .. }) = response {
assert_eq!(placeholder, "user");
} else {
panic!("Expected RangeWithPlaceholder response");
}
}

// ─── Non-Renameable Symbols ─────────────────────────────────────────────────

#[tokio::test]
Expand Down
2 changes: 1 addition & 1 deletion src/semantic_tokens.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ impl Backend {
(tt, mods)
}

SymbolKind::Variable { name } => {
SymbolKind::Variable { name } | SymbolKind::CompactVariable { name } => {
// Check if this variable is a parameter.
let (tt, mut mods) =
self.classify_variable(name, span.start, symbol_map, uri, ctx);
Expand Down
Loading
Loading