Skip to content

Use using for cycle tracker #1434

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 22, 2025
Merged

Use using for cycle tracker #1434

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
915e464
Reworks the cycle tracker to use the using feature
ubik2 Jul 17, 2025
d474285
added IDisposable type
ubik2 Jul 17, 2025
8453876
Typescript provides the Disposable type already
ubik2 Jul 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 108 additions & 135 deletions packages/runner/src/traverse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,20 @@ export class CycleTracker<K> {
constructor() {
this.partial = new Set<K>();
}
enter(k: K): boolean {
include(k: K, context?: unknown): Disposable | null {
if (this.partial.has(k)) {
console.error("Cycle Detected!");
return false;
console.error(
"Cycle Detected!",
context == null ? JSON.stringify(k) : JSON.stringify(context),
);
return null;
}
this.partial.add(k);
return true;
}
exit(k: K) {
this.partial.delete(k);
return {
[Symbol.dispose]: () => {
Copy link
Contributor

@ellyxir ellyxir Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realised, does Firefox support Symbol.dispose?

Copy link
Collaborator

@jsantell jsantell Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently have any polyfills (e.g. babel) in our main building pipeline, and would think the platform needs to support the feature translated by typescript. It looks like this resource management API is still not in release on Firefox, and unsupported on Safari https://caniuse.com/mdn-javascript_statements_using

We could 1) investigate polyfilling this feature for browsers, 2) only use these features in Deno components, or 3) avoid using using, edit: 4) or only support browsers that implement this feature

Copy link
Collaborator

@jsantell jsantell Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Looks like the TypeScript compiler tsc translates the using functionality to non-"using" JS, but we use esbuild in most cases (shell, deno-web-test, jumble->vite->esbuild) which transparently copies over using functionality, requiring runtime support. We could explore esbuild plugins to translate this, or switch to using tsc for compiling (a larger lift)

this.partial.delete(k);
},
};
}
}

