Skip to content

Moving to new Salsa version using query-group repo (DRAFT) #7805

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

Draft
wants to merge 1 commit into
base: spr/main/4d7fca4f
Choose a base branch
from

Conversation

eytan-starkware
Copy link
Contributor

@eytan-starkware eytan-starkware commented May 28, 2025

This is a WIP

  • Using an updated version of query-group to better support lifetimes.
  • Embedded query-group in cairo-lang-proc-macros.
  • cairo-lang-proc-macro's derive debug with db now supprots generics better.
  • Using salsa::Update on non salsa structs to get the db to work correctly.
  • Moved some SmolStr and Arc to use be interned for salsa::Update support.
  • Implemented Ord for SmolStrId to have BTreeMap's salsa::Update work.
  • Removing upcasting and 'static where applicable
  • Adding alot of <'_> for a happy clippy.
  • Debug with db now supports lifetimes. Fixed implementations across the codebase.
  • Debug with db now has an associated type for the db for better support of upcasting.
  • I need to overcome limitations of associated types in traits. To allow tuples to implement DebugWithDb I:
  • Switched to using (ExprId, MemberId) instead of (MemberId, ExprId) in ExprStructCtor.
  • Switched to using (PatternId, MemberId) instead of (MemberId, PatternId) in PatternStruct.
  • Fixed ref in defs cache.
  • DebugWithDb now supports custom formatting using DebugWithDbOverride trait.
  • Doc + Tests pass

  • Upgraded cairo-lang-debug by:
  1. saying Db has the salsa::Database trait
  2. making Dummy in test not use shortid
  3. Debug with Db can make some trouble but using .long where needed fixes it.
  • cairo-lang-utils:
    1 .The short_id macro is not really needed so I gutted it and have short_id double interned
    1.1. I made short_id have a single long field that is returned by ref.
  1. Implemented salsa::Update for OrderedHashMap.
  2. Implemented salsa::Update for OrderedHashSet.
  3. Added an as_intern_id and from_intern_id using asid and fromid
  4. No need for upcast to be static
  • cairo-lang-filesystem
  1. Removed all interned valued from input.
  2. Updated code with lifetimes to better support <'db> interned values.
  3. Tests needed updating as set methods are a bit more tricky now with lifetimes.
  4. Added utility function for setting input values with using lifetimed objects.
  5. Added macro_rules! to set_crate_config and override_file_content so they still accept a short id.
  • cairo-lang-syntax:
  • Updated all code to use lifetimes.
  • Updated all code to use the new query-group.
  • Updated codegen.
  • Updated ast.
  • cairo-lang-diagnostics:
  • Updated all code to use lifetimes.
  • Updated all code to use the new query-group.
  • Added lifetime to DiagnosticEntry and DiagnosticBuilder as it was necessary by plugin tests.
  • cairo-lang-parser:
  • Updated all code to use lifetimes.
  • Updated all code to use the new query-group.
  • Removed the references to empty data structs as they are no longer needed.
  • Using long to turn StrId into a ref for the parser.
  • cairo-lang-sierra
  • No need for internkey (as query group will use #[salsa::interned] instead)
  • cairo-lang-defs
  • Added lifetime to db
  • Added lifetime to diagnostic_utils
  • Added lifetime to ids
  • Fixing macros in ids to support lifetimes
  • Using self.long(db) instead of lookup_intern to get a ref with a 'db lifetime
  • Change from Arc<[T]> to Arc<Vec> which allows for salsa::Update.
  • Tests to use macro_rules! to get lifetimes right.
  • Updated tests to work with all the changes
  • Fixed debug print formatting.
  • cairo-lang-plugins:
  • Updated all code to use lifetimes.
  • cairo-lang-semantic:
  • All if my personal lifetime has been transferred to the db. I now have no life.
  • cairo-lang-lowering:
  • Added lifetimes and debug and update support.
  • Implemented maybe_update.
  • cairo-lang-doc:
  • Updated all code to use lifetimes.
  • Updated all code to use the new query-group.
  • Added Update where needed.
  • Fixed tests (and removed additional DB creation)

Added preparation for semantic db.
Debug lifetime fixes.
Cycle support for queries
More salsa::Update support.
More lifetimes.
Update for unordered_hash_map and unordered_hash_set.
Deque wrapper and Update for it.
Some rewriter macro support.
Moved priv queries back to salsa db - using update on FunctionBodyData

  • Update assumes that ExprId and PatternId are from the FunctionBody in the struct.

AI helped a bit:
Core Upgrade
Migrated to a new Salsa version with better lifetime support by embedding an updated query-group implementation directly into cairo-lang-proc-macros
Added comprehensive lifetime annotations (<'db>, <'_>) throughout the codebase to support the new Salsa architecture
Key Technical Changes
Salsa Integration Improvements:
Implemented salsa::Update trait for non-Salsa structs (OrderedHashMap, OrderedHashSet, Deque wrapper)
Converted SmolStr and Arc to interned values for Salsa compatibility
Added Ord implementation for SmolStrId to support BTreeMap's salsa::Update
Database & Query System:
Removed static lifetime requirements and upcasting where possible
Added cycle support for queries
Improved generic support in proc-macro's derive debug with db
Major Crate Updates:
cairo-lang-proc-macros: Added 11 new test files and embedded query-group functionality
cairo-lang-utils: Gutted short_id macro, added Deque wrapper, intern conversion utilities
cairo-lang-filesystem: Removed interned values from input, added utility functions for lifetime-aware inputs
All syntax/parser/semantic crates: Updated to use explicit lifetimes throughout
New Functionality
Added preparation for semantic database
Improved macro support for lifetimes in test utilities
Changed from Arc<[T]> to Arc<Vec> for salsa::Update compatibility
This is a foundational change that modernizes the entire Cairo compiler's incremental computation infrastructure to better handle borrowed data and lifetimes.


Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 54 files reviewed, all discussions resolved

@eytan-starkware eytan-starkware force-pushed the spr/main/9e4ac020 branch 2 times, most recently from 079b236 to 3a0fc63 Compare June 12, 2025 13:08
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 63 files reviewed, all discussions resolved

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 63 files reviewed, all discussions resolved

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 64 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 70 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 76 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 80 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 84 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 91 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 95 files reviewed, all discussions resolved (waiting on @orizi)

@eytan-starkware eytan-starkware force-pushed the spr/main/9e4ac020 branch 2 times, most recently from 993d16b to e42c063 Compare July 10, 2025 10:27
Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 162 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 164 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 164 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 173 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 177 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 177 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 223 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 231 files reviewed, all discussions resolved (waiting on @orizi)

This is a WIP

* Using an updated version of query-group to better support lifetimes.
* Embedded query-group in cairo-lang-proc-macros.
* cairo-lang-proc-macro's derive debug with db now supprots generics better.
* Using salsa::Update on non salsa structs to get the db to work correctly.
* Moved some SmolStr and Arc<str> to use be interned for salsa::Update support.
* Implemented Ord for SmolStrId to have BTreeMap's salsa::Update work.
* Removing upcasting and 'static where applicable
* Adding alot of <'_> for a happy clippy.
* Debug with db now supports lifetimes. Fixed implementations across the codebase.
* Debug with db now has an associated type for the db for better support of upcasting.
* I need to overcome limitations of associated types in traits. To allow tuples to implement DebugWithDb I:
    - Switched to using (ExprId, MemberId) instead of (MemberId, ExprId) in ExprStructCtor.
    - Switched to using (PatternId, MemberId) instead of (MemberId, PatternId) in PatternStruct.
* Fixed ref in defs cache.
* DebugWithDb now supports custom formatting using DebugWithDbOverride trait.
* Doc + Tests pass

---

* Upgraded cairo-lang-debug by:
    1. saying Db has the salsa::Database trait
    2. making Dummy in test not use shortid
    3. Debug with Db can make some trouble but using .long where needed fixes it.
* cairo-lang-utils:
    1 .The short_id macro is not really needed so I gutted it and have short_id double interned
    1.1. I made short_id have a single long field that is returned by ref.
    2. Implemented salsa::Update for OrderedHashMap.
    3. Implemented salsa::Update for OrderedHashSet.
    4. Added an as_intern_id and from_intern_id using asid and fromid
    5. No need for upcast to be static
* cairo-lang-filesystem
    1. Removed all interned valued from input.
    2. Updated code with lifetimes to better support <'db> interned values.
    3. Tests needed updating as set methods are a bit more tricky now with lifetimes.
    4. Added utility function for setting input values with using lifetimed objects.
    5. Added macro_rules! to set_crate_config and override_file_content so they still accept a short id.
* cairo-lang-syntax:
    + Updated all code to use lifetimes.
    + Updated all code to use the new query-group.
    + Updated codegen.
    + Updated ast.
* cairo-lang-diagnostics:
    + Updated all code to use lifetimes.
    + Updated all code to use the new query-group.
    + Added lifetime to DiagnosticEntry and DiagnosticBuilder as it was necessary by plugin tests.
* cairo-lang-parser:
    + Updated all code to use lifetimes.
    + Updated all code to use the new query-group.
    + Removed the references to empty data structs as they are no longer needed.
    + Using long to turn StrId into a ref for the parser.
* cairo-lang-sierra
    + No need for internkey (as query group will use #[salsa::interned] instead)
* cairo-lang-defs
    + Added lifetime to db
    + Added lifetime to diagnostic_utils
    + Added lifetime to ids
    + Fixing macros in ids to support lifetimes
    + Using self.long(db) instead of lookup_intern to get a ref with a 'db lifetime
    + Change from Arc<[T]> to Arc<Vec<T>> which allows for salsa::Update.
    + Tests to use macro_rules! to get lifetimes right.
    + Updated tests to work with all the changes
    + Fixed debug print formatting.
* cairo-lang-plugins:
    + Updated all code to use lifetimes.
* cairo-lang-semantic:
    + All if my personal lifetime has been transferred to the db. I now have no life.
* cairo-lang-lowering:
    + Added lifetimes and debug and update support.
    + Implemented maybe_update.
* cairo-lang-doc:
    + Updated all code to use lifetimes.
    + Updated all code to use the new query-group.
    + Added Update where needed.
    + Fixed tests (and removed additional DB creation)

Added preparation for semantic db.
Debug lifetime fixes.
Cycle support for queries
More salsa::Update support.
More lifetimes.
Update for unordered_hash_map and unordered_hash_set.
Deque wrapper and Update for it.
Some rewriter macro support.
Moved priv queries back to salsa db - using update on FunctionBodyData
    - Update assumes that ExprId and PatternId are from the FunctionBody in the struct.

AI helped a bit:
Core Upgrade
Migrated to a new Salsa version with better lifetime support by embedding an updated query-group implementation directly into cairo-lang-proc-macros
Added comprehensive lifetime annotations (<'db>, <'_>) throughout the codebase to support the new Salsa architecture
Key Technical Changes
Salsa Integration Improvements:
Implemented salsa::Update trait for non-Salsa structs (OrderedHashMap, OrderedHashSet, Deque wrapper)
Converted SmolStr and Arc<str> to interned values for Salsa compatibility
Added Ord implementation for SmolStrId to support BTreeMap's salsa::Update
Database & Query System:
Removed static lifetime requirements and upcasting where possible
Added cycle support for queries
Improved generic support in proc-macro's derive debug with db
Major Crate Updates:
cairo-lang-proc-macros: Added 11 new test files and embedded query-group functionality
cairo-lang-utils: Gutted short_id macro, added Deque wrapper, intern conversion utilities
cairo-lang-filesystem: Removed interned values from input, added utility functions for lifetime-aware inputs
All syntax/parser/semantic crates: Updated to use explicit lifetimes throughout
New Functionality
Added preparation for semantic database
Improved macro support for lifetimes in test utilities
Changed from Arc<[T]> to Arc<Vec<T>> for salsa::Update compatibility
This is a foundational change that modernizes the entire Cairo compiler's incremental computation infrastructure to better handle borrowed data and lifetimes.

commit-id:9e4ac020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants