[Bug]: sdkPackage.models is missing Error type

Azure / typespec-azure

About TypeSpec Azure Libraries

https://azure.github.io/typespec-azure/

MIT License

14 stars 40 forks source link

[Bug]: sdkPackage.models is missing Error type #1412

Open jhendrixMSFT opened 2 months ago

jhendrixMSFT commented 2 months ago

Describe the bug

Model FailedLockToken contains field error of type Azure.Core.Foundations.Error but it's missing in the array of model types. As a result, the type isn't emitted during codegen.

Reproduction

Build https://github.com/Azure/azure-rest-api-specs/tree/main/specification/eventgrid/Azure.Messaging.EventGrid with tcgc version 0.44.3

Checklist

[X] Follow our Code of Conduct
[X] Check that this issue is about the Azure libraries for typespec. For bug in the typespec language or core libraries file it in the TypeSpec repo
[X] Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
[X] The provided reproduction is a minimal reproducible example of the bug.

tadelesh commented 1 month ago

it is by design. with current tcgc logic, all azure core model will be filtered in sdkPackage.models if filter-out-core-models set to true. but the property type used in FailedLockToken will still refer to that model. @iscai-msft i'm thinking of not do any filtering for tcgc output, leave them to emitter is more flexiable.

iscai-msft commented 3 weeks ago

yeah i'm starting to agree that filtering out core models might be more effort than it's worth. Let me bring this up during discussions on wednesday since this will require emitters to modify their own logic

tadelesh commented 2 weeks ago

We have discussed this issue in DPG sync ups. We all agree that languages' emitter should already deal with the core model's in emitter's logic, so it is safe to remove the filtering logic in TCGC. @jhendrixMSFT , I'm wondering if Go has filtering logic for core models? The most common scenario is the ErrorResponse model. With current TCGC logic, sdkPackage.models will not contain that model, but after change, it will.

jhendrixMSFT commented 2 weeks ago

When you say ErrorResponse model that sounds different from Azure.Core.Foundations.Error. While we can treat these the same, shouldn't services be using Azure.Core.Foundations.Error instead of defining their own error models (at least when the expectation is for a core error to be returned)?

tadelesh commented 2 weeks ago

Sorry, I do mean Azure.Core.Foundations.Error. It will be in sdkPackage.models if we do not filtering core models, and Go needs to ignore generating then.

jhendrixMSFT commented 2 weeks ago

Yep we can filter these out (I think we already do).