sway icon indicating copy to clipboard operation
sway copied to clipboard

Handle case when `Bytes` appends to `self`

Open bitzoic opened this issue 1 year ago • 6 comments

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).
  • [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* or New Feature labels 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.

bitzoic avatar Jul 29 '24 11:07 bitzoic

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])

SwayStar123 avatar Jul 29 '24 13:07 SwayStar123

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.

IGI-111 avatar Jul 30 '24 02:07 IGI-111

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.

bitzoic avatar Jul 30 '24 06:07 bitzoic

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

K1-R1 avatar Jul 30 '24 17:07 K1-R1

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.

IGI-111 avatar Aug 02 '24 02:08 IGI-111

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

bitzoic avatar Aug 02 '24 07:08 bitzoic