Parse Error on Out of Range Binary Integers

[olabusayoT]

• add checks for signed/unsigned in BinaryIntegerBaseParser and BinaryIntegerBaseUnparser
• add tests for parse and unparse
• update failing tests

DAFFODIL-2297

[stevedlawrence]

This is going to call getBitLength twice, once here and once when super is called. We should probably avoid that.

Also, it looks like we don't have any checks on the max size for numbers like we do in parse. Maybe we should just do all the min/max length checks in the main unparse function like we do with parse? Or maybe in BinaryIntegerBaseUnparser.putNumber?

[olabusayoT]

Hmm, the issue is putNumber doesn't have access to state, which we need for Unparse error and BinaryNumberBaseUnparser is used by non Binary Integer classes

[stevedlawrence]

the issue is putNumber doesn't have access to state

When we call putNumber we actually do pass in the state, it is just upcast to a FormatInfo so putNumber doesn't realize it's actually a UState. But we can change the FormatInfo parameter to UState and it should all just work. I don't think there's a reason putNumber must take a FormatInfo instead of a UState.

BinaryNumberBaseUnparser is used by non Binary Integer classes

Could we put all the width logic in BinaryIntegerBaseUnparser.putNumber(). Then it will only apply to integer types, which I think is exactly what we want?

[mbeckerle]

When there is no access to state, I think the technique is to throw a thin-exception to someplace that catches it that has the state needed to report the error in context properly.

[stevedlawrence]

I'm concerned we will have a lot of regressions due to this change, and wonder if we need a process of deprecating this via tunable. It might be worth running this change with the regression suite and seeing how many things rely on this behavior.

[mbeckerle]

Pretty sure we're going to need the compatibility tunable.

Particularly in Cyberian applications, it's quite likely that users have schemas with signed integers of length 1 bit, since we've been tolerating that, and their application really doesn't care if the values were 0 and -1 or 0 and 1.
But their test cases might have expected infoset XML that assumes that 0 and -1 are the values for a signed integer of length 1.

Our regression test set doesn't include all schemas, and I think we have to assume many others are using DFDL now, and have large sets of schemas. (I recently learned of a group which has 300+ DFDL schemas that we have no access to.)

