Enhance graph.findConnectedGraph to use type hierarchy

[dselman]

Enhance graph.findConnectedGraph to use type hierarchy

[mttrbrts]

I'd argue that this is a bug. We should be creating edges that follow the type hierarchy in the GraphVisitor.

[dselman]

Similar (but different...) would be decorators that referenced types.

[dselman]

Also Map key and value types.

[sanketshevkar]

I'd argue that this is a bug. We should be creating edges that follow the type hierarchy in the GraphVisitor.

I think I agree with Matt, earlier I thought it could be a addition, but now as I played around it, it feels like a bug.
From the perspective of the mermaid diagram it makes sense and feels like a new addition. But if we want to used the tree shaken model, as given in the example we won't be able instantiate the abstract Pet concept and would need ClassDeclarations for Cat and Dog concepts.

Coming to the point, should we still add the fix as optional? I don't think so. But downside I saw is that after the fix the graph become Directed Cyclic graph, this makes the Flowchart representation a bit clunky.

[sanketshevkar]

Similar (but different...) would be decorators that referenced types.

Apart from the bug we identified from today's tech-wg call.
I have used the following REPL to describe two scenarios.
Referring to both scenarios, won't having a fully qualified type name be more beneficial in decorator arguments object?
It would be easier to get the derived types especially in cases where we might have similar named types in two different namespaces?

currently:
{ type: 'Identifier', name: 'Color', array: false }
suggested change
{ type: 'Identifier', name: '[email protected]', array: false }

[dselman]

We should make this consistent with what we are doing for other type references, e.g. TypeIdentifier for super class. What does that look like? I also noticed that your code in the REPL seems to be just doing a toString on the object, look at the AST instead, or the API methods.

[sanketshevkar]

I also noticed that your code in the REPL seems to be just doing a toString on the object, look at the AST instead, or the API methods.

I am bit confused about the toString part

• I am not explicitly doing any toString, is there implicit call to toString you are referring to?
• Referring to the AST, this how the arguments object looks like this and even here we don't any reference to the fully qualified type.

"arguments": [
                  {
                    "$class": "[email protected]",
                    "type": {
                      "$class": "[email protected]",
                      "name": "Color"
                    },
                    "isArray": false,
                    "location": {
                      "$class": "[email protected]",
                      "start": {
                        "offset": 117,
                        "line": 9,
                        "column": 17,
                        "$class": "[email protected]"
                      },
                      "end": {
                        "offset": 122,
                        "line": 9,
                        "column": 22,
                        "$class": "[email protected]"
                      }
                    }
                  }
                ]

[sanketshevkar]

We should make this consistent with what we are doing for other type references, e.g. TypeIdentifier for super class. What does that look like?

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

"arguments": [
                  {
                    "$class": "[email protected]",
                    "type": {
                      "$class": "[email protected]",
                      "name": "Color"
                    }, .....

"superType": {
              "$class": "[email protected]",
              "name": "Color"
            }

But for super class when I do
declaration.getSuperClass() I can get a resolved fully qualified type [email protected]

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 ?

[mttrbrts]

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

Yes, you're right, it should. There's a comment in the code that questions this design too!
https://github.com/accordproject/concerto/blob/main/packages/concerto-core/lib/introspect/decorator.js#L80

Can you open an issue for an enhancement to the Introspect API to make it more consistent here, please?

[sanketshevkar]

accordproject/concerto#797
Added it to v4