Skip to content

fix code action for Call to deprecated overload contextlib.contextmanager #4245 - #4281

Open
asukaminato0721 wants to merge 2 commits into
meta:mainfrom
asukaminato0721:4245
Open

asukaminato0721 wants to merge 2 commits into
meta:mainfrom
asukaminato0721:4245

Conversation

@asukaminato0721

Copy link
Copy Markdown
Collaborator

Summary

Fixes #4245

Adds a quick fix replacing Iterator with Generator, including the required import.
Supports qualified annotations such as typing.Iterator.
Also supports the equivalent AsyncIterator -> AsyncGenerator deprecation.

Test Plan

add test

@github-actions

This comment has been minimized.

@meta-codesync

meta-codesync Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D114356296. (Because this pull request was imported automatically, there will not be any future comments.)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@stroxler stroxler 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 number of issues to clear up before we merge this:

  1. The replacement fails at runtime before Python 3.13.
    [deprecated_contextmanager.rs:38–51](https://github.com/meta/pyrefly/blob/7bd91ff3c57020fc2f9e9856967b3173a07b5262/pyrefly/lib/state/lsp/quick_fixes/deprecated_contextmanager.rs#L38-L51)
    Without postponed annotations, the generated typing.Generator[int] and typing.AsyncGenerator[int] raise TypeError: Too few arguments on Python 3.12. Emit full-arity annotations for older Python versions.

  2. Adding the import can invalidate __future__ imports.
    [lsp.rs:3633–3644](https://github.com/meta/pyrefly/blob/7bd91ff3c57020fc2f9e9856967b3173a07b5262/pyrefly/lib/state/lsp.rs#L3633-L3644)
    The action inserts from typing import Generator before from __future__ import annotations, causing a SyntaxError. This is an existing import-helper defect exposed by the new action. Ordinary imports must go after future imports.

  3. Explicit calls rewrite the wrong function.
    [deprecated_contextmanager.rs:36–44](https://github.com/meta/pyrefly/blob/7bd91ff3c57020fc2f9e9856967b3173a07b5262/pyrefly/lib/state/lsp/quick_fixes/deprecated_contextmanager.rs#L36-L44)
    For contextmanager(producer) inside def wrapper() -> int, the fix changes the wrapper’s return annotation to Generator, leaving producer untouched. Require the diagnostic to belong to the decorator of the function being edited.

  4. The annotation’s outer name is replaced indiscriminately.
    [deprecated_contextmanager.rs:38–51](https://github.com/meta/pyrefly/blob/7bd91ff3c57020fc2f9e9856967b3173a07b5262/pyrefly/lib/state/lsp/quick_fixes/deprecated_contextmanager.rs#L38-L51)
    Annotated[Iterator[int], "tag"] becomes Generator[Iterator[int], "tag"], corrupting both the yielded type and metadata. Resolve the annotation base to the expected iterator, or decline wrapped annotations and bound aliases.

  5. Replacement bindings are not checked.
    [deprecated_contextmanager.rs:42–76](https://github.com/meta/pyrefly/blob/7bd91ff3c57020fc2f9e9856967b3173a07b5262/pyrefly/lib/state/lsp/quick_fixes/deprecated_contextmanager.rs#L42-L76)
    A module exporting only Iterator gets rewritten from api.Iterator[int] to nonexistent api.Generator[int]. A local Generator = int also shadows the inserted import. Preserve qualifiers only when they export the replacement, and use collision-safe bindings.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@stroxler stroxler 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 for addressing the earlier feedback. This seems to need another rebase (we're churning things a lot by finally landing the backlog!).

With that in mind, let's also address another chunk of AI review notes:

  1. Preserve the original annotation source. Reconstructing the whole annotation drops comments, formatting, and trailing commas. Copying multiline source into the replacement also double-normalizes CRLF into \r\r\n, including inside string literals. Replace only the iterator name, and insert any pre-3.13 generator arguments separately. Preserve grouping parentheses when choosing the insertion point. Please cover LF/CRLF, multiline Literal annotations, comments, trailing commas, and parenthesized arguments for both sync and async fixes.

  2. Check wildcard imports inside module-level blocks. Checking only the module body's immediate statements misses from api import * inside if, try, or loop bodies. Such an import can overwrite the newly inserted Generator binding before the annotation is evaluated. For an unqualified replacement, conservatively decline the fix when a wildcard import makes binding safety uncertain.

  3. Choose fresh names using normalized identifiers, not source substrings. Python normalizes identifiers: an enclosing parameter or assignment named Generator (fullwidth G) collides with Generator, but the source-string check misses it. Include enclosing scopes and normalized parameter, assignment, and definition names. Conversely, comments and unrelated names such as AsyncGenerator or GeneratorExit should not force Generator_1.

  4. Only offer the annotation rewrite for an actual generator function. A decorated function that merely does return iter([1]) does not return a generator, so changing its annotation to Generator[...] is not a valid fix. A yield inside a nested function does not make the outer function a generator either. Require a yield belonging to the decorated function's own body.

  5. Reuse an existing safe generator import. With from typing import Iterator, Generator already present, the action unnecessarily adds Generator as Generator_1; subsequent fixes accumulate more aliases. Reuse an earlier, unshadowed standard import from typing or collections.abc. Do not blindly reuse shadowed or late imports; conservative treatment of aliased imports is acceptable. Cover repeated fixes in one file.

Two smaller maintainability requests:

  • Represent the supported replacements with a sync/async enum rather than arbitrary owned from/to string pairs. Keep iterator names, generator names, and required default arguments together, and match quick-fix variants exhaustively.
  • Remove the unrelated override_import_edit → typing_import_edit(export_name) generalization: the new action does not use it. Update the quick-fix comments to acknowledge multiple multi-edit producers.

@github-actions

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

This branch has not been deployed

No deployments
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.

code action for Call to deprecated overload contextlib.contextmanager

3 participants