Repository navigation
Fix separator not backtracked when complex-typed array element fails - #1716
olabusayoT wants to merge 13 commits into
Conversation
dcdf762 to
4eeab17
Compare
b499806 to
80f9b07
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. Added tests to exercise. DAFFODIL-3094
…backtracking The prior fix gated AbsentRep vs MissingItem on the static isEmptyRepZeroLength flag, but that only describes whether some successful parse of the type could be zero-length, not whether this particular failed attempt was genuinely empty. A choice whose real branch matches part of its content then fails nets zero bits once backed out, indistinguishable from a genuine empty match by bit position alone. Track this via pointOfUncertaintyResetCount, which counts resets that actually undo bit position movement, and a mark stack letting a sequence child's attempt check, via hadPointOfUncertaintyResetSinceMark, whether such a reset happened during it. Added tests for both cases. DAFFODIL-3094
…tion for complex array elements The Optional+zero-length branch of anyTypeElementFailedParseAttemptStatus unconditionally returned AbsentRep on a failed parse attempt. AbsentRep's caller-side handling backs out infoset side effects then re-jumps forward past whatever was consumed, retaining any separator found along the way. That is correct for the documented simple-type idiom (adjacent delimiters of an enclosing group indicating a genuinely empty positional slot), but wrong for complex types: per DFDL Spec 9.3.2.2, a complex type's empty or absent representation can only be established by a descent that returns successfully, never by one that fails. Per 9.2.4/9.2.6, Absent is the narrower, positionally-locatable subset of the broader Missing case; a failed complex-type descent belongs to Missing, not Absent. The prior fix (4eeab17) tried to patch this with a static isEmptyRepZeroLength check and, when that proved insufficient, a PoU-resolve-count runtime signal; both are reverted here in favor of an unconditional type check, gating AbsentRep to erd.isSimpleType and letting all complex-type failures fall through to the existing MissingItem/FailureUnspecified handling, which performs a genuine full backtrack. Also updates the emptyElementDetectionAssertion idiom across SepTests.tdml to the bare pattern-discriminator form used in production schemas. DAFFODIL-3094
Removes the erd.isSimpleType restriction added by 485fc8b (gating AbsentRep on a failed zero-length attempt to simple types only) and restores the original comment, making this method byte-identical to its state before any DAFFODIL-3094 work. The complex-type separator-retention problem this guarded against needs a different fix, built at the caller (SeparatedParseHelper's separator-handling code) instead of baking a type check into this shared classify method, which should stay untouched so its behavior is easy to reason about independently of caller-side verification logic. DAFFODIL-3094
… sequences Complex-typed elements determine zero length from bit-position bookkeeping alone, which nested backtracking makes unreliable. Adds a non-consuming scanner (ZeroLengthDelimiterScanner/ZeroLengthComplexTypeDelimitedParser) that checks for a genuine in-scope delimiter where such an element's content would begin, consulted on the failure path. A potentially-trailing occurrence can fail on real content, backtracking its whole attempt including its separator; if the next sibling then succeeds, that separator was silently reused. Adds a check to arrayCompleteChecks (previously only for separatorSuppressionPolicy='never') via a new priorSiblingResultOfTry that survives a repeating child's own inner loop, left Uninitialized for a multi-occurrence sibling whose final probe is expected to fail. separatorSuppressionPolicy='anyEmpty' also requires a zero-length occurrence to omit its separator; a matching check uses a new pstate.lastSeparatorWasFound, gated off occursCountKind 'parsed'/'stopValue'. Updated tests. DAFFODIL-3094
…ling occurrences under 'never', 'trailingEmpty', and 'trailingEmptyStrict'. Term.scala's couldBePotentiallyTrailing excluded any element carrying its own discriminator, so such elements fell back to plain Positional classification, silently skipping the separator-suppression check entirely regardless of policy. SequenceChild.scala now classifies these as PositionalNever/PositionalTrailingLax/PositionalTrailingStrict too, matching how potentially-trailing elements under the same policies are already handled, and restoring the intended enforcement for discriminator-bearing elements. Also removes the anyEmpty-specific arrayCompleteChecks check and its supporting plumbing (lastSeparatorWasFound/lastAttemptSeparatorWasFound, isAnyEmptySeparatorSuppressionPolicy), since it was based on a misreading of the spec's permissive "may be omitted" wording for 'anyEmpty', unlike the "must" wording for 'trailingEmptyStrict'. Adds test_sep_ssp_any_5 through _8 and updates never_/strict_ expectations to match corrected behavior. Also fixes NonPositional (anyEmpty) failed zero-length occurrences always classifying as MissingItem, discarding a separator that genuinely belonged to them; this broke real-world schemas mixing skipped optional elements with real separators. computeParseAttemptStatus now threads a separatorWasFound flag, and returns AbsentRep instead when another separator immediately follows (peeked via the new PState.probeNonDestructively), scoped to non-array occurrences since a repeating array's own loop can claim further separators itself. Also fixes an engine abort when a discriminator resolves the PoU on the same attempt that classifies AbsentRep, and corrects a wrong expected value in test_sep_ssp_any_7's fixture. DAFFODIL-3094
…parated sequences
…ly-trailing occurrences under 'never', 'trailingEmpty', and 'trailingEmptyStrict'.
…ly-trailing occurrences under 'never', 'trailingEmpty', and 'trailingEmptyStrict'.
…ly-trailing occurrences under 'never', 'trailingEmpty', and 'trailingEmptyStrict'.
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?
…ly-trailing occurrences under 'never', 'trailingEmpty', and 'trailingEmptyStrict'. Enforce separatorSuppressionPolicy for non-potentially-trailing occurrences Elements carrying a discriminator were excluded from potentially trailing, so they skipped the separator suppression checks under 'never', 'trailingEmpty' and 'trailingEmptyStrict'; they are now classified like every other element under those policies. The anyEmpty-specific arrayCompleteChecks check and its lastSeparatorWasFound plumbing are removed, since the spec says anyEmpty occurrences 'may' be omitted, unlike the 'must' for trailingEmptyStrict. A failed zero-length anyEmpty occurrence whose separator is not followed by another is now released to the next item instead of always classifying as MissingItem, so skipped optional elements mixed with real separators parse. 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. The trailingEmptyStrict check now sees a prior optional sibling that consumed its separator, so prefix, infix and postfix sequences reject a trailing empty occurrence. 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.
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? |
…ly-trailing occurrences under 'never', 'trailingEmpty', and 'trailingEmptyStrict'. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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