Display traits don't mask

[bbaldino]

Today I noticed the following code:

let mut x = u1::new(1);
x <<= 1;
println("{x}");

prints 2, which is an invalid value for u1.

I looked at the implementation of ShlAssign and saw:

fn shl_assign(&mut self, rhs: T) {
    *self = self.mask();
    self.0.shl_assign(rhs);
}

I think the mask call should be after the shl_assign call (or, perhaps, in both places)? If that sounds right, happy to open a PR. There are unit tests for ShlAssign, but none involve shifting beyond the type's 'size'.

EDIT: Updated the title of this ticket after discovering the current strategy is to mask on read, so the issue isn't with ShlAssign, but actually with the various display traits. See discussion below.

[bbaldino]

@chrysn @Kijewski does this seem legitimate to you? If so I'll clean up some local changes I have and do a PR.

[chrysn]

The current shl_assign code uses shl(), which already masks the result of the left shift. Can you verify that this is still an issue with the latest git version?

[bbaldino]

On master? Still looks like I pasted above from what I can tell: https://github.com/rust-ux/uX/blob/10819303ffeb8093556bbdd81fca94fb77b58237/src/lib.rs

[chrysn]

My apologies, I was on the pr-niche branch (which would, consequently, fix this issue).

The fix you propose looks good; could you provide a PR?

(If we go through with #49, which I'm generally positive about, it'll be after a breaking version bump, and this would be a good fix to have in 0.1.6).

[chrysn]

A more correct version (consistent with documented behavior) would be to panic when in debug mode, so feel free to address that in the PR as well, but fixing the correctness issue has high enough priority over the debug panic that a fix for it alone would be good just as well.

[bbaldino]

Ah that's interesting, is that a stdlib doc for shlassign? Or something here? I was thinking it'd just silently drop, just like you can shift-assign a top bit "out" of u8, u16, etc.

[chrysn]

Bits are silently shifted out all the time, but shifting by more than the bit width is an error in development mode according to https://doc.rust-lang.org/reference/expressions/operator-expr.html#overflow

[bbaldino]

So as part of digging into this a bit, I noticed it's not straightforward to test, because PartialEq, for example, does masking. Looking around, it looks like many trait implementations do the masking there (on "read"), as opposed to doing the mask on modification (on "write").

I suspect that some trait implementations may be missing the mask call--unfortunately I don't remember which one I was trying to use when I stumbled across this before. I took a look through, though, and did find some:

Display doesn't mask, and will show an "unmasked" value (e.g. a u9 could show a value of 512 when printed). Same for UpperHex, LowerHex, Octal, Binary. I'd think there's value in enforcing that a uX type is always in a "valid" state: i.e. masking should be done on write, not read, but I assume this was done intentionally, so perhaps there was good reason. So perhaps the fix should just to change the display traits mentioned above to mask.

[kjetilkjeka]

I don't really remember the complete picture here. But I assume the inconsistent masking on read/write is due to changing the strategy somewhere along the way.

I guess the two valid options are:

1. Always ensure consistent state on write. Hope the redundant masks will be optimized away when writes are done consecutively in the same function.
2. Accept that leftmost bits cannot be made assumptions on. Mask when it makes a difference (PartialEq, Display, right shift, etc). Ignore it when it doesn't (wrapping mul, left shift, etc).

I'm not sure which one is the correct one to use. Maybe the greatest virtue of (2) is that a buggy implementation will be observed when calling the function itself, and not on different and later function calls. But if there are notable performance differences those should be valued higher as we cannot assume it's not important for such a basic library.

[bbaldino]

@kjetilkjeka thanks for the info. That sounds right, and, fwiw, (2) there is pretty nice. My thought is to make the display trait implementations consistent with the rest (add a call to mask); a potential refactor to shift consistency to "write" could be something to consider in the future.

[bbaldino]

Opened #59

[chrysn]

Musing on this, both mask-on-read and mask-on-write could have their merits. Only when we start handing out guarantees to the compiler on inhabitation we must mask-on-write. It's certainly practical that so far this is an implementation detail we can play with.