feat(lsp): implement rename, prepareRename, and hover support by close2 · Pull Request #3002 · toitlang/toit

close2 · 2026-02-27T08:04:37Z

LSP Rename, Prepare-Rename, and Hover Support

Summary

This PR adds textDocument/rename, textDocument/prepareRename, and
textDocument/hover support to the Toit Language Server. These are standard
LSP features that allow editors to safely rename symbols across a project and
display documentation on hover.

prepareRename checks whether the symbol under the cursor can be renamed
and returns the symbol's range and current name (used as the placeholder in the
editor's rename dialog).

rename performs the actual rename by finding all references to the symbol
and returning a WorkspaceEdit with text edits for every occurrence.

hover displays documentation and type information for the symbol under the
cursor. It renders toitdoc comments as Markdown and includes the symbol's
signature.

Architecture

C++ Compiler Backend (`src/compiler/`)

Three compiler pipelines handle the heavy lifting:

FindReferencesPipeline — Used by textDocument/rename. It locates the
target symbol via the LSP selection mechanism, then traverses the entire
program IR to collect every reference. Results are emitted as a stream of
(path, start_line, start_col, end_line, end_col) tuples over the compiler
protocol.
PrepareRenamePipeline — Used by textDocument/prepareRename. It
identifies the target symbol and emits its name range and placeholder text.
It tries three strategies in order:
1. Handler callback (works for reference/usage sites).
2. find_definition_at_cursor (works for definition sites — globals,
  methods, classes, fields, locals, parameters).
3. Named constructor/factory name lookup via the IR-to-AST map.
4. Post-type-check callback (for virtual method call sites).
HoverPipeline — Used by textDocument/hover. It resolves the symbol
under the cursor and emits documentation/type information. It uses a toitdoc
Markdown visitor to render documentation comments.

Both rename pipelines reuse a single handler class, FindReferencesHandler,
which implements LspSelectionHandler and captures the resolved target node.

The FindReferencesVisitor traverses the program IR and matches all
Reference* nodes whose target matches the captured definition node, using
pointer identity after unwrapping ir::Reference wrappers.

A VirtualCallFilter traverses the type-checked program to find virtual
call sites that resolve to the target method. This ensures that virtual method
calls (e.g., obj.method where the target is determined at runtime) are
included in rename references.

Key Design Decisions

Reference unwrapping — is it necessary?

Yes, but only for callbacks that receive ir::Reference* wrappers. The
resolver passes different node types depending on the callback:

Callback	What's passed
`call_static`, `call_class`, `call_prefixed`	Reference-wrapped (`ReferenceMethod`, `ReferenceGlobal`, etc.)
`class_interface_or_mixin`	Bare definition (`ir::Class*`)
`type`, `show`, `export`	Bare definitions inside `ResolutionEntry`

The FindReferencesVisitor compares nodes by pointer identity:
node->target() == target_. Since node->target() returns the unwrapped
definition (e.g., ir::Method*), the stored target_ must also be an
unwrapped definition. If we stored a ReferenceMethod* from call_static,
the pointer would never match.

The existing code in goto_definition.cc faces the same issue — its
_print_range(ir::Node*) method has an 8-branch if/else chain checking each
Reference* subclass plus each bare definition type. Our unwrap_reference
helper is a cleaner centralized solution.

Note: the IR class hierarchy has a common base ir::Reference with a virtual
target() method, so the four-subclass chain in unwrap_reference could be
simplified to:

if (node->is_Reference()) return node->as_Reference()->target();

This is a possible follow-up cleanup (also applicable to goto_definition.cc).

Definition-site fallback (find_definition_at_cursor):

The LSP selection mechanism works by modifying the parser: when the cursor
position falls inside a token, the parser replaces that token with an
LspSelection node (or LspSelectionDot for dot-expressions). The resolver
then encounters this synthetic node and treats it as a call expression, which
triggers the handler callbacks (call_static, call_virtual, etc.).

This works well for usage sites — places where a symbol is referenced
as part of an expression (e.g., foo 42, MyClass.bar, x + 1). The
resolver resolves the call, and the handler captures the target.

However, at definition sites, the symbol is being declared, not called.
For example:
- foo x: — the method name foo appears in a method declaration
- my-global := 42 — the global name appears in an assignment define
- class MyClass: — the class name appears in a class declaration
- field := 0 — the field name appears in a field declaration
At these positions, the LspSelection node is part of the declaration
syntax, not a call expression. The resolver does not invoke any handler
callback for it. The handler's target_ stays null.