Expand Down Expand Up @@ -204,22 +208,17 @@ export abstract class BaseObjectTraverser<K, S> {
if (isPrimitive(doc.value)) {
return doc.value;
} else if (Array.isArray(doc.value)) {
if (tracker.enter(doc.value)) {
try {
return doc.value.map((item, index) =>
this.traverseDAG(
{ ...doc, path: [...doc.path, index.toString()], value: item },
tracker,
schemaTracker,
)
) as Immutable<JSONValue>[];
} finally {
tracker.exit(doc.value);
}
} else {
console.log("Cycle detected", JSON.stringify(doc));
using t = tracker.include(doc.value, doc);
if (t === null) {
return null;
}
return doc.value.map((item, index) =>
this.traverseDAG(
{ ...doc, path: [...doc.path, index.toString()], value: item },
tracker,
schemaTracker,
)
) as Immutable<JSONValue>[];
} else if (isObject(doc.value)) {
// First, see if we need special handling
if (isAnyCellLink(doc.value)) {
Expand All @@ -236,31 +235,26 @@ export abstract class BaseObjectTraverser<K, S> {
}
return this.traverseDAG(newDoc, tracker, schemaTracker);
} else {
if (tracker.enter(doc.value)) {
try {
return Object.fromEntries(
Object.entries(doc.value).map((
[k, value],
) => [
k,
this.traverseDAG(
{
...doc,
path: [...doc.path, k],
value: value,
},
tracker,
schemaTracker,
),
]),
) as Immutable<JSONValue>;
} finally {
tracker.exit(doc.value);
}
} else {
console.log("Cycle detected", JSON.stringify(doc));
using t = tracker.include(doc.value, doc);
if (t === null) {
return null;
}
return Object.fromEntries(
Object.entries(doc.value).map((
[k, value],
) => [
k,
this.traverseDAG(
{
...doc,
path: [...doc.path, k],
value: value,
},
tracker,
schemaTracker,
),
]),
) as Immutable<JSONValue>;
}
} else {
console.error("Encountered unexpected object: ", doc.value);
Expand Down Expand Up @@ -360,75 +354,69 @@ function followPointer<K, S>(
schemaTracker?: MapSet<string, SchemaPathSelector>,
selector?: SchemaPathSelector,
): [ValueAtPath<K>, SchemaPathSelector | undefined] {
if (!tracker.enter(doc.value!)) {
console.log("Cycle detected", JSON.stringify(doc));
using t = tracker.include(doc.value!, doc);
if (t === null) {
return [{ ...doc, path: [], value: undefined }, selector];
}
try {
const link = parseLink(doc.value)!;
const target = (link.id !== undefined)
? manager.getTarget(link.id)
: doc.doc;
let [targetDoc, targetDocRoot] = [doc.doc, doc.docRoot];
if (selector !== undefined) {
// We'll need to re-root the selector for the target doc
// Remove the portions of doc.path from selector.path, limiting schema if needed
// Also insert the portions of cellTarget.path, so selector is relative to new target doc
// We do this even if the target doc is the same doc, since we want the
// selector path to match.
selector = narrowSchema(doc.path, selector, link.path as string[]);
const link = parseLink(doc.value)!;
const target = (link.id !== undefined) ? manager.getTarget(link.id) : doc.doc;
let [targetDoc, targetDocRoot] = [doc.doc, doc.docRoot];
if (selector !== undefined) {
// We'll need to re-root the selector for the target doc
// Remove the portions of doc.path from selector.path, limiting schema if needed
// Also insert the portions of cellTarget.path, so selector is relative to new target doc
// We do this even if the target doc is the same doc, since we want the
// selector path to match.
selector = narrowSchema(doc.path, selector, link.path as string[]);
}
if (link.id !== undefined) {
// We have a reference to a different cell, so track the dependency
// and update our targetDoc and targetDocRoot
const valueEntry = manager.load(target);
if (valueEntry === null) {
return [{ ...doc, path: [], value: undefined }, selector];
}
if (link.id !== undefined) {
// We have a reference to a different cell, so track the dependency
// and update our targetDoc and targetDocRoot
const valueEntry = manager.load(target);
if (valueEntry === null) {
return [{ ...doc, path: [], value: undefined }, selector];
}
if (schemaTracker !== undefined && selector !== undefined) {
schemaTracker.add(manager.toKey(target), selector);
}
// If the object we're pointing to is a retracted fact, just return undefined.
// We can't do a better match, but we do want to include the result so we watch this doc
if (valueEntry.value === undefined) {
return [
{ doc: target, docRoot: undefined, path: [], value: undefined },
selector,
];
}
// Otherwise, we can continue with the target.
// an assertion fact.is will be an object with a value property, and
// that's what our schema is relative to.
targetDoc = target;
const targetObj = valueEntry.value as Immutable<JSONObject>;
targetDocRoot = targetObj["value"];
// Load any sources (recursively) if they exist and any linked recipes
loadSource(
manager,
valueEntry,
new Set<string>(),
schemaTracker,
);
if (schemaTracker !== undefined && selector !== undefined) {
schemaTracker.add(manager.toKey(target), selector);
}

// We've loaded the linked doc, so walk the path to get to the right part of that doc (or whatever doc that path leads to),
// then the provided path from the arguments.
return getAtPath(
// If the object we're pointing to is a retracted fact, just return undefined.
// We can't do a better match, but we do want to include the result so we watch this doc
if (valueEntry.value === undefined) {
return [
{ doc: target, docRoot: undefined, path: [], value: undefined },
selector,
];
}
// Otherwise, we can continue with the target.
// an assertion fact.is will be an object with a value property, and
// that's what our schema is relative to.
targetDoc = target;
const targetObj = valueEntry.value as Immutable<JSONObject>;
targetDocRoot = targetObj["value"];
// Load any sources (recursively) if they exist and any linked recipes
loadSource(
manager,
{
doc: targetDoc,
docRoot: targetDocRoot,
path: [],
value: targetDocRoot,
},
[...link.path, ...path] as string[],
tracker,
valueEntry,
new Set<string>(),
schemaTracker,
selector,
);
} finally {
tracker.exit(doc.value!);
}

// We've loaded the linked doc, so walk the path to get to the right part of that doc (or whatever doc that path leads to),
// then the provided path from the arguments.
return getAtPath(
manager,
{
doc: targetDoc,
docRoot: targetDocRoot,
path: [],
value: targetDocRoot,
},
[...link.path, ...path] as string[],
tracker,
schemaTracker,
selector,
);
}

// Recursively load the source from the doc ()
Expand Down Expand Up @@ -677,19 +665,14 @@ export class SchemaObjectTraverser<K, S> extends BaseObjectTraverser<K, S> {
return this.isValidType(schemaObj, "number") ? doc.value : undefined;
} else if (Array.isArray(doc.value)) {
if (this.isValidType(schemaObj, "array")) {
if (this.tracker.enter(doc.value)) {
try {
return this.traverseArrayWithSchema(doc, {
schema: schemaObj,
rootSchema: schemaContext.rootSchema,
});
} finally {
this.tracker.exit(doc.value);
}
} else {
console.log("Cycle detected", JSON.stringify(doc));
using t = this.tracker.include(doc.value, doc);
if (t === null) {
return null;
}
return this.traverseArrayWithSchema(doc, {
schema: schemaObj,
rootSchema: schemaContext.rootSchema,
});
}
return undefined;
} else if (isObject(doc.value)) {
Expand All @@ -701,19 +684,14 @@ export class SchemaObjectTraverser<K, S> extends BaseObjectTraverser<K, S> {
// TODO(@ubik2): it might be technically ok to follow the same pointer more than once, since we might have
// a different schema the second time, which could prevent an infinite cycle, but for now, just reject these.
} else if (this.isValidType(schemaObj, "object")) {
if (this.tracker.enter(doc.value)) {
try {
return this.traverseObjectWithSchema(doc, {
schema: schemaObj,
rootSchema: schemaContext.rootSchema,
});
} finally {
this.tracker.exit(doc.value);
}
} else {
console.log("Cycle detected", JSON.stringify(doc));
using t = this.tracker.include(doc.value, doc);
if (t === null) {
return null;
}
return this.traverseObjectWithSchema(doc, {
schema: schemaObj,
rootSchema: schemaContext.rootSchema,
});
}
}
}
Expand Down Expand Up @@ -837,16 +815,11 @@ export class SchemaObjectTraverser<K, S> extends BaseObjectTraverser<K, S> {
// but we may have a pointer cycle of docs, and we've finished resolving
// the pointer now. To avoid descending into a cycle, track entry to the
// doc we were called with (not the one we resolved, which may be a pointer).
if (this.tracker.enter(doc.value!)) {
try {
return this.traverseWithSelector(newDoc, newSelector!);
} finally {
this.tracker.exit(doc.value!);
}
} else {
console.log("Cycle detected", JSON.stringify(doc));
using t = this.tracker.include(doc.value!, doc);
if (t === null) {
return null;
}
return this.traverseWithSelector(newDoc, newSelector!);
}
}

Expand Down