Skip to content

Commit

Permalink
[red-knot] Use memory address as AST node key (#14317)
Browse files Browse the repository at this point in the history
## Summary

Use the memory address to uniquely identify AST nodes, instead of
relying on source range and kind. The latter fails for ASTs resulting
from invalid syntax examples. See #14313 for details.

Also results in a 1-2% speedup
(https://codspeed.io/astral-sh/ruff/runs/67349cf55f36b36baa211360)

closes #14313 

## Review

Here are the places where we use `NodeKey` directly or indirectly (via
`ExpressionNodeKey` or `DefinitionNodeKey`):

```rs
// semantic_index.rs
pub(crate) struct SemanticIndex<'db> { 
    // [...]
    /// Map expressions to their corresponding scope.
    scopes_by_expression: FxHashMap<ExpressionNodeKey, FileScopeId>,

    /// Map from a node creating a definition to its definition.
    definitions_by_node: FxHashMap<DefinitionNodeKey, Definition<'db>>,

    /// Map from a standalone expression to its [`Expression`] ingredient.
    expressions_by_node: FxHashMap<ExpressionNodeKey, Expression<'db>>,
    // [...]
}

// semantic_index/builder.rs
pub(super) struct SemanticIndexBuilder<'db> {
    // [...]
    scopes_by_expression: FxHashMap<ExpressionNodeKey, FileScopeId>,
    definitions_by_node: FxHashMap<ExpressionNodeKey, Definition<'db>>,
    expressions_by_node: FxHashMap<ExpressionNodeKey, Expression<'db>>,
}

// semantic_index/ast_ids.rs
pub(crate) struct AstIds {
    /// Maps expressions to their expression id.
    expressions_map: FxHashMap<ExpressionNodeKey, ScopedExpressionId>,
    /// Maps expressions which "use" a symbol (that is, [`ast::ExprName`]) to a use id.
    uses_map: FxHashMap<ExpressionNodeKey, ScopedUseId>,
}

pub(super) struct AstIdsBuilder {
    expressions_map: FxHashMap<ExpressionNodeKey, ScopedExpressionId>,
    uses_map: FxHashMap<ExpressionNodeKey, ScopedUseId>,
}
```

## Test Plan

Added two failing examples to the corpus.
  • Loading branch information
sharkdp authored Nov 13, 2024
1 parent 95c8f5f commit b946cfd
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 11 deletions.
19 changes: 8 additions & 11 deletions crates/red_knot_python_semantic/src/node_key.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,21 @@
use ruff_python_ast::{AnyNodeRef, NodeKind};
use ruff_text_size::{Ranged, TextRange};
use ruff_python_ast::AnyNodeRef;

/// Compact key for a node for use in a hash map.
///
/// Compares two nodes by their kind and text range.
/// Stores the memory address of the node, because using the range and the kind
/// of the node is not enough to uniquely identify them in ASTs resulting from
/// invalid syntax. For example, parsing the input `for` results in a `StmtFor`
/// AST node where both the `target` and the `iter` field are `ExprName` nodes
/// with the same (empty) range `3..3`.
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
pub(super) struct NodeKey {
kind: NodeKind,
range: TextRange,
}
pub(super) struct NodeKey(usize);

impl NodeKey {
pub(super) fn from_node<'a, N>(node: N) -> Self
where
N: Into<AnyNodeRef<'a>>,
{
let node = node.into();
NodeKey {
kind: node.kind(),
range: node.range(),
}
NodeKey(node.as_ptr().as_ptr() as usize)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x if $z
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
for

0 comments on commit b946cfd

Please sign in to comment.