DAG does not compute relevance for all repeats

[lognaturel]

Software versions

JavaRosa v3.0.1

Problem description

The form posted as part of getodk/collect#4090 still prompts for adding egg_photos and light_source repeats even when their relevance expressions should evaluate to false.

Steps to reproduce the problem

Run the form from getodk/collect#4090, set a GPS coordinate, swipe forward.

Expected behavior

There should be no prompt to add repeats.

Other information

Troubleshooting notes. May or may not be related to #601.

I see that on initialization, all the other repeats' relevance are computed in evaluateTriggerable. The triggerables for egg_photos and light_source's relevances are not evaluated then.

[lognaturel]

Looks like it has to do with a repeat and a group sharing the same relevance condition. Minimal repro form.

[lognaturel]

The issue has to do with how relevance expressions (and all expressions) are reduced to a single Triggerable with a contextRef that represents the highest common parent between the expression's targets. For example, if /data/q0 and /data/repeat both have the same relevance expression, that will be represented by a single Triggerable with a contextRef of /data/.

This really helps explain how folks ended up with the strange-looking code replaced in #581. I'm going to try again with a clear head tomorrow morning to get a solution that doesn't involve saving and recomputing the relevance expressions for each repeat. If anyone has good ideas in the mean time, please try them out!

Failing test:

   @Test
    public void whenRepeatAndTopLevelNodeHaveSameRelevanceExpression_andExpressionEvaluatesToFalse_repeatPromptIsSkipped() throws Exception {
        Scenario scenario = Scenario.init("Repeat relevance same as other", html(
            head(
                title("Repeat relevance same as other"),
                model(
                    mainInstance(t("data id=\"repeat_relevance_same_as_other\"",
                        t("selectYesNo", "no"),
                        t("repeat1",
                            t("q1")),
                        t("q0")
                    )),
                    bind("/data/q0").relevant("/data/selectYesNo = 'yes'"),
                    bind("/data/repeat1").relevant("/data/selectYesNo = 'yes'")
                ),
                body(
                    select1("/data/selectYesNo",
                        item("yes", "Yes"),
                        item("no", "No")),
                    input("/data/q0"),
                    repeat("/data/repeat1",
                        input("/data/repeat1/q1")
                    )
                ))));

        scenario.jumpToBeginningOfForm();
        scenario.next();
        int event = scenario.next();

        assertThat(event, is(FormEntryController.EVENT_END_OF_FORM));
    }

I've tried adding the following in TriggerableDag.evaluateTriggerable:

        // For targets that are repeats, update the template because it's used to determine whether there's a prompt or not
        // We can't do this to the contextRef because it is the highest common parent of the targets (see intersectContextWith)
        for (TreeReference target : toTrigger.getTargets()) {
            TreeElement template = mainInstance.getTemplate(target);
            if (template != null) {
                TreeReference templateRef = template.getRef();
                qualifiedTargets.add(templateRef);
            }
        }

It doesn't work with the test above but it does work if the order of the binds is swapped. This points to some real weirdness in originalContextRef and how it's used. @ggalmazor and I did a ton of simplifying and cleanup in this area of the code but TriggerableDag.evaluateTriggerable and Triggerable.apply are still mostly as-is and very confusing.

This makes me concerned that there may be other issues when the same expression is repeated multiple times. I want to add more tests for cases where intersectContextWith will be called, particularly that involve getTriggerablesAffectingAllInstances.

[lognaturel]

I can get my test to pass by adding the includeTemplates flag for the expandReference call in Triggerable.apply. But the ChildVaccination smoketest fails.

I'm going to revert #581. I think we've learned a lot here but there's not a lot to gain by the simplification and we're taking on too much risk.