Bugfix | Fixes not operator creating invalid types

[BenLeadbetter]

The Issue

The implementation of the Not operator has a bug where it creates types with invalid representations, which in turn leads to panics when the type is used.

For example.

let _ = !u4(0b0101); // this u4 has a value of u4(0b1111_1010) -> invalid!

Using such an invalid type will result in a panic:

let _ = !u4(5) + u4(1);
// -> assertion failed: Self::MAX.0 - other.0 >= self.0

The unit tests did not catch this one because the equality PartialEq implementation ignores the bits in the representation which lie outside the type mask.

The Fix

The not operator should ensure not to flip the bits of the underlying value which lie outside of the type mask.

[bbaldino]

Nice find...this is actually related to some discussion I'd brought up a while back (here) when running into something similar.

I think this again brings up whether or not masking should be done on write or on read/whenever it's necessary. I believe the current thinking (though maybe not entirely consistent, as mentioned in the ticket linked above) is that the value should be masked when accessing it (i.e. don't assume it's already masked correctly) so here the issue is in the Add impl: it's assuming that all masking is being done on "write", so it's asserting that it's already correct.

In my opinion I think we should probably make it such that the internal value is always "correct" (i.e. masking should be done on "write"). There are some pros listed in that ticket about mask-on-read, but I have a feeling we'll continue to bump into situations like this, and upholding the invariant that the held value is always correct seems like a bigger win.

But because not returns a new value, I think we need this change regardless though.

[BenLeadbetter]

Interesting idea with the "mask on read" approach. I guess the performance benefits might be worth considering. It might make inspection with the debugger (where you can see the representation value) a bit confusing, though.

I could have a go at changing this assertion in the Add implementation, if we think this approach is preferred. It would be good to add some test coverage which captures this "mask on read" design philosophy too.

In this particular case there is no performance change, I think. We aren't adding any operations, just changing the order of existing operations. I guess if we commit to "mask on read" then we can remove the mask call in Not altogether(?).

[bbaldino]

@BenLeadbetter sorry for the slow reply. I'm trying to spend some time on Ux and taking a look at doing a mask-on-write approach. Let me try and take a shot at that before we move forward here.

[BenLeadbetter]

All good. I'm keen to see the changes

[bbaldino]

@BenLeadbetter have a draft of mask-on-write up in #65. I tested the case that you included in your description and that no longer panics.

[BenLeadbetter]

Thanks! I'll try out your branch against my crate

[bbaldino]

@BenLeadbetter the mask-on-write is now merged and, as far as I know, this issue should be fixed. Closing but please re-open if you still see an issue. We're working on getting a release pushed with this fix.