Skip to content

Use ConstructorExpression in CSharpRuntimeAnnotationCodeGenerator#38409

Merged
AndriySvyryd merged 9 commits into
mainfrom
copilot/use-constructor-expression-in-csharpruntimeannotat
Jun 17, 2026
Merged

Use ConstructorExpression in CSharpRuntimeAnnotationCodeGenerator#38409
AndriySvyryd merged 9 commits into
mainfrom
copilot/use-constructor-expression-in-csharpruntimeannotat

Conversation

Copilot AI commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

The compiled-model generator hand-rolled C# for ValueConverter and JsonValueReaderWriter instances via reflection-based string building, duplicating logic already available on each type's constructor expression metadata. This change has the generator render those expressions through ICSharpHelper.Expression(...), with reviewer follow-up fixes applied.

Changes

  • CSharpRuntimeAnnotationCodeGenerator

    • Plain converters now emit codeHelper.Expression(converter.ConstructorExpressionWithoutMappingHints, ...) so compiled models don't redundantly persist ConverterMappingHints when type mapping is already explicit.
    • Leaf JSON reader/writers emit codeHelper.Expression(jsonValueReaderWriter.ConstructorExpression, ...).
    • Recursion is intentionally retained for CollectionToJsonStringConverter, IJsonConvertedValueReaderWriter, and ICompositeJsonValueReaderWriter: their ConstructorExpression embeds Expression.Constant(converterInstance), which cannot be rendered to source.
    • missingUnsafeAccessors is created only in the scope where constructor expressions are rendered and asserted empty there (post-validation only).
  • ValueConverter / ValueConverter<TModel,TProvider>

    • Added ConstructorExpressionWithoutMappingHints (base virtual + generic override).
    • ConstructorExpression still handles convertsNulls via the 4-arg constructor when needed.
    • ConstructorExpressionWithoutMappingHints uses dedicated 2-arg (convertTo, convertFrom) and 3-arg (convertTo, convertFrom, convertsNulls) constructors so no ConverterMappingHints argument is passed at all — not even default(ConverterMappingHints).
  • ValueConverter.MappingHintsCtor (latent bug)

    • Previously resolved a non-existent 5-arg ConverterMappingHints constructor and returned null. Corrected to the current 4-arg (int?, int?, int?, bool?) constructor.
  • Regenerated baselines

    • Compiled-model baselines were regenerated (including Cosmos and SqlServer) to match the new output.
    • src/EFCore/EFCore.baseline.json was updated by running EFCore.ApiBaseline.Tests locally.

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI changed the title [WIP] Use ConstructorExpression in CSharpRuntimeAnnotationCodeGenerator Use ConstructorExpression in CSharpRuntimeAnnotationCodeGenerator Jun 11, 2026
Copilot AI requested a review from AndriySvyryd June 11, 2026 01:49
@AndriySvyryd AndriySvyryd requested a review from Copilot June 11, 2026 02:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates EF Core’s compiled-model code generation to reuse each ValueConverter/JsonValueReaderWriter instance’s ConstructorExpression instead of hand-rolled C# string construction. This consolidates logic, fixes a latent reflection bug in MappingHintsCtor, and makes generated compiled-model baselines more faithful by including mapping-hint values.

Changes:

  • Switch CSharpRuntimeAnnotationCodeGenerator to render converter/reader-writer construction via ICSharpHelper.Expression(...ConstructorExpression...) (while keeping the existing recursive special-cases where ConstructorExpression contains non-renderable constants).
  • Fix ValueConverter<TModel,TProvider>.ConstructorExpression to correctly represent ConvertsNulls by using the 4-arg constructor when needed, and to emit mapping-hints via the correct ConverterMappingHints(int?, int?, int?, bool?) constructor.
  • Regenerate InMemory/Sqlite compiled-model scaffolding baselines to reflect the new (single-line) emitted constructor expressions and mapping-hint arguments.

Reviewed changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/EFCore/Design/Internal/CSharpRuntimeAnnotationCodeGenerator.cs Use ConstructorExpression rendering for plain converters and leaf JSON reader/writers; retain recursion for converted/composite cases.
src/EFCore/Storage/ValueConversion/ValueConverter.cs Fix MappingHintsCtor reflection to target the existing 4-arg ConverterMappingHints constructor.
src/EFCore/Storage/ValueConversion/ValueConverter`.cs Update ConstructorExpression to include ConvertsNulls and emit mapping-hints via the corrected ctor.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/No_NativeAOT/ManyTypesEntityType.cs Baseline regen: converters now include mapping-hints args and single-line ctor emission.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel/PrincipalBaseEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel/OwnedTypeEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel/OwnedType0EntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel/DependentBaseEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel_with_JSON_columns/PrincipalBaseEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel_with_JSON_columns/OwnedTypeEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel_with_JSON_columns/OwnedType0EntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.Sqlite.FunctionalTests/Scaffolding/Baselines/BigModel_with_JSON_columns/DependentBaseEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.InMemory.FunctionalTests/Scaffolding/Baselines/Self_referential_property/SelfReferentialEntityEntityType.cs Baseline regen: converter ctor now includes mapping-hints emission.
test/EFCore.InMemory.FunctionalTests/Scaffolding/Baselines/RelationshipCycles/PrincipalBaseEntityType.cs Baseline regen for converter/JSON reader-writer construction output.
test/EFCore.InMemory.FunctionalTests/Scaffolding/Baselines/No_NativeAOT/ManyTypesEntityType.cs Baseline regen: converters now include mapping-hints args and single-line ctor emission.
test/EFCore.InMemory.FunctionalTests/Scaffolding/Baselines/Custom_value_converter/MyEntityEntityType.cs Baseline regen for converter construction output.
test/EFCore.InMemory.FunctionalTests/Scaffolding/Baselines/ComplexTypes/PrincipalDerivedEntityType.cs Baseline regen for converter/JSON reader-writer construction output (complex types).
test/EFCore.InMemory.FunctionalTests/Scaffolding/Baselines/ComplexTypes/PrincipalBaseEntityType.cs Baseline regen for converter/JSON reader-writer construction output (complex types).

Comment thread src/EFCore/Design/Internal/CSharpRuntimeAnnotationCodeGenerator.cs Outdated
Comment thread src/EFCore/Design/Internal/CSharpRuntimeAnnotationCodeGenerator.cs Outdated
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.

Comment thread src/EFCore/Storage/ValueConversion/ValueConverter`.cs Outdated
Comment thread src/EFCore/Design/Internal/CSharpRuntimeAnnotationCodeGenerator.cs Outdated
…from ValueConverter constructors

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Comment thread src/EFCore/Design/Internal/CSharpRuntimeAnnotationCodeGenerator.cs Outdated
@AndriySvyryd AndriySvyryd marked this pull request as ready for review June 11, 2026 20:03
@AndriySvyryd AndriySvyryd requested a review from a team as a code owner June 11, 2026 20:03
Copilot AI review requested due to automatic review settings June 11, 2026 20:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 4 comments.

…pingHints

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…onverter to single-line

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot June 12, 2026 01:20
Copilot AI requested a review from AndriySvyryd June 12, 2026 01:21
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot June 12, 2026 01:56
Copilot AI requested a review from AndriySvyryd June 12, 2026 01:57
@AndriySvyryd AndriySvyryd requested a review from cincuranet June 12, 2026 19:33
@AndriySvyryd AndriySvyryd merged commit fe6b8ee into main Jun 17, 2026
13 checks passed
@AndriySvyryd AndriySvyryd deleted the copilot/use-constructor-expression-in-csharpruntimeannotat branch June 17, 2026 04:35
@github-actions github-actions Bot added the api-review This PR or issue is introducing public API changes that need to be reviewed label Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

API review baseline changes for src/EFCore/EFCore.baseline.json

Show diff

The diff below was generated by ApiChief between the base and the PR.

  class Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter<TModel,
      TProvider> : Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter
- ValueConverter(Expression<Func<TModel, TProvider>> convertToProviderExpression, Expression<Func<TProvider, TModel>> convertFromProviderExpression,
-     ConverterMappingHints? mappingHints = null);
+ ValueConverter(Expression<Func<TModel, TProvider>> convertToProviderExpression, Expression<Func<TProvider, TModel>> convertFromProviderExpression);
+ ValueConverter(Expression<Func<TModel, TProvider>> convertToProviderExpression, Expression<Func<TProvider, TModel>> convertFromProviderExpression,
+     ConverterMappingHints? mappingHints);
+ override Expression ConstructorExpressionWithoutMappingHints { get; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review This PR or issue is introducing public API changes that need to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use ConstructorExpression in CSharpRuntimeAnnotationCodeGenerator

4 participants