Skip to content

Commit

Permalink
[red-knot] fix lookups of possibly-shadowed builtins (#12898)
Browse files Browse the repository at this point in the history
If a builtin is conditionally shadowed by a global, we didn't correctly
fall back to builtins for the not-defined-in-globals path (see added
test for an example.)
  • Loading branch information
carljm committed Aug 15, 2024
1 parent 52d27be commit 80efb86
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 8 deletions.
29 changes: 27 additions & 2 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,33 @@ impl<'db> Type<'db> {
matches!(self, Type::Unbound)
}

pub const fn is_unknown(&self) -> bool {
matches!(self, Type::Unknown)
pub const fn is_never(&self) -> bool {
matches!(self, Type::Never)
}

pub fn may_be_unbound(&self, db: &'db dyn Db) -> bool {
match self {
Type::Unbound => true,
Type::Union(union) => union.contains(db, Type::Unbound),
// Unbound can't appear in an intersection, because an intersection with Unbound
// simplifies to just Unbound.
_ => false,
}
}

#[must_use]
pub fn replace_unbound_with(&self, db: &'db dyn Db, replacement: Type<'db>) -> Type<'db> {
match self {
Type::Unbound => replacement,
Type::Union(union) => union
.elements(db)
.into_iter()
.fold(UnionBuilder::new(db), |builder, ty| {
builder.add(ty.replace_unbound_with(db, replacement))
})
.build(),
ty => *ty,
}
}

#[must_use]
Expand Down
31 changes: 29 additions & 2 deletions crates/red_knot_python_semantic/src/types/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ impl<'db> InnerIntersectionBuilder<'db> {
self.negative.retain(|elem| !pos.contains(elem));
}
Type::Never => {}
Type::Unbound => {}
_ => {
if !self.positive.remove(&ty) {
self.negative.insert(ty);
Expand All @@ -214,9 +215,13 @@ impl<'db> InnerIntersectionBuilder<'db> {

// Never is a subtype of all types
if self.positive.contains(&Type::Never) {
self.positive.clear();
self.positive.retain(Type::is_never);
self.negative.clear();
}

if self.positive.contains(&Type::Unbound) {
self.positive.retain(Type::is_unbound);
self.negative.clear();
self.positive.insert(Type::Never);
}
}

Expand Down Expand Up @@ -426,4 +431,26 @@ mod tests {

assert_eq!(ty, Type::Never);
}

#[test]
fn build_intersection_simplify_positive_unbound() {
let db = setup_db();
let ty = IntersectionBuilder::new(&db)
.add_positive(Type::Unbound)
.add_positive(Type::IntLiteral(1))
.build();

assert_eq!(ty, Type::Unbound);
}

#[test]
fn build_intersection_simplify_negative_unbound() {
let db = setup_db();
let ty = IntersectionBuilder::new(&db)
.add_negative(Type::Unbound)
.add_positive(Type::IntLiteral(1))
.build();

assert_eq!(ty, Type::IntLiteral(1));
}
}
44 changes: 40 additions & 4 deletions crates/red_knot_python_semantic/src/types/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1354,18 +1354,22 @@ impl<'db> TypeInferenceBuilder<'db> {
let symbol = symbols.symbol_by_name(id).unwrap();
if !symbol.is_defined() || !self.scope.is_function_like(self.db) {
// implicit global
let mut unbound_ty = if file_scope_id == FileScopeId::global() {
let unbound_ty = if file_scope_id == FileScopeId::global() {
Type::Unbound
} else {
global_symbol_ty_by_name(self.db, self.file, id)
};
// fallback to builtins
if matches!(unbound_ty, Type::Unbound)
if unbound_ty.may_be_unbound(self.db)
&& Some(self.scope) != builtins_scope(self.db)
{
unbound_ty = builtins_symbol_ty_by_name(self.db, id);
Some(unbound_ty.replace_unbound_with(
self.db,
builtins_symbol_ty_by_name(self.db, id),
))
} else {
Some(unbound_ty)
}
Some(unbound_ty)
} else {
Some(Type::Unbound)
}
Expand Down Expand Up @@ -2163,6 +2167,38 @@ 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("Expected 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_by_name(&db, function_scope, "y");

assert_eq!(
y_ty.display(&db).to_string(),
"Literal[1] | Literal[copyright]"
);

Ok(())
}

/// Class name lookups do fall back to globals, but the public type never does.
#[test]
fn unbound_class_local() -> anyhow::Result<()> {
Expand Down

0 comments on commit 80efb86

Please sign in to comment.