Disallow generating IEquatable for Ref Struct#41680
Disallow generating IEquatable for Ref Struct#41680CyrusNajmabadi merged 13 commits intodotnet:masterfrom
Conversation
Unlike Equals(object), which discussion seems to prefer generating broken code, generating IEquatable is always invalid This PR makes it so IEquatable can not be generated as a fix for ref structs
| if (containingType.IsRefLikeType) | ||
| { | ||
| statements.Add(factory.ReturnStatement(factory.FalseLiteralExpression())); | ||
| statements.ToImmutableAndFree(); |
There was a problem hiding this comment.
either the return value should be consumed or this builder should be freed in a different place.
There was a problem hiding this comment.
Fixed. Also fixed the tests to actually pass, was waiting for the solution to load.
|
Can you add |
...bers/GenerateEqualsAndGetHashCodeFromMembers/GenerateEqualsAndGetHashCodeFromMembersTests.cs
Show resolved
Hide resolved
...sAndGetHashCodeFromMembers/GenerateEqualsAndGetHashCodeFromMembersCodeRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions_CreateEqualsMethod.cs
Outdated
Show resolved
Hide resolved
...bers/GenerateEqualsAndGetHashCodeFromMembers/GenerateEqualsAndGetHashCodeFromMembersTests.cs
Outdated
Show resolved
Hide resolved
|
Next design meeting we will review whether this should silently ignore the "Generate IEquatable<T>" box, or if it should disable it from the start. It certainly seems like we want to avoid generating this code, but also we want to avoid having users confused by the behavior. |
|
Based on discussions and the code, where the skip was added it will disable the "Generate IEquatable" box. It's never added as a picker option if the if statement is not entered. The same mechanism is used to disable the box if IEquatable is already implemented, but not the other overloads. |
|
@ThadHouse That sounds like what we want. Can you include some screenshots? |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
great comments, thanks.
|
@ThadHouse @CyrusNajmabadi @sharwell Are we still interested in taking this change? |
|
@RikkiGibson I think yes. If someone gets a chance to include a screenshot for this, it would be good. I can try to get one added today. Also I linked this to the underlying issue #25708 |
There was a problem hiding this comment.
📝 It seems like it would be easier to read this method in the following form:
if (containingType.IsRefLikeType)
{
// A ref struct can never implement an interface, therefore never add IEquatable to the selection
// options if the type is a ref struct.
constructedType = null;
return false;
}
var equatableTypeOpt = semanticModel.Compilation.GetTypeByMetadataName(typeof(IEquatable<>).FullName);
if (equatableTypeOpt != null)
{
constructedType = equatableTypeOpt.Construct(containingType);
return !containingType.AllInterfaces.Contains(constructedType);
}
constructedType = null;
return false;There was a problem hiding this comment.
I can see how this merge conflict resolution is the correct one (the PR originally implemented the check in the same form you used), but perhaps it could be simplified in a follow-up commit?
|
Is there an option for generating "effectively" |
Unlike Equals(object), which discussion seems to prefer generating broken code, generating IEquatable is always invalid
This PR makes it so IEquatable can not be generated as a fix for ref structs.
Fixes #25708