fix visitor related issue in MethodProvider. #7682
Conversation
haiyuazhang
requested review from
m-nash,
JoshLove-msft,
jorgerangel-msft and
christothes
as code owners
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
...es/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/MethodProvider.cs
Outdated
Show resolved
Hide resolved
haiyuazhang
force-pushed
the
haiyzhan/fix_in_methodProvider
branch
from
3493f9b to
e748091
Compare
haiyuazhang
force-pushed
the
haiyzhan/fix_in_methodProvider
branch
from
82e7725 to
d62c5c7
Compare
haiyuazhang
force-pushed
the
haiyzhan/fix_in_methodProvider
branch
from
d62c5c7 to
2ba76bd
Compare
|
No changes needing a change description found. |
| @@ -20,6 +20,8 @@ public class MethodProvider | |||
|
|
|||
| public TypeProvider EnclosingType { get; } | |||
|
|
|||
| private bool _rebuildXmlDocsOnAccept; | |||
There was a problem hiding this comment.
why not just cache xmlDocProvider?
There was a problem hiding this comment.
XmlDocs should already be cached.
There was a problem hiding this comment.
We can't directly check XmlDocs, because it falls back to Build if the input xmlDocProvider is null, we should check the input xmlDocProvider from user, either from constructor or from update method.
There was a problem hiding this comment.
Why is it a problem that XmlDocs falls back to BuildXmlDocs? We just care what XmlDocs is at the time when we are deciding whether to rebuild it or not. It doesn't matter what the history is.
There was a problem hiding this comment.
As explained in #7682 (comment), we need to check if XmlDocs is null or Empty or InheritDocs, which might be the user input for it, and it might grow.
Basically, we should check if user has set it intentionally, if not we should rebuild it.
So, I think it would be easier to check if the user input is null.
...erator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ScmMethodProviderCollection.cs
Outdated
Show resolved
Hide resolved
| @@ -100,6 +105,10 @@ public void Update( | |||
| } | |||
|
|
|||
| Signature = updated.Signature; | |||
| if (_rebuildXmlDocsOnAccept) | |||
There was a problem hiding this comment.
We should just check if XmlDocs is currently not null.
There was a problem hiding this comment.
change the condition to if (XmlDocs is not null) will bring large regen diffs: see here #7709. I found to leave what the current docs in the test projects as what it is ,we should avoid rebuild docs when
- xmlDocProvider in constructor is not null
- XmlDocs is not explicitly updated by Update method (xmlDocprovider parameter is not null)
There was a problem hiding this comment.
We should check null or XmlDocProvider.Empty.
There was a problem hiding this comment.
And it could be xmlDocProvider.InheritDocs, so I think we should check if the user input xmlDocProvider is not null.
If we check XmlDoc, then we need to check if it is null or Empty or InheritDocs, there might be more static values we are going to add in the future.
| @@ -100,6 +102,11 @@ public void Update( | |||
| } | |||
|
|
|||
| Signature = updated.Signature; | |||
| var updatedParameterNames = Signature.Parameters.Select(p => p.Name).ToList(); | |||
| if (!originalPartaMeterNames.SequenceEqual(updatedParameterNames) || XmlDocs is null) | |||
There was a problem hiding this comment.
adding XmlDocs.IsEmpty into the condition will introduce regen diff
There was a problem hiding this comment.
There might be other case that we should rebuild xmlDocs, which can be added later. My primary goal for this PR is to unblock Azure/azure-sdk-for-net#50776.
There was a problem hiding this comment.
If we want to reuse XmlDoc.
if (XmlDoc is null || XmlDoc == XmlDocHelper.InheritDocs || XmlDoc == XmlDoc.Empty) should work.
There was a problem hiding this comment.
Had an offline discussion with @JoshLove-msft, we can have something like below to unblock this
if (Signature.Equals(updated.Signature))
{
Signature = updated.Signature;
if (XmlDocs is null && XmlDocs != XmlDocProvider.Empty && XmlDocs != XmlDocProvider.InheritDocs)
{
XmlDocs = MethodProviderHelpers.BuildXmlDocs(Signature);
}
}There was a problem hiding this comment.
Should be !Signature.Equals(updated.Signature)
There was a problem hiding this comment.
Also, I think we would need to use the MethodSignatureComparer.Equals
There was a problem hiding this comment.
Signature and updated.Signature may point to the same object, we need to find other solution if only Comparing parameter names change is not good engough.
There was a problem hiding this comment.
Since it is always the same instance, which is I think a good thing, we should remove the signature assignment in L104.
And the only option we have is to let the user update signature or rebuild xml docs.
The method parameter name may has been changed by the visitor, so we need to rebuild the xmlDoc for the method as well.