[ty] Avoid duplicate diagnostic in unpacking (#18897)

## Summary

This PR fixes astral-sh/ty#185 by avoiding to infer the value expression
for an unpacking.

This is done simply by only inferring the value expression in a
non-unpacking branch for assignment statement, for statement, with
statement and comprehensions.

This is a simpler alternative to
https://github.com/astral-sh/ruff/pull/18890 which I only realized in
hindsight! Ideally, the solution would to consider the "unpack" as it's
own region and do all of the inference of every expressions involved in
an unpacking inside the unpack query and then merge the results in the
outer query. This would require access to the `Unpack` ingredient which
is stored on the `Definition`. And, this would require create the said
`Definition`s for all attributes and subscript expressions. It does
simplify the target inference logic by streamlining it into a single
`infer_target` method instead of the `infer_target`/`infer_target_impl`
split.

Additionally, #18890 also solves a couple of TODOs around raising errors
around attribute / subscript assignment.

## Test Plan

Update the existing test, go through a couple of ecosystem diagnostic.
This commit is contained in:
Dhruv Manilawala 2025-06-24 07:49:44 +05:30 committed by GitHub
parent da16e00751
commit e474f36473
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 52 additions and 45 deletions

View file

@ -734,9 +734,6 @@ with ContextManager() as (a, *b):
### Unbound context manager expression ### Unbound context manager expression
```py ```py
# TODO: should only be one diagnostic
# error: [unresolved-reference] "Name `nonexistant` used when not defined"
# error: [unresolved-reference] "Name `nonexistant` used when not defined"
# error: [unresolved-reference] "Name `nonexistant` used when not defined" # error: [unresolved-reference] "Name `nonexistant` used when not defined"
with nonexistant as (x, y): with nonexistant as (x, y):
reveal_type(x) # revealed: Unknown reveal_type(x) # revealed: Unknown

View file

@ -2752,9 +2752,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let context_expr = with_item.context_expr(self.module()); let context_expr = with_item.context_expr(self.module());
let target = with_item.target(self.module()); let target = with_item.target(self.module());
let context_expr_ty = self.infer_standalone_expression(context_expr);
let target_ty = if with_item.is_async() { let target_ty = if with_item.is_async() {
let _context_expr_ty = self.infer_standalone_expression(context_expr);
todo_type!("async `with` statement") todo_type!("async `with` statement")
} else { } else {
match with_item.target_kind() { match with_item.target_kind() {
@ -2766,11 +2765,14 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
} }
unpacked.expression_type(target_ast_id) unpacked.expression_type(target_ast_id)
} }
TargetKind::Single => self.infer_context_expression( TargetKind::Single => {
context_expr, let context_expr_ty = self.infer_standalone_expression(context_expr);
context_expr_ty, self.infer_context_expression(
with_item.is_async(), context_expr,
), context_expr_ty,
with_item.is_async(),
)
}
} }
}; };
@ -3744,8 +3746,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let value = assignment.value(self.module()); let value = assignment.value(self.module());
let target = assignment.target(self.module()); let target = assignment.target(self.module());
let value_ty = self.infer_standalone_expression(value);
let mut target_ty = match assignment.target_kind() { let mut target_ty = match assignment.target_kind() {
TargetKind::Sequence(unpack_position, unpack) => { TargetKind::Sequence(unpack_position, unpack) => {
let unpacked = infer_unpack_types(self.db(), unpack); let unpacked = infer_unpack_types(self.db(), unpack);
@ -3759,6 +3759,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
unpacked.expression_type(target_ast_id) unpacked.expression_type(target_ast_id)
} }
TargetKind::Single => { TargetKind::Single => {
let value_ty = self.infer_standalone_expression(value);
// `TYPE_CHECKING` is a special variable that should only be assigned `False` // `TYPE_CHECKING` is a special variable that should only be assigned `False`
// at runtime, but is always considered `True` in type checking. // at runtime, but is always considered `True` in type checking.
// See mdtest/known_constants.md#user-defined-type_checking for details. // See mdtest/known_constants.md#user-defined-type_checking for details.
@ -4063,9 +4065,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let iterable = for_stmt.iterable(self.module()); let iterable = for_stmt.iterable(self.module());
let target = for_stmt.target(self.module()); let target = for_stmt.target(self.module());
let iterable_type = self.infer_standalone_expression(iterable);
let loop_var_value_type = if for_stmt.is_async() { let loop_var_value_type = if for_stmt.is_async() {
let _iterable_type = self.infer_standalone_expression(iterable);
todo_type!("async iterables/iterators") todo_type!("async iterables/iterators")
} else { } else {
match for_stmt.target_kind() { match for_stmt.target_kind() {
@ -4077,10 +4078,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let target_ast_id = target.scoped_expression_id(self.db(), self.scope()); let target_ast_id = target.scoped_expression_id(self.db(), self.scope());
unpacked.expression_type(target_ast_id) unpacked.expression_type(target_ast_id)
} }
TargetKind::Single => iterable_type.try_iterate(self.db()).unwrap_or_else(|err| { TargetKind::Single => {
err.report_diagnostic(&self.context, iterable_type, iterable.into()); let iterable_type = self.infer_standalone_expression(iterable);
err.fallback_element_type(self.db()) iterable_type.try_iterate(self.db()).unwrap_or_else(|err| {
}), err.report_diagnostic(&self.context, iterable_type, iterable.into());
err.fallback_element_type(self.db())
})
}
} }
}; };
@ -5078,34 +5082,37 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let iterable = comprehension.iterable(self.module()); let iterable = comprehension.iterable(self.module());
let target = comprehension.target(self.module()); let target = comprehension.target(self.module());
let expression = self.index.expression(iterable); let mut infer_iterable_type = || {
let result = infer_expression_types(self.db(), expression); let expression = self.index.expression(iterable);
let result = infer_expression_types(self.db(), expression);
// Two things are different if it's the first comprehension: // Two things are different if it's the first comprehension:
// (1) We must lookup the `ScopedExpressionId` of the iterable expression in the outer scope, // (1) We must lookup the `ScopedExpressionId` of the iterable expression in the outer scope,
// because that's the scope we visit it in in the semantic index builder // because that's the scope we visit it in in the semantic index builder
// (2) We must *not* call `self.extend()` on the result of the type inference, // (2) We must *not* call `self.extend()` on the result of the type inference,
// because `ScopedExpressionId`s are only meaningful within their own scope, so // because `ScopedExpressionId`s are only meaningful within their own scope, so
// we'd add types for random wrong expressions in the current scope // we'd add types for random wrong expressions in the current scope
let iterable_type = if comprehension.is_first() && target.is_name_expr() { if comprehension.is_first() && target.is_name_expr() {
let lookup_scope = self let lookup_scope = self
.index .index
.parent_scope_id(self.scope().file_scope_id(self.db())) .parent_scope_id(self.scope().file_scope_id(self.db()))
.expect("A comprehension should never be the top-level scope") .expect("A comprehension should never be the top-level scope")
.to_scope_id(self.db(), self.file()); .to_scope_id(self.db(), self.file());
result.expression_type(iterable.scoped_expression_id(self.db(), lookup_scope)) result.expression_type(iterable.scoped_expression_id(self.db(), lookup_scope))
} else { } else {
let scope = self.types.scope; let scope = self.types.scope;
self.types.scope = result.scope; self.types.scope = result.scope;
self.extend(result); self.extend(result);
self.types.scope = scope; self.types.scope = scope;
result.expression_type( result.expression_type(
iterable.scoped_expression_id(self.db(), expression.scope(self.db())), iterable.scoped_expression_id(self.db(), expression.scope(self.db())),
) )
}
}; };
let target_type = if comprehension.is_async() { let target_type = if comprehension.is_async() {
// TODO: async iterables/iterators! -- Alex // TODO: async iterables/iterators! -- Alex
let _iterable_type = infer_iterable_type();
todo_type!("async iterables/iterators") todo_type!("async iterables/iterators")
} else { } else {
match comprehension.target_kind() { match comprehension.target_kind() {
@ -5118,10 +5125,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
target.scoped_expression_id(self.db(), unpack.target_scope(self.db())); target.scoped_expression_id(self.db(), unpack.target_scope(self.db()));
unpacked.expression_type(target_ast_id) unpacked.expression_type(target_ast_id)
} }
TargetKind::Single => iterable_type.try_iterate(self.db()).unwrap_or_else(|err| { TargetKind::Single => {
err.report_diagnostic(&self.context, iterable_type, iterable.into()); let iterable_type = infer_iterable_type();
err.fallback_element_type(self.db()) iterable_type.try_iterate(self.db()).unwrap_or_else(|err| {
}), err.report_diagnostic(&self.context, iterable_type, iterable.into());
err.fallback_element_type(self.db())
})
}
} }
}; };