Skip to content

Commit

Permalink
[red-knot] add Declarations support to semantic indexing (#13334)
Browse files Browse the repository at this point in the history 
Add support for declared types to the semantic index. This involves a
lot of renaming to clarify the distinction between bindings and
declarations. The Definition (or more specifically, the DefinitionKind)
becomes responsible for determining which definitions are bindings,
which are declarations, and which are both, and the symbol table
building is refactored a bit so that the `IS_BOUND` (renamed from
`IS_DEFINED` for consistent terminology) flag is always set when a
binding is added, rather than being set separately (and requiring us to
ensure it is set properly).

The `SymbolState` is split into two parts, `SymbolBindings` and
`SymbolDeclarations`, because we need to store live bindings for every
declaration and live declarations for every binding; the split lets us
do this without storing more than we need.

The massive doc comment in `use_def.rs` is updated to reflect bindings
vs declarations.

The `UseDefMap` gains some new APIs which are allow-unused for now,
since this PR doesn't yet update type inference to take declarations
into account.
  • Loading branch information
@carljm
carljm authored Sep 13, 2024
1 parent 8558126 commit d988204
Show file tree
Hide file tree
Showing 8 changed files with 958 additions and 474 deletions.
176 changes: 76 additions & 100 deletions crates/red_knot_python_semantic/src/semantic_index.rs
View file

Large diffs are not rendered by default.

101 changes: 56 additions & 45 deletions crates/red_knot_python_semantic/src/semantic_index/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::semantic_index::definition::{
};
use crate::semantic_index::expression::Expression;
use crate::semantic_index::symbol::{
FileScopeId, NodeWithScopeKey, NodeWithScopeRef, Scope, ScopeId, ScopedSymbolId, SymbolFlags,
FileScopeId, NodeWithScopeKey, NodeWithScopeRef, Scope, ScopeId, ScopedSymbolId,
SymbolTableBuilder,
};
use crate::semantic_index::use_def::{FlowSnapshot, UseDefMapBuilder};
Expand All @@ -28,7 +28,8 @@ use crate::Db;

use super::constraint::{Constraint, PatternConstraint};
use super::definition::{
ExceptHandlerDefinitionNodeRef, MatchPatternDefinitionNodeRef, WithItemDefinitionNodeRef,
DefinitionCategory, ExceptHandlerDefinitionNodeRef, MatchPatternDefinitionNodeRef,
WithItemDefinitionNodeRef,
};

pub(super) struct SemanticIndexBuilder<'db> {
Expand Down Expand Up @@ -168,31 +169,38 @@ impl<'db> SemanticIndexBuilder<'db> {
self.current_use_def_map_mut().merge(state);
}

fn add_or_update_symbol(&mut self, name: Name, flags: SymbolFlags) -> ScopedSymbolId {
let symbol_table = self.current_symbol_table();
let (symbol_id, added) = symbol_table.add_or_update_symbol(name, flags);
fn add_symbol(&mut self, name: Name) -> ScopedSymbolId {
let (symbol_id, added) = self.current_symbol_table().add_symbol(name);
if added {
let use_def_map = self.current_use_def_map_mut();
use_def_map.add_symbol(symbol_id);
self.current_use_def_map_mut().add_symbol(symbol_id);
}
symbol_id
}

fn mark_symbol_bound(&mut self, id: ScopedSymbolId) {
self.current_symbol_table().mark_symbol_bound(id);
}

fn mark_symbol_used(&mut self, id: ScopedSymbolId) {
self.current_symbol_table().mark_symbol_used(id);
}

fn add_definition<'a>(
&mut self,
symbol: ScopedSymbolId,
definition_node: impl Into<DefinitionNodeRef<'a>>,
) -> Definition<'db> {
let definition_node: DefinitionNodeRef<'_> = definition_node.into();
#[allow(unsafe_code)]
// SAFETY: `definition_node` is guaranteed to be a child of `self.module`
let kind = unsafe { definition_node.into_owned(self.module.clone()) };
let category = kind.category();
let definition = Definition::new(
self.db,
self.file,
self.current_scope(),
symbol,
#[allow(unsafe_code)]
unsafe {
definition_node.into_owned(self.module.clone())
},
kind,
countme::Count::default(),
);

Expand All @@ -201,8 +209,18 @@ impl<'db> SemanticIndexBuilder<'db> {
.insert(definition_node.key(), definition);
debug_assert_eq!(existing_definition, None);

self.current_use_def_map_mut()
.record_definition(symbol, definition);
if category.is_binding() {
self.mark_symbol_bound(symbol);
}

let use_def = self.current_use_def_map_mut();
match category {
DefinitionCategory::DeclarationAndBinding => {
use_def.record_declaration_and_binding(symbol, definition);
}
DefinitionCategory::Declaration => use_def.record_declaration(symbol, definition),
DefinitionCategory::Binding => use_def.record_binding(symbol, definition),
}

definition
}
Expand Down Expand Up @@ -284,10 +302,13 @@ impl<'db> SemanticIndexBuilder<'db> {
..
}) => (name, &None, default),
};
// TODO create Definition for typevars
self.add_or_update_symbol(name.id.clone(), SymbolFlags::IS_DEFINED);
if let Some(bound) = bound {
self.visit_expr(bound);
let symbol = self.add_symbol(name.id.clone());
// TODO create Definition for PEP 695 typevars
// note that the "bound" on the typevar is a totally different thing than whether
// or not a name is "bound" by a typevar declaration; the latter is always true.
self.mark_symbol_bound(symbol);
if let Some(bounds) = bound {
self.visit_expr(bounds);
}
if let Some(default) = default {
self.visit_expr(default);
Expand Down Expand Up @@ -350,8 +371,7 @@ impl<'db> SemanticIndexBuilder<'db> {
}

fn declare_parameter(&mut self, parameter: AnyParameterRef) {
let symbol =
self.add_or_update_symbol(parameter.name().id().clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(parameter.name().id().clone());

let definition = self.add_definition(symbol, parameter);

Expand Down Expand Up @@ -462,17 +482,15 @@ where
// The symbol for the function name itself has to be evaluated
// at the end to match the runtime evaluation of parameter defaults
// and return-type annotations.
let symbol = self
.add_or_update_symbol(function_def.name.id.clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(function_def.name.id.clone());
self.add_definition(symbol, function_def);
}
ast::Stmt::ClassDef(class) => {
for decorator in &class.decorator_list {
self.visit_decorator(decorator);
}

let symbol =
self.add_or_update_symbol(class.name.id.clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(class.name.id.clone());
self.add_definition(symbol, class);

self.with_type_params(
Expand All @@ -498,7 +516,7 @@ where
Name::new(alias.name.id.split('.').next().unwrap())
};

let symbol = self.add_or_update_symbol(symbol_name, SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(symbol_name);
self.add_definition(symbol, alias);
}
}
Expand All @@ -510,8 +528,7 @@ where
&alias.name.id
};

let symbol =
self.add_or_update_symbol(symbol_name.clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(symbol_name.clone());
self.add_definition(symbol, ImportFromDefinitionNodeRef { node, alias_index });
}
}
Expand Down Expand Up @@ -725,8 +742,7 @@ where
// which is invalid syntax. However, it's still pretty obvious here that the user
// *wanted* `e` to be bound, so we should still create a definition here nonetheless.
if let Some(symbol_name) = symbol_name {
let symbol = self
.add_or_update_symbol(symbol_name.id.clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(symbol_name.id.clone());

self.add_definition(
symbol,
Expand Down Expand Up @@ -756,24 +772,18 @@ where

match expr {
ast::Expr::Name(name_node @ ast::ExprName { id, ctx, .. }) => {
let flags = match (ctx, self.current_assignment) {
let (is_use, is_definition) = match (ctx, self.current_assignment) {
(ast::ExprContext::Store, Some(CurrentAssignment::AugAssign(_))) => {
// For augmented assignment, the target expression is also used.
SymbolFlags::IS_DEFINED | SymbolFlags::IS_USED
}
(ast::ExprContext::Store, Some(CurrentAssignment::AnnAssign(ann_assign)))
if ann_assign.value.is_none() =>
{
// An annotated assignment that doesn't assign a value is not a Definition
SymbolFlags::empty()
(true, true)
}
(ast::ExprContext::Load, _) => SymbolFlags::IS_USED,
(ast::ExprContext::Store, _) => SymbolFlags::IS_DEFINED,
(ast::ExprContext::Del, _) => SymbolFlags::IS_DEFINED,
(ast::ExprContext::Invalid, _) => SymbolFlags::empty(),
(ast::ExprContext::Load, _) => (true, false),
(ast::ExprContext::Store, _) => (false, true),
(ast::ExprContext::Del, _) => (false, true),
(ast::ExprContext::Invalid, _) => (false, false),
};
let symbol = self.add_or_update_symbol(id.clone(), flags);
if flags.contains(SymbolFlags::IS_DEFINED) {
let symbol = self.add_symbol(id.clone());
if is_definition {
match self.current_assignment {
Some(CurrentAssignment::Assign(assignment)) => {
self.add_definition(
Expand Down Expand Up @@ -830,7 +840,8 @@ where
}
}

if flags.contains(SymbolFlags::IS_USED) {
if is_use {
self.mark_symbol_used(symbol);
let use_id = self.current_ast_ids().record_use(expr);
self.current_use_def_map_mut().record_use(symbol, use_id);
}
Expand Down Expand Up @@ -970,7 +981,7 @@ where
range: _,
}) = pattern
{
let symbol = self.add_or_update_symbol(name.id().clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(name.id().clone());
let state = self.current_match_case.as_ref().unwrap();
self.add_definition(
symbol,
Expand All @@ -991,7 +1002,7 @@ where
rest: Some(name), ..
}) = pattern
{
let symbol = self.add_or_update_symbol(name.id().clone(), SymbolFlags::IS_DEFINED);
let symbol = self.add_symbol(name.id().clone());
let state = self.current_match_case.as_ref().unwrap();
self.add_definition(
symbol,
Expand Down
104 changes: 102 additions & 2 deletions crates/red_knot_python_semantic/src/semantic_index/definition.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub struct Definition<'db> {

#[no_eq]
#[return_ref]
pub(crate) node: DefinitionKind,
pub(crate) kind: DefinitionKind,

#[no_eq]
count: countme::Count<Definition<'static>>,
Expand All @@ -33,6 +33,21 @@ impl<'db> Definition<'db> {
pub(crate) fn scope(self, db: &'db dyn Db) -> ScopeId<'db> {
self.file_scope(db).to_scope_id(db, self.file(db))
}

#[allow(unused)]
pub(crate) fn category(self, db: &'db dyn Db) -> DefinitionCategory {
self.kind(db).category()
}

#[allow(unused)]
pub(crate) fn is_declaration(self, db: &'db dyn Db) -> bool {
self.kind(db).category().is_declaration()
}

#[allow(unused)]
pub(crate) fn is_binding(self, db: &'db dyn Db) -> bool {
self.kind(db).category().is_binding()
}
}

#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -302,6 +317,41 @@ impl DefinitionNodeRef<'_> {
}
}

#[derive(Clone, Copy, Debug)]
pub(crate) enum DefinitionCategory {
/// A Definition which binds a value to a name (e.g. `x = 1`).
Binding,
/// A Definition which declares the upper-bound of acceptable types for this name (`x: int`).
Declaration,
/// A Definition which both declares a type and binds a value (e.g. `x: int = 1`).
DeclarationAndBinding,
}

impl DefinitionCategory {
/// True if this definition establishes a "declared type" for the symbol.
///
/// If so, any assignments reached by this definition are in error if they assign a value of a
/// type not assignable to the declared type.
///
/// Annotations establish a declared type. So do function and class definition.
pub(crate) fn is_declaration(self) -> bool {
matches!(
self,
DefinitionCategory::Declaration | DefinitionCategory::DeclarationAndBinding
)
}

/// True if this definition assigns a value to the symbol.
///
/// False only for annotated assignments without a RHS.
pub(crate) fn is_binding(self) -> bool {
matches!(
self,
DefinitionCategory::Binding | DefinitionCategory::DeclarationAndBinding
)
}
}

#[derive(Clone, Debug)]
pub enum DefinitionKind {
Import(AstNodeRef<ast::Alias>),
Expand All @@ -321,6 +371,52 @@ pub enum DefinitionKind {
ExceptHandler(ExceptHandlerDefinitionKind),
}

impl DefinitionKind {
pub(crate) fn category(&self) -> DefinitionCategory {
match self {
// functions and classes always bind a value, and we always consider them declarations
DefinitionKind::Function(_) | DefinitionKind::Class(_) => {
DefinitionCategory::DeclarationAndBinding
}
// a parameter always binds a value, but is only a declaration if annotated
DefinitionKind::Parameter(parameter) => {
if parameter.annotation.is_some() {
DefinitionCategory::DeclarationAndBinding
} else {
DefinitionCategory::Binding
}
}
// presence of a default is irrelevant, same logic as for a no-default parameter
DefinitionKind::ParameterWithDefault(parameter_with_default) => {
if parameter_with_default.parameter.annotation.is_some() {
DefinitionCategory::DeclarationAndBinding
} else {
DefinitionCategory::Binding
}
}
// annotated assignment is always a declaration, only a binding if there is a RHS
DefinitionKind::AnnotatedAssignment(ann_assign) => {
if ann_assign.value.is_some() {
DefinitionCategory::DeclarationAndBinding
} else {
DefinitionCategory::Declaration
}
}
// all of these bind values without declaring a type
DefinitionKind::Import(_)
| DefinitionKind::ImportFrom(_)
| DefinitionKind::NamedExpression(_)
| DefinitionKind::Assignment(_)
| DefinitionKind::AugmentedAssignment(_)
| DefinitionKind::For(_)
| DefinitionKind::Comprehension(_)
| DefinitionKind::WithItem(_)
| DefinitionKind::MatchPattern(_)
| DefinitionKind::ExceptHandler(_) => DefinitionCategory::Binding,
}
}
}

#[derive(Clone, Debug)]
#[allow(dead_code)]
pub struct MatchPatternDefinitionKind {
Expand Down Expand Up @@ -441,8 +537,12 @@ pub struct ExceptHandlerDefinitionKind {
}

impl ExceptHandlerDefinitionKind {
pub(crate) fn node(&self) -> &ast::ExceptHandlerExceptHandler {
self.handler.node()
}

pub(crate) fn handled_exceptions(&self) -> Option<&ast::Expr> {
self.handler.node().type_.as_deref()
self.node().type_.as_deref()
}

pub(crate) fn is_star(&self) -> bool {
Expand Down
Loading

0 comments on commit d988204

Please sign in to comment.