find_definition_at_cursor addresses this by scanning the fully-resolved
ir::Program for the node whose name range contains the cursor. It checks:
1. Top-level globals (program->globals()) — each ir::Global has a
  range() for its name.
2. Top-level methods (program->methods()) — each ir::Method has a
  range().
3. Classes (program->classes()) — class name, fields, methods,
  unnamed constructors, factories, and statics.
4. Locals and parameters inside method bodies — uses a
  TraversingVisitor subclass (DefinitionFinder) that walks method
  bodies looking for ir::Parameter nodes and ir::AssignmentDefine nodes
  (which introduce local variables).
The range comparison is done using SourceManager::compute_location to
convert opaque Source::Position values to line/column pairs, since the
cursor position arrives as 1-based line+column.

This fallback is the reason rename works when the cursor is placed directly
on a declaration like my-global := 42 and not just on a usage.
Unnamed constructor → class redirect: When clicking on MyObj (which
calls an unnamed constructor), the rename target is redirected to the class
itself, since the user's intent is to rename the class, not the hidden
constructor.
post_resolve signature change: Pipeline::post_resolve() now receives
Resolver* and ir::Program* so that subclasses can access the
ir_to_ast_map() while the resolver is still alive. This was necessary
because FindReferencesPipeline needs the IR-to-AST map for accurate source
ranges, and PrepareRenamePipeline uses it for named constructor name
resolution.
Virtual call site discovery: FindReferencesPipeline continues to
type-checking after initial reference collection. A VirtualCallFilter
traverses the type-checked program, matching CallVirtual nodes whose
selector and arity match the target method. This ensures that all virtual
call sites are included in rename references, not just statically-resolved
ones.

Toit LSP Server (`tools/lsp/server/`)

server.toit: Registers handlers for textDocument/rename,
textDocument/prepareRename, and textDocument/hover. The rename handler
calls find-references, groups results by URI, and returns a WorkspaceEdit.
The hover handler returns Markdown-formatted documentation.
compiler.toit: New methods find-references, prepare-rename, and
hover that communicate with the compiler backend using the REFERENCES,
PREPARE RENAME, and HOVER protocol modes.
protocol/document.toit: New RenameParams and WorkspaceEdit wrappers.
client.toit: New send-rename-request, send-prepare-rename-request,
and send-hover-request methods for test clients.
summary.toit: New element-at method for locating summary elements at
a given position.
toitdoc_markdown.toit: New toitdoc-to-Markdown visitor for rendering
hover documentation.
Server capabilities: renameProvider is set to RenameOptions(true),
declaring that the server supports both rename and prepare-rename.
hoverProvider is set to true.

Changes to Existing Code

Pipeline::post_resolve signature changed from () to
(Resolver*, ir::Program*). All existing overrides (HoverPipeline,
GotoDefinitionPipeline, default) updated accordingly.
Pipeline::resolve now calls post_resolve(&resolver, result) before
the resolver is destroyed, so subclasses can access the IR-to-AST map.
resolver_method.cc: Added handling for the case where the cursor is on
the class name in a static call (e.g., Foo.bar with cursor on Foo).
A new contains_lsp_selection helper replaces hardcoded is_LspSelection()
checks to support nested dot expressions.
HoverPipeline: Added override keywords (previously missing).
Hover handler import path: import_path now emits a proper response
format (text + range) instead of a "Import: " prefix string.

Known Limitations

1. Cross-file rename

Cross-file references do work for statically-resolved symbols, since the
compiler loads all imported modules and the FindReferencesVisitor traverses
the entire ir::Program.

2. Local variable rename at definition

Works correctly thanks to find_definition_at_cursor.

Test Coverage

Prepare-Rename Tests (21 test files)

Each test places a cursor (^) and expects either a placeholder name or null:

