Handle case when `Bytes` appends to `self`
Description
When calling self.append(self) for Bytes, we experienced undefined behavior as the logic would clear self. Now if this case is encountered, the function will revert.
Checklist
- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have requested support from the DevRel team
- [x] I have added tests that prove my fix is effective or that my feature works.
- [x] I have added (or requested a maintainer to add) the necessary
Breaking*orNew Featurelabels where relevant. - [x] I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
- [x] I have requested a review from the relevant team or maintainers.
This is not the behaviour id expect personally. Id expect this to duplicate the array. eg psuedocode
let a = [1,2,3]
let b = a.append(a)
assert( b == [1,2,3,1,2,3])
I think there's ambiguity here since appending should both add the elements to the slice it's called on and consume the slice argument it's called with. The combination of these two does lead to the identity function, but it is a surprising result.
Rust gets around this problem by making it simply illegal to append onto oneself since that would be a double mutable borrow.
C++ is the more similar memory model to us and this:
#include <string>
int main() {
std::string str = "abc";
str.append(str);
printf("%s\n", str.c_str());
return 0;
}
Does print abcabc.
I'm with @SwayStar123 here that doubling the bytes is the more intuitive result. Though it bares mention in the docs. And we should be clear about what the exact semantics are here.
I think there's ambiguity here since appending should both add the elements to the slice it's called on and consume the slice argument it's called with. The combination of these two does lead to the identity function, but it is a surprising result.
Rust gets around this problem by making it simply illegal to append onto oneself since that would be a double mutable borrow.
C++ is the more similar memory model to us and this:
#include <string> int main() { std::string str = "abc"; str.append(str); printf("%s\n", str.c_str()); return 0; }Does print
abcabc.I'm with @SwayStar123 here that doubling the bytes is the more intuitive result. Though it bares mention in the docs. And we should be clear about what the exact semantics are here.
I think I would prefer Rust's approach here and make it illegal.
I agree with @bitzoic that we should follow Rust's approach for append. The addition of a concat method could be valuable as a follow up PR
Making it illegal is not reasonable without a borrow checker. We can't reliably check that you are trying to mutate the same memory twice at once without heavy annotations.
Sway is like C++ in that way.
We could just panic, but that seems a lot less coherent that picking a correct output for the reflexive call and sticking to it.
We can't reliably check that you are trying to mutate the same memory twice at once without heavy annotations.
This is what line 696 does.
https://github.com/FuelLabs/sway/blob/2776aff6e2bfb28728b1babe45bc86f5faa76da1/sway-lib-std/src/bytes.sw#L696
We could just panic, but that seems a lot less coherent that picking a correct output for the reflexive call and sticking to it.
This is what line 697 does
https://github.com/FuelLabs/sway/blob/2776aff6e2bfb28728b1babe45bc86f5faa76da1/sway-lib-std/src/bytes.sw#L697