javarosa icon indicating copy to clipboard operation
javarosa copied to clipboard

Validate DAG check precludes coalesce(.,${foo})

Open tiritea opened this issue 5 years ago • 4 comments

Software versions

javaRosa as it exists in Validate as it exists in XSLForm Online v1.3.1

Problem description

x = coalesce(.,${foo}) is a useful tool to implement dynamic defaults in a form, which avoids having to use the non-standard once() function (which is misleading, and arguably a candidate for deprecating once instance-first-load events are working). However, validate/javaRosa reject forms implementing this because it detects a self-reference.

Steps to reproduce the problem

x = coalesce(.,${foo})

Expected behavior

this XPath function is in fact perfectly fine and should keep current value of the the XML element if non-null, otherwise assign it to the value of ${foo} (and hence maintain this value thereafter because it is no longer null)

Other information

No obvious fix, other than having to somehow explicitly detect . as the first argin a coalesce() call and not throw a DAG error.

Workaround is to use current once() function, which performs the exact same behavior, but passes DAG by virtue of omitting . reference, because its implicit in once()

tiritea avatar May 24 '19 03:05 tiritea

sample form: coalesce.xlsx

Note: can mimic same coalesce() behavior with if(), which likewise fails DAG check: if.xlsx

tiritea avatar May 25 '19 20:05 tiritea

The odk-instance-first-load event doesn't help for the case where the value to be used as a default is set as the form is being filled, right? To truly get the desired behavior in that case, we'd want something like a value-first-set event.

It feels like rejecting self references is generally useful, right? For example, given an integer node that, for simplicity's sake, is never blank, the calculate expression . + 3 should not be allowed?

lognaturel avatar Jul 11 '19 03:07 lognaturel

Detecting (and rejecting legitimate) self-references is certainly useful! The problem as I see it is that XPath expressions have sufficient expressiveness that it is not strictly possible to statically determine their validity in the general case when the precise nature of the XML document is still unknown, and may change dynamically (eg repeat groups, hence the bogus indexed-repeat() self reference problem). And in some cases apparent syntactic self-reference are in fact conditionally - at runtime - non-self referencing; ie coalesce(., ...) and if(...).

Also, coalesce() is a pretty fundamental XPath 2 function, so it really should work as defined. Whereas once() is arguably a hack, ill-defined (it doesn't actually only ever fire once...) and arguably should probably be deprecated someday [its only needed to fake out Validate otherwise throwing a self-reference error against coalesce].

This is also all rather related to the issue Validate has trying to validate function argument types, which is IMO is not possible to do statically against what is in fact a dynamic XML document.

[soapbox] I view DAG checking and XForm type checking akin to optimizations: great to have but they mustn't break functionality. And I simply dont see how Validate can in ever be 100% reliable trying to statically detect self-references against an otherwise dynamic document, nor ever be 100% reliable statically checking data types against an XML document whose element have not yet been populated (!). So given these fundamental limitations it should probably only throw (very) useful warnings, but never break valid functionality by throwing bogus fatal errors. [/soapbox]

tiritea avatar Jul 12 '19 00:07 tiritea

I view DAG checking

The graph is not used for checking, it's used as an optimization to only recompute expressions when necessary. That is, when a value changes at a particular path, that change propagates to its dependent references. So it's not a matter of turning some kind of verification on and off, the dependency graph is fundamental to form re-evaluation. ~Given this recomputation model, it's not clear to me how any self-reference should be accepted.~

Update: well, https://www.w3.org/TR/2006/REC-xforms-20060314/sliceD.html#rpm-processing-recalc-mddg has an answer: "Vertex v is excluded from its own depList to allow self-references to occur without causing a circular reference exception." Ok. So the action item here would be to implement that special case.

lognaturel avatar Jul 12 '19 03:07 lognaturel