Test	What it covers
`basic-prepare-rename-test`	Functions, locals, classes, fields, member access, literals, numbers
`abstract-method-prepare-rename-test`	Abstract method declarations
`block-param-prepare-rename-test`	Block parameter names
`class-in-static-call-prepare-rename-test`	Class name in `Foo.bar`
`constant-prepare-rename-test`	Global constant definition and usage
`extends-prepare-rename-test`	Class name in `extends` clause
`field-storing-prepare-rename-test`	Field-storing constructor parameter
`global-var-prepare-rename-test`	Global variable definition and usage
`instantiation-prepare-rename-test`	Class name at constructor call site
`locals-prepare-rename-test`	Parameters and local variables
`member-access-prepare-rename-test`	Virtual member access
`mixin-with-prepare-rename-test`	Mixin name in `with` clause
`named-ctor-prepare-rename-test`	Named constructor name
`named-param-prepare-rename-test`	Named parameter declaration and usage
`param-decl-prepare-rename-test`	Parameter at declaration site
`param-usage-prepare-rename-test`	Parameter at usage site
`return-null-prepare-rename-test`	`return` and `this` keywords (expect null)
`return-type-prepare-rename-test`	Class name as return type annotation
`static-const-prepare-rename-test`	Static class constant
`static-method-prepare-rename-test`	Static method access
`type-annot-prepare-rename-test`	Class name as type annotation

Rename Tests (7 test files)

Each test places a cursor and expects a specific number of renamed occurrences:

Test	What it covers
`basic-rename-test`	Top-level function definition + call sites
`locals-rename-test`	Parameters and local variables
`class-rename-test`	Class name (definition, type annotations, instantiation)
`global-rename-test`	Global variable (definition + usages)
`field-rename-test`	Class field (definition + internal usages)
`constant-rename-test`	Global constant (definition + usage)
`static-method-rename-test`	Static method (definition + call-site)

Test Infrastructure

prepare-rename-test-runner.toit — Toit test runner that parses /*^*/
annotations and sends textDocument/prepareRename requests.
rename-test-runner.toit — Toit test runner that sends textDocument/rename
requests and verifies the edit count.
run-all-prepare-rename-tests.toit — Optional convenience runner for
executing all prepare-rename tests locally (replaces a previous bash script).
Tests are integrated into CMake (tests/lsp/CMakeLists.txt) and are
runnable via ctest -C slow -R prepare-rename or ctest -C slow -R rename.

close2 · 2026-02-28T07:36:50Z

Virtual method call sites are now included

floitsch

Nice!
A few comments, but nothing major (I hope).

floitsch · 2026-03-01T19:00:13Z

+### 4. Node Types (`ir::Reference`)
+**Issue:** `HoverHandler` or other handlers fail to match the expected node type (e.g., `is_Method()`), resulting in empty responses or errors.
+**Cause:** The compiler's IR often wraps resolved nodes in `ir::Reference` nodes (like `ReferenceMethod`, `ReferenceClass`).
+**Fix:** Always check if a node is a `Reference` and unwrap it using `reference->target()` before performing type checks or casting.
+```cpp
+if (auto reference = node->as_Reference()) {
+  node = reference->target();
+}
+// Now it's safe to check node->is_Method()
+```


Also feels like a "one-time" issue. remove.

Done. File removed.

floitsch · 2026-03-01T19:00:52Z

+### 5. LSP Test Integration (`CMakeLists.txt`)
+**Issue:** Hover tests (`*hover-test.toit`) were not being executed correctly; they parsed but skipped hover assertions, silently passing.
+**Cause:** The test infrastructure relies on `CMakeLists.txt` mapping glob patterns (like `*definition-test.toit`) to their specific Toit runner scripts `goto-definition-test-runner.toit`. Because `*hover-test.toit` was initially unmapped, it fell back to a generic syntax check without firing the LSP mock client.
+**Fix:** Explicitly map `*hover-test.toit` files to `hover-test-runner.toit` within `tests/lsp/CMakeLists.txt` using the `add_test` CMake directive.


Not sure we need that either.

Done. File removed.

floitsch · 2026-03-01T19:01:31Z

+### 6. Test Runner Argument Order
+**Issue:** Directly invoking a test runner (e.g. `toit run tests/lsp/hover-test-runner.toit`) failed with `OUT_OF_BOUNDS` or `ILLEGAL_UTF_8` errors.
+**Cause:** Target classes like `LocationCompilerTestRunner` enforce a strict arguments structure: `[test_file_path, compiler_executable, lsp_server_script, mock_compiler]`. Misordering strings causes the framework to treat binary paths as source tests.
+**Fix:** Validate the exact invocation from `CTestTestfile.cmake`. Running tests via `ctest -R <test-name>` or `make test` ensures correct formatting rather than manually assembling the CLI parameters.
+


By using the ctest this shouldn't happen. We already mention earlier that 'ctest' is the way to go. -> remove.

Done. File removed.

floitsch · 2026-03-01T19:03:27Z

