Skip to content

Fix handling of implicitly declared EqualityContract on record types in validation generator #62511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 2, 2025
Merged

Fix handling of implicitly declared EqualityContract on record types in validation generator #62511

merged 3 commits into from
Jul 2, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 30, 2025 •
edited
Loading

When a record class being inspected comes from metadata (i.e., a referenced assembly or a prior incremental step), its compiler-synthesized EqualityContract property is not flagged with IsImplicitlyDeclared == true. The current filter in ExtractValidatableMembers relies solely on that flag, so the property slips through and a GeneratedValidatablePropertyInfo is produced for it.

As a result, every metadata-based record type ends up with a spurious ValidatablePropertyInfo for EqualityContract which the runtime resolver will attempt to apply and fail for.

Changes Made

This PR adds a helper method that recognizes EqualityContract by its signature, not just by IsImplicitlyDeclared, and uses it in the skip logic:

private static bool IsEqualityContract(IPropertySymbol prop) =>
    prop.Name == "EqualityContract"
    && prop.Type.ToDisplayString() == "System.Type"
    && prop.DeclaredAccessibility == Accessibility.Protected;

The member filtering loop is updated to use this helper:

if (member.IsImplicitlyDeclared                     
    || IsEqualityContract(member) 
    || resolvedRecordProperty.Contains(member, SymbolEqualityComparer.Default))
{
    continue;
}

Testing

Added a specific test RecordTypes_DoNotIncludeEqualityContractProperty that verifies record types from metadata don't incorrectly include the EqualityContract property in generated validation info. The test shows that only the actual record properties (like Name with [Required]) are included, not the compiler-generated EqualityContract.

All existing tests continue to pass, ensuring no regressions.

Fixes #62510.

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot @captainsafia
Fix handling of implicitly declared EqualityContract on record types …
6f9b7b8 
…in validation generator

Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Fix handling of implicitly declared EqualityContract on record types in validation generator Fix handling of implicitly declared EqualityContract on record types in validation generator Jun 30, 2025
@Copilot Copilot AI requested a review from captainsafia June 30, 2025 23:19
Copilot finished work on behalf of captainsafia June 30, 2025 23:19
captainsafia
captainsafia reviewed Jun 30, 2025
View reviewed changes
...ation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.RecordType.cs Outdated
@@ -370,4 +370,31 @@ async Task ValidInputProducesNoWarnings(Endpoint endpoint)
});

}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the test as requested. a6e7085

...Tests.RecordTypes_DoNotIncludeEqualityContractProperty#ValidatableInfoResolver.g.verified.cs Outdated
@@ -0,0 +1,160 @@
//HintName: ValidatableInfoResolver.g.cs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the snapshot file as requested. a6e7085

src/Validation/gen/Parsers/ValidationsGenerator.TypesParser.cs Outdated
@@ -105,6 +105,11 @@ internal bool TryExtractValidatableType(ITypeSymbol incomingTypeSymbol, WellKnow
return true;
}

private static bool IsEqualityContract(IPropertySymbol prop) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this method to src/Validation/gen/Extensions/ISymbolExtensions.cs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the method to ISymbolExtensions.cs as requested and updated it to use WellKnownTypes for type comparison. a6e7085

src/Validation/gen/Parsers/ValidationsGenerator.TypesParser.cs Outdated
@@ -105,6 +105,11 @@ internal bool TryExtractValidatableType(ITypeSymbol incomingTypeSymbol, WellKnow
return true;
}

private static bool IsEqualityContract(IPropertySymbol prop) =>
prop.Name == "EqualityContract"
&& prop.Type.ToDisplayString() == "System.Type"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add System.Type to https://github.com/dotnet/aspnetcore/blob/main/src/Shared/RoslynUtils/WellKnownTypeData.cs then use WellKnownTypes to do the type comparison here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added System.Type to WellKnownTypeData.cs and updated the method to use WellKnownTypes for type comparison instead of string comparison. a6e7085

@Copilot @captainsafia
Address code review feedback: move IsEqualityContract to ISymbolExten…
a6e7085 
…sions and use WellKnownTypes

Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
@Copilot Copilot AI requested a review from captainsafia June 30, 2025 23:32
Copilot finished work on behalf of captainsafia June 30, 2025 23:32
@captainsafia captainsafia marked this pull request as ready for review June 30, 2025 23:32
@build-analysis build-analysis bot mentioned this pull request Jul 1, 2025
Open
@captainsafia captainsafia merged commit c18e93a into main Jul 2, 2025
28 checks passed
@captainsafia captainsafia deleted the copilot/fix-62510 branch July 2, 2025 19:43
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview7 milestone Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captainsafia captainsafia captainsafia approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@javiercn javiercn Awaiting requested review from javiercn

@oroztocil oroztocil Awaiting requested review from oroztocil

Assignees

@captainsafia captainsafia

Copilot code review Copilot

Labels
Attention: Shared Code Modified
Projects
None yet
Milestone
10.0-preview7
Development

Successfully merging this pull request may close these issues.

Fix handling of implicitly declared EqualityContract on record types in validation generator
3 participants
@Copilot @captainsafia @BrennanConroy