surround_context: resolve trailing edge of a symbol (#1038 / #1027) - #340
Conversation
`SurroundContext.Toxic` mirrored `Code.Fragment.surround_context/2`, which returns `:none` when the cursor sits one column past the last character of a symbol, and returns the enclosing remote call when it sits at the end of an alias before a `.`. Both break navigation: goto-definition / references / hover fail at the visual end of a name (elixir-lsp/elixir-ls#1038), and an alias at the end resolves to the function instead of the module (#1027 - the upstream lexical fix elixir-lang/elixir#13150 was reverted). Because toxic2 gives every node an exact end-exclusive range, `resolve/3` fixes this precisely by classifying at the cursor and one column to its left: * at the trailing edge of an alias / var / atom / attribute leaf, the leaf wins even over an enclosing dot -> end of `Foo` in `Foo.bar()` is the module `Foo`, not the remote call (#1027); * otherwise, if the cursor resolves to nothing, retry one column left, but only surface a genuine navigation target (a `key:` / operator / sigil trailing edge stays `:none`) -> end of a bare symbol or of a whole remote call resolves (#1038). This never reaches across whitespace or a separator: when the cursor is `:none`, a node found one column left cannot extend past it (contiguous ranges), so it must end exactly there. Fuzzed across lib + fixtures at every column: 0 results where the cursor falls outside the returned [begin, end], and by construction the result is `:none` only where the pre-existing classification was, so nothing regresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XRd15BX6wBpZ8ThaNSo232
… test Code.Fragment.surround_context behavior at the arity slash varies across Elixir 1.16-1.20; assert only Toxic's (version-independent) trailing-edge result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XRd15BX6wBpZ8ThaNSo232
The trailing-edge override that makes the end of an alias before `.` resolve to the module (#1027) was firing for any name leaf, including bare-atom / var / attribute LHS of a dot. That regressed erlang remote calls: hovering the `.` in `:timer.sleep(...)` resolved the module `:timer` instead of the function. #1027 / elixir-lang/elixir#13150 is specifically about `:alias` (uppercase Elixir modules), so restrict the override to `{:alias, _}`. Bare atoms / vars / attributes still resolve at their true trailing edges via the `:none` retry (when they are not shadowing a remote call), so #1038 is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XRd15BX6wBpZ8ThaNSo232
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR adjusts ElixirSense.Core.SurroundContext.Toxic.surround_context/2 to resolve symbols at the trailing edge (caret one column past the last character), fixing navigation failures at the visual end of names and ensuring an alias before . resolves to the module rather than the enclosing remote call.
Changes:
- Introduces
resolve/3to classify at the cursor and (when applicable) one column to the left, with special handling for alias-vs-dot trailing-edge behavior. - Restricts the one-column-left retry to “navigable” context shapes to avoid surfacing non-navigation lexical contexts.
- Updates and extends tests with dedicated trailing-edge coverage and adjusts two regression expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/elixir_sense/core/surround_context/toxic_test.exs | Updates regression assertions and adds a new describe block covering trailing-edge behavior and false-positive guards. |
| lib/elixir_sense/core/surround_context/toxic.ex | Replaces direct classification with resolve/3 to support trailing-edge symbol resolution while preserving fallbacks to Code.Fragment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| defp resolve(ast, source, {line, column} = position) do | ||
| r0 = classify_at(ast, source, position) | ||
|
|
||
| if column > 1 do | ||
| r1 = classify_at(ast, source, {line, column - 1}) | ||
|
|
||
| cond do | ||
| # #1027: at the trailing edge of a name leaf, the leaf wins even over an enclosing dot | ||
| trailing_name_leaf?(r1, position) -> r1 | ||
| # otherwise keep whatever the cursor itself resolved to | ||
| r0 != :none -> r0 | ||
| # #1038: the cursor resolved to nothing - retry one column left, but only surface a genuine | ||
| # navigation target (a `key:` / operator / sigil trailing edge stays `:none`, as before) | ||
| navigable_result?(r1) -> r1 | ||
| true -> :none | ||
| end | ||
| else | ||
| r0 | ||
| end | ||
| end |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee88a26f47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| cond do | ||
| # #1027: at the trailing edge of a name leaf, the leaf wins even over an enclosing dot | ||
| trailing_name_leaf?(r1, position) -> r1 |
There was a problem hiding this comment.
When an alias is immediately followed by an operator, e.g. Foo+1 or Foo==Bar, r0 is the operator context at that cursor column, but this first condition returns r1 just because the alias ended there. That regresses hover/definition on the operator itself (which SurroundContext.to_binding/2 supports via {:operator, _}) and makes the operator column resolve to the left-hand module instead; the override should be limited to the dot case that #1027 targets, or to positions where r0 is :none.
Useful? React with 👍 / 👎.
| defp trailing_name_leaf?(%{context: {:alias, _}, end: ending}, position) | ||
| when ending == position, | ||
| do: true |
There was a problem hiding this comment.
For aliases such as __MODULE__.Foo.bar, the one-column-left fallback returns the alias shape {:alias, {:local_or_var, '__MODULE__'}, 'Foo'} ending at the dot, while r0 is the enclosing remote call; because this helper only matches two-element aliases, the new #1027 rule still resolves the function instead of the module at the end of __MODULE__.Foo. to_binding/2 already knows how to resolve this qualified alias form, so it should participate in the same trailing-edge override.
Useful? React with 👍 / 👎.
…lified aliases Review of #340: - Perf: classify one column left lazily. The fast path returns immediately when the cursor already sits inside a real, in-range, non-dot result, so the common navigation path does a single AST descent instead of two. - Operators: the alias trailing-edge override no longer fires when the cursor is on an operator (`Foo+1` / `Foo==Bar`) - those keep the operator context. The override is limited to the remote-call dot case (and to fallbacks that don't cover the cursor), which is what #1027 targets. - Qualified aliases: the override now also matches `{:alias, _, _}` (e.g. `__MODULE__.Foo` in `__MODULE__.Foo.bar`), which `to_binding/2` already resolves. A `covers?/2` guard keeps a lexical `Code.Fragment` fallback (whose begin can point past the cursor) from short-circuiting the fast path, so the alias override can still win there. Fuzzed every column across lib: the out-of-range result set is now byte-identical to the pre-change baseline (zero new false positives), while the end-of-alias regressions the naive dot-only gate introduced are fixed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XRd15BX6wBpZ8ThaNSo232
What
ElixirSense.Core.SurroundContext.Toxic.surround_context/2mirroredCode.Fragment.surround_context/2, which returns:nonewhen the cursor is one column past the last character of a symbol, and returns the enclosing remote call when the cursor is at the end of an alias before a.. Both break navigation:a65dae9, was reverted).How
Because toxic2 gives every node an exact end-exclusive range,
resolve/3classifies at the cursor and one column to its left:FooinFoo.bar()is the moduleFoo) — #1027;key:/operator/sigil trailing edge stays:none) — #1038.Safety
This never reaches across whitespace/separators: when the cursor is
:none, a node found one column left cannot extend past it (contiguous integer ranges), so it must end exactly there. Fuzzed across lib + fixtures at every column:[begin, end];:noneonly where the pre-existing classification was — nothing regresses.Only the two differential tests whose asserted column is now a trailing edge changed (both to the more-correct #1027/#1038 result); a dedicated
trailing edgedescribe block covers the new behavior plus false-positive guards.🤖 Generated with Claude Code