Use &mut self in invoice updaters, not take-self-return-Self #1331
Conversation
The take-self-return-Self idiom in Rust is substantially less usable than it is in Java, where its more common. Because we have to take self by move, it prevents using the update methods to actually update features, something we occasionally want to do. See, eg, the change in lightning-invoice where we previously had to copy and re-create an entire vec of fields just to update the features field, which is nuts. There are a few places where this makes things a little less clean, but the tradeoff to enable more effecient and broader uses of the update methods seems worth it.
Codecov Report
@@ Coverage Diff @@
## main #1331 +/- ##
==========================================
+ Coverage 90.52% 90.62% +0.10%
==========================================
Files 72 72
Lines 39618 40370 +752
==========================================
+ Hits 35864 36587 +723
- Misses 3754 3783 +29
Continue to review full report at Codecov.
|
| pub fn $optional_setter(mut self) -> Self { | ||
| pub fn $optional_setter(&mut self) { | ||
| <T as $feature>::set_optional_bit(&mut self.flags); | ||
| self | ||
| } | ||
|
|
||
| /// Set this feature as required. | ||
| pub fn $required_setter(mut self) -> Self { | ||
| pub fn $required_setter(&mut self) { | ||
| <T as $feature>::set_required_bit(&mut self.flags); | ||
| self | ||
| } |
There was a problem hiding this comment.
Could we still support the chained-notation by taking a &mut self and returning &Self?
There was a problem hiding this comment.
I'd tried that and realized you still have to declare the variable but you're right, you can at least then be able to chain two setters which is nice. That said, its hard to map that in language bindings, cause we don't have first-class support for references in client languages.
There was a problem hiding this comment.
In addition to the updated mutators, I wonder if we should have a different notation for initialization. Something like:
let features = InvoiceFeatures:from_bits(VariableLengthOnion::required() | PaymentSecret::required());Would still need to do it in such away that it could be checked at compile time, of course.
There was a problem hiding this comment.
Yea, that'd be cool, I'd think we'd have to use macros, tho maybe we can override |, as ugly as that is.
| pub fn $optional_setter(mut self) -> Self { | ||
| pub fn $optional_setter(&mut self) { | ||
| <T as $feature>::set_optional_bit(&mut self.flags); | ||
| self | ||
| } | ||
|
|
||
| /// Set this feature as required. | ||
| pub fn $required_setter(mut self) -> Self { | ||
| pub fn $required_setter(&mut self) { | ||
| <T as $feature>::set_required_bit(&mut self.flags); | ||
| self | ||
| } |
There was a problem hiding this comment.
In addition to the updated mutators, I wonder if we should have a different notation for initialization. Something like:
let features = InvoiceFeatures:from_bits(VariableLengthOnion::required() | PaymentSecret::required());Would still need to do it in such away that it could be checked at compile time, of course.
The take-self-return-Self idiom in Rust is substantially less
usable than it is in Java, where its more common. Because we have
to take self by move, it prevents using the update methods to
actually update features, something we occasionally want to do.
See, eg, the change in lightning-invoice where we previously had
to copy and re-create an entire vec of fields just to update the
features field, which is nuts.
There are a few places where this makes things a little less clean,
but the tradeoff to enable more effecient and broader uses of the
update methods seems worth it.