Repository navigation
fix code action for Call to deprecated overload contextlib.contextmanager #4245 - #4281
asukaminato0721 wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
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.) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5e796cf to
10b589f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
Thanks! I think there are a number of issues to clear up before we merge this:
-
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 generatedtyping.Generator[int]andtyping.AsyncGenerator[int]raiseTypeError: Too few argumentson Python 3.12. Emit full-arity annotations for older Python versions. -
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 insertsfrom typing import Generatorbeforefrom __future__ import annotations, causing aSyntaxError. This is an existing import-helper defect exposed by the new action. Ordinary imports must go after future imports. -
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)
Forcontextmanager(producer)insidedef wrapper() -> int, the fix changes the wrapper’s return annotation toGenerator, leavingproduceruntouched. Require the diagnostic to belong to the decorator of the function being edited. -
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"]becomesGenerator[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. -
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 onlyIteratorgets rewritten fromapi.Iterator[int]to nonexistentapi.Generator[int]. A localGenerator = intalso shadows the inserted import. Preserve qualifiers only when they export the replacement, and use collision-safe bindings.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
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:
-
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, multilineLiteralannotations, comments, trailing commas, and parenthesized arguments for both sync and async fixes. -
Check wildcard imports inside module-level blocks. Checking only the module body's immediate statements misses
from api import *insideif,try, or loop bodies. Such an import can overwrite the newly insertedGeneratorbinding before the annotation is evaluated. For an unqualified replacement, conservatively decline the fix when a wildcard import makes binding safety uncertain. -
Choose fresh names using normalized identifiers, not source substrings. Python normalizes identifiers: an enclosing parameter or assignment named
Generator(fullwidth G) collides withGenerator, but the source-string check misses it. Include enclosing scopes and normalized parameter, assignment, and definition names. Conversely, comments and unrelated names such asAsyncGeneratororGeneratorExitshould not forceGenerator_1. -
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 toGenerator[...]is not a valid fix. Ayieldinside a nested function does not make the outer function a generator either. Require a yield belonging to the decorated function's own body. -
Reuse an existing safe generator import. With
from typing import Iterator, Generatoralready present, the action unnecessarily addsGenerator as Generator_1; subsequent fixes accumulate more aliases. Reuse an earlier, unshadowed standard import fromtypingorcollections.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/tostring 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.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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