Skip to content

Fix false positive bad-function-definition for TypeVar defaults#2788

Open
Adist319 wants to merge 1 commit intofacebook:mainfrom
Adist319:fix/typevar-default-function-2711
Open

Fix false positive bad-function-definition for TypeVar defaults#2788
Adist319 wants to merge 1 commit intofacebook:mainfrom
Adist319:fix/typevar-default-function-2711

Conversation

@Adist319
Copy link
Contributor

Summary

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.

Addresses #2711.

Caveat: The issue also mentions a false negative where result: str = arg.get() doesn't error even though get() returns str | None when called with no arguments. That's not fixed here. Pyrefly's hint-based instantiation pins S=str from 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

  • 4 new tests: reproducer, both infer_with_first_use modes, mismatched defaults (negative case), scope guard for unchanged behavior

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.
@meta-cla meta-cla bot added the cla signed label Mar 12, 2026
@github-actions
Copy link

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]

@github-actions
Copy link

Primer Diff Classification

❌ 1 regression(s) | ✅ 1 improvement(s) | 2 project(s) total | +2, -10 errors

1 regression(s) across psycopg. error kinds: bad-function-definition. 1 improvement(s) across steam.py.

Project Verdict Changes Error Kinds Root Cause
psycopg ❌ Regression +2 bad-function-definition pyrefly/lib/alt/function.rs
steam.py ✅ Improvement -10 bad-function-definition get_requiredness()
Detailed analysis

❌ Regression (1)

psycopg (+2)

These are false positive errors. The code uses explicit cast() calls to ensure type compatibility between tuple_row and RowFactory[Row]/AsyncRowFactory[Row]. The PR's TypeVar default handling logic appears to be incorrectly stripping generic type parameters during validation, causing pyrefly to see a type mismatch where none exists. This is a regression - the old behavior was correct to accept these explicit casts.
Attribution: The change to parameter default validation in pyrefly/lib/alt/function.rs appears to be incorrectly processing generic types during the TypeVar default substitution logic, causing it to lose the [Row] type parameter and see a mismatch between RowFactory[Row] and bare RowFactory

✅ Improvement (1)

steam.py (-10)

The removed errors were false positives caused by pyrefly incorrectly validating parameter defaults against abstract TypeVars instead of their default types. Looking at the specific code:

NameT = TypeVar("NameT", bound=str | None, default=str | None, covariant=True)

class App(Generic[NameT]):
    def __init__(self, *, name: NameT = None): ...

The error claimed Default None is not assignable to parameter name with type NameT, but this is wrong. Since NameT has default=str | None, the validation should check None <: str | None (which passes) rather than None <: NameT (which fails for abstract TypeVars). The PR fix correctly substitutes TypeVar defaults during parameter validation, eliminating this class of false positives. This aligns with how mypy and pyright handle TypeVar defaults per PEP 696.

Attribution: The change to the get_requiredness() call in pyrefly/lib/alt/function.rs at line 861 now substitutes TypeVar defaults before validating parameter defaults. The new logic transforms quantified types with defaults into their gradual types, so None <: NameT becomes None <: str | None, eliminating the false positive.

Suggested fixes

Summary: 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 pyrefly/lib/alt/function.rs around line 845-860, add a guard condition to only apply TypeVar default substitution when the default value is being checked against a direct TypeVar parameter, not when checking against complex generic types that contain TypeVars. Specifically, modify the transform closure to check if the Type::Quantified is at the top level of the parameter type before substituting defaults.

Files: pyrefly/lib/alt/function.rs
Confidence: high
Affected projects: psycopg
Fixes: bad-assignment
The regression occurs because the TypeVar default substitution is being applied too broadly during parameter validation. In psycopg, explicit cast() calls ensure compatibility between tuple_row and RowFactory[Row]/AsyncRowFactory[Row], but the current logic strips the [Row] type parameter during validation. Adding a guard to only substitute TypeVar defaults for direct TypeVar parameters (not nested ones in generic types) would preserve the correct behavior for explicit casts while maintaining the improvement for direct TypeVar parameter defaults. Expected outcome: eliminates 2 false positive errors in psycopg while preserving the 10 improvements in steam.py.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (2 LLM)

@yangdanny97
Copy link
Contributor

yangdanny97 commented Mar 17, 2026

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 .transform call you added is substituting away Row in RowFactory[Row], so now we have a mismatch between RowFactory[tuple[Any, ...]] v.s. RowFactory[Row]

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?

Copy link
Contributor

@yangdanny97 yangdanny97 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Back to you with a comment

@yangdanny97 yangdanny97 self-assigned this Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants