making a concept abstract or changing its inheritance should be considered a breaking change

[DS-AdamMilazzo]

Bug Report 🐛

1. When comparing two models, Concerto doesn't consider changing a concept from concrete to abstract to be a breaking change, but it is because existing data using the concrete type will no longer validate.
2. It also doesn't detect changes in inheritance, even when that changes the fields in a concept.

Expected Behavior

When comparing two models where the new model changes a concept from concrete to abstract, the change should be detected as a major (breaking) change. When changing the other way, from abstract to concrete, the change should be detected as a minor or patch (non-breaking) change.

When comparing two models where the new model changes a concept's inheritance, the change should be detected as a major (breaking) change. This could potentially be relaxed to a minor change if the new base type is derived (directly or indirectly) from the old base type and the newly intervening base types only add fields that would be allowed to be added directly to the concept at issue (i.e. optional fields whose names don't collide with existing fields).

Current Behavior

Currently, changing a concept between abstract and concrete is not detected as a model change, and neither is changing a concept's inheritance.

Possible Solution

Detect a change to a concept's abstractness in the model comparer and return one of two new result codes. Detect a change in base class and return a new result code (or two new result codes if you want to get fancy).

Steps to Reproduce

First, create two model files. A.cto:

namespace [email protected]
concept B { o String }
concept C extends B { }

and B.cto:

namespace [email protected]
concept B { o String }
abstract concept C { }

Then, run concerto compare --old A.cto --new B.cto
No differences are reported.

Context (Environment)

It just seems like a bug that needs fixing.

Desktop

• OS: Windows 11
• Browser: Chrome
• Version: Concerto 3.16.4; Chrome 130.0.6723.117

Detailed Description

Possible Implementation

In classDeclarationTypeChanged, check if the two concepts are equally abstract and if not return new comparison result type-made-concrete (minor or patch) or type-made-abstract (major).

Also check if the base type has changed and if so return a new comparison result supertype-changed (major) (or concept-extends-changed to parallel the existing scalar-extends-changed result). If you want to get fancy, you could allow some base type changes that are not breaking, as described above in the "Expected Behavior" section.

[Mamatha1718]

Hi @DS-AdamMilazzo , I'd love to tackle this issue. Any ideas where I should start looking?

[DS-AdamMilazzo]

Hi @DS-AdamMilazzo , I'd love to tackle this issue. Any ideas where I should start looking?

See the code I linked to above:

concerto/packages/concerto-analysis/src/comparers/class-declarations.ts

Line 47 in b10d02f

const classDeclarationTypeChanged: ComparerFactory = (context) => ({

It reports incompatibilities in classes. For example, it's currently checking some aspects of the type (computed by getDeclarationType):

        context.report({
            key: 'class-declaration-type-changed',
            message: `The ${aType} "${a.getName()}" changed type from ${aType} to ${bType}`,
            element: a
        });

A new check should be added there to flag the abstractness of the class changing. Going from abstract -> concrete is a minor (non-breaking) change, and going from concrete -> abstract is a major (breaking) change. Search the code for "class-declaration-type-changed" to see where those are defined, and add the new entry there.

[Mamatha1718]

Hi @DS-AdamMilazzo ,
The code I would like to change like this.
Adding Abstractness to check the class comparer.

const isAbstract =(declaration) => declaration.isAbstract( );
if( isAbstract(a) !== isAbstract(b) ) {
const changeType = isAbstract(a) ? abstract to concrete : concrete to abstract ;
const changeSeverity = isAbstract(a) ? minor : major ;
context.report({
key : class-declaration-abstractness-changed ,
message : The class "${a.getName( )}" changed from ${changeType} (${changeSeverity} change) ,
element : a
});
}

Abstract -> Concrete : Minor change.
Concrete -> Abstract : Major change.
will you please let me know if there are any another changes required.
Thank you.

[Mamatha1718]

Hi @DS-AdamMilazzo , The pull request is ready for review.
Thank you.

[DS-AdamMilazzo]

You should link to the PR. I guess it is #974