mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-03 18:28:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[red-knot] don't include Unknown in the type for a conditionally-defined import (#13563)

Browse source 
## Summary

Fixes the bug described in #13514 where an unbound public type defaulted
to the type or `Unknown`, whereas it should only be the type if unbound.

## Test Plan

Added a new test case

---------

Co-authored-by: Carl Meyer <carl@astral.sh>
This commit is contained in:
aditya pillai 2024-10-16 16:46:03 -04:00 • committed by GitHub
parent 2095ea8372
commit ed4a0b34ba
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 126 additions and 91 deletions
Download patch file Download diff file Expand all files Collapse all files

16
crates/red_knot_python_semantic/resources/mdtest/assignment/unbound.md
View file

@ -1,24 +1,16 @@
# Unbound
## Maybe unbound
```py
if flag:
y = 3
x = y
reveal_type(x) # revealed: Unbound | Literal[3]
```
## Unbound
```py
x = foo; foo = 1
x = foo
foo = 1
reveal_type(x) # revealed: Unbound
```
## Unbound class variable
Class variables can reference global variables unless overridden within the class scope.
Name lookups within a class scope fall back to globals, but lookups of class attributes don't.
```py
x = 1
@ -27,6 +19,6 @@ class C:
if flag:
x = 2
reveal_type(C.x) # revealed: Unbound | Literal[2]
reveal_type(C.x) # revealed: Literal[2]
reveal_type(C.y) # revealed: Literal[1]
```

1
crates/red_knot_python_semantic/resources/mdtest/conditional/if_statement.md
View file

@ -28,6 +28,7 @@ else:
y = 5
s = y
x = y
reveal_type(x) # revealed: Literal[3, 4, 5]
reveal_type(r) # revealed: Unbound | Literal[2]
reveal_type(s) # revealed: Unbound | Literal[5]

8
crates/red_knot_python_semantic/resources/mdtest/exception/except_star.md
View file

@ -1,14 +1,12 @@
# Except star
TODO(Alex): Once we support `sys.version_info` branches, we can set `--target-version=py311` in these tests and the inferred type will just be `BaseExceptionGroup`
## Except\* with BaseException
```py
try:
x
except* BaseException as e:
reveal_type(e) # revealed: Unknown | BaseExceptionGroup
reveal_type(e) # revealed: BaseExceptionGroup
```
## Except\* with specific exception
@ -18,7 +16,7 @@ try:
x
except* OSError as e:
# TODO(Alex): more precise would be `ExceptionGroup[OSError]`
reveal_type(e) # revealed: Unknown | BaseExceptionGroup
reveal_type(e) # revealed: BaseExceptionGroup
```
## Except\* with multiple exceptions
@ -28,5 +26,5 @@ try:
x
except* (TypeError, AttributeError) as e:
#TODO(Alex): more precise would be `ExceptionGroup[TypeError | AttributeError]`.
reveal_type(e) # revealed: Unknown | BaseExceptionGroup
reveal_type(e) # revealed: BaseExceptionGroup
```

37
crates/red_knot_python_semantic/resources/mdtest/import/conditional.md
View file

@ -1,5 +1,39 @@
# Conditional imports
## Maybe unbound
```py path=maybe_unbound.py
if flag:
y = 3
x = y
reveal_type(x) # revealed: Unbound | Literal[3]
reveal_type(y) # revealed: Unbound | Literal[3]
```
```py
from maybe_unbound import x, y
reveal_type(x) # revealed: Literal[3]
reveal_type(y) # revealed: Literal[3]
```
## Maybe unbound annotated
```py path=maybe_unbound_annotated.py
if flag:
y: int = 3
x = y
reveal_type(x) # revealed: Unbound | Literal[3]
reveal_type(y) # revealed: Unbound | Literal[3]
```
Importing an annotated name prefers the declared type over the inferred type:
```py
from maybe_unbound_annotated import x, y
reveal_type(x) # revealed: Literal[3]
reveal_type(y) # revealed: int
```
## Reimport
```py path=c.py
@ -14,8 +48,7 @@ else:
```
```py
# TODO we should not emit this error
from b import f # error: [invalid-assignment] "Object of type `Literal[f, f]` is not assignable to `Literal[f, f]`"
from b import f
# TODO: We should disambiguate in such cases, showing `Literal[b.f, c.f]`.
reveal_type(f) # revealed: Literal[f, f]
```

9
crates/red_knot_python_semantic/resources/mdtest/import/errors.md
View file

