Error in validation for BitVector32

[oznetmaster]

It appears that there is an error in the code for the validation of a new Section in BitVector32.

The code:

        private static Section CreateSectionHelper(short maxValue, short priorMask, short priorOffset)
        {
            if (maxValue < 1)
            {
                throw new ArgumentException(SR.Format(SR.Argument_InvalidValue_TooSmall, nameof(maxValue), 1), nameof(maxValue));
            }

            short offset = (short)(priorOffset + CountBitsSet(priorMask));
            if (offset >= 32)
            {
                throw new InvalidOperationException(SR.BitVectorFull);
            }
            return new Section(CreateMaskFromHighValue(maxValue), offset);
        }

creates a new section, given the previous section and the maxValue for the new section.

However, the lines:

            short offset = (short)(priorOffset + CountBitsSet(priorMask));
            if (offset >= 32)
            {
                throw new InvalidOperationException(SR.BitVectorFull);
            }

test for the previous section being invalid, not the new one being created.

It would seem like the code should be:

            short offset = (short)(priorOffset + CountBitsSet(priorMask));
            if (offset + CountBitsSet (CreateMaskFromHighValue(maxValue)) >= 32)
            {
                throw new InvalidOperationException(SR.BitVectorFull);
            }

Obviously, there is better ways to actually write the code. I am just demonstrating the issue.

The documentation states that the InvalidOperationException should be thrown if "maxValue is greater than the highest value that can be represented by the number of bits after previous."

[oznetmaster]

It appears that my suggested code is wrong :( The test should be > 32, not >= 32.

[tannergooding]

@eiriktsarpalis @layomia, would this fall under System.Collections? I'm not sure if this was mislabeled or not...

The issue does look like a bug:

var section1 = BitVector32.CreateSection(short.MaxValue);           // 0x0FFF, 0x00     0b0000_0000_0000_0000_0000_1111_1111_1111   Bits 00 to 11
var section2 = BitVector32.CreateSection(short.MaxValue, section1); // 0x0FFF, 0x0C     0b0000_0000_1111_1111_1111_0000_0000_0000   Bits 12 to 23
var section3 = BitVector32.CreateSection(short.MaxValue, section2); // 0x0FFF, 0x18     0b1111_1111_0000_0000_0000_0000_0000_0000   Bits 24 to 35!

[eiriktsarpalis]

Yes, looks like it. Relabeling..

[huoyaoyuan]

There is a test expecting highest section to be truncated:

runtime/src/libraries/System.Collections.Specialized/tests/BitVector32Tests.cs

Lines 388 to 405 in a822d39

	[Theory]
	[InlineData(4)]
	[InlineData(5)]
	[InlineData(short.MaxValue)]
	[InlineData(byte.MaxValue)]
	// Regardless of mask size, values will be truncated if they hang off the end
	public static void Set_Section_OverflowTest(int value)
	{
	BitVector32 data = new BitVector32();
	BitVector32.Section offset = BitVector32.CreateSection(short.MaxValue, BitVector32.CreateSection(short.MaxValue));
	BitVector32.Section final = BitVector32.CreateSection(short.MaxValue, offset);
	data[final] = value;
	Assert.Equal((3 & value) << 30, data.Data);
	Assert.NotEqual(value, data.Data);
	Assert.Equal(3 & value, data[final]);
	Assert.NotEqual(value, data[final]);
	}

And the exception documentation implies that:
https://docs.microsoft.com/en-us/dotnet/api/system.collections.specialized.bitvector32.createsection?view=net-5.0

previous includes the final bit in the BitVector32.
-or-
maxValue is greater than the highest value that can be represented by the number of bits after previous.

What should we do here? @eiriktsarpalis

[eiriktsarpalis]

I suppose we could close the issue as by-design, but the behavior as-is still bothers me.

cc @Clockwork-Muse

[Clockwork-Muse]

... I tried to fix this a long time ago, dotnet/corefx#2920 , but was told not to bother since it was a legacy type, so wrote the test as documentation of as-current behavior (although I left the existing validation alone, for some reason).

[eiriktsarpalis]

Thanks for the context. In that case I'm going to close this issue as by-design.