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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
31fb5cb
Add info when using 'with' but implementing async variants __aenter_…
lipefree May 25, 2025
de8aeed
Fix typo in mdtest with sync
lipefree May 25, 2025
cd9fbe3
Fix format in mdtest with sync
lipefree May 25, 2025
75c71ce
Fix format in mdtest and update snapshot in with sync
lipefree May 25, 2025
43cd589
Remove markdown syntax for highlighting
lipefree May 26, 2025
8ef2373
Subdiagnosis are given even when the function is not well typed or wh…
lipefree May 26, 2025
72f9a91
Change arguments type for __exit__ from Type::none(db) to Type::unkno…
lipefree May 26, 2025
86721f1
Minor rewording and simplification
sharkdp May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions ...dtest/snapshots/sync.md_-_With_statements_-_Context_manager_with…_(380131e1948d8d01).snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
---
source: crates/ty_test/src/lib.rs
expression: snapshot
---
---
mdtest name: sync.md - With statements - Context manager with `__aenter__` and `__aexit__`
mdtest path: crates/ty_python_semantic/resources/mdtest/with/sync.md
---

# Python source files

## mdtest_snippet.py

```
1 | class Manager:
2 | async def __aenter__(self): ...
3 | async def __aexit__(self, *args): ...
4 |
5 | # error: [invalid-context-manager] "Object of type `Manager` cannot be used with `with` because it does not implement `__enter__` and `__exit__`"
6 | with Manager():
7 | ...
```

# Diagnostics

```
error[invalid-context-manager]: Object of type `Manager` cannot be used with `with` because it does not implement `__enter__` and `__exit__`
--> src/mdtest_snippet.py:6:6
|
5 | # error: [invalid-context-manager] "Object of type `Manager` cannot be used with `with` because it does not implement `__enter__` and...
6 | with Manager():
| ^^^^^^^^^
7 | ...
|
info: Objects of type `Manager` *can* be used as async context managers
info: Consider using `async with` here
info: rule `invalid-context-manager` is enabled by default

```
17 changes: 17 additions & 0 deletions crates/ty_python_semantic/resources/mdtest/with/sync.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,20 @@ context_expr = Manager()
with context_expr as f:
reveal_type(f) # revealed: str
```

## Context manager with `__aenter__` and `__aexit__`

<!-- snapshot-diagnostics -->

If `__aenter__` and `__aexit__` are implemented, then the `async with` is probably the intentional
usage.

```py
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():
...
```
18 changes: 17 additions & 1 deletion crates/ty_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6051,12 +6051,28 @@ impl<'db> ContextManagerError<'db> {
} => format_call_dunder_errors(enter_error, "__enter__", exit_error, "__exit__"),
};

builder.into_diagnostic(
let mut diag = builder.into_diagnostic(
format_args!(
"Object of type `{context_expression}` cannot be used with `with` because {formatted_errors}",
context_expression = context_expression_type.display(db)
),
);

// If `__aenter__` and `__aexit__` are implemented, using `async with` was maybe the intention here
if let (Ok(_), Ok(_)) = (
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

@sharkdp sharkdp May 26, 2025

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

@lipefree lipefree May 26, 2025

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

@sharkdp sharkdp May 26, 2025

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

@lipefree lipefree May 26, 2025

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

@sharkdp sharkdp May 26, 2025

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

@lipefree lipefree May 26, 2025

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 !

) {
diag.info(format_args!(
"Objects of type `{context_expression}` *can* be used as async context managers",
context_expression = context_expression_type.display(db)
));
diag.info("Consider using `async with` here");
}
}
}

Expand Down
Loading