Need a method to resolve type inference for decorators with type references

[sanketshevkar]

Feature Request 🛍️

In the AST both superType and decorator TypeIdentifiers have a similar shape.

"arguments": [
                  {
                    "$class": "concerto.metamodel@1.0.0.DecoratorTypeReference",
                    "type": {
                      "$class": "concerto.metamodel@1.0.0.TypeIdentifier",
                      "name": "Color"
                    }, .....

"superType": {
              "$class": "concerto.metamodel@1.0.0.TypeIdentifier",
              "name": "Color"
            }

But for super class when I do declaration.getSuperClass() I can get a resolved fully qualified type workshop@1.0.0.Color

But for reference type decorators I only get [{ type: 'Identifier', name: 'Color', array: false }] when I do decorator.getArguments() , which I don't think is incorrect.

But shouldn't the decorator class have some internal methods that can resolve the type reference, similar to the one we have in classDeclaration ?

Use Case

Possible Solution

Context

Detailed Description

[RINO-GAELICO]

Hi @sanketshevkar it seems that this issue involves adding a helper method inside the decorator class (packages/concerto-core/lib/introspect/decorator.js) that resolves arguments that are neither Boolean, Numbers nor String to a fully qualified type. Is my interpretation correct? May I be assigned to this issue?

[sanketshevkar]

Yes you are correct @RINO-GAELICO. I've assigned this to you! Thanks for showing interest in our project :)

Also design your solution considering #798 as an extension to this issue.

[sanketshevkar]

Also instead of master please point your PR towards the https://github.com/accordproject/concerto/tree/v4.0.0 branch.

[ahmad-kashkoush]

Hi @RINO-GAELICO, what is your progress on this Issue?

[RINO-GAELICO]

Hi @sanketshevkar,

I've reviewed the documentation and attempted to gain a better understanding of the overall architecture. Could you please provide guidance on the following points?

1. I am considering calling the to-be-implemented helper method within decorator.getArguments(). Does this approach align with the requirements? :

if (thing.$class === '${MetaModelNamespace}.DecoratorTypeReference') {
                        this.arguments.push({
                            **type: this._resolveType(thing),**
                            name: thing.type.name,
                            array: thing.isArray
                        });
                    }

2. I'm unsure about the method header and return type. Would something like this be appropriate?

     /**
     * Resolves the type of the decorator argument
     * @param {argument} argument - the decorator argument
     * @return {string} the type of the decorator argument
     */
    _resolveType(argument) {}

Or instead should it return TypeIdentifier ?

3. Can I assume that the Model file of the parent is the same as the decorator? I am thinking of doing something like this to resolve the type:

if (this.parent.getModelFile().isImportedType(argument.type)){ ... } 

Then if no type is found within the namespace we throw an error as per #798

Thank you for your guidance.

[sanketshevkar]

I am considering calling the to-be-implemented helper method within decorator.getArguments(). Does this approach align with the requirements?

I'd suggested implementing this in process method would be better. We might want to resolve the reference during the execution of the constructor itself.

I'm unsure about the method header and return type. Would something like this be appropriate?

/**
* Resolves the type of the decorator argument
* @param {Object} argument - the decorator argument // Idt we have argument as any defined type
* @return {ClassDeclaration} the type of the decorator argument // similar to [this](https://github.com/accordproject/concerto/blob/0d7f108d4961f87782436dd689639ce0f35e97f8/packages/concerto-core/lib/introspect/classdeclaration.js#L155) method
*/
_resolveArguemntType(argument) {}

Can I assume that the Model file of the parent is the same as the decorator?

Parent refers to the property or the classDeclaration on which the decorator is added, so I think it should be in the same ModelFile. @mttrbrts can you please confirm?

 if (this.parent.getModelFile().isImportedType(argument.type)){ ... } 

I am a bit skeptical if this would resolve this issue.

[RINO-GAELICO]

I'd suggested implementing this in process method would be better. We might want to resolve the reference during the execution of the constructor itself.

Yes, sorry I meant within process()

I am a bit skeptical if this would resolve this issue.

According to my understanding Modelfile should be responsible for resolving the type, as it keeps track of the imports from another namespace and the local types.

Therefore I thought we need a way to access the Modelfile, either through the parent (property decorated) or through the decorator itself this.getModelFile()

Is this logic correct?

Thanks for the feedback @sanketshevkar

[mttrbrts]

Parent refers to the property or the classDeclaration on which the decorator is added, so I think it should be in the same ModelFile. @mttrbrts can you please confirm?

Yes, that's right

[sanketshevkar]

According to my understanding Modelfile should be responsible for resolving the type, as it keeps track of the imports from another namespace and the local types. Therefore I thought we need a way to access the Modelfile, either through the parent (property decorated) or through the decorator itself this.getModelFile()

From a high level I think its a fair enough approach. Also regarding

either through the parent (property decorated)

Decorators can be applied on both classDeclarations(concept, enum, asset ...) and also properties.

[RINO-GAELICO]

Thanks @sanketshevkar

I am considering retrieving the ModelFile and then querying whether the type is an imported type. If it is not, we check within the namespace. If nothing is found, an exception is thrown.

Something like this:

`` let classDecl = null;

    const modelFile = this.parent.getModelFile();

    if (modelFile.isImportedType(argument.type.name)) {
        let fullyQualifiedTypeName = modelFile.resolveImport(argument.type.name);
        classDecl = modelFile.getModelManager().getType(fullyQualifiedTypeName);
    } else {
        classDecl = modelFile.getType(argument.type.name);
    }

    if (!classDecl) {
        // THROW AN EXCEPTION TYPE NOT FOUND NEITHER IN THE NAMESPACE NOR IN THE IMPORTS
    }

    return classDecl;

``

Also, is there a particular test that you'd recommend to modify to test this new method?

[sanketshevkar]

@RINO-GAELICO that sounds like a good approach!

I don't have any top of my mind. You'll probably find out when the existing ones break. You might want to create new unit tests for the changes you have made.

[RINO-GAELICO]

Hi @sanketshevkar

Here is my tentative method:

``

_resolveArguemntType(argument) {

    let classDecl = null;

    const modelFile = this.parent.getModelFile();

    if (modelFile.isImportedType(argument.type.name)) {
        let fullyQualifiedTypeName = modelFile.resolveImport(argument.type.name);
        classDecl = modelFile.getModelManager().getType(fullyQualifiedTypeName);
    } else {
        classDecl = modelFile.getType(argument.type.name);
    }

    if (!classDecl) {
        // THROW AN EXCEPTION TYPE NOT FOUND NEITHER IN THE NAMESPACE NOR IN THE IMPORTS
        throw new IllegalModelException('Could not find type ' + this.type.name, modelFile, this.ast.location);
    }

    return classDecl;

}

``

When I run the test I get 5 fails, all with the same message: Error: Internal error: local types are not yet initialized. Do not try to resolve types inside "process".

Which is thrown by ModelFile.getLocalType method.

It seems that when I ask the modelFile to resolve the type it still has not initialized the local types. Could it be because inside the ModeFile constructor process() is called before the localTypes are initialized? Here is the ModeFile constructor code snippet for your reference:

``

   // Set up the decorators.
    this.process();
    // Populate from the AST
    this.fromAst(this.ast);
    // Check version compatibility
    this.isCompatibleVersion();

    // Now build local types from Declarations
    this.localTypes = new Map();
    for(let index in this.declarations) {
        let classDeclaration = this.declarations[index];
        let localType = this.getNamespace() + '.' + classDeclaration.getName();
        this.localTypes.set(localType, this.declarations[index]);
    }

``

In that case I to around this? It seems a circular issue: we try to resolve the types before the types are initialized.

Am I analyzing this correctly? There is probably something I am missing

[sanketshevkar]

I'll have to dig up into this. I think this is good progress. Can you please raise a draft PR, so that I can navigate across the code in your branch?

In your method _resolveArguemntType which line internally leads to calling ModelFile.getLocalType method?

[RINO-GAELICO]

@sanketshevkar

In your method _resolveArguemntType which line internally leads to calling ModelFile.getLocalType method?

modelFile.getType(argument.type.name)

Which is the method that is called to determine the local type.

I created a draft pull request

https://github.com/accordproject/concerto/pull/820

I hope this is helpful. Please, let me know the next steps.

[dselman]

Yes, the error is (thankfully) quite good: Error: Internal error: local types are not yet initialized. Do not try to resolve types inside "process".

The process method is setting up the types in the model file. It's therefore not possible to use the local types from within code called during process.

If you want to do a 1-time initialization after the local types are initialized you should do it within validate. Or, if it is expensive you could do it lazily (on-demand) and cache the result.

[RINO-GAELICO]

Validation inside the decorator class is not implemented yet, and I am wondering what role it would play.

Would it be a good idea to use the validate method inside the classDeclaration as an example? Essentially, a method that sets unique names for each argument in that decorator instance.

@dselman @sanketshevkar Is my understanding correct?

[sanketshevkar]

I think so we should do that. But we need to be aware about this validate method in decorated.js. Decorated is the super class for classDeclarations, properties etc, ie, identifiers that can be decorated using decorators.

This validate will always execute for validating decorators which is currently empty. We just might need to cautious for the case wherein we might have to validate a lot of decorators with type reference.