Table of Contents
Finding where the issue lives #
After installing ty locally for development and ensure both the tests and the type checking works I took a look at their beginner friendly issues list and decided to pick this one about diagnostic improvement, this is my first time working on this kind of system so I think it'll be quite fun.
From the issue's description, I believe we just need update the error reported back to the user, the author was so nice that he even added a snippet to replicate this:
1class Context:
2 def __enter__(self): ...
3 def __exit__(self, *args): ...
4
5
6class NotContext:
7 pass
8
9
10def _(x: Context | NotContext):
11 with x:
12 pass
I copied that snipper to my pydummy project and ran ty on it, I could verify the message returned back:
1$ cargo run --bin ty -- check --project ../pydummy
2 Compiling ty v0.0.0 (ruff/crates/ty)
3 Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.84s
4 Running `target/debug/ty check --project ../pydummy`
5WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
6Checking ------------------------------------------------------------ 1/1 files error[invalid-context-manager]: Object of type `Context | NotContext` cannot be used with `with` because the methods `__enter__` and `__exit__` are possibly unbound
7 --> ../pydummy/main.py:11:10
8 |
910 | def _(x: Context | NotContext):
1011 | with x:
11 | ^
1212 | pass
13 |
14info: rule `invalid-context-manager` is enabled by default
15
16Found 1 diagnostic
Look quite easy to find this out, let's (rip)grep the code base:
1$ rg "invalid-context-manager" -l crates/ty_python_semantic/
2crates/ty_python_semantic/resources/mdtest/with/async.md
3crates/ty_python_semantic/resources/mdtest/with/sync.md
4crates/ty_python_semantic/resources/mdtest/snapshots/async.md_-_Async_with_statement…_-_Accidental_use_of_as…_(5b8c1b4d846bc544).snap
5crates/ty_python_semantic/resources/mdtest/snapshots/sync.md_-_With_statements_-_Accidental_use_of_no…_(b07503f9b773ea61).snap
Tests only so far, must find the rule defined somewhere else, I found a line in crates/ty/docs/rules.md that points to crates/ty_python_semantic/src/types/diagnostic.rs#735, the module were diagnostic rules are defined apparently:
1
2declare_lint! {
3 /// ## What it does
4 /// Checks for expressions used in `with` statements
5 /// that do not implement the context manager protocol.
6 ///
7 /// ## Why is this bad?
8 /// Such a statement will raise `TypeError` at runtime.
9 ///
10 /// ## Examples
11 /// ```python
12 /// # TypeError: 'int' object does not support the context manager protocol
13 /// with 1:
14 /// print(2)
15 /// ```
16 pub(crate) static INVALID_CONTEXT_MANAGER = {
17 summary: "detects expressions used in with statements that don't implement the context manager protocol",
18 status: LintStatus::preview("1.0.0"),
19 default_level: Level::Error,
20 }
21}
Nice! Now we can (rip)grep for INVALID_CONTEXT_MANAGER, that will eventually lead us to crates/ty_python_semantic/src/types.rs#7847 where it declares the enum ContextManagerError:
1/// Error returned if a type is not (or may not be) a context manager.
2#[derive(Debug)]
3enum ContextManagerError<'db> { ... }
4impl<'db> ContextManagerError<'db> { ... }
Once I read that enum and its implementation I confirmed I've found the correct place, more specifically this method:
1fn report_diagnostic(
2 &self,
3 context: &InferContext<'db, '_>,
4 context_expression_type: Type<'db>,
5 context_expression_node: ast::AnyNodeRef,
6)
After some research and dbg! calls I found that context_expression_type is holds the Context | NoContext union type, I've updated my python snippet to this:
1class Bound:
2 def __enter__(self): ...
3
4 def __exit__(self, *args): ...
5
6
7class EnterUnbound:
8 def __exit__(self): ...
9
10
11class ExitUnbound:
12 def __enter__(self): ...
13
14
15class Unbound: ...
16
17
18def bound(x: Bound):
19 with x: ...
20
21
22def enter_unbound(x: EnterUnbound):
23 with x: ...
24
25
26def exit_unbound(x: ExitUnbound):
27 with x: ...
28
29
30def unbound(x: Unbound):
31 with x: ...
32
33
34def union_enter_unbound(x: Bound | EnterUnbound):
35 with x: ...
36
37
38def union_exit_unbound(x: Bound | ExitUnbound):
39 with x: ...
40
41
42def union_unbound(x: Bound | Unbound):
43 with x: ...
Finally, after some executions and dbg! calls, I've identified the place that should be updated:
1let format_call_dunder_error = |call_dunder_error: &CallDunderError<'db>, name: &str| {
2 match call_dunder_error {
3 CallDunderError::MethodNotAvailable => format!("it does not implement `{name}`"),
4 CallDunderError::PossiblyUnbound(_) => {
5 format!("the method `{name}` is possibly unbound")
6 }
7 // TODO: Use more specific error messages for the different error cases.
8 // E.g. hint toward the union variant that doesn't correctly implement enter,
9 // distinguish between a not callable `__enter__` attribute and a wrong signature.
10 CallDunderError::CallError(_, _) => {
11 format!("it does not correctly implement `{name}`")
12 }
13 }
14};
15
16let format_call_dunder_errors = |error_a: &CallDunderError<'db>,
17 name_a: &str,
18 error_b: &CallDunderError<'db>,
19 name_b: &str| {
20 match (error_a, error_b) {
21 (CallDunderError::PossiblyUnbound(_), CallDunderError::PossiblyUnbound(_)) => {
22 format!("the methods `{name_a}` and `{name_b}` are possibly unbound")
23 }
24 (CallDunderError::MethodNotAvailable, CallDunderError::MethodNotAvailable) => {
25 format!("it does not implement `{name_a}` and `{name_b}`")
26 }
27 (CallDunderError::CallError(_, _), CallDunderError::CallError(_, _)) => {
28 format!("it does not correctly implement `{name_a}` or `{name_b}`")
29 }
30 (_, _) => format!(
31 "{format_a}, and {format_b}",
32 format_a = format_call_dunder_error(error_a, name_a),
33 format_b = format_call_dunder_error(error_b, name_b)
34 ),
35 }
36};
Crafting a solution #
Now it's time to figure out how to update those messages, context_expression_type is the variable that holds the Union type, we need to unpack the types that compose the Union and check if they have an unbound method or not.
1let analyze_union_method_issues = |method_name: &str| -> (Vec<Type<'db>>, Vec<Type<'db>>) {
2 if let Type::Union(union) = context_expression_type {
3 let mut missing_method = Vec::new();
4 let mut has_method = Vec::new();
5
6 for element in union.elements(db) {
7 let member_lookup = element.member_lookup_with_policy(
8 db,
9 method_name.into(),
10 MemberLookupPolicy::default() | MemberLookupPolicy::NO_INSTANCE_FALLBACK,
11 );
12
13 match member_lookup.place {
14 Place::Unbound => missing_method.push(*element),
15 Place::Type(_, _) => has_method.push(*element),
16 }
17 }
18
19 (missing_method, has_method)
20 } else {
21 (Vec::new(), Vec::new())
22 }
23};
I'm not sure it's the most idiomatic Rust snippet, but it seems to get the job done, now we need to use it within the format_call_dunder_error and format_call_dunder_errors functions.
1if let Type::Union(_) = context_expression_type {
2 let (missing_method, _) = analyze_union_method_issues(name);
3 if !missing_method.is_empty() {
4 let missing_types: Vec<_> = missing_method.iter()
5 .map(|t| t.display(db).to_string())
6 .collect();
7 return format!(
8 "the method `{name}` of `{}` {} possibly unbound",
9 missing_types.join("` and `"),
10 if missing_types.len() == 1 { "is" } else { "are" }
11 );
12 }
13}
Let's run it with our pydummy project to see what it reports:
$ cargo run --bin ty -- check --project ../pydummy"
Blocking waiting for file lock on package cache
Blocking waiting for file lock on build directory
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.36s
Running `target/debug/ty check --project ../pydummy`
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
Checking ------------------------------------------------------------ 1/1 files error[invalid-context-manager]: Object of type `EnterUnbound` cannot be used with `with` because it does not implement `__enter__`, and it does not correctly implement `__exit__`
--> ../pydummy/main.py:23:10
|
22 | def enter_unbound(x: EnterUnbound):
23 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
error[invalid-context-manager]: Object of type `ExitUnbound` cannot be used with `with` because it does not implement `__exit__`
--> ../pydummy/main.py:27:10
|
26 | def exit_unbound(x: ExitUnbound):
27 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
error[invalid-context-manager]: Object of type `Unbound` cannot be used with `with` because it does not implement `__enter__` and `__exit__`
--> ../pydummy/main.py:31:10
|
30 | def unbound(x: Unbound):
31 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
error[invalid-context-manager]: Object of type `Bound | EnterUnbound` cannot be used with `with` because the method `__enter__` of `EnterUnbound` is possibly unbound
--> ../pydummy/main.py:35:10
|
34 | def union_enter_unbound(x: Bound | EnterUnbound):
35 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
error[invalid-context-manager]: Object of type `Bound | ExitUnbound` cannot be used with `with` because the method `__exit__` of `ExitUnbound` is possibly unbound
--> ../pydummy/main.py:39:10
|
38 | def union_exit_unbound(x: Bound | ExitUnbound):
39 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
error[invalid-context-manager]: Object of type `Bound | Unbound` cannot be used with `with` because the methods `__enter__` and `__exit__` of `Unbound` are possibly unbound
--> ../pydummy/main.py:43:10
|
42 | def union_unbound(x: Bound | Unbound):
43 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
error[invalid-context-manager]: Object of type `Bound | EnterUnbound | ExitUnbound` cannot be used with `with` because the method `__enter__` of `EnterUnbound` is possibly unbound, and the method `__exit__` of `ExitUnbound` is possibly unbound
--> ../pydummy/main.py:47:10
|
46 | def multiple(x: Bound | EnterUnbound | ExitUnbound):
47 | with x: ...
| ^
|
info: rule `invalid-context-manager` is enabled by default
It looks like it worked!
Fixing tests #
However the tests run by cargo nextest run failed, I needed to update the mdtest files but once updated everything was green, guess it's time to send this change upstream.
Upstream pull request #
Here it is, my first ty / Rust pull request ever.
last updated: