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

Mutable: Modify values in place #52

Closed
wants to merge 2 commits into from

Conversation

uklotzde
Copy link

@uklotzde uklotzde commented Apr 3, 2022

See tokio-rs/tokio#4591 for a discussion why this is useful and mandatory for implementing certain FRP use cases.

Currently, I am using std::tokio::watch that I consider more mature than this crate. But I would actually prefer using a dedicated crate with less dependencies for this purpose.

@Pauan
Copy link
Owner

Pauan commented Apr 3, 2022

Currently, I am using std::tokio::watch that I consider more mature than this crate.

I'm curious why you consider that. futures-signals has been around for years, it's an extremely solid design, excellent performance, and very stable. I created futures-signals based on my many years of expertise creating FRP systems.

As for your feature request, that's already possible:

let mut lock = mutable.lock_mut();

if some_condition(lock) {
   *lock = new_value;
}

This will only trigger a change if some_condition(lock) is true.

This is quite a lot more convenient and flexible than your approach.

It also works correctly with &mut self methods, e.g.lock.some_mut_method() will trigger a change. However &self methods will not trigger a change.

You simply use the lock as normal (it auto-derefs to the Mutable's value), and it will automatically determine if you used the lock mutably or not. If you used it mutably, then it will trigger a change, otherwise it doesn't.

@uklotzde
Copy link
Author

uklotzde commented Apr 3, 2022

Your proposed approach does not allow to invoke a &mut self function on the inner value without marking it as dirty. Many update functions require &mut self although they do not always result in modifying the inner value.

Borrowing mutably is a low-level syntactic concept while modification is a high-level semantic concept that involves business logic. Only the invoked function could provide the information if it has actually modified the value in-place or not.

@Pauan
Copy link
Owner

Pauan commented Apr 3, 2022

Your proposed approach does not allow to invoke a &mut self function on the inner value without marking it as dirty.

That's generally a good thing. It should be difficult to miss updates, because that results in very hard-to-debug problems. Your approach makes it very easy to accidentally miss an update, which leads to bugs.

It's better to have too many updates rather than too few, since the former causes performance problems but the latter causes behavior bugs. And it's always possible for the downstream consumer to use things like dedupe in order to avoid spurious updates.

However, you are right that sometimes the user wants to intentionally avoid triggering an update even when mutating the value. This should be possible, but not by accident. Something like this should be a good API:

let mut lock = mutable.lock_mut();

if some_condition(lock) {
   *lock = new_value;
}

MutableLockMut::mark_no_change(&mut lock);

The exact name can be bikeshedded.

@uklotzde
Copy link
Author

uklotzde commented Apr 3, 2022

Manually resetting the mutated flag before dropping the lock guard would also work as an alternative approach. It is just less convenient and requires more statements instead of a single and concise expression. I don't consider it more safe.

@uklotzde
Copy link
Author

uklotzde commented Apr 3, 2022

Btw, dedupe() is for a completely different use case.

@Pauan
Copy link
Owner

Pauan commented Apr 3, 2022

It is just less convenient and requires more statements instead of a single and concise expression.

Yes, it should be less convenient, because it is an uncommon use case, and it's a footgun which can easily cause bugs. Rust's design philosophy is to avoid bugs and footguns.

Also, it's not necessarily more statements: it only needs 1 statement at the end, whereas with your approach you need minimum 2 statements (one for true and one for false).

If you are concerned about spurious updates, you should be using something like dedupe anyways: my code uses dedupe frequently.

Btw, dedupe() is for a completely different use case.

Is it? The point of dedupe is that it will only trigger a change if the value actually changed. Maybe you could explain more about what exactly you are trying to do.

@uklotzde
Copy link
Author

uklotzde commented Apr 3, 2022

My use case involves non-primitive, non-comparable values, i.e. complex state that is mutated only partially.

@Pauan
Copy link
Owner

Pauan commented Apr 3, 2022

My use case involves non-primitive, non-comparable values, i.e. complex state that is mutated only partially.

I had been planning on implementing a dedupe_if method which would allow you to choose how to do the deduping.

However, that's not really what I'm asking: I need to better understand your state and how you're using it, since there might be a better way of doing things.

I have written large apps with Signals, and I've never once felt the need for what you're asking for, so either you're doing something quite unusual (and I want to understand it so I can support it), or there's a different way of structuring your code.

@uklotzde
Copy link
Author

uklotzde commented Apr 3, 2022

As you seem to focus on different use cases I will close this PR.

@uklotzde uklotzde closed this Apr 3, 2022
@Pauan
Copy link
Owner

Pauan commented Apr 3, 2022

That's not at all what I'm saying, I'm asking because I want to understand your use case, so I can improve futures-signals.

@uklotzde
Copy link
Author

uklotzde commented Apr 3, 2022

I tried to explain it.

@Pauan
Copy link
Owner

Pauan commented Apr 3, 2022

Yes, but I need more details: like your type definitions, how you are consuming the Signals, what your app is doing, etc.

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.

None yet

2 participants