search

Azure / typespec-azure

About TypeSpec Azure Libraries
https://azure.github.io/typespec-azure/
MIT License
11 stars 37 forks source link

`model Foo extends Array<T>{}` is parsed as a model in TCGC #1020

Closed ArcturusZhang closed 2 months ago

ArcturusZhang commented 3 months ago

if we have:

model Foo extends Array<string>{}
model Bar extends int32[] {}

both Foo and Bar will be returned as SdkModelType and appear in the getAllModels's models list. This is incorrect. They should be SdkArrayType.

### Tasks
- [ ] https://github.com/Azure/typespec-azure/issues/1014
ArcturusZhang commented 3 months ago

it is claimed as by design in typespec that extends Array<T> does not inherit the indexer: https://github.com/microsoft/typespec/issues/3597 which I did not totally buy in.

but if typespec will not change this, we could always work this around in TCGC.

Currently we do not have urgent needs for this. Maybe we could convince the typespec team to remove this syntax completely since it stops making sense if the indexer is not inherited.

iscai-msft commented 3 months ago

@ArcturusZhang what is the usecase for this?

ArcturusZhang commented 3 months ago

@ArcturusZhang what is the usecase for this?

there is no real use cases, just coming from the thought that since we could write it without compilation error, then we need to support it properly, such as this case:

// in my own typespec
model MyArray extends Array<int32> {}

The compiler shows no errors on this, but in the TCGC result, MyArray is a model with no properties, therefore an empty model is generated in the downstream SDKs which is incorrect. We should generate the property using this type as an array if we cannot identify its usage

https://github.com/Azure/typespec-azure/issues/1014 this issue is created for a more complicated case such as

// in azure core
model EmbeddingVector<T> is Array<T>;

// in my own tsp
model MyEmbedding extends EmbeddingVector<int32>{}

Assuming we solve the above issue, now MyEmbedding is an array with a proper name and namespace but the information that its base is missing, we have no information about that this type comes from the azure core's embedding vector which might be problematic.

iscai-msft commented 2 months ago

It seems this has been fixed by your pr, going to close this. If there's still an issue, feel free to reopen