@ -4,12 +4,14 @@
```py
import bar # error: "Cannot resolve import `bar`"
reveal_type(bar) # revealed: Unknown
```
## Unresolved import from statement
```py
from bar import baz # error: "Cannot resolve import `bar`"
reveal_type(baz) # revealed: Unknown
```
## Unresolved import from resolved module
@ -19,12 +21,14 @@ from bar import baz # error: "Cannot resolve import `bar`"
```py
from a import thing # error: "Module `a` has no member `thing`"
reveal_type(thing) # revealed: Unknown
```
## Resolved import of symbol from unresolved import
```py path=a.py
import foo as foo # error: "Cannot resolve import `foo`"
reveal_type(foo) # revealed: Unknown
```
Importing the unresolved import into a second file should not trigger an additional "unresolved
@ -32,9 +36,10 @@ import" violation:
```py
from a import foo
reveal_type(foo) # revealed: Unknown
```
## No implicit shadowing error
## No implicit shadowing
```py path=b.py
x: int
@ -43,5 +48,5 @@ x: int
```py
from b import x
x = 'foo' # error: "Object of type `Literal["foo"]"
x = 'foo' # error: [invalid-assignment] "Object of type `Literal["foo"]"
```

2
crates/red_knot_python_semantic/resources/mdtest/import/relative.md
View file

@ -126,7 +126,7 @@ reveal_type(y) # revealed: Unknown
```
```py path=package/bar.py
# TODO: submodule imports possibly not supported right now?
# TODO: support submodule imports
from . import foo # error: [unresolved-import]
reveal_type(foo) # revealed: Unknown

32
crates/red_knot_python_semantic/resources/mdtest/scopes/builtin.md Normal file
View file

@ -0,0 +1,32 @@
# Builtin scope
## Conditionally global or builtin
If a builtin name is conditionally defined as a global, a name lookup should union the builtin type
with the conditionally-defined type:
```py
def returns_bool() -> bool:
return True
if returns_bool():
copyright = 1
def f():
reveal_type(copyright) # revealed: Literal[copyright] | Literal[1]
```
## Conditionally global or builtin, with annotation
Same is true if the name is annotated:
```py
def returns_bool() -> bool:
return True
if returns_bool():
copyright: int = 1
def f():
reveal_type(copyright) # revealed: Literal[copyright] | int
```

7
crates/red_knot_python_semantic/src/stdlib.rs
View file

@ -37,6 +37,13 @@ fn core_module_symbol_ty<'db>(
) -> Type<'db> {
resolve_module(db, &core_module.name())
.map(|module| global_symbol_ty(db, module.file(), symbol))
.map(|ty| {
if ty.is_unbound() {
ty
} else {
ty.replace_unbound_with(db, Type::Never)
}
})
.unwrap_or(Type::Unbound)
}

13
crates/red_knot_python_semantic/src/types.rs
View file

@ -60,7 +60,7 @@ fn symbol_ty_by_id<'db>(db: &'db dyn Db, scope: ScopeId<'db>, symbol: ScopedSymb
use_def.public_bindings(symbol),
use_def
.public_may_be_unbound(symbol)
.then_some(Type::Unknown),
.then_some(Type::Unbound),
))
} else {
None
@ -79,7 +79,7 @@ fn symbol_ty_by_id<'db>(db: &'db dyn Db, scope: ScopeId<'db>, symbol: ScopedSymb
}
}
/// Shorthand for `symbol_ty` that takes a symbol name instead of an ID.
/// Shorthand for `symbol_ty_by_id` that takes a symbol name instead of an ID.
fn symbol_ty<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Type<'db> {
let table = symbol_table(db, scope);
table
@ -381,7 +381,7 @@ impl<'db> Type<'db> {
Type::Union(union) => {
union.map(db, |element| element.replace_unbound_with(db, replacement))
}
ty => *ty,
_ => *self,
}
}
@ -444,6 +444,9 @@ impl<'db> Type<'db> {
///
/// [assignable to]: https://typing.readthedocs.io/en/latest/spec/concepts.html#the-assignable-to-or-consistent-subtyping-relation
pub(crate) fn is_assignable_to(self, db: &'db dyn Db, target: Type<'db>) -> bool {
if self.is_equivalent_to(db, target) {
return true;
}
match (self, target) {
(Type::Unknown | Type::Any | Type::Todo, _) => true,
(_, Type::Unknown | Type::Any | Type::Todo) => true,
@ -1426,7 +1429,8 @@ impl<'db> ClassType<'db> {
pub fn class_member(self, db: &'db dyn Db, name: &str) -> Type<'db> {
let member = self.own_class_member(db, name);
if !member.is_unbound() {
return member;
// TODO diagnostic if maybe unbound?
return member.replace_unbound_with(db, Type::Never);
}
self.inherited_class_member(db, name)
@ -1625,6 +1629,7 @@ mod tests {
#[test_case(Ty::BytesLiteral("foo"), Ty::BuiltinInstance("bytes"))]
#[test_case(Ty::IntLiteral(1), Ty::Union(vec![Ty::BuiltinInstance("int"), Ty::BuiltinInstance("str")]))]
#[test_case(Ty::IntLiteral(1), Ty::Union(vec![Ty::Unknown, Ty::BuiltinInstance("str")]))]
#[test_case(Ty::Union(vec![Ty::IntLiteral(1), Ty::IntLiteral(2)]), Ty::Union(vec![Ty::IntLiteral(1), Ty::IntLiteral(2)]))]
fn is_assignable_to(from: Ty, to: Ty) {
let db = setup_db();
assert!(from.into_type(&db).is_assignable_to(&db, to.into_type(&db)));

92
crates/red_knot_python_semantic/src/types/infer.rs
View file

@ -1699,10 +1699,30 @@ impl<'db> TypeInferenceBuilder<'db> {
.ok_or(ModuleNameResolutionError::InvalidSyntax)
};
let module_ty = match module_name {
Ok(name) => {
if let Some(ty) = self.module_ty_from_name(&name) {
ty
let ty = match module_name {
Ok(module_name) => {
if let Some(module_ty) = self.module_ty_from_name(&module_name) {
let ast::Alias {
range: _,
name,
asname: _,
} = alias;
let member_ty = module_ty.member(self.db, &ast::name::Name::new(&name.id));
if member_ty.is_unbound() {
self.add_diagnostic(
AnyNodeRef::Alias(alias),
"unresolved-import",
format_args!("Module `{module_name}` has no member `{name}`",),
);
Type::Unknown
} else {
// For possibly-unbound names, just eliminate Unbound from the type; we
// must be in a bound path. TODO diagnostic for maybe-unbound import?
member_ty.replace_unbound_with(self.db, Type::Never)
}
} else {
self.unresolved_module_diagnostic(import_from, *level, module);
Type::Unknown
@ -1732,34 +1752,6 @@ impl<'db> TypeInferenceBuilder<'db> {
}
};
let ast::Alias {
range: _,
name,
asname: _,
} = alias;
let member_ty = module_ty.member(self.db, &ast::name::Name::new(&name.id));
// TODO: What if it's a union where one of the elements is `Unbound`?
if member_ty.is_unbound() {
self.add_diagnostic(
AnyNodeRef::Alias(alias),
"unresolved-import",
format_args!(
"Module `{}{}` has no member `{name}`",
".".repeat(*level as usize),
module.unwrap_or_default()
),
);
}
// If a symbol is unbound in the module the symbol was originally defined in,
// when we're trying to import the symbol from that module into "our" module,
// the runtime error will occur immediately (rather than when the symbol is *used*,
// as would be the case for a symbol with type `Unbound`), so it's appropriate to
// think of the type of the imported symbol as `Unknown` rather than `Unbound`
let ty = member_ty.replace_unbound_with(self.db, Type::Unknown);
self.add_declaration_with_binding(alias.into(), definition, ty, ty);
}
@ -2368,6 +2360,7 @@ impl<'db> TypeInferenceBuilder<'db> {
return symbol_ty(self.db, enclosing_scope_id, name);
}
}
// No nonlocal binding, check module globals. Avoid infinite recursion if `self.scope`
// already is module globals.
let ty = if file_scope_id.is_global() {
@ -2375,6 +2368,7 @@ impl<'db> TypeInferenceBuilder<'db> {
} else {
global_symbol_ty(self.db, self.file, name)
};
// Fallback to builtins (without infinite recursion if we're already in builtins.)
if ty.may_be_unbound(self.db) && Some(self.scope()) != builtins_module_scope(self.db) {
let mut builtin_ty = builtins_symbol_ty(self.db, name);
@ -3877,7 +3871,7 @@ mod tests {
)?;
// TODO: sys.version_info, and need to understand @final and @type_check_only
assert_public_ty(&db, "src/a.py", "x", "Unknown | EllipsisType");
assert_public_ty(&db, "src/a.py", "x", "EllipsisType | Unknown");
Ok(())
}
@ -3994,38 +3988,6 @@ mod tests {
Ok(())
}
#[test]
fn conditionally_global_or_builtin() -> anyhow::Result<()> {
let mut db = setup_db();
db.write_dedented(
"/src/a.py",
"
if flag:
copyright = 1
def f():
y = copyright
",
)?;
let file = system_path_to_file(&db, "src/a.py").expect("file to exist");
let index = semantic_index(&db, file);
let function_scope = index
.child_scopes(FileScopeId::global())
.next()
.unwrap()
.0
.to_scope_id(&db, file);
let y_ty = symbol_ty(&db, function_scope, "y");
assert_eq!(
y_ty.display(&db).to_string(),
"Literal[copyright] | Literal[1]"
);
Ok(())
}
#[test]
fn local_inference() -> anyhow::Result<()> {
let mut db = setup_db();