Fix TypeGuard and TypeIs narrowing for functions with union return types - #11608
Fix TypeGuard and TypeIs narrowing for functions with union return types#11608Henry Su (hsusul) wants to merge 1 commit into
Conversation
|
🔒 Automated review in progress — Stella Huang (@StellaHuang95) is auto-reviewing this PR. |
| const isIncomplete = !!callTypeResult.isIncomplete || !!functionReturnTypeResult.isIncomplete; | ||
|
|
||
| return (type: Type) => { | ||
| return { |
There was a problem hiding this comment.
Issue · Please address or respond
Combining TypeIs[int] and TypeIs[str] from overload resolution and treating the result as strict is unsound in the negative branch. Only one overload is selected at runtime: with x: str and target: type[int], check_overload(x, target) is false, but this removes str and narrows x to bytes. Treat overload-joined guard unions as non-strict, or otherwise retain the original type in the negative branch.
| return hasTypeGuard; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The gate accepts a union if any member is a TypeGuard or TypeIs, while the later collector silently discards non-guard members. A return such as TypeIs[int] | None can therefore apply TypeIs narrowing even when the non-guard arm produced the result. Require every union subtype to be a supported guard before applying this narrowing.
| # This sample tests type narrowing when comparing class types | ||
| # with equality (== and !=) operators against class objects. | ||
|
|
||
| from typing import TypeVar, assert_type, final |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This sample and the accompanying ==/!= class-comparison narrowing change are unrelated to the stated TypeGuard/TypeIs union-return fix. Please split them into a separate PR, or explicitly document the additional feature and give the sample a class-equality-specific name.
|
|
||
| def test_guard(x: object, target: type[int] | type[str]): | ||
| if check_guard(x, target): | ||
| assert_type(x, int | str) |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Add coverage for mixed TypeIs/TypeGuard unions, unions containing a non-guard member, and the relevant negative branches. These cases exercise the new strictness and subtype-collection behavior, including the distinction between TypeIs negative narrowing and TypeGuard positive-only narrowing.
Stella Huang (StellaHuang95)
added
the
review-auto:changes-requested
b605ac0 to
329d1d8
Compare
|
Thank you for the detailed review Stella Huang (@StellaHuang95)! I have addressed all the feedback in the latest commit:
|
| isStrictTypeGuard = false; | ||
| typeGuardType = combineTypes(typeGuardSubtypes.map((subtype) => subtype.priv.typeArgs![0])); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue · Please address or respond
This now narrows directly declared TypeIs[int] | TypeIs[str] return annotations, but _validateTypeGuardFunction only validates class-instance return types and therefore skips the TypeIs assignability check for these unions. Please either validate every union member in the checker or restrict this handling to overload-produced unions; otherwise an invalid direct TypeIs union can influence narrowing without the usual soundness validation.
| isStrictTypeGuard = false; | ||
| typeGuardType = combineTypes(typeGuardSubtypes.map((subtype) => subtype.priv.typeArgs![0])); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue · Please address or respond
This newly enables narrowing for explicitly declared TypeIs[A] | TypeIs[B] returns, but _validateTypeGuardFunction still skips TypeIs validation when the declared return type is a union. As a result, an invalid declaration such as def guard(x: str) -> TypeIs[int] | TypeIs[str] bypasses the required TypeIs assignability check and is nevertheless treated as a guard here. Please either validate every union arm in the checker or reject/scope out directly declared guard unions; overload-produced unions remain validated per overload.
329d1d8 to
bfc7466
Compare
|
Thank you Stella Huang (@StellaHuang95)! You make a great point about checker validation. I have updated For example, an invalid declaration like
Added test coverage for this validation in |
|
Diff from mypy_primer, showing the effect of this PR on open source code: sympy (https://github.com/sympy/sympy)
- .../projects/sympy/sympy/solvers/diophantine/diophantine.py:176:44 - error: Cannot access attribute "expand" for class "Basic"
- Attribute "expand" is unknown (reportAttributeAccessIssue)
- .../projects/sympy/sympy/solvers/diophantine/tests/test_diophantine.py:314:30 - error: Cannot access attribute "as_independent" for class "Basic"
- Attribute "as_independent" is unknown (reportAttributeAccessIssue)
- .../projects/sympy/sympy/solvers/diophantine/tests/test_diophantine.py:383:30 - error: Cannot access attribute "as_independent" for class "Basic"
- Attribute "as_independent" is unknown (reportAttributeAccessIssue)
- .../projects/sympy/sympy/solvers/ode/hypergeometric.py:247:67 - error: Operator "**" not supported for types "Basic" and "Literal[2]" (reportOperatorIssue)
+ .../projects/sympy/sympy/solvers/ode/lie_group.py:619:61 - error: Operator "-" not supported for type "Unknown | Basic" (reportOperatorIssue)
- .../projects/sympy/sympy/solvers/ode/nonhomogeneous.py:468:28 - error: Argument of type "Expr | Unknown | None" cannot be assigned to parameter "expr" of type "Expr" in function "make_args"
+ .../projects/sympy/sympy/solvers/ode/nonhomogeneous.py:468:28 - error: Argument of type "Unknown | None" cannot be assigned to parameter "expr" of type "Expr" in function "make_args"
- Type "Expr | Unknown | None" is not assignable to type "Expr"
+ Type "Unknown | None" is not assignable to type "Expr"
- .../projects/sympy/sympy/solvers/ode/ode.py:1433:17 - error: Argument of type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Infinity | NegativeInfinity | Float | Number | Expr" cannot be assigned to parameter "value" of type "Zero" in function "__setitem__"
+ .../projects/sympy/sympy/solvers/ode/ode.py:1433:17 - error: Argument of type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Unknown | Expr" cannot be assigned to parameter "value" of type "Zero" in function "__setitem__"
- Type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Infinity | NegativeInfinity | Float | Number | Expr" is not assignable to type "Zero"
+ Type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Unknown | Expr" is not assignable to type "Zero"
- .../projects/sympy/sympy/solvers/ode/ode.py:1579:38 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1580:38 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1590:9 - error: No overloads for "update" match the provided arguments (reportCallIssue)
- .../projects/sympy/sympy/solvers/ode/ode.py:1590:12 - error: "update" is not a known attribute of "None" (reportOptionalMemberAccess)
- .../projects/sympy/sympy/solvers/ode/ode.py:1590:19 - error: Argument of type "Unknown | dict[Unknown, Unknown] | None" cannot be assigned to parameter "m" of type "Iterable[tuple[str, Unknown]]" in function "update"
- Type "Unknown | dict[Unknown, Unknown] | None" is not assignable to type "Iterable[tuple[str, Unknown]]"
- "None" is incompatible with protocol "Iterable[tuple[str, Unknown]]"
- "__iter__" is not present (reportArgumentType)
- .../projects/sympy/sympy/solvers/ode/ode.py:1597:43 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1597:64 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1603:9 - error: No overloads for "update" match the provided arguments (reportCallIssue)
- .../projects/sympy/sympy/solvers/ode/ode.py:1603:12 - error: "update" is not a known attribute of "None" (reportOptionalMemberAccess)
- .../projects/sympy/sympy/solvers/ode/ode.py:1603:19 - error: Argument of type "Unknown | dict[Unknown, Unknown] | None" cannot be assigned to parameter "m" of type "Iterable[tuple[str, Unknown]]" in function "update"
- Type "Unknown | dict[Unknown, Unknown] | None" is not assignable to type "Iterable[tuple[str, Unknown]]"
- "None" is incompatible with protocol "Iterable[tuple[str, Unknown]]"
- "__iter__" is not present (reportArgumentType)
- .../projects/sympy/sympy/solvers/ode/ode.py:1610:43 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1610:74 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1616:9 - error: No overloads for "update" match the provided arguments (reportCallIssue)
- .../projects/sympy/sympy/solvers/ode/ode.py:1616:12 - error: "update" is not a known attribute of "None" (reportOptionalMemberAccess)
- .../projects/sympy/sympy/solvers/ode/ode.py:1616:19 - error: Argument of type "Unknown | dict[Unknown, Unknown] | None" cannot be assigned to parameter "m" of type "Iterable[tuple[str, Unknown]]" in function "update"
- Type "Unknown | dict[Unknown, Unknown] | None" is not assignable to type "Iterable[tuple[str, Unknown]]"
- "None" is incompatible with protocol "Iterable[tuple[str, Unknown]]"
- "__iter__" is not present (reportArgumentType)
- .../projects/sympy/sympy/solvers/ode/ode.py:1623:49 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:1623:70 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
+ .../projects/sympy/sympy/solvers/ode/ode.py:1753:36 - error: Cannot access attribute "lhs" for class "Expr"
+ Attribute "lhs" is unknown (reportAttributeAccessIssue)
+ .../projects/sympy/sympy/solvers/ode/ode.py:1753:55 - error: Cannot access attribute "rhs" for class "Expr"
+ Attribute "rhs" is unknown (reportAttributeAccessIssue)
+ .../projects/sympy/sympy/solvers/ode/ode.py:1754:36 - error: Cannot access attribute "lhs" for class "Expr"
+ Attribute "lhs" is unknown (reportAttributeAccessIssue)
+ .../projects/sympy/sympy/solvers/ode/ode.py:1755:17 - error: No overloads for "__setitem__" match the provided arguments (reportCallIssue)
+ .../projects/sympy/sympy/solvers/ode/ode.py:1755:17 - error: Argument of type "Equality | BooleanFalse | BooleanTrue | Unknown | Expr" cannot be assigned to parameter "value" of type "Equality | BooleanFalse | BooleanTrue" in function "__setitem__"
+ Type "Equality | BooleanFalse | BooleanTrue | Unknown | Expr" is not assignable to type "Equality | BooleanFalse | BooleanTrue"
+ Type "Expr" is not assignable to type "Equality | BooleanFalse | BooleanTrue"
+ "Expr" is not assignable to "Equality"
+ "Expr" is not assignable to "BooleanFalse"
+ "Expr" is not assignable to "BooleanTrue" (reportArgumentType)
- .../projects/sympy/sympy/solvers/ode/ode.py:2961:17 - error: Argument of type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Infinity | NegativeInfinity | Float | Number | Expr" cannot be assigned to parameter "value" of type "Zero" in function "__setitem__"
+ .../projects/sympy/sympy/solvers/ode/ode.py:2961:17 - error: Argument of type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Unknown | Expr" cannot be assigned to parameter "value" of type "Zero" in function "__setitem__"
- Type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Infinity | NegativeInfinity | Float | Number | Expr" is not assignable to type "Zero"
+ Type "One | NegativeOne | Zero | Integer | NaN | ComplexInfinity | Rational | Unknown | Expr" is not assignable to type "Zero"
- .../projects/sympy/sympy/solvers/ode/ode.py:3442:7 - error: "update" is not a known attribute of "None" (reportOptionalMemberAccess)
- .../projects/sympy/sympy/solvers/ode/ode.py:3442:38 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3443:7 - error: "update" is not a known attribute of "None" (reportOptionalMemberAccess)
- .../projects/sympy/sympy/solvers/ode/ode.py:3443:38 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3444:14 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3445:14 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3446:14 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3458:50 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3459:50 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3460:50 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
- .../projects/sympy/sympy/solvers/ode/ode.py:3500:5 - error: No overloads for "update" match the provided arguments (reportCallIssue)
- .../projects/sympy/sympy/solvers/ode/ode.py:3500:7 - error: "update" is not a known attribute of "None" (reportOptionalMemberAccess)
- .../projects/sympy/sympy/solvers/ode/ode.py:3500:14 - error: Argument of type "Unknown | dict[Unknown, Unknown] | None" cannot be assigned to parameter "m" of type "Iterable[tuple[str, Unknown]]" in function "update"
- Type "Unknown | dict[Unknown, Unknown] | None" is not assignable to type "Iterable[tuple[str, Unknown]]"
- "None" is incompatible with protocol "Iterable[tuple[str, Unknown]]"
- "__iter__" is not present (reportArgumentType)
... (truncated 425 lines) ...
|
|
|
||
|
|
||
| # This should generate an error because "int" is not a subtype of "str". | ||
| def invalid_typeis_union(val: str) -> TypeIs[int] | TypeIs[str]: # pyright: ignore[reportGeneralTypeIssues] |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This suppresses the diagnostic the checker.ts change is intended to produce, while the test expects zero diagnostics. Reverting that validation would therefore leave this test green. Remove the ignore and update TypeIs5 to expect the resulting diagnostic so this behavior is actually covered.
[verified]
Stella Huang (StellaHuang95)
left a comment
Stella Huang (StellaHuang95)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Stella Huang (StellaHuang95)
added
review-auto:approved
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
3 participants
Summary
Fixes type narrowing when evaluating user-defined type guard functions (
TypeGuard/TypeIs) whose return type evaluates to a union of type guard instances (for example,TypeIs[int] | TypeIs[str]orTypeGuard[int] | TypeGuard[str]).Problem Description
When a function call returns a union of
TypeIsorTypeGuardtypes — such as when an overloaded function matches arguments and returnsTypeIs[int]for one overload andTypeIs[str]for another, or when a single function is annotated with a union return typeTypeIs[int] | TypeIs[str]— Pyright failed to apply type narrowing.Specifically, in
typeGuards.ts,narrowTypeForUserDefinedTypeGuardwas only invoked ifisClassInstance(functionReturnType)returnedtrue. WhenfunctionReturnTypewas aUnionTypecontaining multipleTypeIs/TypeGuardinstances,isClassInstancereturnedfalse, causing Pyright to skip type guard evaluation entirely and leave the subject variable unnarrowed (object).Minimal Reproduction
Current vs Corrected Behavior
if check(x, target)leavesxasobjectbecausefunctionReturnType(TypeIs[int] | TypeIs[str]) is aUnionTypeand was not recognized as a user-defined type guard.TypeGuardorTypeIsinstances, Pyright combines their type arguments (int | str) and narrowsxtoint | strin the positive branch (and eliminatesint | strin the negative branch if all subtypes areTypeIs).Implementation Details
isFunctionReturnTypeGuardinpackages/pyright-internal/src/analyzer/typeGuards.tsto inspect union return types viaisUnion()and verify if any subtype is aTypeGuardorTypeIsinstance.typeGuards.ts(around line 740), added support forfunctionReturnTypewhen it is aUnionType. If all subtypes areTypeGuardorTypeIsclass instances,typeGuardTypeis calculated viacombineTypes(...)across all type arguments, andisStrictTypeGuardis set totrueif all subtypes areTypeIs.Regression Coverage
Added test sample
typeIs5.pyand registered test caseTypeIs5intypeEvaluator6.test.tscovering:TypeIstypes (TypeIs[int] | TypeIs[str])TypeIstypes called with a union argument (type[int] | type[str])TypeGuardtypes called with a union argumentif) and negative (else) branch type narrowing assertions usingassert_typeValidation Results
npm run check(syncpack, eslint, prettier): PASS (0 errors)npm run typecheck(tsc --noEmit across all packages): PASS (0 errors)npm run test:norebuild(all 62 Jest test suites / 2552 tests): PASS (0 failures)