concerto-analysis: Compare routes MapDeclaration and ScalarDeclaration through class-declaration comparison path

[yashhzd]

Summary

Compare.compareModelFiles in concerto-analysis passes getAllDeclarations() to compareClassDeclarations, which includes MapDeclaration and ScalarDeclaration instances. Since neither extends ClassDeclaration, this causes wrong finding keys for added/removed maps and scalars, and reveals that dedicated add/remove comparers for these types are missing entirely.

Root Cause

In packages/concerto-analysis/src/compare.ts:

private compareModelFiles(comparers: Comparer[], a: ModelFile, b: ModelFile) {
    comparers.forEach(comparer => comparer.compareModelFiles?.(a, b));
    this.compareClassDeclarations(comparers, a.getAllDeclarations(), b.getAllDeclarations());
    this.compareMapDeclarations(comparers, a.getMapDeclarations(), b.getMapDeclarations());
    this.compareScalarDeclarations(comparers, a.getScalarDeclarations(), b.getScalarDeclarations());
}

ModelFile.getAllDeclarations() (in concerto-core/lib/introspect/modelfile.js) returns the raw this.declarations array, which contains all declaration types — including MapDeclaration and ScalarDeclaration. These are pushed alongside ClassDeclaration subtypes in fromAst().

However, MapDeclaration and ScalarDeclaration both extend Declaration directly — not ClassDeclaration:

Declaration
├── ClassDeclaration (→ ConceptDeclaration, AssetDeclaration, EnumDeclaration, etc.)
├── MapDeclaration
└── ScalarDeclaration

So compareClassDeclarations receives non-ClassDeclaration instances, and the compareClassDeclaration comparers in class-declarations.ts fire for them.

Impact

1. Wrong finding keys for added/removed maps and scalars

When a MapDeclaration is added to a model, classDeclarationAdded fires and reports:

key: "class-declaration-added"
message: "The map \"MyMap\" was added"

The message is cosmetically correct (because getDeclarationType handles MapDeclaration), but the finding key is class-declaration-added instead of a hypothetical map-declaration-added. Same for removed maps, and for added/removed scalars.

This matters because downstream tooling that processes findings by key will misclassify these changes. The semver rules in compare-config.ts also map by key — class-declaration-added is MINOR, but a project might want different semver rules for map vs class additions.

2. Missing add/remove comparers for Map and Scalar

The dedicated comparers only handle type changes for existing declarations:

• map-declarations.ts → mapDeclarationTypeChanged (key/value type changes only, returns early if !a || !b)
• scalar-declarations.ts → scalar validator/extends/default-value changes (returns early if !a || !b)

There are no map-declaration-added, map-declaration-removed, scalar-declaration-added, or scalar-declaration-removed comparers. The only reason added/removed maps and scalars produce any finding today is because they leak through the class-declaration path.

If someone fixes the getAllDeclarations() call to getClassDeclarations() without also adding these comparers, added/removed maps and scalars would become completely invisible — zero findings emitted.

3. Type safety violation for custom comparers

compareClassDeclaration (singular) fires comparer.compareClassDeclaration?.(a, b) before the instanceof MapDeclaration guard:

private compareClassDeclaration(comparers: Comparer[], a: ClassDeclaration, b: ClassDeclaration) {
    comparers.forEach(comparer => comparer.compareClassDeclaration?.(a, b)); // fires for Maps/Scalars
    if(a instanceof MapDeclaration || b instanceof MapDeclaration) { return; }
    if(a instanceof ScalarDeclaration || b instanceof ScalarDeclaration) { return; }
    this.compareProperties(comparers, a.getOwnProperties(), b.getOwnProperties());
}

Custom ComparerFactory implementations that call ClassDeclaration-specific methods (getOwnProperties(), getSuperType(), isAbstract()) on the received object will get a runtime error, because MapDeclaration and ScalarDeclaration don't have these methods.

4. Unnecessary double-processing

For matching (not added/removed) map and scalar declarations, compareClassDeclarations processes them first (comparers fire, then the instanceof guard returns early), and then compareMapDeclarations/compareScalarDeclarations processes them again through the correct path. This is wasteful.

Existing awareness

The TODO comment in class-declarations.ts suggests this is partially known:

// todo rename to declaration.ts , rename all classDeclaration -> declaration

Proposed Fix

This requires two coordinated changes:

Part A — Fix the routing in compareModelFiles:

Replace getAllDeclarations() with getClassDeclarations() (which already exists on ModelFile and uses instanceof ClassDeclaration filtering):

private compareModelFiles(comparers: Comparer[], a: ModelFile, b: ModelFile) {
    comparers.forEach(comparer => comparer.compareModelFiles?.(a, b));
    this.compareClassDeclarations(comparers, a.getClassDeclarations(), b.getClassDeclarations());
    this.compareMapDeclarations(comparers, a.getMapDeclarations(), b.getMapDeclarations());
    this.compareScalarDeclarations(comparers, a.getScalarDeclarations(), b.getScalarDeclarations());
}

Part B — Add missing add/remove comparers and rules:

Add map-declaration-added / map-declaration-removed comparers in map-declarations.ts and scalar-declaration-added / scalar-declaration-removed comparers in scalar-declarations.ts, following the same pattern as class-declarations.ts.

Add corresponding rules in compare-config.ts:

'map-declaration-added': CompareResult.MINOR,
'map-declaration-removed': CompareResult.MAJOR,
'scalar-declaration-added': CompareResult.MINOR,
'scalar-declaration-removed': CompareResult.MAJOR,

Part C — Strengthen test assertions:

The existing tests use expect.arrayContaining([ expect.objectContaining({...}) ]), which verifies a finding exists but does not detect duplicates or wrong keys. Consider adding toHaveLength checks or exact-match assertions for finding counts.

Test to reproduce

it('should not emit class-declaration-added for a new MapDeclaration', () => {
    const [modelA, modelB] = getModelFiles(
        'namespace org.test@1.0.0',
        `namespace org.test@1.0.0
         map MyMap { o String, o String }`
    );
    const results = new Compare().compare(modelA, modelB);
    // Bug: this currently finds a "class-declaration-added" finding
    const classFindings = results.findings.filter(f => f.key === 'class-declaration-added');
    expect(classFindings).toHaveLength(0); // Fails today — gets 1
});

I'm happy to submit a PR for this if the approach looks right.