+**Limitation:** This fallback only covers program-level definitions. Local variables inside method bodies are not in the program-level lists, so renaming a local requires placing the cursor on a usage.
+
+## Future Improvements
+- **Toitdoc Markdown**: The `HoverHandler` implementation for `ToitdocMarkdownVisitor` can be extended to support more advanced markdown features.


That's more of a question of what Toitdocs support. I think it currently handles the full toitdoc.

Done. File removed.

floitsch · 2026-03-01T19:04:03Z

+**Cause:** Target classes like `LocationCompilerTestRunner` enforce a strict arguments structure: `[test_file_path, compiler_executable, lsp_server_script, mock_compiler]`. Misordering strings causes the framework to treat binary paths as source tests.
+**Fix:** Validate the exact invocation from `CTestTestfile.cmake`. Running tests via `ctest -R <test-name>` or `make test` ensures correct formatting rather than manually assembling the CLI parameters.
+
+### 7. Exact Toitdoc Expectations (`/*^ ... */`)


The rest looks like one-time issues.

Done. File removed.

floitsch

Nice!
A few comments, but nothing major (I hope).

- Update copyrights to '2026 Toit contributors' - Refactor HoverHandler: remove path/line/column params, add emit_toitdoc_ref/emit_string to LspHoverProtocol - Handle length==-1 toitdoc ref protocol in compiler.toit - Simplify show() to delegate to expord() in hover.cc - Use Source::Location in emit_hover for cleaner source lookup - Add is_AnyReference() helper to ir::Node - Rename is_sdk_target to is_renameable (also checks non-path packages) - Simplify unwrap_reference using base Reference::target() - Consolidate FindReferencesVisitor visit_Reference* into single visit_Reference - Add documentation comments to VirtualCallFilter methods - Use ERROR_PACKAGE_ID for null source fallback - Remove redundant participating_classes_ check - Clean up protocol.cc includes

Add a new REFERENCES mode to the C++ compiler that finds all references to a symbol by: 1. Resolving the target definition at the cursor position 2. Walking the IR with FindReferencesVisitor to locate all usages 3. Using ir_to_ast_map to emit exact token ranges Wire this into the Toit LSP server with: - find-references method in compiler.toit - textDocument/rename handler in server.toit - RenameParams and WorkspaceEdit protocol classes - Rename test infrastructure (runner + test data + CMake)

more fixes

Bug: prepareRename on a named constructor call site (e.g., cursor on deserialize in IpAddress.deserialize) incorrectly returned the class name IpAddress instead of the constructor name deserialize. Root cause: In rename.cc:40-52, FindReferencesHandler::call_static unconditionally redirected ALL constructors/factories to their holder class. This was correct for unnamed constructors (where MyObj calls an unnamed constructor — the user wants to rename the class), but wrong for named constructors/factories like constructor.deserialize (where the user wants to rename deserialize). Fix: Added a guard that only redirects to the holder class when the constructor's name matches the class name or is Symbols::constructor (both indicating unnamed constructors). Named constructors keep their own identity as the rename target. Test: Created named-ctor-call-prepare-rename-test.toit with two test cases: Cursor on the named constructor create in MyClass.create 42 → expects create Cursor on the class name MyClass in MyClass.create 42 → expects MyClass

Test for named contructor rename

Root cause: prepareRename on a named argument like --network at a call site to a cross-module function returned null because ir::Method::parameters() is empty for imported methods at LSP resolution time. The resolver populates parameters for the entry module first; imported module methods only have their ResolutionShape (which includes named parameter names) but not the actual ir::Parameter* nodes. Fix in rename.h: The call_static_named method now has a two-step lookup: First tries ir_method->parameters() (works for same-module methods) Falls back to resolution_shape().names() for cross-module methods where parameters aren't yet populated. When a match is found, creates a temporary ir::Local node with the correct name and call-site range.

Uses some heuristic to improve rename functionality.

- Update copyrights to '2026 Toit contributors' - Refactor HoverHandler: remove path/line/column params, add emit_toitdoc_ref/emit_string to LspHoverProtocol - Handle length==-1 toitdoc ref protocol in compiler.toit - Simplify show() to delegate to expord() in hover.cc - Use Source::Location in emit_hover for cleaner source lookup - Add is_AnyReference() helper to ir::Node - Rename is_sdk_target to is_renameable (also checks non-path packages) - Simplify unwrap_reference using base Reference::target() - Consolidate FindReferencesVisitor visit_Reference* into single visit_Reference - Add documentation comments to VirtualCallFilter methods - Use ERROR_PACKAGE_ID for null source fallback - Remove redundant participating_classes_ check - Clean up protocol.cc includes

