Pauan / rust-signals

Zero-cost functional reactive Signals for Rust
MIT License
677 stars 37 forks source link

Mutable: Modify values in place #52

Closed uklotzde closed 2 years ago

uklotzde commented 2 years ago

See https://github.com/tokio-rs/tokio/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Pauan commented 2 years ago

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 commented 2 years ago

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

Pauan commented 2 years ago

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 commented 2 years ago

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

Pauan commented 2 years ago

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 commented 2 years ago

I tried to explain it.

Pauan commented 2 years ago

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