Add basic Goto Implementation Request support

[smheidrich]

Description

As pointed out in #97, support for the LSP Goto Implementation Request (textDocument/implementation) aka implementationProvider capability is still missing.

This PR adds basic support for this feature using Rope's rope.findit.find_implementations function (so it only works if the rope extra is installed).

Rope's find_implementations function seems a bit brittle, e.g. it doesn't appear to work for some slightly more complicated inheritance hierarchies (multiple levels & multiple inheritance & generics, but not sure which of these is the issue), but these things should be fixed upstream & in any case it's better than nothing.

Status

• Blocked by:
  • findit.find_implementations on method call finds implementation for different subclass python-rope/rope#812
• Also at least 1 bug remaining on this side of the implementation:
  • Add basic Goto Implementation Request support #644 (comment)

[krassowski]

Should it be wrapped in try-except to handle exceptions, as in:

python-lsp-server/pylsp/plugins/rope_completion.py

Lines 59 to 65 in 04fa3e5

	try:
	definitions = code_assist(
	rope_project, document.source, offset, document_rope, maxfixes=3
	)
	except Exception as e:
	log.debug("Failed to run Rope code assist: %s", e)
	return []

[smheidrich]

Done

[krassowski]

I guess it would also make sense to add find_definition?

[smheidrich]

If you mean using rope.contrib.findit.find_definition to implement the LSP textDocument/definition request, that one is already implemented in python-lsp-server using Jedi, so I don't think adding an additional Rope-powered implementation makes sense. But maybe I misunderstand it?

[krassowski]

Yes, that's what I meant. My reasoning is twofold:

• I think that jedi's implementation will return both implementation and definitions
• I believe that users who would want to use rope for this would likely want to disable the jedi implementation if they could have a matching rope implementation for consistency

Of note, rope is disabled by default in general because it is hard to guarantee it works well and does not conflict with jedi (e.g. by de-duplciating responses).

For the best UX for an average user I think it would be best to add textDocument/implementation using jedi, but I would not object to adding one with rope as in this PR.

I was just curious as to your reasoning for adding only textDocument/implementation and not also textDocument/definition which would be my choice if I was adding a rope version for it.

[smheidrich]

(Comment deleted because it doesn't make sense, give me a minute while I write a new one)

[smheidrich]

I originally wrote a response disagreeing but there was a glaring mistake in it because I overlooked that the current Rope find_implementations implementation is much buggier than I thought: E.g. for a file like

class A:
  def f(self): ...

class B(A):
  def f(self): ...

a: A = A()
a.f()

using Rope's find_implementations on the a.f() call will go to B.f for some reason 😕

It works well for moving between overridden methods within a class hierarchy, where it nicely complements the Jedi textDocument/definition, which always moves "up", by moving "down" instead.

But my guess is that it would take quite a lot of effort to fix Rope's find_implementations so it works correctly when used on method calls as well 😔

I'll open an issue but this PR should be on hold until it's fixed, no point merging something that's fundamentally buggy from day 1.

[smheidrich]

Rope issue created:

• findit.find_implementations on method call finds implementation for different subclass python-rope/rope#812

[krassowski]

It looks like a few of the tests are failing:

ERROR test/plugins/test_implementations.py::test_implementations - TypeError: expected str, bytes or os.PathLike object, not NoneType
ERROR test/plugins/test_implementations.py::test_implementations_skipping_one_class - TypeError: expected str, bytes or os.PathLike object, not NoneType
ERROR test/plugins/test_implementations.py::test_property_implementations - TypeError: expected str, bytes or os.PathLike object, not NoneType
ERROR test/plugins/test_implementations.py::test_implementations_not_a_method - TypeError: expected str, bytes or os.PathLike object, not NoneType

[smheidrich]

Under certain circumstances I seem to get URIs like ///example.py here for a file that is actually in another directory - will have to investigate & fix.