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

SymbolsOverlay: derive Clone #33

Merged
merged 1 commit into from
Dec 13, 2024
Merged

SymbolsOverlay: derive Clone #33

merged 1 commit into from
Dec 13, 2024

Conversation

dcz-self
Copy link

@dcz-self dcz-self commented Dec 8, 2024

This makes creating an ArithmeticResoler with an immutable Overlay possilbe

This makes creating an ArithmeticResoler with an immutable Overlay possilbe
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.

Clone seems nice to have in general where it makes sense, but I don't quite understand your concrete use case. Could you give an example of what you mean?

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

I think it's more of an artifact of a not thought through design on my part.

I have a static Solutions type that is immutable:

pub struct Solutions<'a> {
    universe: &'a TextualUniverse,
    query: UniverseQuery<'a>,
}

Obviously, I want to get the solutions, so there's the iter method:

/// Iterate over the solutions
    pub fn iter(&'a self) -> impl Iterator<Item=Solution<'a>> {
        use logru::search::query_dfs;

        let names = self.query.query().scope.as_ref().unwrap();
        let resolver = ArithmeticResolver::new(&mut self.query.symbols().clone())
            .or_else(self.universe.resolver());
[...]

ArithmeticResolver mutates a query, so I must clone the reference I have just to bootstrap the resolver.

If I try to store the resolver beforehand, then I need to clone the Resolver:

pub struct Solutions<'a> {
    universe: &'a TextualUniverse,
    resolver: OrElse<ArithmeticResolver, RuleResolver<'a>>,
    query: UniverseQuery<'a>,
}

impl [...] {
    /// Iterate over the solutions
    pub fn iter(&'a self) -> impl Iterator<Item=Solution<'a>> {
        use logru::search::query_dfs;

        let names = self.query.query().scope.as_ref().unwrap();
        query_dfs(self.resolver.clone(), &self.query.query())
[...]
}

Actually, this solution is more elegant (avoids a hack I haven't showed), so I'll submit Clone for the Resolvers as well.

@fatho
Copy link
Owner

fatho commented Dec 13, 2024

I see. I think that you need to resort to this is more of a limitation of my initial design of making resolver functions take &mut self.

My idea with resolvers, like the ArithmeticResolver was that one would bootstrap it once, using the base SymbolStore of the universe, and then reuse it for every query. But that doesn't work so well when you have multiple queries you want to run independently.

Back then I thought, that a resolver might want to provide side-effects. But that is probably more of a niche usage. So perhaps it'd be better to make the resolver functions take &self. Then you could initialize it once based on the underlying SymbolStore, and then re-use it for all queries. That would also ensure that the symbols it reserves actually match, which could become quite confusing otherwise.

If there's ever a need for a resolver with side-effects requiring mut, that can probably best be implemented using interior mutability.

@dcz-self dcz-self mentioned this pull request Dec 13, 2024
@dcz-self
Copy link
Author

I don't have a vision of what side effects a resolver should have, so I just followed my default assumption that all operations are immutable. That doesn't mean that anything I do actually benefits from this, I only designed my API this way.
I think adding a .clone() here and there is a decent, easy workaround for the time being. I'm not going to attempt to change the mutability story without understanding possible other use cases.

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