Refactor XmlDocProvider
#7904
Refactor XmlDocProvider
#7904
Conversation
|
No changes needing a change description found. |
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
…ator otherwise we cannot validate its effect on clients
ArcturusZhang
requested review from
m-nash,
JoshLove-msft,
jorgerangel-msft,
christothes and
jsquire
as code owners
| private int GetHashCode([DisallowNull] ParameterProvider obj) | ||
| { | ||
| // remove type as part of the hash code generation as the type might have changes between versions | ||
| return HashCode.Combine(obj.Name); |
There was a problem hiding this comment.
i change this because the Name property in this class is not immutable - therefore if we use this type as key of dictionary or in hashset in a scenario that its name would change, weird behavior would happen - because of the change of hashcode value, the set would report an instance which we had added before does not exist in the set.
For instance, if we do this:
var set = new HashSet<ParameterProvider>();
set.Add(parameter);
parameter.Update(name: "bar"); // change the name to bar
Console.WriteLine(set.Contains(parameter); // this will return false.
| @@ -271,7 +265,7 @@ private static bool TryBuildMethodArgumentsForOverload( | |||
| return false; | |||
| } | |||
|
|
|||
| var currentParameters = currentMethod.Parameters.ToHashSet(); | |||
| var currentParameters = currentMethod.Parameters.ToHashSet(ParameterProvider.EqualityByNameAndType); | |||
There was a problem hiding this comment.
since in this context, we are trying to compare the parameters by their names and types, we used a comparer here to do that, and its default behavior in a dictionary is by reference equality.
| foreach (var parameter in method.Signature.Parameters) | ||
| { | ||
| if (_visitedParameters.Contains(parameter)) | ||
| { | ||
| // already visited this parameter, skip | ||
| continue; | ||
| } | ||
| _visitedParameters.Add(parameter); | ||
| // modify the parameter name | ||
| parameter.Update(name: parameter.Name + "_modified"); | ||
| } |
There was a problem hiding this comment.
I am not sure if this is the correct usage - I want to use this visitor to change the parameter names by appending a _modified.
But it turns out, some parameter instances are shared between different methods, for instance, between the sync version and async version.
Therefore without this _visitedParameters cache, this modification will be apply to the same parameter instance multiple times giving us multiple _modified suffix.
To make things right, I have to add this visitedParameters mechanism to ensure we only change each parameter once.
Is this the expected behavior?
Also because of this, I have to change the GetHashCode implementation in ParameterProvider to ensure we could get the reference equality comparison.
There was a problem hiding this comment.
I doubt there are realistic scenarios where need to blindly append something to a parameter. The more realistic scenario is that we want to find parameters with a certain name, and update them to a new name. Given that, there should be no need to have a hashset of visited parameters.
There was a problem hiding this comment.
I changed this to "if the parameter name is foo, change it to foo_modified"
| using Microsoft.TypeSpec.Generator.Tests.Common; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace Microsoft.TypeSpec.Generator.ClientModel.Tests.Providers.ClientProviders; |
There was a problem hiding this comment.
nit: I don't think we use file scoped namespaces anywhere in the codebase.
| @@ -333,5 +331,27 @@ public void Update( | |||
| Validation = validation.Value; | |||
| } | |||
| } | |||
|
|
|||
| internal static IEqualityComparer<ParameterProvider> EqualityByNameAndType = new ParameterProviderEqualityComparer(); | |||
There was a problem hiding this comment.
nit: NameAndTypeComparer
| // we must override the GetHashCode method because we overrode Equals | ||
| // but we really do not want to change the behavior of GetHashCode to keep it immutable per instance | ||
| // therefore we use this implementation | ||
| return RuntimeHelpers.GetHashCode(this); // gets the hash code based on object reference |
There was a problem hiding this comment.
How does this solve the problem? If we are getting hashcode based on object reference, won't two different instances end up in different buckets and make the look up fail?
There was a problem hiding this comment.
I wonder if we should just not override Equals here, and force callers to use the new Comparer for use in sets.
Fixes #7722
This PR only solves the prematurely determined issue for parameters since this is the most commonly changable in visitors.