Fix false positive bad-function-definition for TypeVar defaults#2788
Fix false positive bad-function-definition for TypeVar defaults#2788Adist319 wants to merge 1 commit intofacebook:mainfrom
Conversation
def get[S=None](self, default: S = None) was emitting a false positive bad-function-definition because the subset check None <: S fails for abstract Quantified types. The fix specializes the annotation by substituting TypeVar defaults before checking the default expression.
|
Diff from mypy_primer, showing the effect of this PR on open source code: psycopg (https://github.com/psycopg/psycopg)
+ ERROR psycopg/psycopg/connection.py:73:40-72: Default `RowFactory[Row]` is not assignable to parameter `row_factory` with type `RowFactory` [bad-function-definition]
+ ERROR psycopg/psycopg/connection_async.py:77:45-82: Default `AsyncRowFactory[Row]` is not assignable to parameter `row_factory` with type `AsyncRowFactory` [bad-function-definition]
steam.py (https://github.com/Gobot1234/steam.py)
- ERROR steam/app.py:101:23-27: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/app.py:646:78-82: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/bundle.py:36:54-58: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/ext/commands/commands.py:709:24-31: Default `type[Command]` is not assignable to parameter `cls` with type `type[C]` [bad-function-definition]
- ERROR steam/ext/commands/commands.py:807:20-27: Default `type[Command]` is not assignable to parameter `cls` with type `type[C]` [bad-function-definition]
- ERROR steam/package.py:55:54-58: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/package.py:75:75-79: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/tag.py:35:72-76: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/tag.py:77:72-76: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
- ERROR steam/tag.py:99:72-76: Default `None` is not assignable to parameter `name` with type `NameT` [bad-function-definition]
|
Primer Diff Classification❌ 1 regression(s) | ✅ 1 improvement(s) | 2 project(s) total | +2, -10 errors 1 regression(s) across psycopg. error kinds:
Detailed analysis❌ Regression (1)psycopg (+2)
✅ Improvement (1)steam.py (-10)
NameT = TypeVar("NameT", bound=str | None, default=str | None, covariant=True)
class App(Generic[NameT]):
def __init__(self, *, name: NameT = None): ...The error claimed
Suggested fixesSummary: The PR's TypeVar default substitution logic is incorrectly processing generic types during parameter validation, causing false positive type errors in psycopg. 1. In the parameter default validation logic in
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (2 LLM) |
|
I think the LLM analysis on the regressions is correct here The psycopg example is here: https://github.com/psycopg/psycopg/blob/master/psycopg/psycopg/connection.py#L73 the I'm not sure what the non-hacky way to do this is. Maybe if the un-substituted type fails & there is a quantified w/ default somewhere in the type, we can check the substituted type & if either succeed we don't error? |
yangdanny97
left a comment
There was a problem hiding this comment.
Thanks for the PR! Back to you with a comment
Summary
def get[S=None](self, default: S = None)was emitting a false positivebad-function-definitionbecause the subset checkNone <: Sfails for abstract Quantified types. The fix specializes the annotation by substituting TypeVar defaults before checking the default expression.Addresses #2711.
Caveat: The issue also mentions a false negative where
result: str = arg.get()doesn't error even thoughget()returnsstr | Nonewhen called with no arguments. That's not fixed here. Pyrefly's hint-based instantiation pinsS=strfrom the assignment target before argument matching runs, which overrides the TypeVar default. Fixing this would require pinning TypeVars with defaults for omitted parameters during argument matching, similar to what Pyright does. The upstream typing spec discussion on function TypeVar default semantics is still open, so I wanted to wait on this first before moving forward (in a separate PR).Test Plan
infer_with_first_usemodes, mismatched defaults (negative case), scope guard for unchanged behavior