So we will need a compatibility tunable like "allowSignedIntegerLength1Bit" or something like that. False means we strictly enforce min length 2 for signed integers. "true" means we issue a warning (which can be suppressed by usual means) but implement the current behavior (which I think creates values 0 and -1.

Note that issuing a warning, then assuming the user intended to use an unsigned integer instead would be a change in behavior (producing values 0 and 1, not 0 and -1), so we definitely don't want to do that.

The warning message should suggest converting to use an unsigned integer, and the SDE message if disallowing should also suggest that an unsigned integer is the fix for length 1 bit.

[mbeckerle]

Note that there are obscure corner cases involving charset encodings like "bits" where the number of bits could turn out to be 0 or 1 using lengthKind 'pattern'. Detecting these at runtime only is fine with me. Ex:

<element name="b" type="xs:byte" dfdl:representation="text" dfdl:encoding="bits" dfdl:lengthKind="pattern"
    dfdl:lengthPattern=".{0,1}(?=11111111)" />

That captures any zeros and ones followed by a run of 8 consecutive "1"s.

This is just a corner case, and so long as we don't abort, and if the length turns out to be 0 or 1, we issue an error, then I'm good with it.

[olabusayoT]

There were no regressions in our test suite, but Mike makes a great point, so I'll add in the tunable. Do we want to add in a tunable for zero length unsigned integers too i.e allowZeroLengthUnsignedInteger?

[stevedlawrence]

I would suggest we come up with a single tunable that covers both cases (zero length numbers and 1-bit length signed integers), since this tunable really just wants to be a way to slowly deprecate the current/bad behavior. For now we probably want to default that tunable to the curent/bad behavior, but eventually we'll want to change the default use the correct behavior, while still allowing the old behavior if schemas really do depend on it.

[olabusayoT]

I'm not sure I follow.

>  <element name="b" type="xs:byte" dfdl:representation="text" dfdl:encoding="bits" dfdl:lengthKind="pattern"
>     dfdl:lengthPattern=".{0,1}(?=11111111)" />

In this case, the code changes wouldn't apply since representation is text, not binary. If we have the following element e1

    <xs:element name="e1">
      <xs:complexType>
        <xs:sequence>
          <xs:element name="b" type="xs:byte" dfdl:representation="text" dfdl:encoding="X-DFDL-BITS-LSBF"
                      dfdl:lengthKind="pattern"
                      dfdl:lengthPattern=".{0,1}(?=11111111)" />
          <xs:element name="c" type="xs:string" dfdl:representation="text" dfdl:encoding="X-DFDL-US-ASCII-7-BIT-PACKED"
                      dfdl:lengthKind="explicit" dfdl:length="1" />
        </xs:sequence>
      </xs:complexType>
    </xs:element>

and the following data: 011111111, it parses fine and you get

<ex:e1 xmlns:ex="http://example.com">
  <ex:b>0</ex:b>
  <ex:c></ex:c>
</ex:e1>

[stevedlawrence]

I think you can have representation="binary" with lengthKind="pattern". That might trigger the code path? Note that you probably don't even need the regex lookahead to trigger this. In a bits encoding, dfdl:lengthPattern="." should match just a single bit.

[olabusayoT]

I think you can have representation="binary" with lengthKind="pattern".

So we get an SDE if we try that. "Schema Definition Error: Binary data elements cannot have lengthKind='pattern'."

[stevedlawrence]

I guess I was wrong. Seems like lengthKind="pattern" can't be used to get around the binary length limit.

[mbeckerle]

If you use encoding="bits" it is representation="text" You are treating each bit as a letter/character. It's text.

[mbeckerle]

Pretty sure we're going to need the compatibility tunable.

Particularly in Cyberian applications, it's quite likely that users have schemas with signed integers of length 1 bit, since we've been tolerating that, and their application really doesn't care if the values were 0 and -1 or 0 and 1.
But their test cases might have expected infoset XML that assumes that 0 and -1 are the values for a signed integer of length 1.

Our regression test set doesn't include all schemas, and I think we have to assume many others are using DFDL now, and have large sets of schemas. (I recently learned of a group which has 300+ DFDL schemas that we have no access to.)

So we will need a compatibility tunable like "allowSignedIntegerLength1Bit" or something like that. False means we strictly enforce min length 2 for signed integers. "true" means we issue a warning (which can be suppressed by usual means) but implement the current behavior (which I think creates values 0 and -1.

Note that issuing a warning, then assuming the user intended to use an unsigned integer instead would be a change in behavior (producing values 0 and 1, not 0 and -1), so we definitely don't want to do that.

The warning message should suggest converting to use an unsigned integer, and the SDE message if disallowing should also suggest that an unsigned integer is the fix for length 1 bit.

[mbeckerle]

Note that there are obscure corner cases involving charset encodings like "bits" where the number of bits could turn out to be 0 or 1 using lengthKind 'pattern'. Detecting these at runtime only is fine with me. Ex:

<element name="b" type="xs:byte" dfdl:representation="text" dfdl:encoding="bits" dfdl:lengthKind="pattern"
    dfdl:lengthPattern=".{0,1}(?=11111111)" />

That captures any zeros and ones followed by a run of 8 consecutive "1"s.

This is just a corner case, and so long as we don't abort, and if the length turns out to be 0 or 1, we issue an error, then I'm good with it.

[stevedlawrence]

the issue is putNumber doesn't have access to state

When we call putNumber we actually do pass in the state, it is just upcast to a FormatInfo so putNumber doesn't realize it's actually a UState. But we can change the FormatInfo parameter to UState and it should all just work. I don't think there's a reason putNumber must take a FormatInfo instead of a UState.

BinaryNumberBaseUnparser is used by non Binary Integer classes

Could we put all the width logic in BinaryIntegerBaseUnparser.putNumber(). Then it will only apply to integer types, which I think is exactly what we want?

[stevedlawrence]

I would suggest we come up with a single tunable that covers both cases (zero length numbers and 1-bit length signed integers), since this tunable really just wants to be a way to slowly deprecate the current/bad behavior. For now we probably want to default that tunable to the curent/bad behavior, but eventually we'll want to change the default use the correct behavior, while still allowing the old behavior if schemas really do depend on it.

[stevedlawrence]

I think you can have representation="binary" with lengthKind="pattern". That might trigger the code path? Note that you probably don't even need the regex lookahead to trigger this. In a bits encoding, dfdl:lengthPattern="." should match just a single bit.

[stevedlawrence]

Thoughts on renaming width to maxWidth and adding a new minWidth member in NodeInfo. Avoids some of the special logic around figuring out if stuff is signed or not. And then checking maxWidth and minWidth is very similar logic, just something like

if (primNumeric.minWdith.isDefined) {
   val minWidth = primNumeric.minWidth.get
   if(nBits < minWidth) {
     SDE(...)
   }
}

And then the minWidth can be reused in the parser/unparser logic.

Maybe we also add a signed member so we don't have to have the isSigned/isUnsigned stuff? And then parser/unparsers, which should already have access to the primType can just reference the signed member? We pass around isSigned in a bunch of places, wondering if maybe that can all be simplified.

[stevedlawrence]

isSigned and signedStr aren't used unless we fall into the nBits < minWidth block. Suggest we move this val into that block.

[stevedlawrence]

Does this just want to be a lazy val now that it isn't parameterized?

[stevedlawrence]

Seems this function is only use for numerics, this probably wants to be an assert.impossible with code coverage disabed, or just do e.primType.asInstanceOf[PrimNumeric].isSigned

[stevedlawrence]

Same as above, suggest moving vals into the block where they are used. Avoids unnecessary work (though in this case the work is pretty minimal).

[stevedlawrence]

Is this return false correct? If we are only warning then should we still unparse something? For example, if nBits is 1 and we are signed but allowSignedIntegerLength1Bit, then we should output 1 bit. Seems like we want to warn but then drop through and still put a number.

[olabusayoT]

Hmm, so I have a test that exercises it, and we go from an infoset containing -1 to unparsing 1, since it is only 1 bit. Is that ok?

[stevedlawrence]

I think that's probably correct? If we think about the min/max values of a 2's complement value, the min value is given as -(2^n-1) and the max value is (2^n-1)-1, where n is the number of bits. That gives us a range of -1 to 0. So -1 unparsing to a single 1 bit seems reasonable. Hopefully our parse logic is consistent with that when this tunabled is enabled.

[olabusayoT]

So it looks like we parse data that is -1 to an infoset containing 0, which I think we should be unparsing to 0 instead of 1 here since the max value is 0?

[stevedlawrence]

(previous comment removed, it was wrong)

So it looks like, for parse
0x0 -> 0
0x1 -> 1

And for unparse:
0 -> 0x0
1 -> 0x1
-1 -> 0x1

Other infoset values do not throw an error, so I guess we just aren't doing proper range checking that the value fits in the number of bits?

I'm not sure 2's complement really makes sense with just a single bit, so I guess always parsing/unparse to 0 or 1 is just as logical as something else, and at least it's symmetric.

[mbeckerle]

Truncation of a number because it doesn't fit in the available space is supposed to be an unparse error.

[stevedlawrence]

Just below we have this line:

if (nBits == 0) return // zero length is used for outputValueCalc often.

The purpose of this bug was to make it so zero length numbers is an error. I don't understand why outputValueCalc would have zero length, but based on the bug it sounds like that shouldn't be allowed any more. Should that change to an unparse error?

[olabusayoT]

Above this we have PackedDecimal that also returns if nBits is 0, should we also make that a PE? Also PackedDecimal doesn't pass along decimalSigned to the base, is it logical to do something like the below? If so, they I think we need to update the PackedDecimal classes to pass along decimalSigned

<xs:element name="num" type="xs:decimal"  dfdl:representation="binary" dfdl:binaryNumberRep="ibm4690Packed" dfdl:decimalSigned="yes" dfdl:binaryDecimalVirtualPoint="0"/>

[stevedlawrence]

+1, nice improvements. I think my only comments is do reduce a bunch of copy/pasting, and I think we need different behavior/error messages for packed numbers (which we might already have?)

[stevedlawrence]

Are there any concerns about creating SDW's at parse time? Diagnostic overhead should be small, but imagine an array of a bunch of 1-bit length elements--that could create a lot of diagnostics. Also consider that this same warning was already output during compilation unless nbits was calculated at runtime. Maybe the compile time warning is sufficient and we should just be silent at runtime? Or maybe we need a flag so we only warn here only if we're in the BinaryIntegerKnownLengthUnparser?

[mbeckerle]

I'm in favor of silent success at runtime if the flag is set to allow 1-bit signed (for compatibility), and parse-error if the flag is not set.

[stevedlawrence]

I think this same chunk of code with very minor variations appears in multiple places. Can we add trait to avoid all this duplication?

[stevedlawrence]

I'm not sure this error message is correct. I believe packed numbers have different rules about minimum lengths. It's not the same as 2's complement.

Also, this message references "binary decimal", which I think implies 2's complement. Any error messages here should probably reference packed decimals, or leave it up to individual packed parsers to detect bitLength errors and give specific errors.

I think what was intended with the packed implementation is we just call toBigDecimal/toBigInteger below and the different packed implementations do their parsing/validation and throw an exception if it's not a valid length/value, and that exception is converted into a PE.

So I think all this 1-bit signed stuff done elsewhere doesn't even apply to packed decimals. Packed decimals should already be doing bit length checking specific to the packed format. If they handle the zero case correctly, we could probably even remove this check/PE. If not, we probably still need it, but should have an error specific to zero length packed decimals not allowed.

It might also be worth adding a test for some zero length packed things so we get coverage of this edge case.

[mbeckerle]

"binary decimal" means type xs:decimal dfdl:binaryNumberRep="binary" i.e., twos complement.

A fraction part is specified using dfdl:binaryDecimalVirtualPoint="3" for example to get 999.999 (3 digits after the decimal point)

So "binary decimal" behaves like a twos-complement signed integer vis a vis this 1-bit compatibility mode.

The packed representations are entirely different, and 1-bit stuff does not apply. They must be aligned 4-bit and a multiple of 4-bits length.

[olabusayoT]

It doesn't look like the indvidual packed parsers/unparser handle the nBits == 0 case, Before we were just returning when nBits was 0, should we revert the code, or just update the error to remove the suggestion of what could be used instead, and update the messaging to state nBits == 0 is not allowed.

[mbeckerle]

Well, we may be depending on this zero-length behavior. See the comment in the code about zero length is used by outputValueCalc. But this doesn't really make sense to me. Some things, like strings and hexBinary, can be zero length, but many data types zero length makes no sense. There is defaulting, e.g., when we are trying to parse an integer and we get zero-length back we may provide a default value for the integer. But in the absence of defaulting, most simple types require at least 1 bit.

[olabusayoT]

I'm not sure about that comment. Isn't OVC only used on unparsing? Also, none of our OVC tests failed when we changed the Return to a PE

[mbeckerle]

Yes, OVC is only for unparsing. If no test fail when making this a PE, then I would also delete the comment about OVC, which makes no sense if this is parser code anyway.

[stevedlawrence]

Same question as above, I'm not sure this error makes sense for packed numbers and can possibly be removed?