Azure / typespec-azure

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

[linter]discourage to use anonymous model and always give a model name #1727

Open MaryGao opened 1 month ago

MaryGao commented 1 month ago

Clear and concise description of the problem

Close typespec one filed an issue in typespec-azure repo.

Offline discussed with @ArcturusZhang and we think anomymous model may not be a good practice and we should encourage to give a name for models especially for exposed public APIs.

Most of downstreaming language emitters may not have ability to handle inline expressions like typespec so they need to GUESS a name for anomymous models. This could introduce uncertainty if downstreaming name rule is different and for example the name rule introduced breakings between m4 and tcgc and one issue happened in common types is tracked here.

Even we could find a name rule and these names could be fragile and it would be changed with its context or code order if conflicting happened.

For example in TCGC we try to guess this name according to context, including parent model name + property name + model type. So the name could be changed once any of these factors changed. Also we may name some models during conflicting and any code order changes would impact the generated code.

model Ab {
  c: {
    foo: string;
  };
}

// vs
model A {
  b: {
    c: {
      foo: string;
    };
  };
}

Can we add a linter to discourage the anonymous model in typespec? Especially in azure could we avoid using anonymous model then?

Checklist

archerzz commented 4 weeks ago

I think we should re-consider whether we should suppress the occurrence of anonymous models.

MaryGao commented 4 weeks ago

@timotheeguerin regarding your comments, here are some of my thinkings.

At the TypeSpec standpoint do we agree this is a good practice or anti-patterns? how should we guide customers to use anonymous model correctly? Can we set a clear bar for which cases should be allowed and which should be discouraged? Implementing something in TCGC is just kinda patch for TypeSpec or long-term solution?

If we are talking about this in Azure scope, we could set some linters in tcgc because in it we have a chance to calculate models which are supposed to be public then any anonymous models could be reported. The drawback is it is too late for customers the errors happen during runtime and any language emitters are called.

On the other hand we could do this linting in azure-core lib, which could report it in early stage. Imagine customers just writes their specs and they could be guided to not use this. This maybe better integrate with our IDE.

timotheeguerin commented 3 weeks ago

as said in the other issue we cannot do this in azure core it was tried by David when he was working on this [See comment] (https://github.com/Azure/typespec-azure-pr/issues/2367#issuecomment-1540087482) we cannot do that relyably as we don't know what you would care about in clients vs what might just be building blocks. There is no issue implementing this in tcgc.

If we have better way to deal with traits and building blocks in the future we could consider a more generic approach but here its just not possible.

m-nash commented 3 weeks ago

I think its reasonable for a language emitter (maybe tcgc) to throw a warning when an anonymous model is found. We plan to do this in csharp for both branded and unbranded and the user can choose to ignore them if they want.

I think I agree that a tsp level warning doesn't make sense since I can imagine some emitters don't care about anonymous models such as the openapi emitter which has no issue dealing with them.

ArthurMa1978 commented 3 weeks ago

Talked with @m-nash , I agreed with Michael's point:

For anon models its 2 things

1. Poor API shape.  You can get really long names that are ugly and don't make sense.
2. Breaking change potential.  If the location changes at all or if there is an ambiguous anon model in another location that matches the shape it can result in the autogenerated name to change which is a break.

If you ignore the warning you are saying you accept the responsibility of both of those outcomes.

It totally makes sense, so we'd like to skip the ugly name check during apiview review. This will save both the service team and our team the additional effort required to resolve the naming of anonymous models.

timotheeguerin commented 3 weeks ago

Also just for clarity this is a great candidate for using the tcgc linter framework setup here https://github.com/Azure/typespec-azure/pull/1208