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

immutable Universe #27

Merged
merged 3 commits into from
Dec 1, 2024
Merged

immutable Universe #27

merged 3 commits into from
Dec 1, 2024

Conversation

dcz-self
Copy link

Using Queries is currently awkward because creating them requires a &mut Universe. In my case, this means either a RefCell on the database which wraps Universe.

This pushes errors from compile-time to runtime, which is not so great.

I looked into creating the Query, and the only thing the &mut is for is to put additional symbols in the symbols store. This gave me the idea: why not bundle symbols with the Query and call it the UniverseQuery?

struct UniverseQuery {
  symbols: Symbols,
  query: Query,
}

This makes the original Universe obsolete for this query, but the problems with &mut are gone.

In this patch, I decided that the original Universe doesn't have to be copied with every query, so I sketched the type for a lighter SymbolsOverlay:

struct SymbolsOverlay {
  symbols: &SymbolsStore, // lots of symbols
  overlay: SymbolsStore, // only symbols from this query
}

and I stopped there so far.

Does this look like a good direction?

Copy link
Owner

@fatho fatho left a comment

Choose a reason for hiding this comment

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

Definitely makes sense to not pollute the universe with random symbols users might've used in all their queries.

The main thing that's important is that the underlying RuleSet in the universe doesn't get further modified before the query is then actually run, because then you might get conflicts. But your design neatly prevents that, so I think it should be good to go this way.

I commented on a few things I noticed, but I don't see any major blockers.

src/universe.rs Outdated Show resolved Hide resolved
src/search/test.rs Outdated Show resolved Hide resolved
src/textual/parser.rs Outdated Show resolved Hide resolved
@dcz-self dcz-self changed the title WIP: immutable Universe immutable Universe Nov 30, 2024
@dcz-self
Copy link
Author

I removed the WIP, as all feedback has been addressed.

Copy link
Owner

@fatho fatho left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I still have a few smaller comments, but the overall design looks good to me!

BTW, since I noticed some formatting changes (one fixing something cargo fmt would not agree with, but another one that cargo fmt would do differently), I added cargo fmt to the CI checks, so if you could rebase once more?

src/ast.rs Outdated Show resolved Hide resolved
src/resolve/arithmetic.rs Outdated Show resolved Hide resolved
src/universe.rs Outdated Show resolved Hide resolved
src/universe.rs Show resolved Hide resolved
src/universe.rs Show resolved Hide resolved
src/textual/parser.rs Show resolved Hide resolved
dcz added 2 commits November 30, 2024 18:06
This way imports can be conditional on test build.
@dcz-self dcz-self force-pushed the immutable branch 3 times, most recently from 433f6d3 to f497710 Compare November 30, 2024 19:16
src/universe.rs Outdated Show resolved Hide resolved
Added SymbolOverlay which contains changes needed to execute a particular query.
Copy link
Owner

@fatho fatho left a comment

Choose a reason for hiding this comment

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

Awesome!

@fatho fatho merged commit db343a1 into fatho:main Dec 1, 2024
1 check passed
@dcz-self
Copy link
Author

dcz-self commented Dec 1, 2024

Let me know when a new release is out, I want to incorporate those APIs in my own release.

@fatho
Copy link
Owner

fatho commented Dec 1, 2024

Let me know when a new release is out, I want to incorporate those APIs in my own release.

This PR and the others are now available as v0.4.0 on crates.io.

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.

2 participants