Skip to content

Commit

Permalink
Remove most current parse-stage dependency analysis
Browse files Browse the repository at this point in the history
As described in the previous commit, for the most part this usage of dependency analysis during form parsing is completely redundant to logic performed in the course of evaluating XPath expressions with `engineDOMAdapter`.

In theory, this commit could also delete a bunch of the dependency analysis code along with it. But that code will have value for other purposes. Top of mind: a proper implemenation of cycle detection. I anticipate other use cases as well.

There is still some remaining dependency analysis in active use. This largely focuses on:

- Setting up reactive subscriptions to translation state, updating expressions with `jr:itext` calls when the form’s active language state is changed
- Identifying “notes”, as we’ve modeled them to be value nodes with a `readonly` bind expression which will always evaluate to true

There is certainly still some further simplification left on the table at this commit point. It’s not clear how much the `DependencyContext`/`DependentExpression` abstraction still makes sense. We certainly still use `DependentExpression` to set up reactive computations, but there are other aspects of the conceptual pair which are likely redundant now as well. I’ve left these unaddressed to try to wind up this refactor as quickly as possible.
  • Loading branch information
eyelidlessness committed Nov 4, 2024
1 parent fd90224 commit 0d0434e
Show file tree
Hide file tree
Showing 9 changed files with 13 additions and 66 deletions.
2 changes: 1 addition & 1 deletion packages/xforms-engine/src/instance/Note.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class Note
clientStateFactory: this.engineConfig.stateFactory,
};

const isReadonly = createNoteReadonlyThunk(this, definition.bind.readonly);
const isReadonly = createNoteReadonlyThunk(this, definition);
const noteTextComputation = createNoteText(this, definition.noteTextDefinition);

let noteText: ComputedNoteText;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,36 @@
import type { Accessor } from 'solid-js';
import type { EvaluationContext } from '../../instance/internal-api/EvaluationContext.ts';
import type { BindComputationExpression } from '../../parse/expression/BindComputationExpression.ts';
import type { NoteNodeDefinition } from '../../parse/model/NoteNodeDefinition.ts';
import { resolveDependencyNodesets } from '../../parse/xpath/dependency-analysis.ts';
import { createComputedExpression } from './createComputedExpression.ts';

