Fix duplicate unpack diagnostics (#14125)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

crates/red_knot_python_semantic/resources/mdtest/unpacking.md

@@ -145,13 +145,8 @@ reveal_type(f)  # revealed: Unknown
 
 ### Non-iterable unpacking
 
-TODO: Remove duplicate diagnostics. This is happening because for a sequence-like assignment target,
-multiple definitions are created and the inference engine runs on each of them which results in
-duplicate diagnostics.
-
 ```py
 # error: "Object of type `Literal[1]` is not iterable"
-# error: "Object of type `Literal[1]` is not iterable"
 a, b = 1
 reveal_type(a)  # revealed: Unknown
 reveal_type(b)  # revealed: Unknown

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -281,8 +281,12 @@ impl<'db> SemanticIndexBuilder<'db> {
         debug_assert!(popped_assignment.is_some());
     }
 
-    fn current_assignment(&self) -> Option<&CurrentAssignment<'db>> {
+    fn current_assignment(&self) -> Option<CurrentAssignment<'db>> {
-        self.current_assignments.last()
+        self.current_assignments.last().copied()
+    }
+
+    fn current_assignment_mut(&mut self) -> Option<&mut CurrentAssignment<'db>> {
+        self.current_assignments.last_mut()
     }
 
     fn add_pattern_constraint(
@@ -627,6 +631,7 @@ where
                         ast::Expr::List(_) | ast::Expr::Tuple(_) => {
                             Some(CurrentAssignment::Assign {
                                 node,
+                                first: true,
                                 unpack: Some(Unpack::new(
                                     self.db,
                                     self.file,
@@ -640,9 +645,11 @@ where
                                 )),
                             })
                         }
-                        ast::Expr::Name(_) => {
+                        ast::Expr::Name(_) => Some(CurrentAssignment::Assign {
-                            Some(CurrentAssignment::Assign { node, unpack: None })
+                            node,
-                        }
+                            unpack: None,
+                            first: false,
+                        }),
                         _ => None,
                     };
 
@@ -987,14 +994,19 @@ where
                 }
 
                 if is_definition {
-                    match self.current_assignment().copied() {
+                    match self.current_assignment() {
-                        Some(CurrentAssignment::Assign { node, unpack }) => {
+                        Some(CurrentAssignment::Assign {
+                            node,
+                            first,
+                            unpack,
+                        }) => {
                             self.add_definition(
                                 symbol,
                                 AssignmentDefinitionNodeRef {
                                     unpack,
                                     value: &node.value,
                                     name: name_node,
+                                    first,
                                 },
                             );
                         }
@@ -1045,6 +1057,11 @@ where
                     }
                 }
 
+                if let Some(CurrentAssignment::Assign { first, .. }) = self.current_assignment_mut()
+                {
+                    *first = false;
+                }
+
                 walk_expr(self, expr);
             }
             ast::Expr::Named(node) => {
@@ -1245,6 +1262,7 @@ where
 enum CurrentAssignment<'a> {
     Assign {
         node: &'a ast::StmtAssign,
+        first: bool,
         unpack: Option<Unpack<'a>>,
     },
     AnnAssign(&'a ast::StmtAnnAssign),

crates/red_knot_python_semantic/src/semantic_index/definition.rs

@@ -183,6 +183,7 @@ pub(crate) struct AssignmentDefinitionNodeRef<'a> {
     pub(crate) unpack: Option<Unpack<'a>>,
     pub(crate) value: &'a ast::Expr,
     pub(crate) name: &'a ast::ExprName,
+    pub(crate) first: bool,
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -250,10 +251,12 @@ impl<'db> DefinitionNodeRef<'db> {
                 unpack,
                 value,
                 name,
+                first,
             }) => DefinitionKind::Assignment(AssignmentDefinitionKind {
                 target: TargetKind::from(unpack),
                 value: AstNodeRef::new(parsed.clone(), value),
                 name: AstNodeRef::new(parsed, name),
+                first,
             }),
             DefinitionNodeRef::AnnotatedAssignment(assign) => {
                 DefinitionKind::AnnotatedAssignment(AstNodeRef::new(parsed, assign))
@@ -330,6 +333,7 @@ impl<'db> DefinitionNodeRef<'db> {
                 value: _,
                 unpack: _,
                 name,
+                first: _,
             }) => name.into(),
             Self::AnnotatedAssignment(node) => node.into(),
             Self::AugmentedAssignment(node) => node.into(),
@@ -535,10 +539,11 @@ pub struct AssignmentDefinitionKind<'db> {
     target: TargetKind<'db>,
     value: AstNodeRef<ast::Expr>,
     name: AstNodeRef<ast::ExprName>,
+    first: bool,
 }
 
-impl AssignmentDefinitionKind<'_> {
+impl<'db> AssignmentDefinitionKind<'db> {
-    pub(crate) fn target(&self) -> TargetKind {
+    pub(crate) fn target(&self) -> TargetKind<'db> {
         self.target
     }
 
@@ -549,6 +554,10 @@ impl AssignmentDefinitionKind<'_> {
     pub(crate) fn name(&self) -> &ast::ExprName {
         self.name.node()
     }
+
+    pub(crate) fn is_first(&self) -> bool {
+        self.first
+    }
 }
 
 #[derive(Clone, Debug)]

crates/red_knot_python_semantic/src/types/infer.rs

@@ -41,7 +41,8 @@ use crate::module_name::ModuleName;
 use crate::module_resolver::{file_to_module, resolve_module};
 use crate::semantic_index::ast_ids::{HasScopedAstId, HasScopedUseId, ScopedExpressionId};
 use crate::semantic_index::definition::{
-    Definition, DefinitionKind, DefinitionNodeKey, ExceptHandlerDefinitionKind, TargetKind,
+    AssignmentDefinitionKind, Definition, DefinitionKind, DefinitionNodeKey,
+    ExceptHandlerDefinitionKind, TargetKind,
 };
 use crate::semantic_index::expression::Expression;
 use crate::semantic_index::semantic_index;
@@ -532,12 +533,7 @@ impl<'db> TypeInferenceBuilder<'db> {
                 );
             }
             DefinitionKind::Assignment(assignment) => {
-                self.infer_assignment_definition(
+                self.infer_assignment_definition(assignment, definition);
-                    assignment.target(),
-                    assignment.value(),
-                    assignment.name(),
-                    definition,
-                );
             }
             DefinitionKind::AnnotatedAssignment(annotated_assignment) => {
                 self.infer_annotated_assignment_definition(annotated_assignment.node(), definition);
@@ -1427,20 +1423,26 @@ impl<'db> TypeInferenceBuilder<'db> {
 
     fn infer_assignment_definition(
         &mut self,
-        target: TargetKind<'db>,
+        assignment: &AssignmentDefinitionKind<'db>,
-        value: &ast::Expr,
-        name: &ast::ExprName,
         definition: Definition<'db>,
     ) {
+        let value = assignment.value();
+        let name = assignment.name();
+
         self.infer_standalone_expression(value);
 
         let value_ty = self.expression_ty(value);
         let name_ast_id = name.scoped_ast_id(self.db, self.scope());
 
-        let target_ty = match target {
+        let target_ty = match assignment.target() {
             TargetKind::Sequence(unpack) => {
                 let unpacked = infer_unpack_types(self.db, unpack);
+                // Only copy the diagnostics if this is the first assignment to avoid duplicating the
+                // unpack assignments.
+                if assignment.is_first() {
                     self.diagnostics.extend(unpacked.diagnostics());
+                }
+
                 unpacked.get(name_ast_id).unwrap_or(Type::Unknown)
             }
             TargetKind::Name => value_ty,