-
Notifications
You must be signed in to change notification settings - Fork 121
feat(definitionProvider): include base class method in go-to-definition #1803
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,12 @@ unlike pyright/pylance, basedpyright also supports completions for enum values: | |
|
|
||
| this also works for other types of enums such as `IntEnum` and `StrEnum` | ||
|
|
||
| ## 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. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this behavior doesn't seem to mirror 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()
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think to accurately mimic what |
||
|
|
||
| see [#355](https://github.com/DetachHead/basedpyright/issues/355). | ||
|
|
||
| ## improved diagnostic severity system | ||
|
|
||
| in pyright, certain diagnostics such as unreachable and unused code are always reported as a hint and cannot be disabled even when the associated diagnostic rule is disabled (and in the case of unreachable code, [the diagnostic is not reported in most cases where the hint is reported](./fixes-for-rules.md#reportunreachable)). | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| /// <reference path="typings/fourslash.d.ts" /> | ||
|
|
||
| // @filename: superlib/__init__.pyi | ||
| // @library: true | ||
| //// class LibBase: | ||
| //// def [|libm|](self) -> None: ... | ||
|
|
||
| // @filename: test.py | ||
| //// from typing import Generic, Protocol, TypeAlias, TypeVar, overload | ||
| //// from superlib import LibBase | ||
| //// | ||
| //// # 14. base via TypeAlias -> still finds the aliased class method | ||
| //// class S14Base: | ||
| //// def [|m14|](self) -> None: | ||
| //// pass | ||
| //// S14Alias: TypeAlias = S14Base | ||
| //// class S14Child(S14Alias): | ||
| //// def /*marker14*/[|m14|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 15. Generic base -> finds the method in the generic class | ||
| //// T = TypeVar("T") | ||
| //// class S15Base(Generic[T]): | ||
| //// def [|process|](self, value: T) -> T: ... | ||
| //// class S15Child(S15Base[int]): | ||
| //// def /*marker15*/[|process|](self, value: int) -> int: | ||
| //// return value | ||
| //// | ||
| //// # 1. simple inheritance: B(A), both define m1 -> [B.m1, A.m1] | ||
| //// class S1A: | ||
| //// def [|m1|](self) -> None: | ||
| //// pass | ||
| //// class S1B(S1A): | ||
| //// def /*marker1*/[|m1|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 2. transitive, direct parent does NOT define m2 -> only [S2C.m2] | ||
| //// class S2A: | ||
| //// def m2(self) -> None: | ||
| //// pass | ||
| //// class S2B(S2A): | ||
| //// pass | ||
| //// class S2C(S2B): | ||
| //// def /*marker2*/[|m2|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 3. transitive, all define m3 -> [S3C.m3, S3B.m3] (not S3A.m3) | ||
| //// class S3A: | ||
| //// def m3(self) -> None: | ||
| //// pass | ||
| //// class S3B(S3A): | ||
| //// def [|m3|](self) -> None: | ||
| //// pass | ||
| //// class S3C(S3B): | ||
| //// def /*marker3*/[|m3|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 4. multiple inheritance: D(B, C), both define m4 -> [S4D.m4, S4B.m4, S4C.m4] | ||
| //// class S4B: | ||
| //// def [|m4|](self) -> None: | ||
| //// pass | ||
| //// class S4C: | ||
| //// def [|m4|](self) -> None: | ||
| //// pass | ||
| //// class S4D(S4B, S4C): | ||
| //// def /*marker4*/[|m4|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 5. multiple inheritance, only one base defines m5 -> [S5D.m5, S5C.m5] | ||
| //// class S5B: | ||
| //// pass | ||
| //// class S5C: | ||
| //// def [|m5|](self) -> None: | ||
| //// pass | ||
| //// class S5D(S5B, S5C): | ||
| //// def /*marker5*/[|m5|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 6. not overridden -> unchanged (only local decl) | ||
| //// class S6Solo: | ||
| //// def /*marker6*/[|m6|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 7. cursor in the method body -> unchanged | ||
| //// class S7A: | ||
| //// def m7(self) -> None: | ||
| //// pass | ||
| //// class S7B(S7A): | ||
| //// def m7(self) -> None: | ||
| //// [|local7|] = 1 | ||
| //// return /*marker7*/local7 | ||
| //// | ||
| //// # 8. cursor on a call site self.m() -> unchanged (no regression) | ||
| //// class S8Base: | ||
| //// def [|m8|](self) -> None: | ||
| //// pass | ||
| //// class S8Child(S8Base): | ||
| //// def caller(self) -> None: | ||
| //// self./*marker8*/m8() | ||
| //// | ||
| //// # 9. cursor on a parameter -> unchanged | ||
| //// class S9A: | ||
| //// def m9(self) -> None: | ||
| //// pass | ||
| //// class S9B(S9A): | ||
| //// def m9(self, /*marker9*/[|p9: int|]) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 10. free function with the same name as a method -> unchanged | ||
| //// def /*marker10*/[|freefn|]() -> None: | ||
| //// pass | ||
| //// class S10Has: | ||
| //// def freefn(self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 11. overloads in the direct parent -> all overload decls listed | ||
| //// class S11Base: | ||
| //// @overload | ||
| //// def [|m11|](self) -> None: ... | ||
| //// @overload | ||
| //// def [|m11|](self, a: int) -> None: ... | ||
| //// def [|m11|](self, a: int = 0) -> None: | ||
| //// pass | ||
| //// class S11Child(S11Base): | ||
| //// def /*marker11*/[|m11|](self, a: int = 0) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 12. parent defined in a .pyi -> location points to the .pyi | ||
| //// class S12Child(LibBase): | ||
| //// def /*marker12*/[|libm|](self) -> None: | ||
| //// pass | ||
| //// | ||
| //// # 13. parent is a Protocol -> returns the method decl in the Protocol | ||
| //// class S13Proto(Protocol): | ||
| //// def [|m13|](self) -> int: ... | ||
| //// class S13Impl(S13Proto): | ||
| //// def /*marker13*/[|m13|](self) -> int: | ||
| //// return 0 | ||
|
|
||
| { | ||
| const rangeMap = helper.getRangesByText(); | ||
|
|
||
| const expected = (text: string) => ({ | ||
| definitions: rangeMap | ||
| .get(text)! | ||
| .filter((r) => !r.marker) | ||
| .map((r) => ({ path: r.fileName, range: helper.convertPositionRange(r) })), | ||
| }); | ||
|
|
||
| helper.verifyFindDefinitions( | ||
| { | ||
| marker1: expected('m1'), | ||
| marker2: expected('m2'), | ||
| marker3: expected('m3'), | ||
| marker4: expected('m4'), | ||
| marker5: expected('m5'), | ||
| marker6: expected('m6'), | ||
| marker7: expected('local7'), | ||
| marker8: expected('m8'), | ||
| marker9: expected('p9: int'), | ||
| marker10: expected('freefn'), | ||
| marker11: expected('m11'), | ||
| marker12: expected('libm'), | ||
| marker13: expected('m13'), | ||
| marker14: expected('m14'), | ||
| marker15: expected('process'), | ||
| }, | ||
| 'all' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think "go to definition" is the right LSP feature to use here.
typeHierarchy/supertypessounds more suitable, as mentioned in #355 (comment)