Repository navigation
Fix separator not backtracked when complex-typed array element fails - #1716
Conversation
dcdf762 to
4eeab17
Compare
b499806 to
80f9b07
Compare
943373e to
f7f1dfc
Compare
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1, though there seems to be some duplication and doing things in new ways that we already have patterns for
| val savedStatus = processorStatus | ||
| val savedDiagnostics = diagnostics | ||
| val savedDelimitedParseResult = delimitedParseResult | ||
| val savedDataProc = dataProc |
There was a problem hiding this comment.
Should we just create a mark and restore the mark? This feels questionalbe that it's only saving some specific parts of the state without any real explanation why those parts need to be saved/restored and not others. Seems like using marks is the standard way we do this kind of thing.
| val priorInfosetLastChild = pstate.infosetLastChild | ||
| val priorElementState: DIElementSharedInterface = | ||
| if (priorElement.isSimple) DISimpleState() else DIComplexState() | ||
| if (maybePoU.isDefined) priorElementState.captureFrom(priorElement) |
There was a problem hiding this comment.
Does this do anything? It looks like this is capturing state but never doing anything with it.
| * Used to determine whether a representation at this position is | ||
| * genuinely zero-length, independent of any field-content DFA. | ||
| */ | ||
| object ZeroLengthDelimiterScanner { |
There was a problem hiding this comment.
It doesn't really feel like this is specific to zero length delimiter scanning? In fact, it looks very very similar to the dfa.TextParser class. It seems the only difference is that this assumes the passed in delimiters match zero length and the field it returns in the ParseResult is empty string.
Is it possible for ZeroLengthComplexTypeDelimitedParser to use daf.TextParser instead?
There was a problem hiding this comment.
+1 with a question
The changes in how the separator is handled results in different behavior. Namely previous behavior would accept some data that will now result in an error. This doesn't seem to be a problem for our built in tests, but has this been run against the full regression suite?
Also it may make some error messages a little confusing. For example:
<xs:sequence dfdl:separator="," dfdl:separatorSuppressionPolicy="trailingEmpty">
<xs:element name="a" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
<xs:element name="b" type="xs:int" minOccurs="0" dfdl:lengthKind="explicit" dfdl:length="1"/>
<xs:element name="c" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
</xs:sequence>When given "x,Z,y" as input previously would have resulted in
<a>x</a><c>Z</c>as element b would have attempted to parse Z as an int and failed, and would then allow element c to parse the Z, resulting in a left over data error. Now this will result in a ParseError stating that a non-trailing optional occurrence that is absent must keep its separator in a positional separated sequence.
To be fair, neither error message is great, just something to consider.
| try { | ||
| probeImpl(pstate) | ||
| } finally { | ||
| pstate.resetFormatInfoCaches() |
There was a problem hiding this comment.
Should this pstate.resetFormatInfoCaches() call be part of probeImpl? It seems like most of the reset code is in that function so it seemed strange that this one isn't.
There was a problem hiding this comment.
I wonder if this even belongs in Parser.scala. I'm not sure probe is a thing we'll commonly need, it's very specific to the needs of separators so feels like it belongs in the separator code. I'm not sure we want a world were we commonly probe generic parsers. Especially since this is doing things specific to delimteirs like saving delimitedParserEResult
Yes, it was run against the full regression suite, and only one schema fell out...I believe Syslog, but this was due to a bug in Daffodil that was reporting "separator not found" error when a required element failed to parse a alpha character as an int. So the fix would be to just remove the errorneous "separator not found" since the separator did exist for that element, and it can report the accurate "unable to parse...xs:int" instead.
I verified that this would fail with the new branch, which i think is more correct than what main was doing before. Per the spec, b (when processing Z) is neither nil, empty or absent (since it has a value: Z) but because Z is not an int, that should result in a parse error rather than the failed b being treated as having never occurred and the parse giving Z to c. @stevedlawrence thoughts? |
|
Yeah, I think that is correct. Element I think the data would need to indicate b does not exist like this: Also, if it were instead That said, I'm not positive. This part of the spec gets complicated to follow all the terminology and different cases, but that's my interpretation. |
c8fc929 to
967a991
Compare
…d sequences
Separated sequences mishandled optional occurrences whose representation was
absent or zero-length. A separator could be silently reused by the next
sibling, kept when it belonged to a backtracked occurrence, or never checked
at all. This commit makes the parse outcome follow the DFDL spec (9.2.4, 14.2.1).
- Term.couldBePotentiallyTrailing no longer excludes elements carrying a
discriminator, so they are classified under 'never', 'trailingEmpty' and
'trailingEmptyStrict' like every other element (SequenceChild.scala).
- A potentially-trailing occurrence that failed on real content and
backtracked its separator is detected when the next sibling succeeds.
arrayCompleteChecks uses priorSiblingResultOfTry, which survives a
repeating child's inner loop, and ignores a multi-occurrence sibling whose
final probe is expected to fail.
- A non-trailing optional occurrence that omits its separator is rejected
("must keep its separator"). trailingEmptyStrict also sees a prior optional
sibling that consumed its separator, so prefix, infix and postfix
sequences reject a trailing empty occurrence.
Absent versus missing
- computeParseAttemptStatus now receives separatorWasFound. For a
non-array optional element, a failed zero-length attempt whose separator
was found is AbsentRep. For NonPositional (anyEmpty) sequences the
separator parse helper probes for another separator and, if none follows,
releases the occurrence as MissingItem for the next item to claim.
- Complex-typed elements no longer trust bit-position bookkeeping alone to
decide zero length, which nested backtracking makes unreliable.
ZeroLengthComplexTypeDelimitedParser, wired through
ZeroLengthComplexTypeDelimiterScanner, peeks without consuming for an
in-scope delimiter where the content would begin.
- A postfix array occurrence that ends its array leaves its separator for
the enclosing group; other postfix optionals with a found separator are
AbsentRep.
- A discriminator that resolves the point of uncertainty on an absent attempt
keeps the occurrence as empty, or fails the parse if the attempt failed,
instead of aborting the engine.
DAFFODIL-3094
Deprecation/Compatibility
Discriminators are not used when unparsing and the potentially trailing
definition does not exclude them, so an optional element with a discriminator
is now potentially trailing. Under trailingEmpty and trailingEmptyStrict its
trailing zero-length occurrence is now suppressed with its separator when
unparsing, where it was previously written. Schemas that relied on the old
output must be updated.
967a991 to
53481a2
Compare
For a maxOccurs-bounded occursCountKind="implicit" array whose element type is complex with no possible zero-length representation (e.g. every branch of a choice requires a distinct non-empty initiator), the speculative attempt at an occurrence beyond the real data would parse its preceding separator successfully, then fail the content entirely with zero bits consumed. anyTypeElementFailedParseAttemptStatus classified this as AbsentRep, whose contract is to retain the bit position (keep the separator consumed); that is correct for a genuinely empty representation, but wrong here since there is no occurrence at all. This let the separator leak into whatever sequence child comes next, causing spurious "separator not found" / "terminator not found" errors.
Gate the AbsentRep classification on isEmptyRepZeroLength for complex types, so a structurally-impossible zero-length failure returns MissingItem instead, which forces the caller to fully backtrack the point of uncertainty including the separator.
DAFFODIL-3094