Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Find definition to respect function arity #173

Merged

Conversation

timgent
Copy link
Contributor

@timgent timgent commented Nov 15, 2022

Previously find definition did not respect the arity of functions - it would just take you to the first matching function header.

This PR aims to make it respect the arity of functions when going to the definition

@lukaszsamson
Copy link
Collaborator

There are 3 things you need to watch out for and handle

  1. functions with default args - the arity on call won't necessarily match arity on definition (see references provider where this is covered)
  2. macros have one hidden arg (this shouldn't be a problem here)
  3. pipe calls have 1 arg on the left side (shouldn't be a problem, we transform them to normal calls)

@timgent
Copy link
Contributor Author

timgent commented Nov 16, 2022

Thanks for the feedback.

functions with default args - the arity on call won't necessarily match arity on definition (see references provider where this is covered)

I believe this is covered because I am falling back to mods_funs_to_positions[{mod, fun, nil}] in the case where a function definition with a matching arity cannot be found. This line. mods_funs_to_positions looks to contain an entry with arity as nil that we search for here that includes all function definitions regardless of their arity.

macros have one hidden arg (this shouldn't be a problem here)

As you say I don't think this is an issue here. Tested with elixir-ls using this updated version and it seems to work.

pipe calls have 1 arg on the left side (shouldn't be a problem, we transform them to normal calls)

Likewise I've tested this and not an issue

@timgent
Copy link
Contributor Author

timgent commented Dec 17, 2022

@lukaszsamson any thoughts following on from my previous reply? Let me know if I've misunderstood anything, happy to update, but in need of some guidance if you think the PR still needs amendments before being merged

@lukaszsamson lukaszsamson merged commit ed87526 into elixir-lsp:master Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants