Conversation
|
@JesseHerrick would be great to know your thoughts about this approach and naming. |
|
Hey @flowerett, thanks for the PR! This same thing happens with all editors and LSPs when there are multiple function heads for the same definition with the same arity. IMO that is a feature. While it's something we could change in the LSP, this is something I'd actually leave up to the LSP client and editor to configure to their liking. Editors should be able to just say "give me the first option" instead of opening the list if they want it. In NeoVim this isn't too tricky to configure, but maybe in Zed it's a pain. Thoughts? |
ef4e0ff to
9e448a6
Compare
|
Hey @JesseHerrick, thanks for the feedback!
So for Zed users today there's no client-side option at all. While validating the original report locally I also found two real bugs that were producing multi-location results for calls with a single intended target: I've created a small Elixir project to debug and reproduce the issues - [dummy_dexter_ex}(https://github.com/flowerett/dummy_dexter_ex) My preference would be to keep it in this PR with default "all", this won't change default behavior and Zed users get a workable option - "first" if they want to. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit bdcc215. Configure here.
|
Thanks @flowerett. You're right - I was confusing the go-to-ref arity checking with go-to-definition. We indeed should add that filtering. I'm totally fine with adding this change as default off so that those who want it can enable it. I'll review the PR later today. |
|
Hey @JesseHerrick should I finish this fix or is it better to close the PR, wdyt? |
|
Hey @flowerett, sorry, yes I think we should finish this one up. There's two important elements:
|
When a function has multiple heads/clauses, editors like Zed show a picker UI instead of jumping directly.
The new "definitionStyle" initializationOption ("all" or "first") lets users choose whether to return all definition sites or just the first one.
Two related bugs made goto-definition return multiple Location entries for calls a human reads as resolving to a single definition. Zed renders those multi-location results as multi-cursor selections (rather than a picker), which is what knoebber reported on issue remoteoss#38. 1. LookupFunction did not filter by arity, so `Foo.square(3)` returned both `square/1` and `square/2` rows when both were defined. Added LookupFunctionByArity and compute the call-site arity in Definition() via a new TokenizedFile.ArityAtCallsite helper that handles parens, zero-arg calls, `&Foo.bar/2` captures, and pipe context. 2. lookupFollowDelegate only followed delegates when every row was a delegate, so `defdelegate foo/1` + `def foo/2` in the same module returned both rows instead of following the /1 delegate. Rewrote it to partition by arity and follow per-arity, and added LookupFollowDelegateByArity. Existing LookupFunction / LookupFollowDelegate signatures are unchanged so the 20+ other callers still work; only Definition() is arity-aware. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bdcc215 to
380912e
Compare
|
Hey @JesseHerrick ! |
There was a problem hiding this comment.
Some findings
- [important]
internal/lsp/elixir.go:85miscomputes arity for valid nested, block, capture-like, interpolation, and parenthesis-free forms, so go-to-definition can select the wrong function. - [important]
internal/lsp/server.go:1134leaves bare current-module calls on the old first-name-match path, bypassing both arity filtering anddefinitionStyle. - [important]
internal/lsp/name_navigation.go:74filters use-chain results after choosing a provider, so an earlier matching-arity import is missed; initialization coverage and the three direct token loops also need follow-up.
| return arityAtCallsite(tf.tokens, tf.source, tf.lineStarts, line, startCol, endCol) | ||
| } | ||
|
|
||
| func arityAtCallsite(tokens []parser.Token, source []byte, lineStarts []int, line, startCol, endCol int) int { |
There was a problem hiding this comment.
[important] Exact-arity filtering requires Elixir expression nesting, but this scanner only tracks delimiters: it counts commas inside fn bodies as outer arguments, omits the implicit keyword-list argument from trailing do blocks, treats /N as a capture without requiring &, and cannot inspect interpolation or parenthesis-free calls. The later module fallback cannot heal a wrong count—it returns the module or an unrelated arity—so valid calls can jump to the wrong definition. Derive arity with a block-aware TokenWalker, and decline exact filtering when the call form remains ambiguous.
|
|
||
| expr := tf.ResolveModuleExpr(exprCtx.Expr(), lineNum) | ||
| moduleRef, functionName := ExtractModuleAndFunction(expr) | ||
| callArity := tf.ArityAtCallsite(lineNum, exprCtx.ExprStart, exprCtx.ExprEnd) |
There was a problem hiding this comment.
[important] The inferred arity and definitionStyle are bypassed for bare calls into the current module: the unconditional FindFunctionDefinition shortcut returns the first name match only. The later semantic lookup cannot heal this early return, so foo(a, b) can jump to an earlier foo/1, while style all returns only one same-arity head. Preserve unsaved-buffer support, but collect definitions by arity and apply definitionStyle before returning.
flowerett
left a comment
There was a problem hiding this comment.
New findings
- [important]
internal/lsp/elixir.go:191still assigns special-form commas to the outer call, so exact lookup can select the wrong arity or fall back to the module. (narrowed) - [important]
internal/lsp/elixir.go:298scans every module in the buffer, so a bare call in the first module can navigate to an unrelated nested or later declaration. (new) - [important]
internal/lsp/server.go:2538shares cycle state across parameterized uses, so a valid earlier provider can be skipped and navigation falls back to the consumer module. (repeat)
Round 2 · reviewed at fd8780a
| case parser.TokComma: | ||
| if w.Depth() == 1 && w.BlockDepth() == 0 { | ||
| // Elixir's trailing keyword syntax is one list argument even | ||
| // though its entries are separated by top-level commas. | ||
| if keywordTail { | ||
| w.Advance() | ||
| continue | ||
| } | ||
| args++ | ||
| hasContent = false | ||
| w.Advance() | ||
| continue |
There was a problem hiding this comment.
[important] Exact arity remains wrong when an argument is an unparenthesized special form: target(if ready, do: x, else: y) and target(for x <- xs, y <- ys, do: x) count the inner commas as arguments of target. The later semantic lookup cannot heal the incorrect exact-arity selection, so navigation can jump to another arity or the module declaration. Track expression ownership for special forms, or return unknown arity when comma ownership is ambiguous.
| func (tf *TokenizedFile) FindDefinitionLines(functionName string, arity int, preferType bool) []int { | ||
| var functionLines, typeLines []int | ||
| w := parser.NewTokenWalker(tf.source, tf.tokens) | ||
| for w.More() { | ||
| i := w.Pos() | ||
| tok := w.Current() | ||
| w.Advance() | ||
| switch tok.Kind { | ||
| case parser.TokDef, parser.TokDefp, parser.TokDefmacro, parser.TokDefmacrop, | ||
| parser.TokDefguard, parser.TokDefguardp, parser.TokDefdelegate: | ||
| name, j, ok := parser.StaticDeclarationName(tf.source, tf.tokens, tf.n, i) | ||
| if !ok || name != functionName { | ||
| continue | ||
| } | ||
| maxArity, defaultCount := 0, 0 | ||
| pj := tokNextSig(tf.tokens, tf.n, j+1) | ||
| if pj < tf.n && tf.tokens[pj].Kind == parser.TokOpenParen { | ||
| maxArity, defaultCount, _, _ = parser.CollectParams(tf.source, tf.tokens, tf.n, pj) | ||
| } | ||
| if arity < 0 || (arity >= maxArity-defaultCount && arity <= maxArity) { | ||
| functionLines = append(functionLines, tok.Line) |
There was a problem hiding this comment.
[important] This scan collects matching declarations from every module in the buffer, so a bare foo() in the first module can return foo/0 declarations from nested or later modules. The module-aware semantic lookup cannot heal this because the current-buffer path returns early. Restrict declarations to the scope belonging to fullModule.
| for i := len(useCalls) - 1; i >= 0; i-- { | ||
| if result := s.lookupInUsingEntryForWithFollow(useCalls[i].Module, functionName, useCalls[i].dispatchAtom(), useCalls[i].Opts, visited, followDelegates); result != nil { | ||
| if result := s.lookupInUsingEntryForWithFollow(useCalls[i].Module, functionName, useCalls[i].dispatchAtom(), useCalls[i].Opts, visited, followDelegates, arity); result != nil { | ||
| return result | ||
| } |
There was a problem hiding this comment.
[important] Exact-arity use-chain lookup still misses providers when the same using module appears multiple times with different opts: the later call marks the module visited even when its selected provider lacks the requested arity, so the earlier call is skipped and the module fallback returns the consumer declaration. Include resolved opts in the visit key or use independent cycle state for each top-level use call.

Solves this issue
When a function has multiple heads/clauses, editors like Zed show a picker UI instead of jumping directly.
The new "definitionStyle" initializationOption ("all" or "first") lets users choose whether to return all definition sites or just the first one.
the new initializationOption will look like:
Note
Medium Risk
Touches go-to-definition resolution and delegate-following logic, so incorrect arity detection or filtering could cause missed/incorrect definition targets in some call forms.
Overview
Adds a new LSP
initializationOptions.definitionStylesetting (alldefault,firstoptional) to control whethertextDocument/definitionreturns all matching function heads or only the first location (to avoid editor pickers).Updates go-to-definition to infer callsite arity (including capture syntax and pipe-adjusted calls) and uses new arity-filtered store queries (
LookupFunctionByArity,LookupFollowDelegateByArity) to avoid returning unrelated arities and to followdefdelegatechains per-arity. Adds targeted tests to pin arity filtering, delegate/def mixes, anddefinitionStylebehavior, and documents the option in the README (including Zed config example).Reviewed by Cursor Bugbot for commit bdcc215. Bugbot is set up for automated code reviews on this repo. Configure here.