Azure / typespec-azure

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

introduce `name` and `crossLanguageDefinitionId` to `SdkBuiltInType`, `SdkDateTimeType` and `SdkDurationType` #1015

Closed ArcturusZhang closed 2 months ago

ArcturusZhang commented 3 months ago

Fixes https://github.com/Azure/typespec-azure/issues/1010 Fixes https://github.com/Azure/typespec-azure/issues/997 Fixes https://github.com/Azure/typespec-azure/issues/1106

Changes and migration guides

Changes

  1. remove the following types from SdkBuiltInType:
    • uuid (azure type)
    • ipV4Address (azure type)
    • ipV6Address (azure type)
    • eTag (azure type)
    • armId (azure type)
    • azureLocation (azure type)
    • password (removed)
    • guid (removed)
    • uri (removed)
    • ipAddress (removed)
  2. @format can no longer change a string type to above azure types (uuid, eTag, etc).
  3. add name, crossLanguageDefinitionId and baseType to SdkBuiltInType, SdkDateTimeType and SdkDurationType.
  4. now scalars defined using scalar keyword will be parsed into either SdkBuiltInType, SdkDateTimeType or SdkDurationType depending on the base type, with its name and crossLanguageDefinitionId. @encode will be added to the scalar type, and will not propagate to its base type.
  5. isSdkFloatKind now returns false for decimal and decimal128.

Migration guides for emitters

The major breaking change introduced by this change is the removal of azure related kinds for SdkBuiltInType. Therefore for code such as:

if (type.kind === "azureLocation")
{
  // do something for azure-location
}

should be changed to the following if the emitter is doing something special for azure-location:

if (type.kind === "string" && type.crossLanguageDefinitionId === "Azure.Core.azureLocation")
{
  // do something for azure-location
}

or just change it to this:

if (type.kind === "string")
{
  // treat azure-location as a string
}

For those removed kinds, they no longer exist therefore it should be safe just remove related code snippets.

Summary

This PR includes the following changes:

  1. remove the following types from SdkBuiltInType:
    • uuid (azure type)
    • ipV4Address (azure type)
    • ipV6Address (azure type)
    • eTag (azure type)
    • armId (azure type)
    • azureLocation (azure type)
    • password (removed)
    • guid (removed)
    • uri (removed)
    • ipAddress (removed)
  2. @format can no longer change a string type to above azure types (uuid, eTag, etc).
  3. add name, crossLanguageDefinitionId and baseType to SdkBuiltInType, SdkDatetimeType and SdkDurationType.
  4. now scalars defined using scalar keyword will be parsed into either SdkBuiltInType, SdkDatetimeType or SdkDurationType depending on the base type, with its name and crossLanguageDefinitionId. @encode will be added to the scalar type, and will not propagate to its base type.
  5. isSdkFloatKind now returns false for decimal and decimal128.

Reasons and motivations

TCGC should be "linguistic neutrality" which does not favor or disfavor any language and only represent the structures defined in a tsp file/files. Also I think it should be neutral for 3rd party extensions, and azure should be regarded as our first 3rd party extension. Therefore we need a way to be able to handle those azure types without making them special in TCGC's code.

The solution that this PR is applying is similar to the solution we have for models in azure core template. Instead of creating a special kind for models in azure core template (such as the ResponseError, TrackedResource, Resource or ExtendedLocation), we just parse them as regular models with their own name and tsp namespace, so that the downstream genreators could know where these types are defined, and if they want, they could do a filter, to handle those special types. Same thing could apply to scalars, for a scalar defined in azure core template:

namespace Azure.Core;
scalar azureLocation extends string;

for those would care this, this stands for an "azure location", for those would not care, this is just a string. Therefore this PR introduces name and crossLanguageDefinitionId to SdkBuiltInType just like we have in models, this scalar now will be parsed as:

{
    kind: "string",
    name: "azureLocation",
    tspNamespace: "Azure.Core",
    baseType: {
        kind: "string",
        name: "string",
        tspNamespace: "TypeSpec"
    }
}

