Remove incorrect implementation of `Clone` for `Insn<'capstone>`

[danielhenrymantilla]

Fixes #121.

EDIT: superseded by #123 !

Problem

The Instructions<'capstone> owned a heap-allocated [Insn<'capstone>] (imagine a CsBox<[Insn<'capstone>]>), which thus involves owning the backing storage for the Insns themselves, as well as freeing whatever each Insn<'capstone> owns.

• What does a Insn<'capstone> own? It turns out that as written, currently, it technically doesn't own anything, even if, semantically, it's owning kind of a Option<CsBox<cs_detail>>. That is, when its detail field is a non-NULL pointer, when the CsBox<[Insns<'capstone>]> is dropped, such non-NULL pointers are cs_freed as well.

  So a Insn<'capstone>, conceptually, owns a Box-like-ish field. Mainly, a non-Copy field!

• and yet Insn<'capstone> implements Clone, by deriving it from the raw C field it wraps (which is Copy).

The only way doing that kind of thing could be fine would be if Insn<'capstone> were, actually, a Insn<'instructions>

• Rant: should the lifetimes in the codebase have been properly named, this issue could have been identified and debugged more easily. As of now, we just have a gigantic soup of 'as almost everywhere (sadly a code style, or rather, lack thereof, that the Rust book teaches), and the rare cases where there is no 'a, it's even worse: we get elided_lifetimes_lifetimes_in_paths (which ought to be denied, as a Rust idiom since 2018).

  This, combined with unsafe code and these subtle ownership things, lead to this kind of situations, where we end up with an Insn<'a> type that some code treats as owning its detail fields (since it can outlive the Instructions<'capstone> it is derived from), and other code treats it as a borrow of Instructions<'capstone> (the Clone(-and-thus-Copy) semantics it currently has).

So either the lifetime signatures need to change to express that we have a Insn<'instructions> ("treat it as a borrow"), or the Clone needs to go ("treat is an owned thing").

Since the former would require getting rid of those handy Deref and AsRef impls (Deref / AsRef have a very limited, lifetime-wise, signature, that does not allow properly expressing the borrowing semantics desired here), I think it is just better to get rid of the latter.

[danielhenrymantilla]

@clubby789 has mentioned that since a cs_detail has a publicly-known layout, we could still feature Clone by performing a manual alloc-and-bitcopy pattern.

I can amend the current PR so that it does that instead.

EDIT: #123 seems to feature that: that PR supersedes the current one

[codecov[bot]]

Codecov Report

Merging #122 (2aaa270) into master (fa37a37) will decrease coverage by 0.30%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
- Coverage   94.90%   94.59%   -0.31%     
==========================================
  Files          22       22              
  Lines        2670     2666       -4     
  Branches        0     2627    +2627     
==========================================
- Hits         2534     2522      -12     
- Misses        136      144       +8     

Impacted Files	Coverage Δ
capstone-rs/src/instruction.rs	93.81% <ø> (ø)
capstone-rs/src/error.rs	59.52% <0.00%> (-5.48%)	:arrow_down:
capstone-rs/examples/parallel.rs	95.23% <0.00%> (-4.77%)	:arrow_down:
capstone-rs/examples/demo.rs	92.30% <0.00%> (-2.04%)	:arrow_down:
capstone-rs/src/arch/m680x.rs	95.93% <0.00%> (-0.74%)	:arrow_down:
capstone-rs/src/arch/riscv.rs	89.47% <0.00%> (-0.53%)	:arrow_down:
capstone-rs/src/constants.rs	78.82% <0.00%> (-0.49%)	:arrow_down:
capstone-rs/src/arch/sparc.rs	90.90% <0.00%> (-0.40%)	:arrow_down:
capstone-rs/src/arch/xcore.rs	92.00% <0.00%> (-0.31%)	:arrow_down:
capstone-rs/src/test.rs	97.59% <0.00%> (-0.19%)	:arrow_down:
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fa37a37...2aaa270. Read the comment docs.

[tmfink]

Thanks for the PR! I like the simplicity. :+1:

If #123 ends up not panning out, we can always pivot back to this PR. :wink:

[tmfink]

To solve the immediate soundness issue, I merged this PR while #123 is under review.