search

microsoft / typespec

https://typespec.io/
MIT License
4.55k stars 222 forks source link

Duplicated `GetAttributes` and `BuildAttributes` in `TypeProvider` #4524

Open ArcturusZhang opened 2 months ago

ArcturusZhang commented 2 months ago

There are duplicated APIs for attributes:

  1. https://github.com/microsoft/typespec/blob/26c2924413b0452441b9d401e34c6a44143f9e2d/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/TypeProvider.cs#L30
  2. https://github.com/microsoft/typespec/blob/26c2924413b0452441b9d401e34c6a44143f9e2d/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/TypeProvider.cs#L214

For attributes on types, I think we should do the same as we did for other members such as properties, fields, methods, ctors, etc

JoshLove-msft commented 1 month ago

GetAttributes was moved to NamedTypeSymbolProvider and is public as we needed to make NamedTypeSymbolProvider public. It might be better to consolidate into AttributeStatement now that it is public. We could then make NamedTypeSymbolProvider internal.

ArcturusZhang commented 1 month ago

So I think we could (and should) combine them since both of them are public now. It makes no sense if we remove the AttributeStatement part and use the AttributeData for the write - this breaks how our statement system works. It also has issues if we convert the AttributeData from the customized code into AttributeStatement because this statement is having ValueExpressions as its arguments, which is very unintuitive to get those values out again once we put them in.

So how about we meet in a middle place that we modify the AttributeStatement to take in IReadOnlyList<object> as its arguments and similar for its positional arguments? In this way, we could easily get those values out, and the writer part could wrap them by calling Literal(value) to construct the expression and then write them.

Would we ever have a situation that an attribute is writing something other than a compiling time constant? I think the arguments of an attribute could only be compile-time constants.

JoshLove-msft commented 1 month ago

Would we ever have a situation that an attribute is writing something other than a compiling time constant? I think the arguments of an attribute could only be compile-time constants.

Nope the args need to be compile-time constants. I think your suggestion makes sense.

JoshLove-msft commented 1 month ago

@m-nash had another suggestion - introduce an AttributeProvider type that we would convert the AttributeData into. In the TypeProviderWriter we would convert into an AttributeStatement which could now be internal. TypeProvider would contain a public IReadOnly<AttributeProvider> Attributes property.