avoid creating * definitions for invalid syntax

Author: AlexWaygood

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -346,6 +346,10 @@ impl<'db> SemanticIndexBuilder<'db> {
         self.current_symbol_table().mark_symbol_used(id);
     }
 
+    fn add_entry_for_definition_key(&mut self, key: DefinitionNodeKey) -> &mut Definitions<'db> {
+        self.definitions_by_node.entry(key).or_default()
+    }
+
     fn add_definition(
         &mut self,
         symbol: ScopedSymbolId,
@@ -368,9 +372,7 @@ impl<'db> SemanticIndexBuilder<'db> {
             countme::Count::default(),
         );
 
-        self.definitions_by_node
-            .entry(definition_node.key())
-            .or_default()
+        self.add_entry_for_definition_key(definition_node.key())
             .push(definition);
 
         if category.is_binding() {
@@ -997,39 +999,41 @@ where
                 let mut found_star = false;
                 for (alias_index, alias) in node.names.iter().enumerate() {
                     if &alias.name == "*" {
-                        if !found_star {
-                            found_star = true;
-                            // Wildcard imports are invalid syntax everywhere except the top-level scope,
-                            // and thus do not bind any definitions anywhere else
-                            if self.current_scope() == self.scope_stack[0].file_scope_id {
-                                if let Ok(module_name) =
-                                    module_name_from_import_statement(self.db, self.file, node)
-                                {
-                                    if let Some(module) = resolve_module(self.db, &module_name) {
-                                        let exported_names = find_exports(self.db, module.file());
-                                        if !exported_names.is_empty() {
-                                            for export in find_exports(self.db, module.file()) {
-                                                let symbol_id = self.add_symbol(export.clone());
-                                                let node_ref =
-                                                    StarImportDefinitionNodeRef { node, symbol_id };
-                                                self.add_definition(symbol_id, node_ref);
-                                            }
-                                            continue;
-                                        }
-                                    }
-                                }
-                            }
+                        self.add_entry_for_definition_key(alias.into());
+
+                        if found_star {
+                            continue;
+                        }
+
+                        found_star = true;
+
+                        // Wildcard imports are invalid syntax everywhere except the top-level scope,
+                        // and thus do not bind any definitions anywhere else
+                        if self.current_scope() != self.scope_stack[0].file_scope_id {
+                            continue;
+                        }
+
+                        let Ok(module_name) =
+                            module_name_from_import_statement(self.db, self.file, node)
+                        else {
+                            continue;
+                        };
+
+                        let Some(module) = resolve_module(self.db, &module_name) else {
+                            continue;
+                        };
+
+                        let exported_names = find_exports(self.db, module.file());
+                        if exported_names.is_empty() {
+                            continue;
+                        }
+
+                        for export in find_exports(self.db, module.file()) {
+                            let symbol_id = self.add_symbol(export.clone());
+                            let node_ref = StarImportDefinitionNodeRef { node, symbol_id };
+                            self.add_definition(symbol_id, node_ref);
                         }
 
-                        let symbol = self.add_symbol(Name::new_static("*"));
-                        self.add_definition(
-                            symbol,
-                            ImportFromDefinitionNodeRef {
-                                node,
-                                alias_index,
-                                is_reexported: false,
-                            },
-                        );
                         continue;
                     }
 

crates/red_knot_python_semantic/src/semantic_index/re_exports.rs

@@ -237,16 +237,20 @@ impl<'db> Visitor<'db> for ExportFinder<'db> {
                 }
             }
             ast::Stmt::ImportFrom(node) => {
+                let mut found_star = false;
                 for name in &node.names {
                     if &name.name.id == "*" {
-                        self.exports.extend(
-                            module_name_from_import_statement(self.db, self.file, node)
-                                .ok()
-                                .and_then(|module_name| resolve_module(self.db, &module_name))
-                                .iter()
-                                .flat_map(|module| find_exports(self.db, module.file()))
-                                .cloned(),
-                        );
+                        if !found_star {
+                            found_star = true;
+                            self.exports.extend(
+                                module_name_from_import_statement(self.db, self.file, node)
+                                    .ok()
+                                    .and_then(|module_name| resolve_module(self.db, &module_name))
+                                    .iter()
+                                    .flat_map(|module| find_exports(self.db, module.file()))
+                                    .cloned(),
+                            );
+                        }
                     } else {
                         self.visit_alias(name);
                     }

crates/red_knot_python_semantic/src/types/infer.rs

@@ -1295,9 +1295,9 @@ impl<'db> TypeInferenceBuilder<'db> {
     }
 
     fn infer_definition(&mut self, node: impl Into<DefinitionNodeKey>) {
-        for definition in self.index.definitions(node) {
-            self.extend(infer_definition_types(self.db(), *definition));
-        }
+        let definitions = self.index.definitions(node);
+        debug_assert_eq!(definitions.len(), 1);
+        self.extend(infer_definition_types(self.db(), definitions[0]));
     }
 
     fn infer_function_definition_statement(&mut self, function: &ast::StmtFunctionDef) {
@@ -2985,7 +2985,16 @@ impl<'db> TypeInferenceBuilder<'db> {
         } = import;
 
         for alias in names {
-            self.infer_definition(alias);
+            let definitions = self.index.definitions(alias);
+            if definitions.is_empty() {
+                // Ensure `from foo import *` is flagged as invalid if `foo` can't be resolved,
+                // *even if* `foo` is an empty module (and thus has no `Definition`s associated with it)
+                self.resolve_import_from_module(import, alias);
+            } else {
+                for definition in definitions {
+                    self.extend(infer_definition_types(self.db(), *definition));
+                }
+            }
         }
     }
 
@@ -3037,12 +3046,13 @@ impl<'db> TypeInferenceBuilder<'db> {
         }
     }
 
-    fn infer_import_from_definition(
+    /// Resolve the [`ModuleName`], and the type of the module, being referred to by an
+    /// [`ast::StmtImportFrom`] node. Emit a diagnostic if the module cannot be resolved.
+    fn resolve_import_from_module(
         &mut self,
-        import_from: &'db ast::StmtImportFrom,
+        import_from: &ast::StmtImportFrom,
         alias: &ast::Alias,
-        definition: Definition<'db>,
-    ) {
+    ) -> Option<(ModuleName, Type<'db>)> {
         let ast::StmtImportFrom { module, level, .. } = import_from;
         // For diagnostics, we want to highlight the unresolvable
         // module and not the entire `from ... import ...` statement.
@@ -3065,17 +3075,15 @@ impl<'db> TypeInferenceBuilder<'db> {
             Err(ModuleNameResolutionError::InvalidSyntax) => {
                 tracing::debug!("Failed to resolve import due to invalid syntax");
                 // Invalid syntax diagnostics are emitted elsewhere.
-                self.add_unknown_declaration_with_binding(alias.into(), definition);
-                return;
+                return None;
             }
             Err(ModuleNameResolutionError::TooManyDots) => {
                 tracing::debug!(
                     "Relative module resolution `{}` failed: too many leading dots",
                     format_import_from_module(*level, module),
                 );
                 report_unresolved_module(&self.context, module_ref, *level, module);
-                self.add_unknown_declaration_with_binding(alias.into(), definition);
-                return;
+                return None;
             }
             Err(ModuleNameResolutionError::UnknownCurrentModule) => {
                 tracing::debug!(
@@ -3084,13 +3092,26 @@ impl<'db> TypeInferenceBuilder<'db> {
                     self.file().path(self.db())
                 );
                 report_unresolved_module(&self.context, module_ref, *level, module);
-                self.add_unknown_declaration_with_binding(alias.into(), definition);
-                return;
+                return None;
             }
         };
 
         let Some(module_ty) = self.module_type_from_name(&module_name) else {
             report_unresolved_module(&self.context, module_ref, *level, module);
+            return None;
+        };
+
+        Some((module_name, module_ty))
+    }
+
+    fn infer_import_from_definition(
+        &mut self,
+        import_from: &'db ast::StmtImportFrom,
+        alias: &ast::Alias,
+        definition: Definition<'db>,
+    ) {
+        let Some((module_name, module_ty)) = self.resolve_import_from_module(import_from, alias)
+        else {
             self.add_unknown_declaration_with_binding(alias.into(), definition);
             return;
         };
@@ -3102,23 +3123,12 @@ impl<'db> TypeInferenceBuilder<'db> {
                 None
             };
 
-        let is_meaningful_star_import = star_import_info.is_some();
-
         let name = if let Some((star_import, symbol_table)) = star_import_info.as_ref() {
             symbol_table.symbol(star_import.symbol_id()).name()
         } else {
             &alias.name.id
         };
 
-        if &alias.name == "*" && !is_meaningful_star_import {
-            self.add_declaration_with_binding(
-                alias.into(),
-                definition,
-                &DeclaredAndInferredType::AreTheSame(Type::Never),
-            );
-            return;
-        }
-
         // First try loading the requested attribute from the module.
         if let Symbol::Type(ty, boundness) = module_ty.member(self.db(), name).symbol {
             if &alias.name != "*" && boundness == Boundness::PossiblyUnbound {