feat: fall back to other arities in go-to-definition when exact arity has no result#728
Conversation
doorgan
left a comment
doorgan
left a comment
There was a problem hiding this comment.
There's a refactor that seems unnecessary to me, but otherwise this looks good and works, thank you!
| end | ||
| end) | ||
| |> Stream.uniq_by(& &1.subject) | ||
| |> expand_definitions() |
There was a problem hiding this comment.
I don't think this refactor is necessary
katafrakt
left a comment
katafrakt
left a comment
There was a problem hiding this comment.
I am a bit concerned if this would not create inconsistent behaviour. This fixes cases when we can find the fallback in the search index, but I don't think it would works for cases when we fall back to ElixirSense resolution. Perhaps this is okay, as we are doing "best effort", but I'm not sure if it would not create false expectations.
| test "prefers exact arity definitions when present", %{ | ||
| project: project, | ||
| uri: referenced_uri | ||
| } do | ||
| subject_module = ~q[ | ||
| defmodule UsesRemoteFunction do | ||
| alias MyDefinition | ||
|
|
||
| def uses_greet() do | ||
| MyDefinition.gree|t("World") | ||
| end | ||
| end | ||
| ] | ||
|
|
||
| assert {:ok, ^referenced_uri, definition_line} = | ||
| definition(project, subject_module, referenced_uri) | ||
|
|
||
| assert definition_line == ~S[ def «greet(name)» do] | ||
| end | ||
| end |
There was a problem hiding this comment.
I don't think this test does what it says. It does not prefer exact arity, it just uses the only definition here (as there is only one and it's greet/1.
|
@katafrakt feel free to override my approvals and request changes. I got used to the tracers indexer being more complete that I had missed there is no ElixirSense fallback implementation in this PR. |
|
@katafrakt good catch — you're right that this only wires the arity fallback into the search-index path in My read is that this is acceptable as best-effort for #725 (the index path is the common case, and the fallback only triggers on the zero-result case so existing behavior is unchanged), but I share your concern about uneven expectations. Happy to go either way:
Let me know which you'd prefer and I'll adjust. |
|
TBH I'm not even sure it's possible to transfer this behaviour to the ElixirSense fallback. Can you verify that before we make a call here? |
|
Verified, and you were right to be skeptical - it can't transfer cleanly. The fallback calls Making it work would need an upstream ElixirSense API that accepts a candidate-arity override (or returns all same-name arity matches). So I'd propose we keep this PR index-path-only as best effort for #725, and I can file an upstream issue with ElixirSense if you think it's worth pursuing. |
3 participants
Summary
Go-to-definition on a function call now falls back to other arities of the same
module.functionwhen no definition exists for the exact arity. The exact-arity match still wins whenever it is present, so existing behavior is unchanged.Why this matters
As reported in #725, refactoring a function's arity breaks go-to-definition from call sites whose arity no longer matches any definition: the lookup returns nothing even though the function clearly still exists under a different arity. Jumping to the (now differently-aritied) definition is exactly what you want while you are mid-refactor. The fallback only triggers on the zero-result case and returns the alternate-arity definitions ordered lowest-to-highest arity, as the reporter suggested.
The fallback reuses the same delegate-expansion and de-duplication pipeline as the exact-arity path, so
defdelegatetargets are resolved and multi-clause / default-argument functions are not reported multiple times.Testing
Added tests in
apps/expert/test/engine/code_intelligence/definition_test.exs:MyDefinition.greet("a", "b")) resolves to the existinggreet/1definition via the fallback,MyDefinition.greet("World")) still resolves togreet/1directly (behavior preserved).Fixes #725