export const createNoteReadonlyThunk = (
context: EvaluationContext,
readonlyDefinition: BindComputationExpression<'readonly'>
definition: NoteNodeDefinition
): Accessor<true> => {
if (!readonlyDefinition.isConstantTruthyExpression()) {
const { reference } = definition.bodyElement;
const { readonly } = definition.bind;

if (!readonly.isConstantTruthyExpression()) {
throw new Error('Expected a static readonly expression');
}

let result = true;

if (import.meta.env.DEV) {
const { expression } = readonlyDefinition;
const { expression } = readonly;
const dependencyReferences = resolveDependencyNodesets(reference, expression);

if (readonlyDefinition.dependencyReferences.size > 0) {
if (dependencyReferences.length > 0) {
throw new Error(`Expected expression ${expression} to have no dependencies`);
}

const computedExpression = createComputedExpression(context, readonlyDefinition);
const computedExpression = createComputedExpression(context, readonly);

result = computedExpression();

if (result !== true) {
throw new Error(`Expected expression ${readonlyDefinition.expression} to return true`);
throw new Error(`Expected expression ${readonly.expression} to return true`);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { DependencyContext } from '../../../expression/abstract/DependencyContext.ts';
import type { AnyDependentExpression } from '../../../expression/abstract/DependentExpression.ts';
import type { ItemsetDefinition } from './ItemsetDefinition.ts';

export class ItemsetNodesetContext extends DependencyContext {
Expand All @@ -19,8 +18,4 @@ export class ItemsetNodesetContext extends DependencyContext {
super.isTranslated = value;
this.itemset.isTranslated = value;
}

override registerDependentExpression(expression: AnyDependentExpression): void {
this.itemset.registerDependentExpression(expression);
}
}
Original file line number Diff line number Diff line change
@@ -1,28 +1,7 @@
import type { AnyDependentExpression } from './DependentExpression.ts';

export abstract class DependencyContext {
abstract get parentReference(): string | null;
abstract get reference(): string | null;

protected readonly dependentExpressions = new Set<AnyDependentExpression>();

protected dependencyExpressionsCache: ReadonlySet<string> | null = null;

get dependencyExpressions(): ReadonlySet<string> {
let { dependencyExpressionsCache } = this;

if (dependencyExpressionsCache == null) {
dependencyExpressionsCache = new Set(
Array.from(this.dependentExpressions).flatMap((expression) => {
return Array.from(expression.dependencyReferences);
})
);
this.dependencyExpressionsCache = dependencyExpressionsCache;
}

return dependencyExpressionsCache;
}

get isTranslated(): boolean {
return this._isTranslated;
}
Expand All @@ -45,9 +24,4 @@ export abstract class DependencyContext {

// Update note on `set isTranslated` if this internal storage changes.
protected _isTranslated = false;

registerDependentExpression(expression: AnyDependentExpression): void {
this.dependencyExpressionsCache = null;
this.dependentExpressions.add(expression);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { EngineXPathEvaluator } from '../../../integration/xpath/EngineXPathEvaluator.ts';
import { resolveDependencyNodesets } from '../../xpath/dependency-analysis.ts';
import type {
ConstantExpression,
ConstantTruthyExpression,
Expand Down Expand Up @@ -55,7 +54,6 @@ export interface ConstantTruthyDependentExpression extends ConstantDependentExpr
}

export abstract class DependentExpression<Type extends DependentExpressionResultType> {
readonly dependencyReferences: ReadonlySet<string> = new Set();
readonly isTranslated: boolean = false;
readonly evaluatorMethod: DependentExpressionEvaluatorMethod<Type>;
readonly constantExpression: ConstantExpression | null;
Expand All @@ -81,26 +79,17 @@ export abstract class DependentExpression<Type extends DependentExpressionResult
this.evaluatorMethod = evaluatorMethodsByResultType[resultType];

const {
ignoreContextReference = false,
semanticDependencies = {
translations: false,
},
} = options;

this.dependencyReferences = new Set(
resolveDependencyNodesets(context.reference, expression, {
ignoreReferenceToContextPath: ignoreContextReference,
})
);

const isTranslated = semanticDependencies.translations && isTranslationExpression(expression);

if (isTranslated) {
this.isTranslated = true;
context.isTranslated = true;
}

context.registerDependentExpression(this);
}

isConstantExpression(): this is ConstantDependentExpression<Type> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ export abstract class DescendentNodeDefinition<

readonly root: RootDefinition;
readonly nodeset: string;
readonly dependencyExpressions: ReadonlySet<string>;
readonly isTranslated: boolean = false;

constructor(
Expand All @@ -43,11 +42,6 @@ export abstract class DescendentNodeDefinition<
if (bind.isTranslated || bodyElement?.isTranslated) {
this.isTranslated = true;
}

this.dependencyExpressions = new Set([
...bind.dependencyExpressions,
...(bodyElement?.dependencyExpressions ?? []),
]);
}
}

Expand Down
3 changes: 0 additions & 3 deletions packages/xforms-engine/src/parse/model/NodeDefinition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,7 @@ export interface NodeDefinition<Type extends NodeDefinitionType> {
readonly nodeset: string;
readonly nodeName: string;
readonly bodyElement: AnyBodyElementDefinition | RepeatElementDefinition | null;

readonly isTranslated: boolean;
readonly dependencyExpressions: ReadonlySet<string>;

readonly root: RootDefinition;
readonly parent: NodeParent<Type>;
readonly children: NodeChildren<Type>;
Expand Down
1 change: 0 additions & 1 deletion packages/xforms-engine/src/parse/model/RootDefinition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export class RootDefinition implements NodeDefinition<'root'> {
readonly defaultValue = null;

readonly isTranslated = false;
readonly dependencyExpressions: ReadonlySet<string> = new Set<string>();

constructor(
protected readonly form: XFormDefinition,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import type { LocalNamedElement } from '@getodk/common/types/dom.ts';
import type { TextRole } from '../../../client/TextRange.ts';
import type { XFormDefinition } from '../../../parse/XFormDefinition.ts';
import { DependencyContext } from '../../expression/abstract/DependencyContext.ts';
import type { AnyDependentExpression } from '../../expression/abstract/DependentExpression.ts';
import type { AnyTextChunkExpression } from '../../expression/abstract/TextChunkExpression.ts';
import type { AnyMessageDefinition } from '../MessageDefinition.ts';
import type { AnyTextElementDefinition } from './TextElementDefinition.ts';
Expand Down Expand Up @@ -52,11 +51,6 @@ export abstract class TextRangeDefinition<Role extends TextRole> extends Depende
this.parentReference = ownerContext.parentReference;
}

override registerDependentExpression(expression: AnyDependentExpression): void {
this.ownerContext.registerDependentExpression(expression);
super.registerDependentExpression(expression);
}

toJSON(): object {
const { form, ownerContext, ...rest } = this;

Expand Down

0 comments on commit 0d0434e

Please sign in to comment.