Iterators are reversed (not working yet)

Author: dcreager

crates/red_knot_python_semantic/resources/mdtest/snapshots/for.md_-_For_loops_-_With_non-callable_iterator.snap.new

@@ -0,0 +1,58 @@
+---
+source: crates/red_knot_test/src/lib.rs
+assertion_line: 272
+expression: snapshot
+---
+---
+mdtest name: for.md - For loops - With non-callable iterator
+mdtest path: crates/red_knot_python_semantic/resources/mdtest/loops/for.md
+---
+
+# Python source files
+
+## mdtest_snippet.py
+
+```
+ 1 | from typing_extensions import reveal_type
+ 2 | 
+ 3 | def _(flag: bool):
+ 4 |     class NotIterable:
+ 5 |         if flag:
+ 6 |             __iter__: int = 1
+ 7 |         else:
+ 8 |             __iter__: None = None
+ 9 | 
+10 |     # error: [not-iterable]
+11 |     for x in NotIterable():
+12 |         pass
+13 | 
+14 |     # revealed: Unknown
+15 |     # error: [possibly-unresolved-reference]
+16 |     reveal_type(x)
+```
+
+# Diagnostics
+
+```
+error: lint:not-iterable
+  --> /src/mdtest_snippet.py:11:14
+   |
+10 |     # error: [not-iterable]
+11 |     for x in NotIterable():
+   |              ^^^^^^^^^^^^^ Object of type `NotIterable` is not iterable because its `__iter__` attribute has type `int | None`, which is not callable
+12 |         pass
+   |
+
+```
+
+```
+info: revealed-type
+  --> /src/mdtest_snippet.py:16:5
+   |
+14 |     # revealed: Unknown
+15 |     # error: [possibly-unresolved-reference]
+16 |     reveal_type(x)
+   |     -------------- info: Revealed type is `Unknown`
+   |
+
+```

crates/red_knot_python_semantic/src/semantic_index.rs

@@ -426,12 +426,14 @@ mod tests {
     impl UseDefMap<'_> {
         fn first_public_binding(&self, symbol: ScopedSymbolId) -> Option<Definition<'_>> {
             self.public_bindings(symbol)
-                .find_map(|constrained_binding| constrained_binding.binding)
+                .filter_map(|constrained_binding| constrained_binding.binding)
+                .last()
         }
 
         fn first_binding_at_use(&self, use_id: ScopedUseId) -> Option<Definition<'_>> {
             self.bindings_at_use(use_id)
-                .find_map(|constrained_binding| constrained_binding.binding)
+                .filter_map(|constrained_binding| constrained_binding.binding)
+                .last()
         }
     }
 

crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs

@@ -485,7 +485,7 @@ mod tests {
         symbol: &SymbolState,
         expected: &[&str],
     ) {
-        let actual = symbol_states
+        let mut actual = symbol_states
             .bindings
             .iter_reverse(symbol.bindings.live_bindings)
             .map(|(def_id, live_binding)| {
@@ -502,6 +502,7 @@ mod tests {
                 format!("{def}<{constraints}>")
             })
             .collect::<Vec<_>>();
+        actual.reverse();
         assert_eq!(actual, expected);
     }
 
@@ -511,7 +512,7 @@ mod tests {
         symbol: &SymbolState,
         expected: &[&str],
     ) {
-        let actual = symbol_states
+        let mut actual = symbol_states
             .declarations
             .iter_reverse(symbol.declarations.live_declarations)
             .map(|(declaration, _)| {
@@ -522,6 +523,7 @@ mod tests {
                 }
             })
             .collect::<Vec<_>>();
+        actual.reverse();
         assert_eq!(actual, expected);
     }
 

crates/red_knot_python_semantic/src/symbol.rs

@@ -557,66 +557,78 @@ fn symbol_from_bindings_impl<'db>(
         _ => Truthiness::AlwaysFalse,
     };
 
