stevedlawrence commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1146183071
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not
supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
Yeah, something like that is going on. I spent a little time trying to
better understand our TDML runner, because it's still not totally clear to me.
I think I have a better idea the core issue (I thin we have a couple related
bugs).
In this particular case, the `checkDiagnosticMessages` function is called to
verify all expected diagnostics are found:
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L928-L949
The `diagnostics` parameter contains a combination of diagnostics from
schema compilation and from the parse . This function then calls
`verifyAllDiagnosticsFound` twice, both times passing in those diagnostics and
either the expected errors or expected warnings. The
`verifyAllDiagnosticsFound` function iterates over the expected errors or
warnings, and makes sure they exist in any of the passed in actual diagnostics.
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L1784-L1799
But that does no checks to make sure the type of the expected diagnostics
matches the type of the actual diagnostics (i.e. error vs warning). So right
now, I think there is no difference between `<tdml:errors>` and
`<tdml:warnings>` as far as diagnostics checking goes. Note that there is a
difference in the logic in other places, for example, if there are expected
errors then we may require a failed parse result. So there are other cases
where it matters. But this means if there is a parse error, any warnings can be
`<tdml:error>` or `<tdml:warning>` and the warning diagnostic will be found.
So that's a bug that needs to be fixed. `verifyAllDiagnosticsFound` should
look at the type of each `expectedDiag` and should should accept matches in
`actualDiags` of the appropriate error/warning type. Whether or not that is
fixed as part of the PR or a new one doesn't really matter to me, it is sort of
a different issue. Though it's also probably a fairly small change, so might be
worth including in this since it is somewhat related. Note that I think if we
fixed this, the `tdml:error` in this test case would need to change to a
`tdml:warning` and a new `tdml:error` would be added with this new error
message.
But that doesn't explain why the error message changed. So what caused that?
You're right that it's because we aren't serializing those warnings, so a
save/reload loses those warnings, but the TDMLRuner should capture them and use
them for later. When we get a successful `CompileResult`, it is a tuple of
compilation warnings and the actual processor, so we do have those warnings
even if the processor is saved/reloaded.
I think the issue is where we call `runParseExepctedErrors/Success` here:
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L976-L1012
In that above chunk, we save the processor from the CompileResult to the
`processor` var, but we just ignore whatever diagnostics were part of the
`CompileResult`. To get the compile diagnostics for later verification, we end
up calling `processor.getDiagnostics` in a number of places. But if we
saved/reloaded the processor, the processor no longer has those diagnostics.
So I think the issue is that we should never call `processor.getDiagnostics`
(that is an unreliable way to get compile time diags due to this change), but
we should instead pass around the diagnostics from the `CompileResult` and use
that. And the `getDiagnostics` function should probably just be removed from
the `TDMLDFDLProcessor` API--any diagnostics related to compilation should come
from the `CompileResult`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
