Skip to content
Draft
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
80 changes: 63 additions & 17 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ import {
partiallySpecializeType,
selfSpecializeClass,
transformPossibleRecursiveTypeAlias,
someSubtypes,
} from './typeUtils';

interface TypeVarUsageInfo {
Expand Down Expand Up @@ -2830,25 +2831,28 @@ export class Checker extends ParseTreeWalker {
constraints
);

const returnDiag = new DiagnosticAddendum();
if (
!isNever(overloadReturnType) &&
!this._evaluator.assignType(
// based overload consistency part 1: The implementation return type must overlap with each overload
// when distributing unions (i.e., at least one member of the impl union is assignable to the overload).
if (!isNever(overloadReturnType)) {
const returnDiag = new DiagnosticAddendum();
// Succeeds if any union member of the implementation overlaps with the overload return type.
const ok = someSubtypes(
implReturnType,
overloadReturnType,
returnDiag.createAddendum(),
constraints,
AssignTypeFlags.Default
)
) {
returnDiag.addMessage(
LocAddendum.functionReturnTypeMismatch().format({
sourceType: this._evaluator.printType(overloadReturnType),
destType: this._evaluator.printType(implReturnType),
})
(sub) =>
this._evaluator.assignType(overloadReturnType, sub) ||
this._evaluator.assignType(sub, overloadReturnType)
);
diag?.addAddendum(returnDiag);
isConsistent = false;

if (!ok) {
returnDiag.addMessage(
LocAddendum.functionReturnTypeMismatch().format({
sourceType: this._evaluator.printType(implReturnType),
destType: this._evaluator.printType(overloadReturnType),
})
);
diag?.addAddendum(returnDiag);
isConsistent = false;
}
}

return isConsistent;
Expand Down Expand Up @@ -3307,6 +3311,48 @@ export class Checker extends ParseTreeWalker {
}
}
});

// based overload consistency part 2: Implementation return type must be a subtype of the union of overload return types.
if (
this._importResolver.getConfigOptions().strictOverloadConsistency &&
implementation &&
isFunction(implementation)
) {
const implNode = implementation.shared.declaration?.node?.parent;
let implBound = implementation;
if (implNode) {
const liveScopeIds = ParseTreeUtils.getTypeVarScopesForNode(implNode);
implBound = makeTypeVarsBound(implementation, liveScopeIds);
}

const implReturnType =
FunctionType.getEffectiveReturnType(implBound) ?? this._evaluator.getInferredReturnType(implBound);

const mappedReturnUnion = combineTypes(
OverloadedType.getOverloads(type).map((overloadType) => {
const result =
FunctionType.getEffectiveReturnType(overloadType) ??
this._evaluator.getInferredReturnType(overloadType);
// special case CoroutineType, as it's a known instance of a "single covariant" type parameter
// see: https://github.com/DetachHead/basedpyright/issues/1523
if (isClass(result) && result.shared.fullName === 'types.CoroutineType') {
return result.shared.typeParams[2];
}
return result;
})
);

const extraDiag = new DiagnosticAddendum();
const isAssignable = this._evaluator.assignType(mappedReturnUnion, implReturnType, extraDiag);

if (!isAssignable && implementation.shared.declaration) {
this._evaluator.addDiagnostic(
DiagnosticRule.reportInconsistentOverload,
LocMessage.overloadImplementationTooWide() + extraDiag.getString(),
implementation.shared.declaration.node.d.name
);
}
}
}

