Add definitionStyle option to control multi-head goto definition#39
Add definitionStyle option to control multi-head goto definition#39flowerett wants to merge 2 commits intoremoteoss:mainfrom
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? |
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.
ef4e0ff to
9e448a6
Compare
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>
|
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.
| } | ||
| } | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
Keyword args inflate arity, breaking definition lookup
High Severity
countCallArgs counts top-level commas to determine call arity, but Elixir's spread keyword arguments (e.g., Repo.insert(changeset, returning: true, on_conflict: :replace)) use commas between key-value pairs while the entire keyword tail counts as a single argument. The function returns arity 3 here instead of 2. When this inflated arity is passed to LookupFunctionByArity, the exact-match SQL filter finds zero rows, and go-to-definition silently returns nothing — a regression from the previous behavior that returned all arities.
Additional Locations (1)
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. |


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.