From fe28fcba5e1875b9bb8006f377afbaed4af1f753 Mon Sep 17 00:00:00 2001
From: Charlie Marsh <charlie.r.marsh@gmail.com>
Date: Thu, 27 Jul 2023 17:13:13 -0400
Subject: [PATCH] Add documentation and test cases for redefinition

---
 crates/ruff/src/rules/pyflakes/mod.rs      | 39 ++++++++++++
 crates/ruff_python_semantic/src/binding.rs | 70 ++++++++++++++--------
 2 files changed, 84 insertions(+), 25 deletions(-)

diff --git a/crates/ruff/src/rules/pyflakes/mod.rs b/crates/ruff/src/rules/pyflakes/mod.rs
index e2dae95b695c9..08170bb5191d7 100644
--- a/crates/ruff/src/rules/pyflakes/mod.rs
+++ b/crates/ruff/src/rules/pyflakes/mod.rs
@@ -2414,6 +2414,45 @@ mod tests {
         );
     }
 
+    #[test]
+    fn aliased_submodule_import() {
+        flakes(
+            r#"
+        import fu.bar as baz
+        import fu.bar as baz
+        baz
+        "#,
+            &[Rule::RedefinedWhileUnused],
+        );
+
+        flakes(
+            r#"
+        import fu.bar as baz
+        import baz
+        baz
+        "#,
+            &[Rule::RedefinedWhileUnused],
+        );
+
+        flakes(
+            r#"
+        import fu.bar as baz
+        import fu.bar as bop
+        baz, bop
+        "#,
+            &[],
+        );
+
+        flakes(
+            r#"
+        import foo.baz
+        import foo.baz as foo
+        foo
+        "#,
+            &[Rule::RedefinedWhileUnused],
+        );
+    }
+
     #[test]
     fn used_package_with_submodule_import() {
         // Usage of package marks submodule imports as used.
diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs
index 622ae324359c9..6a6e712f5864f 100644
--- a/crates/ruff_python_semantic/src/binding.rs
+++ b/crates/ruff_python_semantic/src/binding.rs
@@ -100,43 +100,62 @@ impl<'a> Binding<'a> {
         self.flags.contains(BindingFlags::PRIVATE_DECLARATION)
     }
 
-    /// Return `true` if this binding redefines the given binding.
+    /// Return `true` if this binding "redefines" the given binding, as per Pyflake's definition of
+    /// redefinition.
     pub fn redefines(&self, existing: &'a Binding) -> bool {
         match &self.kind {
-            BindingKind::Import(Import { qualified_name }) => {
+            // Submodule imports are only considered redefinitions if they import the same
+            // submodule. For example, this is a redefinition:
+            // ```python
+            // import foo.bar
+            // import foo.bar
+            // ```
+            //
+            // This, however, is not:
+            // ```python
+            // import foo.bar
+            // import foo.baz
+            // ```
+            BindingKind::Import(Import {
+                qualified_name: redefinition,
+            }) => {
                 if let BindingKind::SubmoduleImport(SubmoduleImport {
-                    qualified_name: existing,
+                    qualified_name: definition,
                 }) = &existing.kind
                 {
-                    return qualified_name == existing;
+                    return redefinition == definition;
                 }
             }
-            BindingKind::FromImport(FromImport { qualified_name }) => {
+            BindingKind::FromImport(FromImport {
+                qualified_name: redefinition,
+            }) => {
                 if let BindingKind::SubmoduleImport(SubmoduleImport {
-                    qualified_name: existing,
+                    qualified_name: definition,
                 }) = &existing.kind
                 {
-                    return qualified_name == existing;
+                    return redefinition == definition;
                 }
             }
-            BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => {
-                match &existing.kind {
-                    BindingKind::Import(Import {
-                        qualified_name: existing,
-                    })
-                    | BindingKind::SubmoduleImport(SubmoduleImport {
-                        qualified_name: existing,
-                    }) => {
-                        return qualified_name == existing;
-                    }
-                    BindingKind::FromImport(FromImport {
-                        qualified_name: existing,
-                    }) => {
-                        return qualified_name == existing;
-                    }
-                    _ => {}
+            BindingKind::SubmoduleImport(SubmoduleImport {
+                qualified_name: redefinition,
+            }) => match &existing.kind {
+                BindingKind::Import(Import {
+                    qualified_name: definition,
+                })
+                | BindingKind::SubmoduleImport(SubmoduleImport {
+                    qualified_name: definition,
+                }) => {
+                    return redefinition == definition;
                 }
-            }
+                BindingKind::FromImport(FromImport {
+                    qualified_name: definition,
+                }) => {
+                    return redefinition == definition;
+                }
+                _ => {}
+            },
+            // Deletions, annotations, `__future__` imports, and builtins are never considered
+            // redefinitions.
             BindingKind::Deletion
             | BindingKind::Annotation
             | BindingKind::FutureImport
@@ -145,13 +164,14 @@ impl<'a> Binding<'a> {
             }
             _ => {}
         }
+        // Otherwise, the shadowed binding must be a class definition, function definition, or
+        // import to be considered a redefinition.
         matches!(
             existing.kind,
             BindingKind::ClassDefinition(_)
                 | BindingKind::FunctionDefinition(_)
                 | BindingKind::Import(_)
                 | BindingKind::FromImport(_)
-                | BindingKind::SubmoduleImport(_)
         )
     }