Add tunable to control UpStep resolving to arrays #1574
Conversation
- Introduced `allowLastUpStepToResolveToArray` tunable and deprecatedLastUpStepToResolveToArray warning - Deprecated behavior of `UpStep` resolving to an array, SDWs if used; SDE is tunable is false - Updated related tests DAFFODIL-2939
| ) | ||
| } | ||
|
|
||
| def checkIfTargetIsLastStepAndArray(): Unit = { |
There was a problem hiding this comment.
I'm not sure this function belongs in StepExpression. The assumptions and error message are specific to UpSteps, so as written it should never be called from any other StepExpression so probably doesn't belong here to prevent someone from accidentally calling it.
Note that this is different from checkIfNodeIndexedLikeArray, which does not make any assumptions about what the step kind it. It just should be called if the step should disallow indexing, which all steps might need to do.
Suggest we just move this function to UpStepExpressions, or just inline the logic into the UpStepExpression compiledDPath function.
| if (tunable.allowLastUpStepToResolveToArray) { | ||
| SDW( | ||
| WarnID.DeprecatedLastUpStepToResolveToArray, | ||
| ".. resolving to an array is deprecated. Try ../../$array_name instead. Offending expression: '%s'.", |
There was a problem hiding this comment.
Some thoughts on these diagnostics messages
- It's potentially cofusing to start a sentence with '..'. Maybe reword to something like
Last path step '..' resolving to an array is deprecated....
- I would avoid using
$array_namesince it looks like a variable (both Scala and DFDL). I generally use angle brackets, e.g.<array_name>but something else that has unique syntax if fine - Is it normal to include the whole expression in the diagnostics? I feel like we don't normally do that since the schema file location is available in the diagnostic and the user can use that to find the expression.
There was a problem hiding this comment.
Re 3, I thought it would be helpful in this situation to include the whole expression, but you're right re the schema file location, so ill remove the last line
| When .. (UpStep) is the last step in an expression, it should only ever return a single node, | ||
| not an array. Issue a warning when UpStep is the last step and resolves to an array element, | ||
| otherwise (if false) issue an SDE. | ||
| </xs:documentation> |
There was a problem hiding this comment.
Minor tweak, maybe say "If true, allow this but issue a warning. If false, issue an SDE."
Also maybe say something to indicate a value of "true" is deprecated and might be ignored in future versions.
| </tdml:infoset> | ||
|
|
||
| <tdml:warnings> | ||
| <tdml:warning>deprecatedLastUpStepToResolveToArray</tdml:warning> |
There was a problem hiding this comment.
I think we maybe need to make a decision on if we will allow backwards incompatible changes in 4.1.0. It's a minor release so technically we shouldn't but we can always say certain things are bugs and were not intended to be supported. I have no strong opinion either way.
This particular issue seems to only affect mil-std-2045/VMF, though other schemas we don't know about could possibly use the fn:count(..) thing.
There was a problem hiding this comment.
I will test it out with the default being false, and see what breaks in the regression test suite
There was a problem hiding this comment.
You were right, it was only mil-std-2045/VMF projects that were affected in our regression test suite, and the ../../<array_name> fix will work with them
| val areAllArrays = isLastStep && stepElements.forall { | ||
| checkIfNodeIndexedLikeArray() | ||
| checkIfTargetIsLastStepAndArray() | ||
| lazy val areAllArrays = isLastStep && stepElements.forall { |
There was a problem hiding this comment.
Minor cleanup, but I think we can just change this to isLastStep && isArray && isTargetType == NodeInfo.Array? I think we have a check that ensures steps are either all arrays or all non-arrays, so isArray is essentially the same as checking forall { _.isArray }--if one stepElement is an array (i.e. isArray == true) then they all must be arrays.
| } && targetType == NodeInfo.Array | ||
| checkIfNodeIndexedLikeArray() | ||
| if (areAllArrays) { | ||
| if (tunable.allowLastUpStepToResolveToArray && areAllArrays) { |
There was a problem hiding this comment.
Is this tunable check needed? If allowLastUpStepToResolveToArray is false then I think we would have SDE'ed in checkIfTargetIsLastStepAndArray. So if we get here, I think this tunable must always be true.
Based on the comment above, maybe we drop areAllArrays and just do this:
checkIfNodeIndexedLikeArray()
checkIfTargetIsLastStepAndArray()
if (isLastStep && isArray && targetType == NodeInfo.Array) {
CompiledDPath(UpMoveArray)
} else {
CompiledDPath(UpMove)
}- reword SDW/SDE messages and tunable documentation - refactor UpStepExpression.compiledDPath for simplicity and clarity - move checkIfTargetIsLastStepAndArray to UpStepExpression, as it's only relevant to .. DAFFODIL-2939
2 participants
allowLastUpStepToResolveToArraytunable and deprecatedLastUpStepToResolveToArray warningUpStepresolving to an array, SDWs if used; SDE is tunable is falseDAFFODIL-2939
Note that this doesn't actually remove UpMoveArray because schemas depend on the existing functionality ex: the mil-std schemas. Instead it adds a flag whose default results in the current behavior of .. resolving to an array element and an SDW.