Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/benefits-over-pyright/language-server-improvements.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Owner

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/supertypes sounds more suitable, as mentioned in #355 (comment)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.foo
from 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()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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)).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import * as ParseTreeUtils from '../analyzer/parseTreeUtils';
import { SourceMapper, isStubFile } from '../analyzer/sourceMapper';
import { SynthesizedTypeInfo } from '../analyzer/symbol';
import { TypeEvaluator, TypeResult } from '../analyzer/typeEvaluatorTypes';
import { doForEachSubtype } from '../analyzer/typeUtils';
import { TypeCategory } from '../analyzer/types';
import { MemberAccessFlags, doForEachSubtype, lookUpClassMember } from '../analyzer/typeUtils';
import { ClassType, TypeCategory, isInstantiableClass } from '../analyzer/types';
import { throwIfCancellationRequested } from '../common/cancellationUtils';
import { appendArray } from '../common/collectionUtils';
import { isDefined } from '../common/core';
Expand Down Expand Up @@ -201,6 +201,8 @@ class DefinitionProviderBase {
currentNode = currentNode.parent;
}

this._addBaseClassMethodDefinitions(node, definitions);

if (definitions.length === 0) {
return undefined;
}
Expand Down Expand Up @@ -231,6 +233,48 @@ class DefinitionProviderBase {
definitions.push({ uri: fileInfo.fileUri, range });
}
}

// If the position is on the name of a method in its `def` signature, also include
// the definitions of the method with the same name found in the direct base classes
// (one level of the hierarchy, no transitive MRO walk — mirroring `super()`).
private _addBaseClassMethodDefinitions(node: ParseNode, definitions: DocumentRange[]) {
if (node.nodeType !== ParseNodeType.Name) {
return;
}

const functionNode = node.parent;
if (functionNode?.nodeType !== ParseNodeType.Function || functionNode.d.name !== node) {
return;
}

const classNode = ParseTreeUtils.getEnclosingClass(functionNode, /* stopAtFunction */ true);
if (!classNode) {
return;
}

const classType = this.evaluator.getTypeOfClass(classNode)?.classType;
if (!classType || !isInstantiableClass(classType)) {
return;
}

const methodName = node.d.value;
for (const baseClass of classType.shared.baseClasses) {
if (!isInstantiableClass(baseClass) || ClassType.isBuiltIn(baseClass, 'object')) {
continue;
}

const member = lookUpClassMember(
baseClass,
methodName,
MemberAccessFlags.SkipInstanceMembers | MemberAccessFlags.SkipBaseClasses
);
if (!member) {
continue;
}

this.resolveDeclarations(member.symbol.getDeclarations(), definitions);
}
}
}

export class DefinitionProvider extends DefinitionProviderBase {
Expand Down

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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'
);
}
Loading