Discussion: Clone strategy for ModelFile.filter and BaseModelManager.filter

[mttrbrts]

Context

PR #1242 fixes a real mutation bug where BaseModelManager.filter mutates imp.types in-place on import objects that are shared with the original ModelFile AST (because ModelFile.filter only did a shallow spread of this.ast).

The fix adds imports: this.ast.imports?.map(imp => ({...imp})) — a shallow clone of each import object. This is sufficient for the current mutation pattern, but raises a broader design question.

Question

What clone strategy should ModelFile.filter (and related filtering operations) use when producing a "copy" of the AST?

Options

1. Shallow clone (status quo after #1242)

imports: this.ast.imports?.map(imp => ({...imp}))

Pros:

• Fast, zero dependencies, native spread
• Sufficient for current mutation patterns (property reassignment only)

Cons:

• Fragile — if future code mutates nested arrays in-place (e.g. imp.aliasedTypes.push(...) or imp.types[0] = 'Foo'), the bug silently reappears
• Implicit contract that callers must only reassign properties, never mutate nested structures
• Each new nesting level requires remembering to add another spread

2. Targeted deep clone

imports: this.ast.imports?.map(imp => ({
    ...imp,
    types: imp.types ? [...imp.types] : undefined,
    aliasedTypes: imp.aliasedTypes ? imp.aliasedTypes.map(a => ({...a})) : undefined,
}))

Pros:

• Covers known nested structures explicitly
• Still fast, no runtime overhead beyond what's needed

Cons:

• Must be maintained as the import schema evolves
• Verbose

3. structuredClone (or equivalent full deep clone)

const ast = structuredClone(this.ast);
ast.declarations = declarations;

Pros:

• Fully independent copy — immune to any future mutation pattern
• Simpler mental model ("the filtered copy shares nothing")
• Single call, no maintenance burden as schema grows

Cons:

• Copies more than needed (entire AST including declarations that are immediately overwritten)
• Slightly slower for large models (though imports are typically small)
• Requires Node ≥ 17 (already the case for v4)

4. Immutable-by-convention (flip the responsibility)

Instead of cloning in ModelFile.filter, require BaseModelManager.filter to never mutate the objects it receives — always create new arrays/objects when filtering.

Pros:

• No cloning cost at all
• Pushes toward functional/immutable patterns throughout

Cons:

• Hard to enforce without Object.freeze or TypeScript Readonly<> types
• Bug-prone — any single mutation site breaks the contract silently

My Thoughts

There isn't one correct answer — it's a tradeoff between performance, safety, and maintainability. The import AST is small (typically a handful of objects), so performance differences between shallow and deep cloning are negligible in practice. The real question is about the contract: should filter() guarantee a fully independent copy, or should callers know what they can/cannot mutate?

For a metamodel-driven system like Concerto where the AST schema can evolve, option 3 (structuredClone) or option 4 (immutable convention + types) might provide the most durable guarantees.

Related

• PR fix(*): clone imports in ModelFile.filter to prevent mutation #1242 (immediate fix)
• packages/concerto-core/src/introspect/modelfile.ts — ModelFile.filter()
• packages/concerto-core/src/basemodelmanager.ts — BaseModelManager.filter()

[mohabdulkadir-docu-copilot]

I am inclined to the immutable by design direction, the practical path would be Readonly AST types plus an ESLint rule. Compile-time and CI enforcement, zero runtime cost.

[github-actions]

This issue is stale because it has been open with no activity. Remove the stale label or comment to keep it active. Only items with maintainer engagement are auto-closed.