Skip to content

[pyupgrade] Don't offer fix when TypeVar has unpacked keyword arguments (UP040, UP046, UP047) - #27202

Open
TaelanSakay wants to merge 2 commits into
astral-sh:mainfrom
TaelanSakay:fix-up040-up046-up047-unpacked-kwargs
Open

[pyupgrade] Don't offer fix when TypeVar has unpacked keyword arguments (UP040, UP046, UP047)#27202
TaelanSakay wants to merge 2 commits into
astral-sh:mainfrom
TaelanSakay:fix-up040-up046-up047-unpacked-kwargs

Conversation

@TaelanSakay

Copy link
Copy Markdown

Fixes #27008

The fixes for UP040, UP046, and UP047 were silently dropping unpacked
keyword arguments when inlining a TypeVar into PEP 695 syntax. For example:

T = TypeVar("T", **{"default": Any})

would be incorrectly converted, losing the default entirely and changing
runtime behavior from True to False.

The fix: in expr_name_to_type_var, return None when the TypeVar call
contains any unpacked keyword arguments (kw.arg.is_none()). This causes
UP040 to skip the conversion entirely, UP046 to emit a diagnostic without
a fix, and UP047 to skip entirely — consistent with their existing behavior
for unresolvable TypeVars.

Also changed the unwrap_or fallback in non_pep695_type_alias_type to
and_then so that unresolvable TypeVars in type_params cause the rule
to bail out rather than proceeding with incomplete information.

Test Plan:

Added test cases with unpacked kwargs TypeVars to UP040.py, UP046_0.py,
and UP047_0.py fixtures. Verified using the exact reproduction case from
the issue that UP040 no longer flags the conversion, UP046 emits a
diagnostic without a fix, and UP047 produces no diagnostic.

cargo test -p ruff_linter pep695 — 14 passed, 0 failed.

@astral-sh-bot
astral-sh-bot Bot requested a review from ntBre July 26, 2026 23:09
@ntBre ntBre changed the title fix(UP040/UP046/UP047): don't offer fix when TypeVar has unpacked keyword arguments [pyupgrade] Don't offer fix when TypeVar has unpacked keyword arguments (UP040, UP046, UP047) Jul 27, 2026
@ntBre ntBre added bug Something isn't working fixes Related to suggested fixes for violations labels Jul 27, 2026
@astral-sh-bot

astral-sh-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+0 -1 violations, +0 -0 fixes in 1 projects; 56 projects unchanged)

mlflow/mlflow (+0 -1 violations, +0 -0 fixes)

- mlflow/telemetry/track.py:31:32: RUF100 [*] Unused `noqa` directive (unused: `RET504`)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF100 1 0 1 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -1 violations, +0 -0 fixes in 1 projects; 56 projects unchanged)

mlflow/mlflow (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

- mlflow/telemetry/track.py:31:32: unused-noqa [*] Unused `noqa` directive (unused: `RET504`)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
unused-noqa 1 0 1 0 0

@ntBre ntBre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! I think there are a couple of additional fixes missing. I also had a request for the tests, and CI is flagging a minor formatting issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's use mdtests for this:

ruff/CONTRIBUTING.md

Lines 233 to 240 in bd2d6f6

#### Rule testing: fixtures and snapshots
To test rules, Ruff uses the mdtest framework, initially developed for ty. Mdtests are written as
Markdown files with Python code and TOML configuration blocks alongside prose
descriptions. Generally, there will be one directory per linter (e.g. `flake8-bandit` for the `S`
rules) with nested files for each rule (e.g. `unsafe-markup-use.md` for `S704`). Within these
files, you can define additional Markdown sections to group related tests and their settings
together.

We're adding a pep-695.md file that you can reuse in #27133.

Comment on lines +348 to +351
if arguments.keywords.iter().any(|kw| kw.arg.is_none()) {
return None;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right place to check this because it returns None for a single TypeVar, when really we want to avoid a fix if any TypeVar has unpacked keyword arguments. For example, this branch still fixes a case like this:

target/debug/ruff check --no-cache --isolated --target-version py314 --preview --select UP047 --unsafe-fixes --diff - <<'PY'
from typing import Any, TypeVar
  
T = TypeVar("T", **{"default": Any})
U = TypeVar("U")
  
def f(first: T, second: U) -> tuple[T, U]:
    return first, second
PY                             
@@ -3,5 +3,5 @@
 T = TypeVar("T", **{"default": Any})
 U = TypeVar("U")
 
-def f(first: T, second: U) -> tuple[T, U]:
+def f[U](first: T, second: U) -> tuple[T, U]:
     return first, second

Would fix 1 error.

.iter()
.map(|expr| {
expr.as_name_expr()
.and_then(|name| expr_name_to_type_var(checker.semantic(), name))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This only covers the type_alias_type part of UP040, the annotated type alias form of the rule still fires:

target/debug/ruff check --no-cache --isolated --target-version py314 --preview --select UP040 --unsafe-fixes --diff - <<'PY'
from typing import Any, TypeAlias, TypeVar
  
T = TypeVar("T", **{"default": Any})
U = TypeVar("U")
  
Alias: TypeAlias = list[T]
MixedAlias: TypeAlias = tuple[T, U]
PY                                        
@@ -3,5 +3,5 @@
 T = TypeVar("T", **{"default": Any})
 U = TypeVar("U")
 
-Alias: TypeAlias = list[T]
-MixedAlias: TypeAlias = tuple[T, U]
+type Alias = list[T]
+type MixedAlias[U] = tuple[T, U]

@TaelanSakay

Copy link
Copy Markdown
Author

Updated in the second and third commits to address the feedback. The check now uses a dedicated type_var_has_unpacked_kwargs helper called inside TypeVarReferenceVisitor, which correctly handles the multi-TypeVar case, if any TypeVar in the function/class/alias has unpacked kwargs, the entire fix is suppressed rather than partially applied. Also fixed the annotated TypeAlias form of UP040 which wasn't covered in the first pass. Third commit replaces the fixture additions with an mdtest section in non-pep695-type-alias.md. Let me know if there's anything else to address.

@ntBre

ntBre commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Could you address the merge conflicts so I can run CI?

@TaelanSakay
TaelanSakay force-pushed the fix-up040-up046-up047-unpacked-kwargs branch from b3dad50 to b0dedac Compare August 1, 2026 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working fixes Related to suggested fixes for violations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fixes for UP040, UP046, and UP047 remove unpacked keyword arguments in TypeVar

2 participants