Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve dependency resolution, XPath static analysis; support current() in computations, repeat-based itemsets, relative body references #166

Merged
merged 32 commits into from
Jul 25, 2024

Commits on Jul 22, 2024

  1. Warn on missing repeats, add explicit creation when missing

    Followup from [this discussion](#150 (comment))
    
    The idea here is:
    
    1. Explicit repeat creation in tests will improve test clarity
    
    2. Introduce a clear way to make similar changes in JavaRosa as they come up
    
    3. Detect missing repeats (with a still naive approach[^1], albeit now recursive) and **log with a stack trace** so explicit calls can be introduced (conditionally, with parameterization like many other cases where we make adjustments to the JavaRosa direct port)
    
    4. Add a new proposed `Scenario` method which…
    
      - Makes clear where explicit repeat creation calls are added, in a way that can be traced directly in test source, whenever convenient
    
      - Assumes the call occurs in such a sub-suite parameterizing whether to explicitly add repeats as detected; adds repeats as explicitly specified in the true condition, suppresses logging in the false condition
    
    This approach already detected one test which would have passed if adding repeats had been explicit. The test is updated here to demonstrate that.
    
    Notice that the test’s **PORTING NOTES** have also been removed. This is because the notes were wrong! This is an excellent example of how misleading it is that tests fail for lack of this implicit behavior! The actual test logic is not substantially noisier or more complex as a result. This feels like a clear win to me.
    
    [^1]: Keeping this naive seems fine for the limited scope of usage. The reference expressions which reach this point are limited to `Scenario.answer` calls with an explicit reference. If we’re using references of arbitrary complexity in those calls, I think we’ve got much bigger problems than this functionality being so narrowly scoped.
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    d9f5783 View commit details
    Browse the repository at this point in the history
  2. XPath static analysis overhaul: improved function call analysis, path…

    … resolution, dependency identification
    
    Function call analysis is generalized and streamlined, and used to help various other aspects of path-related static analysis.
    
    Path resolution:
    
    - Accounts for arbitrary self/parent steps (of any number, at any depth in a path)
    - Resolves relative paths to arbitray Absolute and FilterExpr contexts
    - Resolves current() calls to either:
      - The step in a [partial] path expression where called in a predicate on that step
      - The context (typically parent) of an expression’s definition
    
    Dependency identification:
    
    - Benefits from all of the above path resolution improvements
    - Accurately contextualizes dependencies referencing current() (which will improve reactivity for a variety of form logic cases)
    - Accurately identifies and contextualizes arbitrary sub-expressions in predicates (which will improve reactivity for form logic cases with more complex logic)
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    840937b View commit details
    Browse the repository at this point in the history
  3. Fix (alignment with Collect): handle relative nodeset references in f…

    …orm definition (typically body)
    
    Note that this breaks up parsing of…
    
    - `<item><label ref>`, where arbitrary references may be contextualized to their parent nodeset
    - `<itemset><label ref>`, where a relative expression is intentionally kept unresolved (ensuring that evaluation of dynamic itemsets update correctly when the itemset nodeset expression is a form state reference (i.e. repeat-based itemsets)
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    722d06d View commit details
    Browse the repository at this point in the history
  4. Fix: ensure that itemset nodeset expression is resolved in select con…

    …text
    
    This ensures that:
    
    - if relative, the nodeset expression itself is contextualized correctly
    - in any case, that any dependencies identified in the dynamic item expression are resolved to the correct context
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    f33e22a View commit details
    Browse the repository at this point in the history
  5. Update all DependentExpressions to use improved dependency analysis l…

    …ogic
    
    This wil improve a variety of edge cases, and will be most visible in the correct tracking of:
    
    - `current()` references in expressions
    - any references found in expressions’ predicates
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    4b24332 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    e41b24f View commit details
    Browse the repository at this point in the history
  7. Update passing secondary-instances test

    Test is fixed by stripping predicates in dependency analysis
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    fd82774 View commit details
    Browse the repository at this point in the history
  8. Scenario: implement canCreateNewRepeat

    This **will change** when we implement support for `jr:count` and `jr:noAddRemove`. The intent of implementing it now is to ensure, in subsequent commits, that adding explicit repeat creation does not cause false positives in new test passage
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    746baf8 View commit details
    Browse the repository at this point in the history
  9. Update now-passing test checking that creation of count-controlled re…

    …peats will not be allowed
    
    Technically this is a bit of a fib! The engine/client API will still permit repeat creation calls. But its passing now will ensure we continue to keep it passing when we implement the pertinent functionality
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    c49bc82 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    c83a049 View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    71250dc View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    717ac4c View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    7ef5ad5 View commit details
    Browse the repository at this point in the history
  14. Configuration menu
    Copy the full SHA
    d0dd2ec View commit details
    Browse the repository at this point in the history
  15. Configuration menu
    Copy the full SHA
    ce44512 View commit details
    Browse the repository at this point in the history
  16. Add changeset

    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    0c1534d View commit details
    Browse the repository at this point in the history
  17. Configuration menu
    Copy the full SHA
    7cb2a5c View commit details
    Browse the repository at this point in the history
  18. Configuration menu
    Copy the full SHA
    9d72a32 View commit details
    Browse the repository at this point in the history
  19. Remove null-as-keyword special case

    As discussed in review, it’s highly unlikely this is actually needed anymore!
    
    Brief backstory context in case we ever have reason to doubt this:
    
    0. The WHO VA form, which I’ve used as a frame of reference for many things, uses two references to `null` that don’t resolve to anything. The inferred intent is to treat `null` as a keyword, just as it would be in languages with the Billion Dollar Mistake.
    1. In early prototyping (pre Web Forms project), the WHO VA form’s references to `null` produced errors where there was assertion logic checking that expected dependencies were found.
    2. Early Web Forms work produced the same, for a time at least producing errors in the console. This produced a lot of distracting noise that made it harder to identify actual implementation errors in dependency resolution.
    3. Subsequent improvements to dependency resolution have been successful enough that we’ve even eschewed logging when a dependency lookup doesn’t resolve (though I sometimes add it in local dev for testing/validation), at least as of #67.
    4. As of #135, dependency resolution was expanded to be form-wide and match all possible nodes; there’s no case where a `null` reference **should** match a node and won’t.
    
    I can imagine these potentially useful followup changes:
    
    1. On parse, identify any path sub-expressions which **will never** resolve to any node in the form.
    2. Short circuit lookup of such never-resolvable paths. There’s no sense walking the full form tree for something we know we won’t find!
    3. Potentially warn when such never-resolvable paths are found. This wouldn’t be particularly useful for the `null` NameTest specifically (where the intent at least in WHO VA is clearly a null reference), but it could be useful for catching typos and other mistaken references like hierarchy confusion.
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    413819d View commit details
    Browse the repository at this point in the history
  20. Split resolveDependencyNodesets set-like behavior tests

    - Cases which previously exercised set-like behvaior implicitly are broken into individual tests, e.g. demonstrating cases where `current()` and `.` are treated the same way
    - Adds an explicit set-like behavior test
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    c5b8ac7 View commit details
    Browse the repository at this point in the history
  21. JSDoc todos on current/instance expression predicates reference grammar

    This corrects the previous copypasta mistake, and makes the notes more general so they don’t overly influence reasoning about the gap in behavior
    eyelidlessness committed Jul 22, 2024
    Configuration menu
    Copy the full SHA
    1d4de28 View commit details
    Browse the repository at this point in the history

Commits on Jul 23, 2024

  1. Add tests for @sadiqkhoja’s questions 1-5

    Two of the tests are currently marked failing:
    
    2. This question caught a bug! I’ll fix in a subsequent commit.
    4. This question suggested a valuable enhancement! I’ll timebox a solution to this, and expect to also have a fix for it in a subsequent commit.
    eyelidlessness committed Jul 23, 2024
    Configuration menu
    Copy the full SHA
    d415510 View commit details
    Browse the repository at this point in the history
  2. Address @sadiqkhoja’s question 6

    This further clarifies that the intent is to identify where an expression **is** a translation call, not where an expression may contain such a call as any arbitrary sub-expression. As mentioned in the added JSDoc todo, it is possible we may want this in some future change. I think we should know more about whether it’s an expected use case before implementing it (as if it’s not an expected use case, it could be an indication of a form design mistake).
    eyelidlessness committed Jul 23, 2024
    Configuration menu
    Copy the full SHA
    7c3f513 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    918062d View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    24a30ec View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    057748b View commit details
    Browse the repository at this point in the history
  6. Capture findings of timeboxed spike on child::text()

    … and add several related tests around the general concept.
    eyelidlessness committed Jul 23, 2024
    Configuration menu
    Copy the full SHA
    b58ac5b View commit details
    Browse the repository at this point in the history

Commits on Jul 25, 2024

  1. Configuration menu
    Copy the full SHA
    fe91584 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    47b4fbc View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    16719b4 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    0c0adc4 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    b07023e View commit details
    Browse the repository at this point in the history