The crossLanguageDefinitionId is important for downstream generators to know whether this scalar is defined. baseType here is also important and we must have this, because we do not want to see if users write scalars extending some of the scalars from azure core template, and it ends up with a string. For example, based on the fact that we have the above azureLocation defined in our azure core template, and one could write:

scalar myLocation extends azureLocation;

model Foo {
    bar: myLocation;
}

in this case, I think the generator should generate the bar property of Foo model as AzureLocation type. But if we do not have the baseType, we only have this:

{
    kind: "string",
    name: "myLocation",
    tspNamespace: "My.Service"
}

which has nothing with azureLocation which is not acceptable.

azure-sdk commented 3 months ago

All changed packages have been documented.

Show changes ### `@azure-tools/typespec-client-generator-core` - _breaking_ [✏️](https://github.com/ArcturusZhang/typespec-azure/edit/add-scalar-type/.chronus/changes/add-scalar-type-2024-5-14-13-23-7.md?pr=/Azure/typespec-azure/pull/1015) > refactor tcgc build-in types, please refer pr's description for details and migration guides ### `@azure-tools/typespec-client-generator-core` - _fix_ [✏️](https://github.com/ArcturusZhang/typespec-azure/edit/add-scalar-type/.chronus/changes/namespace_client_name-2024-6-18-19-15-52.md?pr=/Azure/typespec-azure/pull/1015) > have @clientName work for operation groups as well ### `@azure-tools/typespec-client-generator-core` - _feature_ [✏️](https://github.com/ArcturusZhang/typespec-azure/edit/add-scalar-type/.chronus/changes/xml_usage-2024-6-19-15-33-55.md?pr=/Azure/typespec-azure/pull/1015) > add xml usage and change enumvalue arg representation in generic decorators
azure-sdk commented 3 months ago

You can try these changes at https://cadlplayground.z22.web.core.windows.net/typespec-azure/prs/1015/

Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/typespec-azure/prs/1015/

ArcturusZhang commented 3 months ago

Since this is breaking, can you also write a quick migration guide for emitters? Thanks! We also need approval from all languages

Overall I like the direction, thanks for your contribution @ArcturusZhang

Usually where we write the migration? In the changelog?

ArcturusZhang commented 3 months ago

@iscai-msft I added some guidance in the changelog.

haolingdong-msft commented 3 months ago

@ArcturusZhang I understand and like your your 2 and 3 points in your description. Would like to understand more on 1, why do we remove those types?

ArcturusZhang commented 3 months ago

@ArcturusZhang I understand and like your your 2 and 3 points in your description. Would like to understand more on 1, why do we remove those types?

several reasons:

  1. because we have the name and namespace, we already have the ability to represent azure stuffs, we do not need duplicates.
  2. removing them is "making our library consistent with rules for scalar types which built up from std types"
  3. like I have mentioned in the motivation:

    Also I think it should be neutral for 3rd party extensions, and azure should be regarded as our first 3rd party extension. Therefore we need a way to be able to handle those azure types without making them special in TCGC's code.

we should not make "azure" special in such a fundamental library if we do not have to. This should benefit in the future if we need to trim azure off from TCGC because we already did it.

haolingdong-msft commented 3 months ago

I think Java will still need the tspNamespace info to calculate our schema's namespace. [code] @weidongxu-microsoft please correct me if I'm wrong.

weidongxu-microsoft commented 3 months ago

I think Java will still need the tspNamespace info to calculate our schema's namespace. [code] @weidongxu-microsoft please correct me if I'm wrong.

We can have another discussion or PR to solve for Java namespace problem.

Dapeng's PR handles a different problem.

haolingdong-msft commented 3 months ago

I think Java will still need the tspNamespace info to calculate our schema's namespace. [code] @weidongxu-microsoft please correct me if I'm wrong.

We can have another discussion or PR to solve for Java namespace problem.

Dapeng's PR handles a different problem.

Yeah, sure, just saw tspNamespace is removed, so raise this concern.

tadelesh commented 3 months ago

@ArcturusZhang could you also address this issue in this pr?

tadelesh commented 2 months ago

also seems this pr has a wrong merge from main that need to remove before merge.