Sitelet https://github.com/starkware-libs/cairo/commit/b44c9c9405821c809535f320551918a6ae212e4d
Skip to content

Commit b44c9c9

Browse files
eytan-starkwareeytan-starkware
committed
(opt): keep syntax node ids stable across reparses
Seed the canonical per-file root from the file-keyed parse query (file_syntax) instead of a green-keyed detached root, so reparsing a changed file reuses the previous node ids — early cutoff downstream relies on stable stable-ptrs. Split root construction into a canonical constructor (file_syntax only) and a detached one (for standalone/transient trees). The defs cache now records each root by file id alone and re-derives the tree via file_syntax on load, unifying cached stable ptrs with the live tree. External (plugin-generated) files re-derive the same way; their content is served from the cache blob (see the external-file content cache below this in the stack), so re-deriving never cycles back into module data.
1 parent 0fb9292 commit b44c9c9

13 files changed

Lines changed: 234 additions & 79 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/cairo-lang-defs/src/cache/mod.rs‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use cairo_lang_filesystem::ids::{
1111
VirtualFile,
1212
};
1313
use cairo_lang_filesystem::span::{TextSpan, TextWidth};
14+
use cairo_lang_parser::db::ParserGroup;
1415
use cairo_lang_syntax::node::ast::{
1516
FunctionWithBodyPtr, GenericParamPtr, ItemConstantPtr, ItemEnumPtr, ItemExternFunctionPtr,
1617
ItemExternTypePtr, ItemImplAliasPtr, ItemImplPtr, ItemInlineMacroPtr, ItemMacroDeclarationPtr,
@@ -1705,7 +1706,12 @@ struct SyntaxNodeCached(usize);
17051706

17061707
#[derive(Serialize, Deserialize, Clone, Hash, Eq, PartialEq, Debug)]
17071708
enum SyntaxNodeIdCached {
1708-
Root(FileIdCached, GreenIdCached),
1709+
/// A file root (any kind: on-disk, virtual, or external). On load the root is re-derived via
1710+
/// `db.file_syntax` — the canonical tree, never a detached duplicate — rather than rebuilt from
1711+
/// stored green, and without re-entering module data (so cache load can't cycle).
1712+
Root(FileIdCached),
1713+
/// A non-root node, identified by its `parent` plus the child's `key_fields`, `kind`, and
1714+
/// `index`.
17091715
Child {
17101716
parent: SyntaxNodeCached,
17111717
kind: SyntaxKind,
@@ -1721,10 +1727,7 @@ impl SyntaxNodeIdCached {
17211727
syntax_node: SyntaxNode<'db>,
17221728
) -> Self {
17231729
match id {
1724-
SyntaxNodeId::Root(file_id) => {
1725-
let green = syntax_node.green_node(ctx.db).clone().intern(ctx.db);
1726-
Self::Root(FileIdCached::new(*file_id, ctx), GreenIdCached::new(green, ctx))
1727-
}
1730+
SyntaxNodeId::Root(file_id) => Self::Root(FileIdCached::new(*file_id, ctx)),
17281731
SyntaxNodeId::Child { parent, key_fields, index } => Self::Child {
17291732
parent: SyntaxNodeCached::new(*parent, ctx),
17301733
kind: syntax_node.kind(ctx.db),
@@ -1775,10 +1778,18 @@ impl SyntaxNodeCached {
17751778
}
17761779
let id_cached = ctx.syntax_node_lookup[self.0].clone();
17771780
match &id_cached {
1778-
SyntaxNodeIdCached::Root(file_id, green_id) => {
1781+
SyntaxNodeIdCached::Root(file_id) => {
1782+
// Re-derive the canonical tree via `file_syntax`; walking `get_children` below
1783+
// lands the cached stable ptrs on those canonical nodes. See
1784+
// `SyntaxNodeIdCached::Root`.
17791785
let fid = file_id.embed(ctx);
1780-
let green = green_id.embed(ctx);
1781-
let syntax = cairo_lang_syntax::node::SyntaxNode::new_root(ctx.db, fid, green);
1786+
let syntax = ctx.db.file_syntax(fid).unwrap_or_else(|_| {
1787+
panic!(
1788+
"defs cache: cannot re-derive syntax for file {:?}; its content must be \
1789+
available to load the cache",
1790+
fid.long(ctx.db)
1791+
)
1792+
});
17821793
ctx.syntax_nodes.insert(*self, syntax);
17831794
}
17841795
SyntaxNodeIdCached::Child { parent, .. } => {

‎crates/cairo-lang-formatter/src/formatter_impl.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -978,7 +978,7 @@ impl<'a> FormatterImpl<'a> {
978978
let file_id = syntax_node.stable_ptr(self.db).file_id(self.db);
979979

980980
if let Some(wrapped_arg_list) = as_wrapped_arg_list {
981-
let new_syntax_node = SyntaxNode::new_root_with_offset(
981+
let new_syntax_node = SyntaxNode::new_detached_root_with_offset(
982982
self.db,
983983
file_id,
984984
wrapped_arg_list.0,

‎crates/cairo-lang-lowering/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ thiserror.workspace = true
3030
tracing = { workspace = true, features = ["log"] }
3131

3232
[dev-dependencies]
33+
cairo-lang-parser = { path = "../cairo-lang-parser" }
3334
cairo-lang-plugins = { path = "../cairo-lang-plugins" }
3435
cairo-lang-semantic = { path = "../cairo-lang-semantic", features = ["testing"] }
3536
cairo-lang-test-utils = { path = "../cairo-lang-test-utils", features = ["testing"] }

‎crates/cairo-lang-lowering/src/cache/test.rs‎

Lines changed: 65 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
1+
use cairo_lang_defs::db::DefsGroup;
2+
use cairo_lang_defs::ids::LanguageElementId;
13
use cairo_lang_filesystem::db::{FilesGroup, files_group_input, set_crate_configs_input};
2-
use cairo_lang_filesystem::ids::BlobLongId;
4+
use cairo_lang_filesystem::ids::{BlobLongId, FileLongId};
5+
use cairo_lang_parser::db::ParserGroup;
36
use cairo_lang_semantic::corelib::CorelibSemantic;
47
use cairo_lang_semantic::test_utils::setup_test_function_ex;
58
use cairo_lang_test_utils::parse_test_file::TestRunnerResult;
@@ -26,23 +29,11 @@ fn test_cache_check(
2629
inputs: &OrderedHashMap<String, String>,
2730
args: &OrderedHashMap<String, String>,
2831
) -> TestRunnerResult {
29-
let db = &mut LoweringDatabaseForTesting::default();
30-
3132
let function = &inputs["function_code"];
3233
let function_name = &inputs["function_name"];
3334
let module_code = inputs.get("module_code").map_or("", String::as_str);
34-
let (test_function, _semantic_diagnostics) =
35-
setup_test_function_ex(db, function, function_name, module_code, None, None).split();
36-
37-
let artifact = generate_crate_cache(db, test_function.module_id.owning_crate(db)).unwrap();
38-
let core_artifact = generate_crate_cache(db, db.core_crate()).unwrap();
39-
let mut new_db = LoweringDatabaseForTesting::new();
40-
let crt = new_db.crate_input(new_db.core_crate());
41-
let mut crate_configs = files_group_input(&new_db).crate_configs(&new_db).clone().unwrap();
42-
let config = crate_configs.get_mut(crt).unwrap();
43-
config.cache_file = Some(BlobLongId::Virtual(core_artifact));
44-
set_crate_configs_input(&mut new_db, Some(crate_configs));
4535

36+
let (new_db, artifact) = generate_cached_db(function, function_name, module_code);
4637
let cached_file = BlobLongId::Virtual(artifact).intern(&new_db);
4738
let (test_function, semantic_diagnostics) = setup_test_function_ex(
4839
&new_db,
@@ -78,3 +69,63 @@ fn test_cache_check(
7869
error,
7970
}
8071
}
72+
73+
/// Compiles `function`/`module_code` to generate the crate cache (and the corelib cache), then
74+
/// returns a fresh db with the corelib cache loaded plus the crate-cache artifact. Callers wire the
75+
/// artifact in as the test crate's `cache_file` via
76+
/// `setup_test_function_ex(.., Some(BlobLongId::Virtual(artifact).intern(&db)))`.
77+
fn generate_cached_db(
78+
function: &str,
79+
function_name: &str,
80+
module_code: &str,
81+
) -> (LoweringDatabaseForTesting, Vec<u8>) {
82+
let db = &mut LoweringDatabaseForTesting::default();
83+
let (test_function, _) =
84+
setup_test_function_ex(db, function, function_name, module_code, None, None).split();
85+
86+
let artifact = generate_crate_cache(db, test_function.module_id.owning_crate(db)).unwrap();
87+
let core_artifact = generate_crate_cache(db, db.core_crate()).unwrap();
88+
89+
let mut new_db = LoweringDatabaseForTesting::new();
90+
let crt = new_db.crate_input(new_db.core_crate());
91+
let mut crate_configs = files_group_input(&new_db).crate_configs(&new_db).clone().unwrap();
92+
crate_configs.get_mut(crt).unwrap().cache_file = Some(BlobLongId::Virtual(core_artifact));
93+
set_crate_configs_input(&mut new_db, Some(crate_configs));
94+
(new_db, artifact)
95+
}
96+
97+
/// File syntax roots are canonical (file-keyed via `SyntaxNode::new_canonical_root`). For an
98+
/// external (plugin-generated) file restored from a crate cache, the root reached from a cached
99+
/// stable ptr must therefore be the very node `db.file_syntax` mints — not a detached duplicate.
100+
#[test]
101+
fn cached_external_file_root_is_canonical() {
102+
let function = "fn foo() {}";
103+
let module_code = "\
104+
#[derive(Drop)]
105+
struct MyStruct {
106+
x: felt252,
107+
}";
108+
let (db, artifact) = generate_cached_db(function, "foo", module_code);
109+
let cached_file = BlobLongId::Virtual(artifact).intern(&db);
110+
let (cached_function, _) =
111+
setup_test_function_ex(&db, function, "foo", module_code, None, Some(cached_file)).split();
112+
113+
// The `#[derive(Drop)]` impl lives in an external file; its cached stable ptr must resolve to
114+
// the canonical `file_syntax` root.
115+
let mut checked = 0;
116+
for impl_id in db.module_impls_ids(cached_function.module_id).unwrap() {
117+
let stable_ptr = impl_id.untyped_stable_ptr(&db);
118+
let ext_file = stable_ptr.file_id(&db);
119+
if !matches!(ext_file.long(&db), FileLongId::External(_)) {
120+
continue;
121+
}
122+
let root = stable_ptr.0.ancestors_with_self(&db).last().unwrap();
123+
assert_eq!(
124+
root,
125+
db.file_syntax(ext_file).unwrap(),
126+
"external root differs from file_syntax (detached node minted on load?)"
127+
);
128+
checked += 1;
129+
}
130+
assert!(checked > 0, "expected at least one external (derive-generated) file");
131+
}

‎crates/cairo-lang-parser/src/db.rs‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,20 @@ struct SyntaxData<'db> {
5757
#[salsa::tracked(returns(ref))]
5858
fn file_syntax_data<'db>(db: &'db dyn Database, file_id: FileId<'db>) -> SyntaxData<'db> {
5959
let mut diagnostics = DiagnosticsBuilder::default();
60-
let syntax = db.file_content(file_id).to_maybe().map(|s| match file_id.kind(db) {
61-
FileKind::Module => Parser::parse_file(db, &mut diagnostics, file_id, s).as_syntax_node(),
62-
FileKind::Expr => {
63-
Parser::parse_file_expr(db, &mut diagnostics, file_id, s).as_syntax_node()
64-
}
65-
FileKind::StatementList => {
66-
Parser::parse_file_statement_list(db, &mut diagnostics, file_id, s).as_syntax_node()
67-
}
60+
let syntax = db.file_content(file_id).to_maybe().map(|s| {
61+
let green = match file_id.kind(db) {
62+
FileKind::Module => Parser::parse_file_green(db, &mut diagnostics, file_id, s).0,
63+
FileKind::Expr => Parser::parse_file_expr_green(db, &mut diagnostics, file_id, s).0,
64+
FileKind::StatementList => {
65+
Parser::parse_file_statement_list_green(db, &mut diagnostics, file_id, s).0
66+
}
67+
};
68+
// Seed the canonical root from this file-keyed query (not a green-keyed detached
69+
// constructor), so reparsing a changed file reuses the previous node ids. This keeps
70+
// stable ptrs (and ids derived from them) stable across edits, which early cutoff
71+
// downstream relies on. This is the *only* place a canonical root is created; every other
72+
// consumer reaches it via `db.file_syntax(file_id)`.
73+
SyntaxNode::new_canonical_root(db, file_id, green)
6874
});
6975
SyntaxData::new(db, diagnostics.build(), syntax)
7076
}

‎crates/cairo-lang-parser/src/db_test.rs‎

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
use std::path::PathBuf;
22

3-
use cairo_lang_filesystem::ids::{FileId, SmolStrId};
3+
use cairo_lang_filesystem::db::FilesGroup;
4+
use cairo_lang_filesystem::ids::{Directory, SmolStrId};
5+
use cairo_lang_filesystem::override_file_content;
46
use cairo_lang_filesystem::span::TextSpan;
57
use cairo_lang_syntax::node::ast::{
6-
ModuleItemList, SyntaxFile, TerminalEndOfFile, TokenEndOfFile, Trivia,
8+
ModuleItemList, SyntaxFile, SyntaxFileGreen, TerminalEndOfFile, TokenEndOfFile, Trivia,
79
};
810
use cairo_lang_syntax::node::{SyntaxNode, Terminal, Token as SyntaxToken, TypedSyntaxNode};
911
use indoc::indoc;
@@ -15,22 +17,15 @@ use crate::printer::print_tree;
1517
use crate::test_utils::{MockToken, MockTokenStream, create_virtual_file};
1618
use crate::utils::{SimpleParserDatabase, get_syntax_root_and_diagnostics_from_file};
1719

18-
fn build_empty_file_green_tree<'a>(db: &'a dyn Database, file_id: FileId<'a>) -> SyntaxFile<'a> {
20+
fn build_empty_file_green_tree<'a>(db: &'a dyn Database) -> SyntaxFileGreen<'a> {
1921
let eof_token = TokenEndOfFile::new_green(db, SmolStrId::from(db, ""));
2022
let eof_terminal = TerminalEndOfFile::new_green(
2123
db,
2224
Trivia::new_green(db, &[]),
2325
eof_token,
2426
Trivia::new_green(db, &[]),
2527
);
26-
SyntaxFile::from_syntax_node(
27-
db,
28-
SyntaxNode::new_root(
29-
db,
30-
file_id,
31-
SyntaxFile::new_green(db, ModuleItemList::new_green(db, &[]), eof_terminal).0,
32-
),
33-
)
28+
SyntaxFile::new_green(db, ModuleItemList::new_green(db, &[]), eof_terminal)
3429
}
3530

3631
#[test]
@@ -43,9 +38,11 @@ fn test_parser() {
4338
let diagnostics = db.file_syntax_diagnostics(file_id);
4439
assert_eq!(diagnostics.format(&db), "");
4540

46-
let expected_syntax_file = build_empty_file_green_tree(&db, file_id);
47-
48-
assert_eq!(syntax_file, expected_syntax_file);
41+
// Compare green trees: the test only asserts the parsed structure, and the parsed root (minted
42+
// by the parse query) is a distinct node from any standalone one, so node identity wouldn't
43+
// match anyway.
44+
let expected_green = build_empty_file_green_tree(&db);
45+
assert_eq!(syntax_file.as_syntax_node().green_node(&db), expected_green.0.long(&db));
4946
}
5047

5148
#[test]
@@ -102,3 +99,40 @@ fn test_token_stream_expr_parser() {
10299

103100
assert_eq!(node_text, expr_code);
104101
}
102+
103+
#[test]
104+
fn reparse_reuses_canonical_root_node_id() {
105+
// A file's root is the *canonical* root, seeded by the file-keyed `file_syntax_data` query (via
106+
// `SyntaxNode::new_canonical_root`). Editing the file's content therefore reuses the same root
107+
// node id rather than minting a fresh one — the id stability that downstream salsa early cutoff
108+
// relies on. A detached, green-keyed root (`new_detached_root`) would instead get a new id on
109+
// every content change, so this assertion guards that `file_syntax` stays canonical.
110+
// Each phase is scoped so that no `FileId`/`SyntaxNode` borrow is held across a `&mut` content
111+
// edit. The node id is encoded in `SyntaxNode`'s `Debug`, captured as an owned string.
112+
let mut db = SimpleParserDatabase::default();
113+
114+
{
115+
let db_ref = &mut db;
116+
let file_id = Directory::Real("src".into()).file(db_ref, "lib.cairo");
117+
override_file_content!(db_ref, file_id, Some("fn foo() {}\n".into()));
118+
}
119+
let root_id_before = {
120+
let file_id = Directory::Real("src".into()).file(&db, "lib.cairo");
121+
format!("{:?}", db.file_syntax(file_id).unwrap())
122+
};
123+
124+
{
125+
let db_ref = &mut db;
126+
let file_id = Directory::Real("src".into()).file(db_ref, "lib.cairo");
127+
override_file_content!(db_ref, file_id, Some("fn foo() { let _x = 1; }\n".into()));
128+
}
129+
let root_id_after = {
130+
let file_id = Directory::Real("src".into()).file(&db, "lib.cairo");
131+
format!("{:?}", db.file_syntax(file_id).unwrap())
132+
};
133+
134+
assert_eq!(
135+
root_id_before, root_id_after,
136+
"canonical root node id must stay stable across a reparse"
137+
);
138+
}

‎crates/cairo-lang-parser/src/lexer_test.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ fn test_lex_single_token() {
270270
for (kind, text) in terminal_kind_and_text() {
271271
let terminals = tokenize_all(db, Arc::from(text));
272272
let terminal = &terminals[0];
273-
// TODO(spapini): Remove calling new_root on non root elements.
273+
// TODO(spapini): Remove calling new_detached_root on non root elements.
274274
assert_eq!(terminal.kind, kind, "Wrong token kind, with text: \"{text}\".");
275275
assert_eq!(terminal.text.long(db), text, "Wrong token text.");
276276

‎crates/cairo-lang-parser/src/macro_helpers.rs‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,12 @@ pub fn as_expr_macro_token_tree<'a>(
6363
let expr_green = parser.parse_expr();
6464
let expr = ast::Expr::from_syntax_node(
6565
db,
66-
SyntaxNode::new_root_with_offset(db, file_id, expr_green.0, Some(first_token.offset(db))),
66+
SyntaxNode::new_detached_root_with_offset(
67+
db,
68+
file_id,
69+
expr_green.0,
70+
Some(first_token.offset(db)),
71+
),
6772
);
6873
Some(expr)
6974
}
@@ -93,7 +98,7 @@ impl<'a> AsLegacyInlineMacro<'a> for ExprInlineMacro<'a> {
9398
let offset = self.stable_ptr(db).0.lookup(db).offset(db);
9499
Some(LegacyExprInlineMacro::from_syntax_node(
95100
db,
96-
SyntaxNode::new_root_with_offset(db, file_id, legacy_green.0, Some(offset)),
101+
SyntaxNode::new_detached_root_with_offset(db, file_id, legacy_green.0, Some(offset)),
97102
))
98103
}
99104
}
@@ -123,7 +128,7 @@ impl<'a> AsLegacyInlineMacro<'a> for ItemInlineMacro<'a> {
123128
let offset = self.stable_ptr(db).0.lookup(db).offset(db);
124129
Some(LegacyItemInlineMacro::from_syntax_node(
125130
db,
126-
SyntaxNode::new_root_with_offset(db, file_id, legacy_green.0, Some(offset)),
131+
SyntaxNode::new_detached_root_with_offset(db, file_id, legacy_green.0, Some(offset)),
127132
))
128133
}
129134
}

0 commit comments

Comments
 (0)