Conversation
koenbeuk
commented
Apr 16, 2026
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR extends ExpressiveSharp’s [ExpressiveFor] mapping model to support a more ergonomic “co-located” form (single-argument attribute + instance/property stubs), and updates the generator, EF Core conventions, tests, and docs accordingly.
Changes:
- Add
[ExpressiveFor(string memberName)]shorthand (target defaults to the stub’s containing type) and allow[ExpressiveFor]to be applied to property stubs. - Update the generator to interpret method/property stubs (including instance stubs) and centralize signature matching via a shared matcher.
- Update EF Core integration to ignore
[ExpressiveFor]stubs/targets as unmapped, and add/refresh tests + documentation for the new forms.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ExpressiveSharp.IntegrationTests/Scenarios/Store/Models/DisplayFormatter.cs | Switch Wrap mapping to co-located instance-stub + single-arg [ExpressiveFor]; keep legacy external static mapping for Label. |
| tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressiveForTests.cs | Adjust diagnostics expectation (retire EXP0016) and add test coverage for instance stubs, single-arg form, and property stubs. |
| tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressiveForTests.SingleArgForm_InstanceMethodTarget.verified.txt | Snapshot for generated output of single-arg instance-method target. |
| tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressiveForTests.SingleArgForm_DefaultsToContainingType.verified.txt | Snapshot for generated output of single-arg target defaulting to containing type. |
| tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressiveForTests.PropertyStub_InstanceProperty_SameType.verified.txt | Snapshot for generated output of property-stub mapping. |
| tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressiveForTests.InstanceStub_OnInstanceProperty_SameType.verified.txt | Snapshot for generated output of instance method stub targeting instance property. |
| tests/ExpressiveSharp.EntityFrameworkCore.IntegrationTests/Tests/Sqlite/ExpressivePropertiesNotMappedConventionTests.cs | New test suite for EF convention ignoring [Expressive], [ExpressiveFor] stubs, and [ExpressiveFor] targets. |
| src/ExpressiveSharp.Generator/Models/ExpressiveForAttributeData.cs | Parse both (Type,string) and (string) constructors for [ExpressiveFor]. |
| src/ExpressiveSharp.Generator/Interpretation/ExpressiveForSignatureMatcher.cs | New shared matcher to keep interpreter and registry extraction aligned on signature rules. |
| src/ExpressiveSharp.Generator/Interpretation/ExpressiveForInterpreter.cs | Support method/property stubs and instance stubs; synthesize @this parameter for instance stubs. |
| src/ExpressiveSharp.Generator/Infrastructure/Diagnostics.cs | Retire EXP0016 (static-only stub requirement). |
| src/ExpressiveSharp.Generator/ExpressiveGenerator.cs | Expand [ExpressiveFor] discovery to properties; adapt pipeline types from method-only to member-level; update registry extraction logic. |
| src/ExpressiveSharp.Generator/Comparers/ExpressiveForMemberCompilationEqualityComparer.cs | Broaden comparer tuple to MemberDeclarationSyntax to support property stubs. |
| src/ExpressiveSharp.EntityFrameworkCore/Infrastructure/Internal/ExpressivePropertiesNotMappedConventionPlugin.cs | Inject IExpressiveResolver into convention plugin to detect external mappings. |
| src/ExpressiveSharp.EntityFrameworkCore/Infrastructure/Internal/ExpressivePropertiesNotMappedConvention.cs | Ignore [ExpressiveFor] stubs/targets (including external stubs via resolver) in EF model-building. |
| src/ExpressiveSharp.EntityFrameworkCore/Infrastructure/Internal/ExpressiveOptionsExtension.cs | Register IExpressiveResolver and wire new convention constructor dependency. |
| src/ExpressiveSharp.Abstractions/Mapping/ExpressiveForAttribute.cs | Allow attribute on properties; add single-arg constructor; make TargetType nullable. |
| docs/reference/projectable-properties.md | Update comparison text to mention non-static/co-located stubs. |
| docs/reference/expressive-for.md | Document new mapping rules and co-located forms; remove EXP0016 mention. |
| docs/reference/diagnostics.md | Document EXP0016 retirement and remove its section/table entry. |
| docs/recipes/external-member-mapping.md | Update guidance for new co-located and property-stub forms; remove static-only wording. |
| docs/guide/migration-from-projectables.md | Update migration guidance/examples to prefer single-arg co-located stubs for same-type scenarios. |
Comments suppressed due to low confidence (2)
src/ExpressiveSharp.Generator/ExpressiveGenerator.cs:516
- In
ExtractRegistryEntryForExternal, the property path setsisProperty = stubIsProperty ? true : ...without validating that the stub can actually target that property under the same signature rules enforced byExpressiveForInterpreter(static vs instance, same-type requirement for instance stubs, etc.). To keep the registry extractor and interpreter from diverging, consider resolving the target property usingExpressiveForSignatureMatcher.MatchesPropertyFromPropertyStub/MatchesPropertyFromMethodStub(and returning null when no match).
// Property stubs can only target properties; method stubs may target either.
var isProperty = stubIsProperty
? true
: targetType.GetMembers(memberName).OfType<IPropertySymbol>().Any();
if (isProperty)
{
memberKind = ExpressionRegistryMemberType.Property;
memberLookupName = memberName;
// Properties have no parameter types in the registry
src/ExpressiveSharp.Abstractions/Mapping/ExpressiveForAttribute.cs:27
MemberName’s XML doc still says “on TargetType”, butTargetTypeis now nullable for the single-argument constructor (where the target defaults to the stub’s containing type). Consider updating this doc comment to avoid implyingTargetTypeis always non-null.
/// <summary>
/// The name of the target member on <see cref="TargetType"/>.
/// </summary>
public string MemberName { get; }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <para>For <b>instance properties</b>, the stub takes a single parameter (the receiver) and returns the property type.</para> | ||
| /// <para>For <b>static properties</b>, the stub is parameterless and returns the property type.</para> | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Method, Inherited = false, AllowMultiple = false)] | ||
| [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = false)] |
There was a problem hiding this comment.
The XML docs in the <remarks> still describe only the legacy static-method-stub shapes (receiver as first parameter, etc.). With this PR adding property stubs, instance stubs on the target type (implicit this), and the single-argument constructor (TargetType nullable), these docs are now inaccurate—please update the remarks (and summary if needed) to reflect the new supported stub shapes and matching rules.
| .ForAttributeWithMetadataName( | ||
| attributeFullName, | ||
| predicate: static (s, _) => s is MethodDeclarationSyntax, | ||
| predicate: static (s, _) => s is MethodDeclarationSyntax or PropertyDeclarationSyntax, |
There was a problem hiding this comment.
ExpressiveForAttribute can now be applied to properties, which includes indexers, but this syntax-provider predicate only matches PropertyDeclarationSyntax (not IndexerDeclarationSyntax). As a result, [ExpressiveFor] on an indexer would be silently ignored (no diagnostic, no generated output). Consider including IndexerDeclarationSyntax here (and then rejecting indexers explicitly in the interpreter, since indexers can’t be expressive targets).
| predicate: static (s, _) => s is MethodDeclarationSyntax or PropertyDeclarationSyntax, | |
| predicate: static (s, _) => s is MethodDeclarationSyntax or PropertyDeclarationSyntax or IndexerDeclarationSyntax, |
…iveForInterpreter test: add comprehensive tests for ExpressiveFor signature matching scenarios