Skip to content

Commit 9479f00

Browse files
AlexWaygoodAlexWaygood
committed
avoid creating * definitions for invalid syntax
1 parent 0a9b747 commit 9479f00

File tree

3 files changed

+97
-68
lines changed

3 files changed

+97
-68
lines changed

‎crates/red_knot_python_semantic/src/semantic_index/builder.rs

Lines changed: 38 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,10 @@ impl<'db> SemanticIndexBuilder<'db> {
346346
self.current_symbol_table().mark_symbol_used(id);
347347
}
348348

349+
fn add_entry_for_definition_key(&mut self, key: DefinitionNodeKey) -> &mut Definitions<'db> {
350+
self.definitions_by_node.entry(key).or_default()
351+
}
352+
349353
fn add_definition(
350354
&mut self,
351355
symbol: ScopedSymbolId,
@@ -368,9 +372,7 @@ impl<'db> SemanticIndexBuilder<'db> {
368372
countme::Count::default(),
369373
);
370374

371-
self.definitions_by_node
372-
.entry(definition_node.key())
373-
.or_default()
375+
self.add_entry_for_definition_key(definition_node.key())
374376
.push(definition);
375377

376378
if category.is_binding() {
@@ -997,39 +999,41 @@ where
997999
let mut found_star = false;
9981000
for (alias_index, alias) in node.names.iter().enumerate() {
9991001
if &alias.name == "*" {
1000-
if !found_star {
1001-
found_star = true;
1002-
// Wildcard imports are invalid syntax everywhere except the top-level scope,
1003-
// and thus do not bind any definitions anywhere else
1004-
if self.current_scope() == self.scope_stack[0].file_scope_id {
1005-
if let Ok(module_name) =
1006-
module_name_from_import_statement(self.db, self.file, node)
1007-
{
1008-
if let Some(module) = resolve_module(self.db, &module_name) {
1009-
let exported_names = find_exports(self.db, module.file());
1010-
if !exported_names.is_empty() {
1011-
for export in find_exports(self.db, module.file()) {
1012-
let symbol_id = self.add_symbol(export.clone());
1013-
let node_ref =
1014-
StarImportDefinitionNodeRef { node, symbol_id };
1015-
self.add_definition(symbol_id, node_ref);
1016-
}
1017-
continue;
1018-
}
1019-
}
1020-
}
1021-
}
1002+
self.add_entry_for_definition_key(alias.into());
1003+
1004+
if found_star {
1005+
continue;
1006+
}
1007+
1008+
found_star = true;
1009+
1010+
// Wildcard imports are invalid syntax everywhere except the top-level scope,
1011+
// and thus do not bind any definitions anywhere else
1012+
if self.current_scope() != self.scope_stack[0].file_scope_id {
1013+
continue;
1014+
}
1015+
1016+
let Ok(module_name) =
1017+
module_name_from_import_statement(self.db, self.file, node)
1018+
else {
1019+
continue;
1020+
};
1021+
1022+
let Some(module) = resolve_module(self.db, &module_name) else {
1023+
continue;
1024+
};
1025+
1026+
let exported_names = find_exports(self.db, module.file());
1027+
if exported_names.is_empty() {
1028+
continue;
1029+
}
1030+
1031+
for export in find_exports(self.db, module.file()) {
1032+
let symbol_id = self.add_symbol(export.clone());
1033+
let node_ref = StarImportDefinitionNodeRef { node, symbol_id };
1034+
self.add_definition(symbol_id, node_ref);
10221035
}
10231036

1024-
let symbol = self.add_symbol(Name::new_static("*"));
1025-
self.add_definition(
1026-
symbol,
1027-
ImportFromDefinitionNodeRef {
1028-
node,
1029-
alias_index,
1030-
is_reexported: false,
1031-
},
1032-
);
10331037
continue;
10341038
}
10351039

‎crates/red_knot_python_semantic/src/semantic_index/re_exports.rs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -237,16 +237,20 @@ impl<'db> Visitor<'db> for ExportFinder<'db> {
237237
}
238238
}
239239
ast::Stmt::ImportFrom(node) => {
240+
let mut found_star = false;
240241
for name in &node.names {
241242
if &name.name.id == "*" {
242-
self.exports.extend(
243-
module_name_from_import_statement(self.db, self.file, node)
244-
.ok()
245-
.and_then(|module_name| resolve_module(self.db, &module_name))
246-
.iter()
247-
.flat_map(|module| find_exports(self.db, module.file()))
248-
.cloned(),
249-
);
243+
if !found_star {
244+
found_star = true;
245+
self.exports.extend(
246+
module_name_from_import_statement(self.db, self.file, node)
247+
.ok()
248+
.and_then(|module_name| resolve_module(self.db, &module_name))
249+
.iter()
250+
.flat_map(|module| find_exports(self.db, module.file()))
251+
.cloned(),
252+
);
253+
}
250254
} else {
251255
self.visit_alias(name);
252256
}

‎crates/red_knot_python_semantic/src/types/infer.rs

Lines changed: 47 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1295,9 +1295,9 @@ impl<'db> TypeInferenceBuilder<'db> {
12951295
}
12961296

12971297
fn infer_definition(&mut self, node: impl Into<DefinitionNodeKey>) {
1298-
for definition in self.index.definitions(node) {
1299-
self.extend(infer_definition_types(self.db(), *definition));
1300-
}
1298+
let definitions = self.index.definitions(node);
1299+
debug_assert_eq!(definitions.len(), 1);
1300+
self.extend(infer_definition_types(self.db(), definitions[0]));
13011301
}
13021302

13031303
fn infer_function_definition_statement(&mut self, function: &ast::StmtFunctionDef) {
@@ -2985,7 +2985,16 @@ impl<'db> TypeInferenceBuilder<'db> {
29852985
} = import;
29862986

29872987
for alias in names {
2988-
self.infer_definition(alias);
2988+
let definitions = self.index.definitions(alias);
2989+
if definitions.is_empty() {
2990+
// Ensure `from foo import *` is flagged as invalid if `foo` can't be resolved,
2991+
// *even if* `foo` is an empty module (and thus has no `Definition`s associated with it)
2992+
self.resolve_import_from_module(import, alias, None);
2993+
} else {
2994+
for definition in definitions {
2995+
self.extend(infer_definition_types(self.db(), *definition));
2996+
}
2997+
}
29892998
}
29902999
}
29913000

@@ -3037,12 +3046,12 @@ impl<'db> TypeInferenceBuilder<'db> {
30373046
}
30383047
}
30393048

3040-
fn infer_import_from_definition(
3049+
fn resolve_import_from_module(
30413050
&mut self,
3042-
import_from: &'db ast::StmtImportFrom,
3051+
import_from: &ast::StmtImportFrom,
30433052
alias: &ast::Alias,
3044-
definition: Definition<'db>,
3045-
) {
3053+
definition: Option<Definition<'db>>,
3054+
) -> Option<(ModuleName, Type<'db>)> {
30463055
let ast::StmtImportFrom { module, level, .. } = import_from;
30473056
// For diagnostics, we want to highlight the unresolvable
30483057
// module and not the entire `from ... import ...` statement.
@@ -3065,17 +3074,21 @@ impl<'db> TypeInferenceBuilder<'db> {
30653074
Err(ModuleNameResolutionError::InvalidSyntax) => {
30663075
tracing::debug!("Failed to resolve import due to invalid syntax");
30673076
// Invalid syntax diagnostics are emitted elsewhere.
3068-
self.add_unknown_declaration_with_binding(alias.into(), definition);
3069-
return;
3077+
if let Some(definition) = definition {
3078+
self.add_unknown_declaration_with_binding(alias.into(), definition);
3079+
}
3080+
return None;
30703081
}
30713082
Err(ModuleNameResolutionError::TooManyDots) => {
30723083
tracing::debug!(
30733084
"Relative module resolution `{}` failed: too many leading dots",
30743085
format_import_from_module(*level, module),
30753086
);
30763087
report_unresolved_module(&self.context, module_ref, *level, module);
3077-
self.add_unknown_declaration_with_binding(alias.into(), definition);
3078-
return;
3088+
if let Some(definition) = definition {
3089+
self.add_unknown_declaration_with_binding(alias.into(), definition);
3090+
}
3091+
return None;
30793092
}
30803093
Err(ModuleNameResolutionError::UnknownCurrentModule) => {
30813094
tracing::debug!(
@@ -3084,14 +3097,33 @@ impl<'db> TypeInferenceBuilder<'db> {
30843097
self.file().path(self.db())
30853098
);
30863099
report_unresolved_module(&self.context, module_ref, *level, module);
3087-
self.add_unknown_declaration_with_binding(alias.into(), definition);
3088-
return;
3100+
if let Some(definition) = definition {
3101+
self.add_unknown_declaration_with_binding(alias.into(), definition);
3102+
}
3103+
return None;
30893104
}
30903105
};
30913106

30923107
let Some(module_ty) = self.module_type_from_name(&module_name) else {
30933108
report_unresolved_module(&self.context, module_ref, *level, module);
3094-
self.add_unknown_declaration_with_binding(alias.into(), definition);
3109+
if let Some(definition) = definition {
3110+
self.add_unknown_declaration_with_binding(alias.into(), definition);
3111+
}
3112+
return None;
3113+
};
3114+
3115+
Some((module_name, module_ty))
3116+
}
3117+
3118+
fn infer_import_from_definition(
3119+
&mut self,
3120+
import_from: &'db ast::StmtImportFrom,
3121+
alias: &ast::Alias,
3122+
definition: Definition<'db>,
3123+
) {
3124+
let Some((module_name, module_ty)) =
3125+
self.resolve_import_from_module(import_from, alias, Some(definition))
3126+
else {
30953127
return;
30963128
};
30973129

@@ -3102,23 +3134,12 @@ impl<'db> TypeInferenceBuilder<'db> {
31023134
None
31033135
};
31043136

3105-
let is_meaningful_star_import = star_import_info.is_some();
3106-
31073137
let name = if let Some((star_import, symbol_table)) = star_import_info.as_ref() {
31083138
symbol_table.symbol(star_import.symbol_id()).name()
31093139
} else {
31103140
&alias.name.id
31113141
};
31123142

3113-
if &alias.name == "*" && !is_meaningful_star_import {
3114-
self.add_declaration_with_binding(
3115-
alias.into(),
3116-
definition,
3117-
&DeclaredAndInferredType::AreTheSame(Type::Never),
3118-
);
3119-
return;
3120-
}
3121-
31223143
// First try loading the requested attribute from the module.
31233144
if let Symbol::Type(ty, boundness) = module_ty.member(self.db(), name).symbol {
31243145
if &alias.name != "*" && boundness == Boundness::PossiblyUnbound {

0 commit comments

Comments
 (0)