Skip to content

fix False positive: incompatible default value error on TypeVar-typed parameter when default satisfies Literal bound #3418#3419

Open
asukaminato0721 wants to merge 1 commit into
facebook:mainfrom
asukaminato0721:3418
Open

fix False positive: incompatible default value error on TypeVar-typed parameter when default satisfies Literal bound #3418#3419
asukaminato0721 wants to merge 1 commit into
facebook:mainfrom
asukaminato0721:3418

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

@asukaminato0721 asukaminato0721 commented May 16, 2026

Summary

Fixes #3418

a bare TypeVar parameter default is checked against the TypeVar’s bound instead of against the unresolved quantified type itself.

Test Plan

add test

@meta-cla meta-cla Bot added the cla signed label May 16, 2026
@asukaminato0721 asukaminato0721 marked this pull request as ready for review May 16, 2026 14:48
Copilot AI review requested due to automatic review settings May 16, 2026 14:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions
Copy link
Copy Markdown

Diff from mypy_primer, showing the effect of this PR on open source code:

pwndbg (https://github.com/pwndbg/pwndbg)
- ERROR pwndbg/aglib/kernel/__init__.py:69:50-54: Default `None` is not assignable to parameter `default` with type `D` [bad-function-definition]
- ERROR pwndbg/aglib/kernel/__init__.py:91:38-42: Default `None` is not assignable to parameter `default` with type `D` [bad-function-definition]
- ERROR pwndbg/decorators.py:46:19-23: Default `None` is not assignable to parameter `fallback` with type `K` [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/clan.py:211:30-45: Default `int` is not assignable to parameter `type` with type `BoringEventT` [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]

mypy (https://github.com/python/mypy)
- ERROR mypyc/test-data/fixtures/ir.py:302:45-48: Default `Ellipsis` is not assignable to parameter `val` with type `_V` [bad-function-definition]

artigraph (https://github.com/artigraph/artigraph)
- ERROR src/arti/internal/models.py:203:54-71: Default `PydanticUndefinedType` is not assignable to parameter `fallback` with type `T` [bad-function-definition]
- ERROR src/arti/internal/type_hints.py:119:78-82: Default `None` is not assignable to parameter `default` with type `D` [bad-function-definition]
- ERROR src/arti/internal/type_hints.py:125:79-83: Default `None` is not assignable to parameter `default` with type `D` [bad-function-definition]
- ERROR src/arti/internal/type_hints.py:130:88-92: Default `None` is not assignable to parameter `default` with type `D` [bad-function-definition]
- ERROR src/arti/internal/utils.py:213:63-67: Default `None` is not assignable to parameter `default` with type `D` [bad-function-definition]

@github-actions
Copy link
Copy Markdown

Primer Diff Classification

✅ 4 improvement(s) | 4 project(s) total | -18 errors

4 improvement(s) across pwndbg, steam.py, mypy, artigraph.

Project Verdict Changes Error Kinds Root Cause
pwndbg ✅ Improvement -3 bad-function-definition pyrefly/lib/alt/function.rs
steam.py ✅ Improvement -9 bad-function-definition pyrefly/lib/alt/function.rs
mypy ✅ Improvement -1 bad-function-definition pyrefly/lib/alt/function.rs
artigraph ✅ Improvement -5 bad-function-definition is_type_var()
Detailed analysis

✅ Improvement (4)

pwndbg (-3)

All three removed errors follow the same pattern: D = TypeVar('D') (unbounded) with default: D = None. Since D is unbounded, its effective bound is object, and None is assignable to object. Pyrefly was previously checking None against the raw TypeVar type rather than its bound, producing false positives. The PR correctly resolves the TypeVar to its bound before checking default value compatibility. This is a clear improvement — removing false positives on a well-established Python typing pattern.
Attribution: The change in pyrefly/lib/alt/function.rs around line 950-960 fixes this. Previously, the default value was checked against the raw param_ty (the quantified TypeVar D). The PR adds logic to extract the bound type from a quantified TypeVar (q.bound_type(self.stdlib, self.heap)) and uses that bound type (check_ty) for the default value compatibility check instead. For an unbounded TypeVar, the bound is object, so None is assignable to object, and the error is correctly suppressed.

steam.py (-9)

This is a clear improvement. The PR fixes a false positive where pyrefly was incorrectly rejecting None as a default value for parameters typed with a TypeVar bounded by str | None. The TypeVar NameT has bound=str | None, and None is trivially assignable to str | None. The old behavior was checking the default against the TypeVar itself rather than its bound, which is incorrect per the typing spec. All 9 removed errors across 5 files follow the same pattern and were all false positives.
Attribution: The change in pyrefly/lib/alt/function.rs in the AnswersSolver implementation adds logic (lines around 950-957) to extract the bound type from a Type::Quantified when it's a TypeVar, and then uses that bound type (check_ty) instead of the raw param_ty when checking default value compatibility. This directly fixes the false positive where None was being checked against NameT (the TypeVar itself) rather than against str | None (the TypeVar's bound). The same pattern appears across all 5 affected files (steam/app.py, steam/bundle.py, steam/clan.py, steam/package.py, and likely one more) where similar TypeVar-bounded parameters have None defaults.

mypy (-1)

This is a clear improvement. The removed error was a false positive where pyrefly incorrectly flagged ... (Ellipsis) as not assignable to a TypeVar-typed parameter _V. In stub files and type annotations, ... is a standard placeholder for default values — it indicates that a default exists without specifying the actual value. The type checker should not attempt to check the type of ... when used as a default value sentinel in this context. The error on line 302 (def setdefault(self, key: _K, val: _V = ...) -> _V: pass) was incorrect because this is a standard pattern used throughout Python stub files, including in typeshed. Note that many other parameters in this same file use ... as a default value placeholder with various types (e.g., int, str, Optional types) without triggering errors, so the inconsistency of flagging it only for TypeVar-typed parameters further confirms this was a bug in pyrefly's handling of default value checking for TypeVar parameters.
Attribution: The change in pyrefly/lib/alt/function.rs in the AnswersSolver implementation modifies how default values are checked against TypeVar-typed parameters. Specifically, lines 950-956 extract the bound type when the parameter type is a Type::Quantified TypeVar, and line 957 uses this bound type (check_ty) instead of the raw TypeVar type for the default value check. For an unbounded TypeVar like _V, the bound is object, so Ellipsis (which is an object) passes the check. This correctly removes the false positive for dict.setdefault's val: _V = ... pattern.

artigraph (-5)

These were all false positives where pyrefly was incorrectly rejecting default values for TypeVar-typed parameters. In these cases, the functions use overloads to handle type narrowing — the implementation signature with a TypeVar-typed parameter and a concrete default is not directly called by users, and the overloads ensure type safety at call sites. For example, in get_field_default, the overloads distinguish between the case where fallback is not provided (returning Any) and where it is provided with type T (returning T). The implementation uses PydanticUndefined as a sentinel default. Similarly, get_item_from_annotated uses None as a default with overloads handling the type narrowing. Pyrefly was checking the default value against the quantified TypeVar itself (e.g., checking if None is assignable to D), which is too strict since a TypeVar represents any possible type. Mypy and pyright allow defaults for TypeVar-typed parameters in implementation signatures, understanding that the overloads handle the type safety at call sites. The PR fix correctly relaxes this check for TypeVar-typed parameters.
Attribution: The change in pyrefly/lib/alt/function.rs in the default value checking logic (around line 950-960) now extracts the bound type from a quantified TypeVar and checks the default against that bound instead of against the raw TypeVar type. Specifically, the new code let quantified_bound = match &param_ty { Type::Quantified(q) if q.[is_type_var()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/function.rs) => { Some(q.bound_type(self.stdlib, self.heap)) }, _ => None } gets the bound, and then let check_ty = quantified_bound.[as_ref()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/function.rs).unwrap_or(&param_ty) uses it for the check. For unbounded TypeVars, bound_type() presumably returns a permissive type (like object), which allows None and PydanticUndefined to pass.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (4 LLM)

Copy link
Copy Markdown
Contributor

@rchen152 rchen152 left a comment

Choose a reason for hiding this comment

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

As I described here, this PR only fixes half of the issue. If we accept the default, we need to also use it in function calls. Concretely, we need to fix the revealed type here:

from typing import reveal_type

def f[T: int](x: T = 0) -> T:
    return x

reveal_type(f())  # should be `int`, not `@_`

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.

False positive: incompatible default value error on TypeVar-typed parameter when default satisfies Literal bound

3 participants