Consider instances in TreeReference comparison and set null instanceName for main instance on deserialization

[lognaturel]

Closes #449

This is pretty high-risk and would be good to do an interactive review on.

What has been done to verify that this works as intended?

Thought really hard about what was going on and tried to write as specific of tests as possible.

Why is this the best possible solution? Were any other approaches considered?

The change to TreeReference is the same as in #450 with the addition of an implementation for hashCode. It also avoids using a custom Objects class now that Collect has a minSdkVersion of 21. I feel good about this change and don't really see alternatives.

This unmasks some inconsistencies in how references in form definitions are resolved by JavaRosa and in how the primary instance was represented before and after serialization. The main symptom when we tried to release this previously is that constraints were broken - #475. The reason for this is that before serialization, the instanceName of the primary instance root node was null and after deserialization, it was populated. In some code paths, this led to references to primary instance nodes with an explicit instance (e.g. instance("My cool form")/data/a instead of /data/a). This didn't matter before because instances were ignored when doing reference comparison. Constraints were broken because in the evaluation process there ended up being a comparison between instance("My cool form")/data/a and /data/a that used to pass but now fails. This comparison is made in XPathPathExpr.getRefValue to identify whether the latest value entered by the user should be used.

Even though it's been working forever, it's not right that the primary instance's instanceName would be different before and after serialization. I decided to explicitly set it on deserialization. This is the counterpart to FormInstanceParser.buildInstanceStructure which creates the primary instance initially and sets instanceName to null. I think that in an ideal world there wouldn't be this kind of special case for the primary instance name and some other mechanism would be used to identify that an instance is primary. However, that would require a lot of high-risk refactoring throughout the codebase.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The goal is for this to only unlock new behavior. However' it's high risk because as we saw with #450, it does touch anything having to do with TreeReference comparison or hashing. The places where TreeReferences are compared are:
EvaluationContext.rescope
FormDef.getElementsFromReferences
FormDef.findQuestionByRef
FormInstanceParser.verifyRepeatMemberBindings -- true case never hit
TreeReference.intersect -- true case never hit
TriggerableDag.deleteRepeatGroup
XPathPathExpr.getRefValue
XPathPathExpr.pivot -- not tested at all
XPathReference.equals -- false case never hit

One nice thing about Scenario that I realized writing up this list is that because it gives us a single entry point into tests, we can do things like immediately serialize all form defs on Scenario.init. I did this and ran the tests and one failed in ChoiceNameTest. I initially thought it could be a problem with reference comparison but I'm pretty sure it's just that the test wasn't actually mimicking client behavior.

The jr:choice-name function handler (see FormDef.initEvalContext) makes a call to populate dynamic choices, but only if they haven't already been populated. In the case of an unserialized form definition, the choices were populated on form initialization as an empty list. And then when the jr:choice-name call was evaluated, the choices weren't repopulated. After deserialization, though, the initialization step didn't happen. That meant that when the jr:choice-name call was evaluated, the choices were populated.

But Collect will explicitly call populateDynamicChoices on displaying the choice list for cities. So I updated the test to reflect this and now the behavior is the same with and without serialization.

Do we need any specific form for testing your changes? If so, please attach one.

See form at getodk/collect#3316.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

[codecov-commenter]

Codecov Report

Merging #578 into master will increase coverage by 0.10%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master     #578      +/-   ##
============================================
+ Coverage     54.55%   54.65%   +0.10%     
- Complexity     3221     3228       +7     
============================================
  Files           244      243       -1     
  Lines         13345    13347       +2     
  Branches       2565     2567       +2     
============================================
+ Hits           7280     7295      +15     
+ Misses         5256     5244      -12     
+ Partials        809      808       -1     

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/org/javarosa/core/model/FormIndex.java	58.33% <ø> (ø)	45.00 <0.00> (ø)
...org/javarosa/core/model/instance/DataInstance.java	49.54% <ø> (ø)	24.00 <0.00> (ø)
.../org/javarosa/core/model/instance/TreeElement.java	63.37% <ø> (ø)	121.00 <0.00> (ø)
...ain/java/org/javarosa/form/api/FormEntryModel.java	53.42% <0.00%> (ø)	68.00 <0.00> (ø)
src/main/java/org/javarosa/core/model/FormDef.java	71.12% <100.00%> (+0.04%)	160.00 <0.00> (ø)
...org/javarosa/core/model/instance/FormInstance.java	49.43% <100.00%> (ø)	29.00 <0.00> (ø)
...rg/javarosa/core/model/instance/TreeReference.java	74.64% <100.00%> (+0.27%)	110.00 <0.00> (+2.00)
...main/java/org/javarosa/xpath/XPathConditional.java	80.00% <100.00%> (ø)	24.00 <1.00> (ø)
...in/java/org/javarosa/xpath/expr/XPathPathExpr.java	69.14% <0.00%> (+1.14%)	53.00% <0.00%> (+1.00%)
...rosa/core/model/instance/ExternalDataInstance.java	85.29% <0.00%> (+2.94%)	13.00% <0.00%> (+1.00%)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e86375...9f07913. Read the comment docs.

[lognaturel]

It's strange to describe how this field is used in this comment. However, I couldn't find a better place for it and it seemed valuable to me for making sense of debug contents. I can remove it if others disagree, though.

[lognaturel]

Again, maybe this information shouldn't be here.

[seadowg]

Couple of comments and a merge conflict to fix here!