private _reportFinalInLoop(symbol: Symbol) {
Expand Down
2 changes: 2 additions & 0 deletions packages/pyright-internal/src/common/configOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,8 @@ export class ConfigOptions {
// Overrides the default timeout for file enumeration operations.
fileEnumerationTimeoutInSec?: number;

strictOverloadConsistency = true;
Copy link
Owner

Choose a reason for hiding this comment

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

need to document this new rule (both in config-files.md and in the benefits over pyright section (maybe a new file?)

Copy link
Owner

Choose a reason for hiding this comment

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

hmm, actually i think it probably should be a new diagnostic rule. this makes it much easier for users updating it to instantly know what the new setting to disable is if they don't agree with it


// https://github.com/microsoft/TypeScript/issues/3841
declare ['constructor']: typeof ConfigOptions;

Expand Down
1 change: 1 addition & 0 deletions packages/pyright-internal/src/localization/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,7 @@ export namespace Localizer {
new ParameterizedString<{ name: string; index: number }>(
getRawString('Diagnostic.overloadImplementationMismatch')
);
export const overloadImplementationTooWide = () => getRawString('Diagnostic.overloadImplementationTooWide');
export const overloadOverrideImpl = () => getRawString('Diagnostic.overloadOverrideImpl');
export const overloadOverrideNoImpl = () => getRawString('Diagnostic.overloadOverrideNoImpl');
export const overloadReturnTypeMismatch = () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,7 @@
"comment": "{Locked='@final'}"
},
"overloadImplementationMismatch": "Overloaded implementation is not consistent with signature of overload {index}",
"overloadImplementationTooWide": "Implementation return type is too wide",
"overloadOverrideImpl": {
"message": "@override decorator should be applied only to the implementation",
"comment": "{Locked='@override'}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def overloaded_converter(s: str) -> int: ...
def overloaded_converter(s: list[str]) -> int: ...


def overloaded_converter(s: float | str | list[str], *args: str) -> int | float | str:
def overloaded_converter(s: float | str | list[str], *args: str) -> int | str:
return 0


Expand Down Expand Up @@ -141,8 +141,8 @@ def wrong_converter_overload(s: float) -> str: ...
def wrong_converter_overload(s: str) -> str: ...


def wrong_converter_overload(s: float | str) -> int | str:
return 1
def wrong_converter_overload(s: float | str) -> str:
return ""


class Errors(ModelBase):
Expand Down
4 changes: 2 additions & 2 deletions packages/pyright-internal/src/tests/samples/decorator2.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ def atomic(*, savepoint: bool = True) -> Callable[[F], F]: ...


def atomic(
__func: Optional[Callable[..., None]] = None, *, savepoint: bool = True
) -> Union[Callable[[], None], Callable[[F], F]]: ...
__func: F | None = None, *, savepoint: bool = True
) -> Union[F, Callable[[F], F]]: ...
Comment on lines -17 to +18
Copy link
Owner

Choose a reason for hiding this comment

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

is this now an error? i couldn't reproduce it



@atomic
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def m1(self, x: bool) -> int: ...
@overload
def m1(self, x: str) -> str: ...

def m1(self, x: bool | str) -> int | float | str:
def m1(self, x: bool | str) -> int | str:
return x


Expand All @@ -55,7 +55,7 @@ def m1(self, x: bool) -> int: ...

# This should generate an error because the overloads are
# in the wrong order.
def m1(self, x: bool | str) -> int | float | str:
def m1(self, x: bool | str) -> int | str:
return x


Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/samples/overload2.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def deco2(

def deco2(
x: Callable[[], T | None] = lambda: None,
) -> Callable[[Callable[P, T]], Callable[P, T | None]]: ...
) -> Callable[[Callable[P, T]], Callable[P, T]] | Callable[[Callable[P, T]], Callable[P, T | None]]: ...
Copy link
Owner

Choose a reason for hiding this comment

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

same here



@deco2(x=dict)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def overloaded1(x: A) -> str: ...
def overloaded1(x: _T1) -> _T1: ...


def overloaded1(x: A | B) -> str | B: ...
def overloaded1[T: B](x: A | T) -> str | T: ...
Copy link
Owner

Choose a reason for hiding this comment

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

and here



def func1(a: A | B, b: A | B | C):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def func1(__iter1: Iterable[_T1], __iter2: Iterable[_T2]) -> Tuple[_T1, _T2]: ..
def func1(*iterables: Iterable[_T1]) -> float: ...


def func1(*iterables: Iterable[_T1 | _T2]) -> Tuple[_T1 | _T2, ...] | float: ...
def func1(*iterables: Iterable[_T1 | _T2]) -> Tuple[_T1 | _T2, ...] | float: ... # pyright: ignore the too wide error
Copy link
Owner

Choose a reason for hiding this comment

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

why are we ignoring the error here instead of fixing it? also should add the error code to the ignore comment



def test1(x: Iterable[int]):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ def func20(choices: AnyStr) -> AnyStr: ...
def func20(choices: AllStr) -> AllStr: ...


def func20(choices: AllStr) -> AllStr: ...
def func20(choices: AnyStr | AllStr) -> AnyStr | AllStr: ...
Copy link
Owner

Choose a reason for hiding this comment

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

this seems sus, the error message was "Overloaded implementation is not consistent with signature of overload 1" but that's not the error message you added. did we accidentally effect logic for other cases?



# This should generate an overlapping overload error.
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/samples/typeIs1.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ def func10(
) -> TypeIs[tuple[int, ...]]: ...


def func10(v: tuple[int | str, ...], b: bool = True) -> bool: ...
def func10(v: tuple[int | str, ...], b: bool = True) -> TypeIs[tuple[int, ...]] | TypeIs[tuple[str, ...]]: ...


v0 = is_int(int)
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/typeEvaluator6.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ test('OverloadOverride1', () => {

test('OverloadImpl1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['overloadImpl1.py']);
TestUtils.validateResults(analysisResults, 6);
TestUtils.validateResults(analysisResults, 7);
Copy link
Owner

Choose a reason for hiding this comment

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

can you add a comment in overloadImpl1.py where the new error should be

});

test('OverloadImpl2', () => {
Expand Down
Loading