Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] Avoid panicking when hitting failures looking up AST information #13701

Closed
wants to merge 5 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Allow for definition lookups to be falliable
 In most cases we can "give up" on a type inference with "unknown"
  • Loading branch information
rtpg committed Oct 10, 2024

Verified

This commit was signed with the committer’s verified signature.
Farber98 Juan Farber
commit eb8eae2c7e4de63ac87b09807b158c408200f907
6 changes: 4 additions & 2 deletions crates/red_knot_python_semantic/src/semantic_index.rs
Original file line number Diff line number Diff line change
@@ -198,8 +198,10 @@ impl<'db> SemanticIndex<'db> {
pub(crate) fn definition(
&self,
definition_key: impl Into<DefinitionNodeKey>,
) -> Definition<'db> {
self.definitions_by_node[&definition_key.into()]
) -> Option<Definition<'db>> {
self.definitions_by_node
.get(&definition_key.into())
.copied()
}

/// Returns the [`Expression`] ingredient for an expression node.
20 changes: 10 additions & 10 deletions crates/red_knot_python_semantic/src/semantic_index/ast_ids.rs
Original file line number Diff line number Diff line change
@@ -33,12 +33,12 @@ pub(crate) struct AstIds {
}

impl AstIds {
fn expression_id(&self, key: impl Into<ExpressionNodeKey>) -> ScopedExpressionId {
self.expressions_map[&key.into()]
fn expression_id(&self, key: impl Into<ExpressionNodeKey>) -> Option<ScopedExpressionId> {
self.expressions_map.get(&key.into()).copied()
}

fn use_id(&self, key: impl Into<ExpressionNodeKey>) -> ScopedUseId {
self.uses_map[&key.into()]
fn use_id(&self, key: impl Into<ExpressionNodeKey>) -> Option<ScopedUseId> {
self.uses_map.get(&key.into()).copied()
}
}

@@ -51,7 +51,7 @@ pub trait HasScopedUseId {
type Id: Copy;

/// Returns the ID that uniquely identifies the use in `scope`.
fn scoped_use_id(&self, db: &dyn Db, scope: ScopeId) -> Self::Id;
fn scoped_use_id(&self, db: &dyn Db, scope: ScopeId) -> Option<Self::Id>;
}

/// Uniquely identifies a use of a name in a [`crate::semantic_index::symbol::FileScopeId`].
@@ -61,7 +61,7 @@ pub struct ScopedUseId;
impl HasScopedUseId for ast::ExprName {
type Id = ScopedUseId;

fn scoped_use_id(&self, db: &dyn Db, scope: ScopeId) -> Self::Id {
fn scoped_use_id(&self, db: &dyn Db, scope: ScopeId) -> Option<Self::Id> {
let expression_ref = ExpressionRef::from(self);
expression_ref.scoped_use_id(db, scope)
}
@@ -70,7 +70,7 @@ impl HasScopedUseId for ast::ExprName {
impl HasScopedUseId for ast::ExpressionRef<'_> {
type Id = ScopedUseId;

fn scoped_use_id(&self, db: &dyn Db, scope: ScopeId) -> Self::Id {
fn scoped_use_id(&self, db: &dyn Db, scope: ScopeId) -> Option<Self::Id> {
let ast_ids = ast_ids(db, scope);
ast_ids.use_id(*self)
}
@@ -81,7 +81,7 @@ pub trait HasScopedAstId {
type Id: Copy;

/// Returns the ID that uniquely identifies the node in `scope`.
fn scoped_ast_id(&self, db: &dyn Db, scope: ScopeId) -> Self::Id;
fn scoped_ast_id(&self, db: &dyn Db, scope: ScopeId) -> Option<Self::Id>;
}

/// Uniquely identifies an [`ast::Expr`] in a [`crate::semantic_index::symbol::FileScopeId`].
@@ -93,7 +93,7 @@ macro_rules! impl_has_scoped_expression_id {
impl HasScopedAstId for $ty {
type Id = ScopedExpressionId;

fn scoped_ast_id(&self, db: &dyn Db, scope: ScopeId) -> Self::Id {
fn scoped_ast_id(&self, db: &dyn Db, scope: ScopeId) -> Option<Self::Id> {
let expression_ref = ExpressionRef::from(self);
expression_ref.scoped_ast_id(db, scope)
}
@@ -138,7 +138,7 @@ impl_has_scoped_expression_id!(ast::Expr);
impl HasScopedAstId for ast::ExpressionRef<'_> {
type Id = ScopedExpressionId;

fn scoped_ast_id(&self, db: &dyn Db, scope: ScopeId) -> Self::Id {
fn scoped_ast_id(&self, db: &dyn Db, scope: ScopeId) -> Option<Self::Id> {
let ast_ids = ast_ids(db, scope);
ast_ids.expression_id(*self)
}
15 changes: 11 additions & 4 deletions crates/red_knot_python_semantic/src/semantic_model.rs
Original file line number Diff line number Diff line change
@@ -58,8 +58,13 @@ impl HasTy for ast::ExpressionRef<'_> {
let file_scope = index.expression_scope_id(*self);
let scope = file_scope.to_scope_id(model.db, model.file);

let expression_id = self.scoped_ast_id(model.db, scope);
infer_scope_types(model.db, scope).expression_ty(expression_id)
if let Some(expression_id) = self.scoped_ast_id(model.db, scope) {
let lookup = infer_scope_types(model.db, scope).try_expression_ty(expression_id);
lookup.unwrap_or(Type::Unknown)
} else {
tracing::warn!("Couldn't find expression ID");
Type::Unknown
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it would make a lot of sense for there to be something like Type::Unknown(SourcingInformation), where each callsite could embed where that specific one came from.

That way, if reveal_type gives you an Any, you could probably trace back why you got that.

}
}
}

@@ -153,8 +158,10 @@ macro_rules! impl_binding_has_ty {
#[inline]
fn ty<'db>(&self, model: &SemanticModel<'db>) -> Type<'db> {
let index = semantic_index(model.db, model.file);
let binding = index.definition(self);
binding_ty(model.db, binding)
match index.definition(self) {
Some(binding) => binding_ty(model.db, binding),
None => Type::Unknown,
}
}
}
};
16 changes: 11 additions & 5 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
@@ -114,12 +114,18 @@ fn definition_expression_ty<'db>(
definition: Definition<'db>,
expression: &ast::Expr,
) -> Type<'db> {
let expr_id = expression.scoped_ast_id(db, definition.scope(db));
let inference = infer_definition_types(db, definition);
if let Some(ty) = inference.try_expression_ty(expr_id) {
ty
if let Some(expr_id) = expression.scoped_ast_id(db, definition.scope(db)) {
let inference = infer_definition_types(db, definition);
if let Some(ty) = inference.try_expression_ty(expr_id) {
ty
} else {
infer_deferred_types(db, definition)
.try_expression_ty(expr_id)
.unwrap_or(Type::Unknown)
}
} else {
infer_deferred_types(db, definition).expression_ty(expr_id)
tracing::warn!("Can't find expression ID");
Type::Unknown
}
}

23 changes: 16 additions & 7 deletions crates/red_knot_python_semantic/src/types/narrow.rs
Original file line number Diff line number Diff line change
@@ -154,13 +154,22 @@ impl<'db> NarrowingConstraintsBuilder<'db> {
let scope = self.scope();
let inference = infer_expression_types(self.db, expression);
for (op, comparator) in std::iter::zip(&**ops, &**comparators) {
let comp_ty = inference.expression_ty(comparator.scoped_ast_id(self.db, scope));
if matches!(op, ast::CmpOp::IsNot) {
let ty = IntersectionBuilder::new(self.db)
.add_negative(comp_ty)
.build();
self.constraints.insert(symbol, ty);
};
match comparator.scoped_ast_id(self.db, scope) {
Some(comparator_id) => {
let comp_ty = inference
.try_expression_ty(comparator_id)
.unwrap_or(Type::Unknown);
if matches!(op, ast::CmpOp::IsNot) {
let ty = IntersectionBuilder::new(self.db)
.add_negative(comp_ty)
.build();
self.constraints.insert(symbol, ty);
};
}
None => {
tracing::warn!("Can't find ID for comparator");
}
}
// TODO other comparison types
}
}