Skip to content

Commit

Permalink
Remove null-as-keyword special case
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
eyelidlessness committed Jul 22, 2024
1 parent 9d72a32 commit 413819d
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 33 deletions.
7 changes: 0 additions & 7 deletions packages/xforms-engine/src/expression/DependentExpression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,6 @@ interface DependentExpressionOptions {
*/
readonly ignoreContextReference?: boolean;

/**
* @default true
*/
readonly ignoreNullExpressions?: boolean;

readonly semanticDependencies?: SemanticDependencyOptions;
}

Expand All @@ -61,7 +56,6 @@ export class DependentExpression<Type extends DependentExpressionResultType> {

const {
ignoreContextReference = false,
ignoreNullExpressions = true,
semanticDependencies = {
parentContext: false,
translations: false,
Expand All @@ -71,7 +65,6 @@ export class DependentExpression<Type extends DependentExpressionResultType> {
const dependencyReferences = new Set(
resolveDependencyNodesets(context.reference, expression, {
ignoreReferenceToContextPath: ignoreContextReference,
simulateNullKeyword: ignoreNullExpressions,
})
);

Expand Down
28 changes: 2 additions & 26 deletions packages/xforms-engine/src/parse/xpath/dependency-analysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,29 +29,10 @@ interface PathResolutionOptions {
* cycle analysis.
*/
readonly ignoreReferenceToContextPath?: boolean;

/**
* Treats the literal sub-expression `null` as if it were a keyword. This is a
* deviation from XPath (where `null` would be a RelativeLocationPath
* referencing a child node by that name). However, in real-world usage, some
* forms evidently use `null` as a keyword representing a null and/or blank
* value.
*
* @default true
*
* @todo Some equivalent of this flag has been present, with the same default,
* since very early Web Forms exploratory work. At time of writing, it has
* never been overridden in any equivalent context. This has not been a source
* of any known bug/unexpected/unusual behavior. Howevever, it would probably
* be more robust to override the default whenever a form definition includes
* a node literally named `<null/>`.
*/
readonly simulateNullKeyword?: boolean;
}

const defaultPathResolutionOptions: Required<PathResolutionOptions> = {
ignoreReferenceToContextPath: false,
simulateNullKeyword: true,
};

/**
Expand All @@ -75,19 +56,14 @@ export const resolveDependencyNodesets = (
expression: string,
options: PathResolutionOptions = {}
): readonly string[] => {
const { ignoreReferenceToContextPath, simulateNullKeyword } = {
const { ignoreReferenceToContextPath } = {
...defaultPathResolutionOptions,
...options,
};

const contextNode = getPathExpressionNode(contextNodeset ?? '"not a path, null fallback"');
const expressionRootNode = expressionParser.parse(expression).rootNode;

let subExpressions = findLocationPathSubExpressionNodes(expressionRootNode);

if (simulateNullKeyword) {
subExpressions = subExpressions.filter((subExpression) => subExpression.text !== 'null');
}
const subExpressions = findLocationPathSubExpressionNodes(expressionRootNode);

const resolvedPathLists = subExpressions.flatMap((subExpression) => {
const pathNodes = resolvePath(contextNode, subExpression);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,16 @@ describe('Dependency analysis', () => {
expression: '//. | //./foo | //.. | //../foo',
expected: ['//.', '//foo', '//..'],
},

// This case exists to test removal of such a special case.
//
// Discussion here: https://github.com/getodk/web-forms/pull/166#discussion_r1680168253
{
description: 'null is not a keyword, nor special cased',
contextNodeset: '/data',
expression: 'count(null) > count(foo/null)',
expected: ['/data/null', '/data/foo/null'],
},
])('$description', ({ contextNodeset, expression, expected }) => {
it(`resolves nodeset dependencies in expression ${JSON.stringify(expression)}, with context ${JSON.stringify(contextNodeset)}, producing nodesets: ${JSON.stringify(expected)}`, () => {
const actual = resolveDependencyNodesets(contextNodeset, expression);
Expand Down

0 comments on commit 413819d

Please sign in to comment.