Skip to content

[ty] Add hint if async context manager is used in non-async with statement #18299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 26, 2025

Conversation

lipefree
Copy link
Contributor

Summary

Implements astral-sh/ty#508, considering the following code :

class Manager:
    async def __aenter__(self): ...
    async def __aexit__(self, *args): ...

# error: [invalid-context-manager] "Object of type `Manager` cannot be used with `with` because it does not implement `__enter__` and `__exit__`"
with Manager():
    ...

The error naturally happens since __enter__ and __exit__ are not implemented but we observe that __aenter__ and __aexit__ are implemented. Using this information, the heuristic is that the user probably wanted to use async with which is now mentioned in the subdiagnostics as :

note: Objects of type `Manager` *can* be used as async context managers
note: Consider using `async with` here

Test Plan

I have create a mdtest with snapshot to capture the additional subdiagnostics. I can create more tests if needed.

Copy link
Contributor

github-actions bot commented May 25, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood AlexWaygood added ty Multi-file analysis & type inference diagnostics Related to reporting of diagnostics. labels May 25, 2025
@lipefree
Copy link
Contributor Author

When looking at the mdtest itself, it is hard to know that what is expected, is the additional information provided in the sub-diagnosis. I don't know if I can improve it so that it is clear how the sub-diagnosis should look like instead of having to look at the snap file.

Comment on lines 6063 to 6068
context_expression_type.try_call_dunder(db, "__aenter__", CallArgumentTypes::none()),
context_expression_type.try_call_dunder(
db,
"__aexit__",
CallArgumentTypes::positional([Type::none(db), Type::none(db), Type::none(db)]),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

These __aenter__ and __aexit__ calls will fail if you modify/extend your test example and annotated the parameter types of these two methods on the Manager class accordingly. None will not generally be a valid type for all of the arguments.

We do not necessarily need to be very strict with the passed argument types here (since we're only using it to add a diagnostic hint, and it seems fine to emit it even if that call would fail when passing the wrong parameters). So I guess it's fine to pass Type::unknown() here instead? Another option would be to allow not just Ok(_) results, but also Err(CallDunderError::CallError(..))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey ! Thank you for the review ! If I understand correctly you want to add the sub-diagnosis event if the code was not annotating the types correctly ?
Example :

class Foo:
    async def __aenter__(self):
        ...
    
    def __aexit__(self,
                 exc_type: str, # <= wrong type
                 exc_value: str,
                 traceback: str
                ):
        ...
        
with Foo():
    ...

But even in this case, we should provide the sub-diagnosis even if the provided typing is incorrect. I guess it makes sense. I am just too unexperienced yet, should I implement it to take care of this case ? But then should we also provide the sub-diagnosis if the user is passing the wrong number of arguments ?

class Foo:
    async def __aenter__(self):
        ...
    
    def __aexit__(self,
                 exc_type,
                 exc_value,
                 traceback,
                 extra_arg
                ):
        ...
        
with Foo():
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

But then should we also provide the sub-diagnosis if the user is passing the wrong number of arguments ?

Probably? I think you could achieve this by following this route:

Another option would be to allow not just Ok(_) results, but also Err(CallDunderError::CallError(..))?

I think we should pass Type::unknown() anyway. Passing None is just wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will address this and add more testing then !

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, in case it was unintentional: you're still using Type::none in your latest commit, instead of Type::unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I just changed it in the last commit. Thank you very much for bearing with me !

Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

I pushed one commit with a code simplification and some minor rewordings.

Comment on lines 6062 to 6063
if let (Ok(_) | Err(CallDunderError::CallError(..)), Ok(_))
| (Ok(_), Err(CallDunderError::CallError(..))) = (
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to

Suggested change
if let (Ok(_) | Err(CallDunderError::CallError(..)), Ok(_))
| (Ok(_), Err(CallDunderError::CallError(..))) = (
if let (
Ok(_) | Err(CallDunderError::CallError(..)),
Ok(_) | Err(CallDunderError::CallError(..)),
) = (

I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for all the reviews you gave, I really learned a lot during this PR about python, rust and ty !

@sharkdp sharkdp changed the title [ty] Add subdiagnostic hint if async context manager is used in non-async with statement [ty] Add hint if async context manager is used in non-async with statement May 26, 2025
@sharkdp sharkdp merged commit 1d20cf9 into astral-sh:main May 26, 2025
35 checks passed
carljm added a commit to MatthewMckee4/ruff that referenced this pull request May 28, 2025
* main: (246 commits)
  [ty] Simplify signature types, use them in `CallableType` (astral-sh#18344)
  [ty] Support ephemeral uv virtual environments (astral-sh#18335)
  Add a `ViolationMetadata::rule` method (astral-sh#18234)
  Return `DiagnosticGuard` from `Checker::report_diagnostic` (astral-sh#18232)
  [flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (astral-sh#18337)
  [ty] Support cancellation and retry in the server (astral-sh#18273)
  [ty] Synthetic function-like callables (astral-sh#18242)
  [ty] Support publishing diagnostics in the server (astral-sh#18309)
  Add Autofix for ISC003 (astral-sh#18256)
  [`pyupgrade`]: new rule UP050 (`useless-class-metaclass-type`) (astral-sh#18334)
  [pycodestyle] Make `E712` suggestion not assume a context (astral-sh#18328)
  put similar dunder-call tests next to each other (astral-sh#18343)
  [ty] Derive `PartialOrd, Ord` for `KnownInstanceType` (astral-sh#18340)
  [ty] Simplify `Type::try_bool()` (astral-sh#18342)
  [ty] Simplify `Type::normalized` slightly (astral-sh#18339)
  [ty] Move arviz off the list of selected primer projects (astral-sh#18336)
  [ty] Add --config-file CLI arg (astral-sh#18083)
  [ty] Tell the user why we inferred a certain Python version when reporting version-specific syntax errors (astral-sh#18295)
  [ty] Implement implicit inheritance from `Generic[]` for PEP-695 generic classes (astral-sh#18283)
  [ty] Add hint if async context manager is used in non-async with statement (astral-sh#18299)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostics Related to reporting of diagnostics. ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants