Skip to content

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