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.

Sign up for GitHub

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

Jump to bottom

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
933c4c0
Allow for existing definitions to be evicted
rtpg Oct 10, 2024
eb8eae2
Allow for definition lookups to be falliable
rtpg Oct 10, 2024
e3af8dc
Handle two cases of syntax errors causing crashes
rtpg Oct 10, 2024
8ca348d
Gracefully recover from a cycle in defered type inference
rtpg Oct 10, 2024
c0292e0
Fix up tests
rtpg Oct 10, 2024
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
13 changes: 8 additions & 5 deletions crates/red_knot_python_semantic/src/semantic_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -734,8 +736,9 @@ def f(a: str, /, b: str, c: int = 1, *args, d: int = 2, **kwargs):
.elt
.as_name_expr()
.unwrap();
let element_use_id =
element.scoped_use_id(&db, comprehension_scope_id.to_scope_id(&db, file));
let element_use_id = element
.scoped_use_id(&db, comprehension_scope_id.to_scope_id(&db, file))
.unwrap();

let binding = use_def.first_binding_at_use(element_use_id).unwrap();
let DefinitionKind::Comprehension(comprehension) = binding.kind(&db) else {
Expand Down Expand Up @@ -985,7 +988,7 @@ class C[T]:
let ast::Expr::Name(x_use_expr_name) = x_use_expr.as_ref() else {
panic!("expected a Name");
};
let x_use_id = x_use_expr_name.scoped_use_id(&db, scope);
let x_use_id = x_use_expr_name.scoped_use_id(&db, scope).unwrap();
let use_def = use_def_map(&db, scope);
let binding = use_def.first_binding_at_use(x_use_id).unwrap();
let DefinitionKind::Assignment(assignment) = binding.kind(&db) else {
Expand Down
20 changes: 10 additions & 10 deletions crates/red_knot_python_semantic/src/semantic_index/ast_ids.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand All @@ -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`].
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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`].
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
42 changes: 29 additions & 13 deletions crates/red_knot_python_semantic/src/semantic_index/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use ruff_python_ast as ast;
use ruff_python_ast::name::Name;
use ruff_python_ast::visitor::{walk_expr, walk_pattern, walk_stmt, Visitor};
use ruff_python_ast::AnyParameterRef;
use ruff_python_ast::Expr;

use crate::ast_node_ref::AstNodeRef;
use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey;
Expand Down Expand Up @@ -212,7 +213,9 @@ impl<'db> SemanticIndexBuilder<'db> {
let existing_definition = self
.definitions_by_node
.insert(definition_node.key(), definition);
debug_assert_eq!(existing_definition, None);
if existing_definition.is_some() {
tracing::warn!("Existing definition was unexpectedly evicted");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a bunch of warnings I added just to try and get things working.

In total there are 228 warnings (of which this one is only hit 5 times), so it's pretty surmountable all things considered

}

if category.is_binding() {
self.mark_symbol_bound(symbol);
Expand Down Expand Up @@ -574,13 +577,21 @@ where
}
ast::Stmt::AnnAssign(node) => {
debug_assert!(self.current_assignment.is_none());
self.visit_expr(&node.annotation);
if let Some(value) = &node.value {
self.visit_expr(value);
let valid_target = matches!(
*node.target,
Expr::Attribute(_) | Expr::Subscript(_) | Expr::Name(_)
);
if valid_target {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we visit with an invalid target we get downstream errors on re-defining things. There's likely a way to visit here with an invalid target that doesn't generate issues

self.visit_expr(&node.annotation);
if let Some(value) = &node.value {
self.visit_expr(value);
}
self.current_assignment = Some(node.into());
self.visit_expr(&node.target);
self.current_assignment = None;
} else {
tracing::warn!("Annotated assignment with invalid target received");
}
self.current_assignment = Some(node.into());
self.visit_expr(&node.target);
self.current_assignment = None;
}
ast::Stmt::AugAssign(
aug_assign @ ast::StmtAugAssign {
Expand Down Expand Up @@ -879,12 +890,17 @@ where
walk_expr(self, expr);
}
ast::Expr::Named(node) => {
debug_assert!(self.current_assignment.is_none());
// TODO walrus in comprehensions is implicitly nonlocal
self.visit_expr(&node.value);
self.current_assignment = Some(node.into());
self.visit_expr(&node.target);
self.current_assignment = None;
if self.current_assignment.is_some() {
// This can happen if we have something like x = y := 2
// which is invalid syntax but still is provided in the AST
tracing::warn!("Current assignment is unexpectedly set");
Copy link
Member

Choose a reason for hiding this comment

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

We limit the use of tracing::warn for messages that need the user's attention and they need to address (e.g. when setting up file watching for a directory failed). Here, there's not much a user can do about this and invalid syntax is very common in an LSP. Logging a warning would be very noisy for users. We should use either the debug or possibly even trace level instead.

} else {
// TODO walrus in comprehensions is implicitly nonlocal
self.visit_expr(&node.value);
self.current_assignment = Some(node.into());
self.visit_expr(&node.target);
self.current_assignment = None;
}
}
ast::Expr::Lambda(lambda) => {
if let Some(parameters) = &lambda.parameters {
Expand Down
15 changes: 11 additions & 4 deletions crates/red_knot_python_semantic/src/semantic_model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}
}
}

Expand Down Expand Up @@ -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,
}
}
}
};
Expand Down
16 changes: 11 additions & 5 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down
Loading
Loading