-    let mut types = bindings_with_constraints.filter_map(
-        |BindingWithConstraints {
-             binding,
-             narrowing_constraint,
-             visibility_constraint,
-         }| {
-            let binding = binding?;
-
-            if is_non_exported(binding) {
-                return None;
-            }
-
-            let static_visibility =
-                visibility_constraints.evaluate(db, constraints, visibility_constraint);
-
-            if static_visibility.is_always_false() {
-                return None;
-            }
+    let mut types: Vec<_> = bindings_with_constraints
+        .filter_map(
+            |BindingWithConstraints {
+                 binding,
+                 narrowing_constraint,
+                 visibility_constraint,
+             }| {
+                let binding = binding?;
+
+                if is_non_exported(binding) {
+                    return None;
+                }
 
-            let constraint_tys: Vec<_> = narrowing_constraint
-                .filter_map(|constraint| infer_narrowing_constraint(db, constraint, binding))
-                .collect();
+                let static_visibility =
+                    visibility_constraints.evaluate(db, constraints, visibility_constraint);
 
-            let binding_ty = binding_type(db, binding);
-            if constraint_tys.is_empty() {
-                Some(binding_ty)
-            } else {
-                let intersection_ty = constraint_tys
-                    .into_iter()
-                    .rev()
-                    .fold(
-                        IntersectionBuilder::new(db).add_positive(binding_ty),
-                        IntersectionBuilder::add_positive,
-                    )
-                    .build();
-                Some(intersection_ty)
-            }
-        },
-    );
+                if static_visibility.is_always_false() {
+                    return None;
+                }
 
-    if let Some(first) = types.next() {
-        let boundness = match unbound_visibility {
-            Truthiness::AlwaysTrue => {
-                unreachable!("If we have at least one binding, the scope-start should not be definitely visible")
-            }
-            Truthiness::AlwaysFalse => Boundness::Bound,
-            Truthiness::Ambiguous => Boundness::PossiblyUnbound,
-        };
+                let constraint_tys: Vec<_> = narrowing_constraint
+                    .filter_map(|constraint| infer_narrowing_constraint(db, constraint, binding))
+                    .collect();
+
+                let binding_ty = binding_type(db, binding);
+                if constraint_tys.is_empty() {
+                    Some(binding_ty)
+                } else {
+                    let intersection_ty = constraint_tys
+                        .into_iter()
+                        .rev()
+                        .fold(
+                            IntersectionBuilder::new(db).add_positive(binding_ty),
+                            IntersectionBuilder::add_positive,
+                        )
+                        .build();
+                    Some(intersection_ty)
+                }
+            },
+        )
+        .collect();
+
+    // The bindings iterator is in reverse order, so popping the last element from the collected
+    // vector gives us the first binding.
+    let Some(first) = types.pop() else {
+        return Symbol::Unbound;
+    };
 
-        if let Some(second) = types.next() {
-            Symbol::Type(
-                UnionType::from_elements(db, [first, second].into_iter().chain(types)),
-                boundness,
+    let boundness = match unbound_visibility {
+        Truthiness::AlwaysTrue => {
+            unreachable!(
+                "If we have at least one binding, the scope-start should not be definitely visible"
             )
-        } else {
-            Symbol::Type(first, boundness)
         }
-    } else {
-        Symbol::Unbound
+        Truthiness::AlwaysFalse => Boundness::Bound,
+        Truthiness::Ambiguous => Boundness::PossiblyUnbound,
+    };
+
+    if types.is_empty() {
+        return Symbol::Type(first, boundness);
     }
+
+    Symbol::Type(
+        types
+            .into_iter()
+            .rev()
+            .fold(UnionBuilder::new(db).add(first), |builder, element| {
+                builder.add(element)
+            })
+            .build(),
+        boundness,
+    )
 }
 
 /// Implementation of [`symbol_from_declarations`].
@@ -647,70 +659,77 @@ fn symbol_from_declarations_impl<'db>(
         _ => Truthiness::AlwaysFalse,
     };
 
-    let mut types = declarations.filter_map(
-        |DeclarationWithConstraint {
-             declaration,
-             visibility_constraint,
-         }| {
-            let declaration = declaration?;
+    let mut types: Vec<_> = declarations
+        .filter_map(
+            |DeclarationWithConstraint {
+                 declaration,
+                 visibility_constraint,
+             }| {
+                let declaration = declaration?;
 
-            if is_non_exported(declaration) {
-                return None;
-            }
+                if is_non_exported(declaration) {
+                    return None;
+                }
 
-            let static_visibility =
-                visibility_constraints.evaluate(db, constraints, visibility_constraint);
+                let static_visibility =
+                    visibility_constraints.evaluate(db, constraints, visibility_constraint);
 
-            if static_visibility.is_always_false() {
-                None
-            } else {
-                Some(declaration_type(db, declaration))
-            }
-        },
-    );
-
-    if let Some(first) = types.next() {
-        let mut conflicting: Vec<Type<'db>> = vec![];
-        let declared_ty = if let Some(second) = types.next() {
-            let ty_first = first.inner_type();
-            let mut qualifiers = first.qualifiers();
-
-            let mut builder = UnionBuilder::new(db).add(ty_first);
-            for other in std::iter::once(second).chain(types) {
-                let other_ty = other.inner_type();
-                if !ty_first.is_equivalent_to(db, other_ty) {
-                    conflicting.push(other_ty);
+                if static_visibility.is_always_false() {
+                    None
+                } else {
+                    Some(declaration_type(db, declaration))
                 }
-                builder = builder.add(other_ty);
-                qualifiers = qualifiers.union(other.qualifiers());
-            }
-            TypeAndQualifiers::new(builder.build(), qualifiers)
-        } else {
-            first
-        };
-        if conflicting.is_empty() {
-            let boundness = match undeclared_visibility {
-                Truthiness::AlwaysTrue => {
-                    unreachable!("If we have at least one declaration, the scope-start should not be definitely visible")
-                }
-                Truthiness::AlwaysFalse => Boundness::Bound,
-                Truthiness::Ambiguous => Boundness::PossiblyUnbound,
-            };
+            },
+        )
+        .collect();
+
+    // The bindings iterator is in reverse order, so popping the last element from the collected
+    // vector gives us the first binding.
+    let Some(first) = types.pop() else {
+        return Ok(Symbol::Unbound.into());
+    };
 
-            Ok(SymbolAndQualifiers(
-                Symbol::Type(declared_ty.inner_type(), boundness),
-                declared_ty.qualifiers(),
-            ))
-        } else {
-            Err((
-                declared_ty,
-                std::iter::once(first.inner_type())
-                    .chain(conflicting)
-                    .collect(),
-            ))
+    let boundness = match undeclared_visibility {
+        Truthiness::AlwaysTrue => {
+            unreachable!("If we have at least one declaration, the scope-start should not be definitely visible")
+        }
+        Truthiness::AlwaysFalse => Boundness::Bound,
+        Truthiness::Ambiguous => Boundness::PossiblyUnbound,
+    };
+
+    if types.is_empty() {
+        return Ok(SymbolAndQualifiers(
+            Symbol::Type(first.inner_type(), boundness),
+            first.qualifiers(),
+        ));
+    }
+
+    let mut conflicting: Vec<Type<'db>> = vec![];
+    let ty_first = first.inner_type();
+    let mut qualifiers = first.qualifiers();
+
+    let mut builder = UnionBuilder::new(db).add(ty_first);
+    for other in types.into_iter().rev() {
+        let other_ty = other.inner_type();
+        if !ty_first.is_equivalent_to(db, other_ty) {
+            conflicting.push(other_ty);
         }
+        builder = builder.add(other_ty);
+        qualifiers = qualifiers.union(other.qualifiers());
+    }
+    let declared_ty = builder.build();
+
+    if conflicting.is_empty() {
+        Ok(SymbolAndQualifiers(
+            Symbol::Type(declared_ty, boundness),
+            qualifiers,
+        ))
     } else {
-        Ok(Symbol::Unbound.into())
+        conflicting.insert(0, ty_first);
+        Err((
+            TypeAndQualifiers::new(declared_ty, qualifiers),
+            conflicting.into(),
+        ))
     }
 }
 

crates/red_knot_python_semantic/src/types/infer.rs

@@ -6698,7 +6698,8 @@ mod tests {
         let scope = global_scope(db, file);
         use_def_map(db, scope)
             .public_bindings(symbol_table(db, scope).symbol_id_by_name(name).unwrap())
-            .find_map(|b| b.binding)
+            .filter_map(|b| b.binding)
+            .last()
             .expect("no binding found")
     }