From 413819de1866e96387462ad6c80de61f674cbeab Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Fri, 19 Jul 2024 12:56:25 -0700 Subject: [PATCH] Remove `null`-as-keyword special case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/expression/DependentExpression.ts | 7 ----- .../src/parse/xpath/dependency-analysis.ts | 28 ++----------------- .../parse/xpath/dependency-analysis.test.ts | 10 +++++++ 3 files changed, 12 insertions(+), 33 deletions(-) diff --git a/packages/xforms-engine/src/expression/DependentExpression.ts b/packages/xforms-engine/src/expression/DependentExpression.ts index e343a2567..bdf710d24 100644 --- a/packages/xforms-engine/src/expression/DependentExpression.ts +++ b/packages/xforms-engine/src/expression/DependentExpression.ts @@ -38,11 +38,6 @@ interface DependentExpressionOptions { */ readonly ignoreContextReference?: boolean; - /** - * @default true - */ - readonly ignoreNullExpressions?: boolean; - readonly semanticDependencies?: SemanticDependencyOptions; } @@ -61,7 +56,6 @@ export class DependentExpression { const { ignoreContextReference = false, - ignoreNullExpressions = true, semanticDependencies = { parentContext: false, translations: false, @@ -71,7 +65,6 @@ export class DependentExpression { const dependencyReferences = new Set( resolveDependencyNodesets(context.reference, expression, { ignoreReferenceToContextPath: ignoreContextReference, - simulateNullKeyword: ignoreNullExpressions, }) ); diff --git a/packages/xforms-engine/src/parse/xpath/dependency-analysis.ts b/packages/xforms-engine/src/parse/xpath/dependency-analysis.ts index aedab94f1..02372e736 100644 --- a/packages/xforms-engine/src/parse/xpath/dependency-analysis.ts +++ b/packages/xforms-engine/src/parse/xpath/dependency-analysis.ts @@ -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 ``. - */ - readonly simulateNullKeyword?: boolean; } const defaultPathResolutionOptions: Required = { ignoreReferenceToContextPath: false, - simulateNullKeyword: true, }; /** @@ -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); diff --git a/packages/xforms-engine/test/parse/xpath/dependency-analysis.test.ts b/packages/xforms-engine/test/parse/xpath/dependency-analysis.test.ts index fe23a567c..b568f6401 100644 --- a/packages/xforms-engine/test/parse/xpath/dependency-analysis.test.ts +++ b/packages/xforms-engine/test/parse/xpath/dependency-analysis.test.ts @@ -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);