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

Handle same method name in multiple files #1343

Closed
gottlike opened this issue Jan 31, 2024 · 6 comments · Fixed by #1380
Closed

Handle same method name in multiple files #1343

gottlike opened this issue Jan 31, 2024 · 6 comments · Fixed by #1380
Labels
bug Something isn't working pinned This issue or pull request is pinned and won't be marked as stale

Comments

@gottlike
Copy link

Ruby version

3.2.2

Code snippet

No response

Description

In a project I have multiple folders with basically the same functionality (very modular), so I could have def do_something in folder-one/script.rb and folder-two/script.rb. However, the method description could differ.

Currently Ruby LSP doesn't necessarily display the description of the current one on hover. Also when clicking, it might open another file.

Expected output

I would expect Ruby LSP to show/use the method definition of the current file only, when there's duplicate methods in the project.

@gottlike gottlike added the bug Something isn't working label Jan 31, 2024
@vinistock
Copy link
Member

Thank you for the bug report!

Can you provide an example of the code in one of these files? The Ruby LSP does indeed index the entire codebase, which is needed if you have declarations spread across multiple files.

If you're defining the same method on the same class in both files, then both should appear as declarations. However, method support is not yet finalized so you could be seeing partial behaviour (#899).

@gottlike
Copy link
Author

gottlike commented Feb 6, 2024

Thanks for your reply. Sure, here is a simple example:

test1.rb

# frozen_string_literal: true

module Func
  def start
    something('great')
  end

  # Does something interesting
  def something(input)
    puts input
  rescue StandardError => e
    raise("#{name}.#{__method__} > #{e.message}")
  end
end

test2.rb

# frozen_string_literal: true

module Func
  # Does something else entirely
  def something(input)
    puts input
  rescue StandardError => e
    raise("#{name}.#{__method__} > #{e.message}")
  end
end

image

@vinistock vinistock added the pinned This issue or pull request is pinned and won't be marked as stale label Feb 7, 2024
@vinistock
Copy link
Member

Thank you for the context, I understand the problem now.

There's indeed a bug there, we should be concatenating the documentation for all definitions of Fun#something, since it's being overridden in multiple places.

However, notice that the declarations are not considered file specific. All declarations inside the same workspace are indexed and will show up in the editor.

@gottlike
Copy link
Author

gottlike commented Feb 8, 2024

Well, if both are shown, that's still fine, I guess :). Thanks!

@snutij
Copy link
Contributor

snutij commented Feb 12, 2024

Hi, if I'm not wrong, this behaviour come from the resolve_method in the indexer. We could change the find to filter, to get the right behaviour and having the documentation concatenated.

But this method is also used by the definition and signature_help requests, where having multiple resolved method could create issues.

Happy to have inputs on this, and if it can be fixed without more stuff from #899 I could give it a try. 👍

@vinistock
Copy link
Member

@snutij you are correct both in your understanding and the proposed solution. Methods can be overridden in Ruby, but the language server doesn't know which one "wins" (i.e.: which declaration happened last) because that depends on the require order.

The solution is indeed to use select instead of find to discover all declarations of the same method. For things like definition, we need to return all declaration locations so that the user gets to pick the option they want.

For things that show documentation, like hover or signature help, then we concatenate the documentation into one.

And for things like completion, where having overrides of the same exact method doesn't matter (because both would be invoked the exact same way), then we can concatenate for the documentation and select the first option for the completion result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pinned This issue or pull request is pinned and won't be marked as stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants