Skip to content

feature: expose database contents as inherent method on Ingredients #676

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

Conversation

davidbarsky
Copy link
Contributor

This PR redesigns the functionality introduced in #640 so that the dump-all-database contents methods are on the Salsa struct ingredient instead of the database's Storage. I think this is an improvement for two reasons:

  1. This design is closer to the original DebugQueryTable trait.
  2. At least within rust-analyzer, it's not feasible to get access to the underlying storage from methods that only have a &dyn SomeDatabase. However, it is feasible to access the ingredient

Example

Here's an example for interned structs (input and tracked structs have an identical API):

#[salsa::interned]
struct InternedStruct<'db> {
    name: String,
}

let _ = InternedStruct::new(&db, "Hello, world!".to_string());

let interned = InternedStruct::ingredient(&db)
    .entries(&db)
    .collect::<Vec<_>>();

Note that the entries method is defined on the {interned, input, tracked}::IngredientImpl. The more optimal API would be to define entries on the Salsa struct itself, alongside ingredient. However, this is substantially more annoying to feature-flag through macros.

Notes

While I don't have a good idea how to access a tracked function's memos, I also don't really know how much it makes sense to do so outside of some migrations.

Copy link

netlify bot commented Feb 6, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 7b0744e
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67a528e6b72dd6000895ebee

Copy link

codspeed-hq bot commented Feb 6, 2025

CodSpeed Performance Report

Merging #676 will not alter performance

Comparing davidbarsky:davidbarsky/per-ingredient-debug-entries (7b0744e) with master (9cfa5a8)

Summary

✅ 9 untouched benchmarks

@MichaReiser
Copy link
Contributor

Exposing the methods on the ingredient does make sense to me.

While I don't have a good idea how to access a tracked function's memos, I also don't really know how much it makes sense to do so outside of some migrations.

Not saying we have to do this as part of this PR but having the same functionality for queries seems useful because most queries (e.g. the source text or parsed AST) in Ruff aren't tracked structs or inputs but getting a count of alive values would be a useful debug statistics.

@davidbarsky
Copy link
Contributor Author

davidbarsky commented Feb 7, 2025

Exposing the methods on the ingredient does make sense to me.

How would you feel about moving entries to the Salsa struct itself once we stop exposing the underlying storage mechanism?

While I don't have a good idea how to access a tracked function's memos, I also don't really know how much it makes sense to do so outside of some migrations.

Not saying we have to do this as part of this PR but having the same functionality for queries seems useful because most queries (e.g. the source text or parsed AST) in Ruff aren't tracked structs or inputs but getting a count of alive values would be a useful debug statistics.

Ah, I naively assumed that most queries in Ruff were Salsa structs. In that case, we should add this functionality to tracked functions, but I'd rather do that in a followup: tracked functions feel substantially more difficult than Salsa structs, but that might also be my unfamiliarity.

@davidbarsky davidbarsky added this pull request to the merge queue Feb 7, 2025
Merged via the queue into salsa-rs:master with commit c02959d Feb 7, 2025
10 checks passed
@davidbarsky davidbarsky deleted the davidbarsky/per-ingredient-debug-entries branch February 7, 2025 16:37
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