Skip to content
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

[red-knot] Remove <Db: SemanticDb> contraints in favor of dynamic dispatch #11339

Merged
merged 1 commit into from
May 8, 2024

Conversation

MichaReiser
Copy link
Member

Summary

We used to have a mixture of functions/methods that accept &dyn Db + HasJar and methods that were generic over Db.

The problem with this approach is that calling a method that accepted a Db: HasJar wasn't possible when only holding a reference to a &dyn Db or it required repeating the HasJar constraint.

This PR aligns our traits with Salsa by:

  • Introducing a new DbWithJar<Jar> trait. The trait itself is empty, but it requires the type to implement HasJar<Jar> and Database
  • Make DbWithJar a base trait for all the Db traits (SourceDb, SemanticDb, LintDb etc)
  • Change all methods to take db: &dyn SemanticDb as the argument. This simplifies the function signatures a lot

The last change isn't strictly related and maybe I should have done it as a separate PR. However, it removes the
query and mutation methods from the database traits because we can just call the functions directly. This aligns our API with salsa20202.

I've mixed feelings about it. It is nice, because it reduces repetition. However, it does reduce discoverability because you can no longer write
db. to discover all queries/mutations.

Test Plan

cargo build

@MichaReiser MichaReiser added the internal An internal refactor or improvement label May 8, 2024
fn resolve_module(&self, name: ModuleName) -> QueryResult<Option<Module>> {
resolve_module(self, name)
}
impl DbWithJar<SourceJar> for Program {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Creating a Db is now a bit more work but I don't mind that. We should have at most one Db per crate.

Copy link
Contributor

github-actions bot commented May 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser requested a review from carljm May 8, 2024 16:03
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks good!

@MichaReiser MichaReiser merged commit 4541337 into main May 8, 2024
19 checks passed
@MichaReiser MichaReiser deleted the db-with-jar branch May 8, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants