feat(definitionProvider): include base class method in go-to-definition#1803
feat(definitionProvider): include base class method in go-to-definition#1803NeOzay wants to merge 3 commits into
Conversation
When "go to definition" is triggered on a method name in its `def` signature, also return the same-named method from direct base classes (one MRO level, mirroring `super()`). Refs DetachHead#355
…efinition Covers TypeAlias base and Generic[T] base edge cases in addition to the core scenarios (simple, transitive, multiple inheritance, overloads, Protocol, .pyi).
There was a problem hiding this comment.
Pull request overview
Adds a language-service enhancement to BasedPyright’s go-to-definition behavior so that invoking go-to-definition on an overriding method name in its def signature also returns the same-named method declaration(s) from the direct base class(es), enabling editors to show a picker between override and overridden declarations.
Changes:
- Extend
DefinitionProviderto include same-named member declarations from direct base classes when the cursor is on a method name within itsdefsignature. - Add a dedicated fourslash test suite covering single/multiple inheritance, non-direct bases, overloads, Protocols,
.pyibases,TypeAliasbases, and generic bases. - Document the new behavior in the language server improvements guide.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/pyright-internal/src/languageService/definitionProvider.ts | Adds logic to append direct-base-class member declarations to go-to-definition results for method names in def signatures. |
| packages/pyright-internal/src/tests/fourslash/findDefinitions.superMethods.fourslash.ts | Introduces fourslash coverage to validate direct-base inclusion and ensure no regressions for other definition contexts. |
| docs/benefits-over-pyright/language-server-improvements.md | Documents the new “go to definition on overridden methods” behavior and links to the motivating issue. |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
|
||
| ## go to definition on overridden methods | ||
|
|
||
| when you run "go to definition" on the name of a method in its `def` signature, basedpyright also includes the location of the method of the same name defined in the **direct** base class(es) (one level of the class hierarchy — it does not walk the full MRO, mirroring the behavior of `super()`). this makes it easy to jump from a method you're overriding to the method it overrides. if multiple results are returned, your editor will show a picker to choose between them. |
There was a problem hiding this comment.
i don't think "go to definition" is the right LSP feature to use here. typeHierarchy/supertypes sounds more suitable, as mentioned in #355 (comment)
There was a problem hiding this comment.
this looks like quite a few different scenarios crammed into one test which makes it pretty difficult to understand what's going on. can you split it up into separate testsplease
|
|
||
| ## go to definition on overridden methods | ||
|
|
||
| when you run "go to definition" on the name of a method in its `def` signature, basedpyright also includes the location of the method of the same name defined in the **direct** base class(es) (one level of the class hierarchy — it does not walk the full MRO, mirroring the behavior of `super()`). this makes it easy to jump from a method you're overriding to the method it overrides. if multiple results are returned, your editor will show a picker to choose between them. |
There was a problem hiding this comment.
this behavior doesn't seem to mirror super():
from typing import override
class Foo:
def foo(self):
...
class Bar(Foo): ...
class Baz(Bar):
@override
def foo(self): # doesn't work
return super().foo() # correctly goes to Foo.foofrom typing import override
class Foo:
def foo(self):
print(1)
class Bar:
def foo(self):
print(2)
class Baz(Bar): ...
class Qux(Baz,Foo):
@override
def foo(self): # incorrectly goes to Foo.foo, when Bar.foo is executed at runtime
return super().foo() # "go to definition" correctly goes to Bar.foo
Qux().foo()There was a problem hiding this comment.
i think to accurately mimic what super does we gotta do what TypeEvaluator.getTypeOfSuperCall does. i dont really get how it works but it looks like here's where it resolves the super method https://github.com/DetachHead/basedpyright/blob/c0b24ea7af194c5fe3dfb79e875f4b0c93cab743/packages/pyright-internal/src/analyzer/typeEvaluator.ts#L9432-L9434
Summary
defsignature, also return the declaration of the same-named method in the direct base class(es) (one MRO level, mirroringsuper()).objectto avoid noise.Ref #355
Changes
definitionProvider.ts: new private_addBaseClassMethodDefinitionscalled before the empty-result guard; useslookUpClassMemberwithSkipBaseClassesto stay at one hierarchy level.language-server-improvements.md: documents the new behavior.findDefinitions.superMethods.fourslash.ts: 15 test cases covering simple inheritance, transitive (no direct override), multiple inheritance, overloads, Protocol,.pyistubs,TypeAliasbase, andGeneric[T]base.Possible improvements
currently only explicitly declared bases (
class Foo(aProtocol)) are inspected, not protocols satisfied without explicit inheritance.textDocument.typeHierarchy,typeHierarchy/supertypesandtypeHierarchy/subtypesTest plan
jest --testPathPatterns="fourslash" --testNamePattern="superMethods"passes (15 cases)def <name>, trigger go-to-definition → picker shows both locations