[ty] Improve invalid assignment diagnostics with type context#22643
[ty] Improve invalid assignment diagnostics with type context#22643ibraheemdev merged 2 commits intomainfrom
Conversation
9fc1eae to
d6a2820
Compare
Typing conformance resultsNo changes detected ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
0 | 5 | 29 |
invalid-return-type |
6 | 0 | 25 |
invalid-assignment |
5 | 0 | 25 |
possibly-missing-attribute |
2 | 0 | 2 |
no-matching-overload |
0 | 2 | 0 |
unresolved-attribute |
0 | 2 | 0 |
| Total | 13 | 9 | 81 |
Merging this PR will improve performance by 4.23%
Performance Changes
Comparing Footnotes
|
| // We silence diagnostics until we successfully narrow to a specific type. | ||
| let mut speculated_bindings = bindings.clone(); | ||
| let was_in_multi_inference = self.context.set_multi_inference(true); | ||
|
|
||
| let mut try_narrow = |narrowed_ty| { | ||
| let mut speculated_bindings = bindings.clone(); | ||
| let narrowed_tcx = TypeContext::new(Some(narrowed_ty)); |
There was a problem hiding this comment.
Is this (only lazily cloning the bindings if it's necessary) the reason for the performance improvement?
It looks like try_narrow() could be called multiple times, so I wonder if you could do this to micro-optimize it even more:
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index 911f9b5253..2063c7a09d 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -8700,9 +8700,12 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
// We silence diagnostics until we successfully narrow to a specific type.
let was_in_multi_inference = self.context.set_multi_inference(true);
+ let mut speculated_bindings_cache = None;
let mut try_narrow = |narrowed_ty| {
- let mut speculated_bindings = bindings.clone();
+ let speculated_bindings =
+ speculated_bindings_cache.get_or_insert_with(|| bindings.clone());
+
let narrowed_tcx = TypeContext::new(Some(narrowed_ty));There was a problem hiding this comment.
This was actually a bug. We should be creating a fresh Bindings every time, because the speculative calls to check_types mutate internal state. This led to a few bugs after the changes (I'm not sure why we didn't notice before).
As a performance consideration, it is probably possible to reset the fields that were mutated instead of cloning, but I'm not sure it matters.
ac78c7e to
e4c20ab
Compare
Summary
Resolves astral-sh/ty#2537.