Apply all maintainer review feedback items except toitdoc references: - Fix ir_to_ast_map keying for calls with block/lambda args: when CallBuilder::with_hoisted_args wraps a CallStatic in a Sequence, unwrap to store the actual Call node as the map key. This fixes named-arg call-site rename for overloaded functions with blocks. - Add overloaded-named-param tests verifying that renaming --some-name on one overload does not affect the other overload (both rename and prepare-rename). - Extract add-transitive-reverse-deps_ helper in server.toit, shared by both compilation-entry-points-for_ and analyze. - Collect show/export references during resolution for the rename pipeline to find import-clause references. - Various test fixes: correct caret positions for indented call sites, add SDK target rejection, operator rename tests. All 52 rename tests pass (26 rename + 26 prepare-rename).

When renaming a symbol that is referenced in toitdoc comments (e.g. $helper), the rename now correctly updates those references as well. Part A: The FindReferencesHandler::toitdoc_ref callback now captures the target when the cursor is on a toitdoc reference, enabling prepareRename to work from within toitdoc comments. Part B: emit_all_references scans the ToitdocRegistry for IR refs matching the rename target, then uses parallel AST arrays to emit the correct selection ranges for each toitdoc reference. Addresses maintainer feedback item 7 (toitdoc reference rename).

add more tests.

- Restore .NOTPARALLEL in Makefile (accidentally removed) - Revert unnecessary --radix=10 additions (10 is the default) - Remove dead set_toitdocs() method (resolver now writes directly) - Fix trailing whitespace in resolver_method.cc

- Extract walk_class_hierarchy helper into selection.h, used by hover, completion, and rename handlers to reduce duplication - rename.cc: rename variable t→target, fix resolution order (resolved2→resolved1→candidates), combine interface/mixin loops, add limitation comment to hierarchy computation - rename.h: fix long lines, trim is_sdk_target comment - compiler.cc: add find_references and prepare_rename Kind enum entries - compiler.toit: remove defensive --if-error clauses and try/finally drain loop, rename length→kind - hover.cc: make import_path a no-op (emitting during import resolution disrupts the file-server protocol) - resolver.cc: remove stale 'is_operator_name defined in token.h' comment - Delete brittle hover tests (pipe, wait, wait-pkg) - Fix import-hover-test to use local import (.imported) - Add 'this' test case to return-null-prepare-rename-test

- package_id_for_range now falls back to ERROR_PACKAGE_ID instead of ENTRY_PACKAGE_ID when the source cannot be determined. - Remove unnecessary participating_classes_ check between the interface and mixin loops in Phase 3 of compute_participating_classes.

Rewrite the interface/mixin discovery phase in compute_participating_classes to walk outward from existing participants and up through interface inheritance. This fixes the limitation where multi-path hierarchies (e.g., I1 extended by both I2 and I3 with different implementing classes) would not be fully discovered. The new algorithm: 1. Walks outward from participants to their interfaces/mixins. 2. Walks up through interface/mixin super-classes. 3. Checks for reverse connections (classes implementing a matching interface/mixin). 4. Iterates to a fixed point so all transitive connections are found. Also merges the separate interface and mixin loops using a conditional as suggested in review.

…name.cc Move VirtualCallFilter and FindReferencesVisitor class definitions from rename.h to rename.cc since they are implementation details of the reference-finding logic. Extract find_and_emit_all_references() as a free function in rename.cc (declared in rename.h), replacing the FindReferencesPipeline::emit_all_references private method. This moves the reference-finding logic from compiler.cc to the rename handler module where it belongs.

Refuse to rename symbols defined in non-path packages (git dependencies) since their source files are read-only. Also skip emitting individual references that fall in non-path packages. Only the entry package and local path packages are considered editable.

Add coverage for renaming from return-type annotations and mixin with clauses. Both prepare-rename tests existed, but the corresponding full rename tests were missing.

floitsch · 2026-03-22T21:27:01Z

+}
+
+void HoverHandler::show(ast::Node* node, ResolutionEntry entry, ModuleScope* scope) {
+  expord(node, entry, scope);


A little bit hackish. I would prefer if we had a helper function.

Done. The emit_hover function covers this logic now — it unwraps references, extracts the range, and delegates to the protocol. The deferred-node pattern is needed for the case where the range isn't valid during initial resolution.

I think that was a misunderstanding. I just meant that calling expord from show is hackish, and having a shared function would be nice.
That said: having the few lines duplicated isn't that bad either.

Extracted a shared emit_hover_for_entry helper that both show and expord now call.

Add a definition() callback to LspSelectionHandler that the resolver calls whenever it creates an IR definition node whose AST name is an LspSelection (i.e. the cursor is on the definition's name). This replaces the post-resolution find_definition_at_cursor() function that iterated the entire program looking for ranges containing the cursor. The resolver naturally knows when it's creating definitions and can notify the handler immediately. The callback covers: classes, top-level functions, globals, class methods (including named constructors/factories), fields, parameters, and local variables.

The test runner now calls prepareRename first to obtain the canonical symbol name, then checks that every edit range in the rename response covers text matching that name. Underscores are normalized to hyphens before comparison to account for Toit's interchangeable identifier syntax.

Document the purpose of the ir_to_ast_map in resolver.h: it preserves AST nodes for the small set of IR nodes where the rename/reference visitor needs source ranges that the IR does not carry (setters, type annotations, named arguments, field-storing parameters). Fix inconsistent line-wrapping of the run command in compiler.toit's prepare-rename method to match the style used by other methods in the same file.

…ting - Move ^ markers to same line as /* in 17 test files - Add trailing periods to // Test: comments in 10 test files - Add trailing periods to // 1-based comments in compiler.cc - Format post_type_check if/else on multiple lines - Remove redundant ASSERT in resolver_method.cc - Fix --my-flag=true to --my-flag in named-param test

- Handle LSP selection marker on operator tokens in the parser. When the marker lands on e.g. '+' in 'operator +', the scanner produces a spurious empty IDENTIFIER with is_lsp_selection set. The parser now detects this and creates an LspSelection node for the operator name. - Add operator name checks in emit_prepare_rename() and find_and_emit_all_references() to exit early for operators, which cannot be meaningfully renamed. - Add braces to if/else in visit_Reference (review thread 24). - Add comment explaining why ir_to_ast_map is preferred over node->range() for references (review thread 23). - Add comment explaining hover protocol encoding in compiler.toit.

floitsch

Also make sure to run make test-health to avoid warnings in the Toit files.

floitsch · 2026-03-27T16:55:47Z

@@ -0,0 +1,427 @@
+# LSP Rename Implementation — Architecture & Design


Nice for review, but I would remove it before actually committing.

Done. Deleted docs/LSP_RENAME.md.

floitsch · 2026-03-27T16:57:34Z

+- The resolver's `call_static_named` callback does not fire for dot-call
+  virtual invocations (only for statically-resolved calls and super calls).
+
+This limitation is well-scoped: named parameters on virtual methods are less


I disagree with that, but I think it's better to land something and than improve on it.

Agreed, landing and iterating is the right approach. The doc is now deleted.

floitsch · 2026-03-27T17:05:30Z

+      return false;
+    });
+  } else if (type.is_any()) {
+    for (int i = 0; i < classes.length(); i++) {


I wonder if that's not a bit too agressive. I could imagine getting the completely wrong hover here.

Good point. Removed the type.is_any() fallback — it could easily produce wrong hover results when the type is unknown.

floitsch · 2026-03-28T23:25:04Z

+}
+
+void HoverHandler::show(ast::Node* node, ResolutionEntry entry, ModuleScope* scope) {
+  expord(node, entry, scope);


I think that was a misunderstanding. I just meant that calling expord from show is hackish, and having a shared function would be nice.
That said: having the few lines duplicated isn't that bad either.

floitsch · 2026-03-29T19:58:03Z

+class Greeter:
+  greet name/string -> string:
+/*^
+  3


All the rename tests still have integers. Those should be unnecessary now.
Some tests still only have integers.

floitsch · 2026-03-29T20:07:51Z

+Rename test runner that verifies rename correctness by:
+1. Copying test files to a temp directory.
+2. Sending rename requests at marked positions.
+3. Applying all returned edits.
+4. Re-analyzing the modified files to check they still compile.


That's a bit heavy.
I would just check that the resulted ranges correspond to the ones we have marked. This is, why i would prefer to remove the integers and have "links" to the identifiers that need to be changed. Not 100% sure how we would want to check the length. The easiest is probably to just look for a complete identifier at the target-location. A rename should never change only part of an identifier.

Done. The test runner now uses named location lists instead of integer counts. Each test line specifies [loc1, loc2, ...] linking to /*@ loc1 */ markers, which also encode the expected column. Empty [] means not renamable.

I like the new approach, but we still have the heavy old approach in place.
I don't think it's necessary that we do the actual edits and reanalyze the code.

However, we should need to check that the returned ranges fit the complete identifier.

Done. Removed apply-rename-edits and the heavy edit+reanalyze steps (5-7). The runner now validates that edit ranges cover the complete identifier and checks that locations match across all references.

…x bugs Major changes addressing all review comments from floitsch: C++ (rename.cc/rename.h): - Rewrite rename.h as declarations-only header (no inline bodies) - Use anonymous namespaces and static for file-local helpers - Use deterministic Set<T> instead of UnorderedSet<T> for participating_classes - Use Source* pointer comparison instead of path string comparison - Extract helper functions for find_and_emit_all_references - Add ASSERTs for ir/ast list length agreement - Remove deprecated holder-name check in is_unnamed_constructor_or_factory - Remove find_identifier_on_same_line (use class_reference_range instead) - Use // comment style throughout - Add definition_emitted_ flag to fix duplicate definition emission bug - Add range_matches_name guard for implicit super constructor calls - Add TODO about renaming params across all overloads C++ (hover.cc/hover.h): - Add TODO for import_path delayed callback - Remove aggressive type.is_any() fallback in call_virtual - Extract emit_hover_for_entry shared helper for show/export - Change emit_hover to call exit(0) instead of setting has_emitted_ flag - Simplify finalize() to just check deferred_node_ - Add definition override Tests: - Delete docs/LSP_RENAME.md - Rewrite rename-test-runner.toit to parse named location lists - Update extract-locations in utils.toit to walk backward past consecutive marker blocks - Convert all ~30 rename test files to use named location markers (/*@ name */ format with [loc1, loc2] expected lists) - Fix column-0 markers using multi-line format All 31 rename tests pass.

floitsch · 2026-03-31T09:55:48Z

+  Source::Range definition_range = target_range(target);
+
+  // For named constructors and factories, ir::Method::range() covers only the
+  // "constructor" keyword (set from ast::Method::selection_range() during


I meant that it's a bug that there isn't any range in the ir::Method that contains the named constructor name.
Add a TODO that we should investigate whether we want to change that.

floitsch · 2026-03-31T09:56:26Z

-  return node;
-}
+// Unwraps an ir::Reference* node to its underlying definition.
+ir::Node* unwrap_reference(ir::Node* node);


Unless these functions are used outside rename.h|cc would remove all these forward declarations from the header (and make them static to the cc file).
If other functions in the header use them, then they should just go into the cc file.

If they are used outside rename.cc, then they are probably in the wrong place in the first place.

Done. Moved unwrap_reference, target_name, target_range, and is_sdk_target to selection.h/cc since they're shared with compiler.cc (used in PrepareRenamePipeline).

floitsch · 2026-03-31T10:00:59Z

 foo x:
 /*
+@ foo-def */
+/*


It would be nicer, if we could have a single comment:

/* @ foo-def ^ [foo-def, foo-call] */

Here and in all other rename tests.

Done. Consolidated all 32 rename test files to use single comment blocks combining @ name, ^, and [list] markers together. Also fixed extract-locations in utils.toit to preserve the @ column from indented lines.

floitsch · 2026-03-31T10:05:18Z

+Rename test runner that verifies rename correctness by:
+1. Copying test files to a temp directory.
+2. Sending rename requests at marked positions.
+3. Applying all returned edits.
+4. Re-analyzing the modified files to check they still compile.


I like the new approach, but we still have the heavy old approach in place.
I don't think it's necessary that we do the actual edits and reanalyze the code.

However, we should need to check that the returned ranges fit the complete identifier.

…s, simplify runner - Move unwrap_reference, target_name, target_range, is_sdk_target from rename.h/cc to selection.h/cc since they're used by compiler.cc - Add TODO for named constructor name range in ir::Method - Consolidate separate /*@ name */ and /* ^ [list] */ test markers into single comment blocks across all 32 rename test files - Simplify rename-test-runner: remove apply-rename-edits and the heavy edit+reanalyze steps (5-7), just validate edit ranges cover identifiers - Fix extract-locations in utils.toit to preserve @ column from indented lines in multi-line blocks

…s for pkg-tar warnings

…erscores

floitsch

LGTM.
Thanks!

…8-char path limit The Toit LSP compiler on Windows silently fails to analyze files when the full path exceeds 127 characters. The previous temp directory prefix 'lsp_rename_test-' combined with the GUID and long test filenames pushed paths over this limit. Shortened to 'rn-' to stay well under. Also moved file-contents/temp-to-original maps outside the per-marker loop (built once), and removed unnecessary send-did-change calls and debug prints.

floitsch

LGTM.

floitsch · 2026-04-03T11:32:40Z

+  // Use a short prefix to keep paths under 128 characters, which avoids
+  //   a path-length issue in the LSP compiler on Windows.
+  temp-dir := directory.mkdtemp "/tmp/rn-"
  try:


I would prefer a with-tmp-directory, but let's not delay this PR even longer.

close2 mentioned this pull request Mar 1, 2026

lsp selection range #3003

Merged

floitsch reviewed Mar 1, 2026

View reviewed changes

close2 added 14 commits March 20, 2026 23:14

feat: implement LSP hover functionality

4ed543f

implement lsp hover using mostly toit code

a407e7a

feat(lsp): implement textDocument/rename via C++ REFERENCES pipeline

74376a5

more fixes

feat(lsp): rename named constructor test

3b3ec84

Test for named contructor rename

feat(lsp): rename virtual call sites

138e8dc

Uses some heuristic to improve rename functionality.

feedback

9a53fb2

feat: impr rename

d987364

add more tests.

fix: minor fixes and revert copyright changes

c2127f5

close2 force-pushed the CL/lsp_rename branch from bac9923 to c2127f5 Compare March 20, 2026 22:15

fix: address review feedback

285672a

- Restore .NOTPARALLEL in Makefile (accidentally removed) - Revert unnecessary --radix=10 additions (10 is the default) - Remove dead set_toitdocs() method (resolver now writes directly) - Fix trailing whitespace in resolver_method.cc

close2 changed the title ~~lsp rename~~ feat(lsp): implement rename, prepareRename, and hover support Mar 21, 2026

close2 added 6 commits March 22, 2026 17:37

Add rename tests for return-type and mixin-with clauses

2224344

Add coverage for renaming from return-type annotations and mixin with clauses. Both prepare-rename tests existed, but the corresponding full rename tests were missing.

floitsch reviewed Mar 22, 2026

View reviewed changes

close2 added 5 commits March 23, 2026 07:16

feedback

d4c6539

close2 force-pushed the CL/lsp_rename branch from e6dab58 to 131b2bf Compare March 25, 2026 09:08

close2 added 2 commits March 28, 2026 08:57

Improve class rename references and test coverage

d53828d

Add prefixed static-call LSP coverage

fd6a7e3

floitsch reviewed Mar 29, 2026

View reviewed changes

close2 added 3 commits March 31, 2026 00:57

Remove unusual separator comment lines from rename.cc

1fa98b3

minor comment changes

1f86452

floitsch reviewed Mar 31, 2026

View reviewed changes

close2 added 6 commits March 31, 2026 22:40

Fix health test failures: compile errors in test files, add gold file…

6ae799d

…s for pkg-tar warnings

Use /// for doc comments in rename.h to match codebase convention

f7d921f

Use idiomatic Toit in hover test: show clause, hyphens instead of und…

7871b12

…erscores

Fix Windows path separators in rename test runner with fs.to-slash

8f86225

Add debug prints for Windows rename test failures

4beac38

floitsch approved these changes Apr 2, 2026

View reviewed changes

floitsch approved these changes Apr 3, 2026

View reviewed changes

floitsch merged commit be99d56 into master Apr 3, 2026
59 of 65 checks passed

floitsch deleted the CL/lsp_rename branch April 3, 2026 11:35

		@@ -0,0 +1,427 @@
		# LSP Rename Implementation — Architecture & Design

Conversation

close2 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LSP Rename, Prepare-Rename, and Hover Support

Summary

Architecture

C++ Compiler Backend (src/compiler/)

Key Design Decisions

Toit LSP Server (tools/lsp/server/)

Changes to Existing Code

Known Limitations

1. Cross-file rename

2. Local variable rename at definition

Test Coverage

Prepare-Rename Tests (21 test files)

Rename Tests (7 test files)

Test Infrastructure

Uh oh!

close2 commented Feb 28, 2026

Uh oh!

floitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

close2 commented Feb 27, 2026 •

edited

Loading

C++ Compiler Backend (`src/compiler/`)

Toit LSP Server (`tools/lsp/server/`)