Support Covariant Return Types#334
Conversation
|
已解决:
|
| var baseDef = method.GetBaseDefinition(); | ||
| foreach (var property in m.DeclaringType.GetTypeInfo().GetProperties()) | ||
| { | ||
| if (property.CanRead && property.GetMethod == baseDef) |
There was a problem hiding this comment.
这里会把普通 override 属性的 getter/setter 误判成非属性方法。对于 Derived : Base 中的 public override string Name { get; set; },Derived.get_Name.GetBaseDefinition() 返回的是 Base.get_Name,但 Derived.Name.GetMethod 仍然是 Derived.get_Name,所以这里比较失败,IsPropertyBinding() 返回 false。
结果是 class proxy 构建时会在 BuildClassMethods 里把 get_Name/set_Name 当普通方法生成一次,又在 BuildClassProperties 里作为属性 accessor 再生成一次。我用临时探针看到 proxy 类型里普通方法 token 和 PropertyInfo.GetMethod/SetMethod 指向的 token 已经不同。
建议同时匹配当前 accessor 和 base definition,例如 getter/setter 分别判断 property.GetMethod == m || property.GetMethod.GetBaseDefinition() == baseDef,并补一个普通 override property 的回归测试。
| attributes |= MethodAttributes.FamORAssem; | ||
| var (serviceMethod, implMethod) = covariantReturnMethod is null | ||
| ? (method, InterfaceImplBuilder.ResolveImplementationMethod(method, _implType)) | ||
| : (covariantReturnMethod, covariantReturnMethod); |
There was a problem hiding this comment.
这里把 serviceMethod 也替换成了 covariant override,本身能解决发射签名问题,但会丢掉调用方传入的原始 service 方法身份,导致配置型拦截器不再按 service type 命中。
可复现:
configuration.Interceptors.AddDelegate(async (ctx, next) =>
{
await next(ctx);
ctx.ReturnValue = new LeafResult(((BaseResult)ctx.ReturnValue).Name + ":intercepted");
}, Predicates.ForService(nameof(BaseService)));
var service = generator.CreateClassProxy<BaseService, LeafService>();
service.Method();
class BaseService { public virtual BaseResult Method() => new("base"); }
class LeafService : BaseService { public override LeafResult Method() => new("leaf"); }当前结果是 leaf,同样场景如果 LeafService.Method() 非协变地返回 BaseResult,结果是 leaf:intercepted。原因是 aspect body 后续用这个替换后的 serviceMethod 去做 InterceptorCollector.Collect,Predicates.ForService(nameof(BaseService)) 看到的是 LeafService.Method,非 inherited 的配置拦截器不会被收集。
建议发射签名仍使用 covariant method,但验证/收集/AspectContext 里的 service metadata 保留原来的 method,或者给 MethodNode/body 分开建模 emitted signature method 与 logical service method。属性 getter 的协变分支也有同类风险。
There was a problem hiding this comment.
我在context里面加了一个字段叫public MethodInfo PredicateMethod { get; }专门用于InterceptorCollector.Collect, 不过相关改动就有点多了= =
相关测试用例在CovariantReturnTypeWithInterceptorTests.cs这个文件
| @@ -202,15 +243,56 @@ private void BuildClassProperties(List<PropertyNode> properties, List<MethodNode | |||
|
|
|||
| properties.Add(new PropertyNode(property.Name, property.PropertyType, property.Attributes, attrs, getMethod, setMethod)); | |||
There was a problem hiding this comment.
协变属性这里仍用原始 property.PropertyType 创建 PropertyNode,会让生成类型的属性元数据和 getter 签名不一致。
复现:
class BaseService { public virtual BaseResult Value { get; } = new("base"); }
class LeafService : BaseService { public override LeafResult Value { get; } = new("leaf"); }
var proxy = generator.CreateClassProxy<BaseService, LeafService>();原始 LeafService 的 declared property 是 LeafResult Value,但当前 proxy 的 declared property 变成 BaseResult Value,同时 PropertyInfo.GetMethod.ReturnType 是 LeafResult。我用反射探针看到:
LeafService.Value property=LeafResult getter=LeafResult
Proxy.Value property=BaseResult getter=LeafResult
这会破坏依赖 PropertyInfo.PropertyType 的调用方,也让 proxy metadata 不再等价于实际 override。协变 getter 分支里应当用实际生成 getter 的返回类型(例如 getMethod.ServiceMethod.ReturnType / serviceMethod.ReturnType)作为 PropertyNode 的 property type,而不是始终使用原始 service property type。
There was a problem hiding this comment.
换成getter.ReturnType了
相关测试用例: CreateClassProxy_ForCovariantProperty_ShouldKeepPropertyTypeAlignedWithGetterReturnType
liuhaoyang
left a comment
There was a problem hiding this comment.
Code Review Summary
Reviewed the full PR (52 files, +2640/-194 lines). The covariant return types implementation is well-structured with good test coverage (77 new tests all passing). Build succeeds with 0 errors and all 574 tests pass.
Overall: Ship with minor fixes. Please see inline comments for specific findings.
| @@ -57,22 +57,22 @@ public IEnumerable<IInterceptor> Collect(MethodInfo serviceMethod, MethodInfo im | |||
|
|
|||
| return (IEnumerable<IInterceptor>)_aspectCaching.GetOrAdd(GetKey(serviceMethod, implementationMethod), key => | |||
There was a problem hiding this comment.
[HIGH] Cache key inconsistency — predicateMethod not included in cache key
GetKey(serviceMethod, implementationMethod) only uses 2 parameters, but the factory lambda on line 60 calls CollectFromService(serviceMethod, predicateMethod) which uses predicateMethod to select interceptors via CollectFromSelector(predicateMethod).
If the same (serviceMethod, implementationMethod) pair is ever evaluated with two different predicateMethod values that yield different interceptor selections, the first call's result will be cached and returned for both — producing wrong interceptors.
Compare with AspectBuilderFactory (line 43-45) which correctly includes predicateMethod in its cache key: Tuple.Create(serviceMethod, implementationMethod, predicateMethod).
Suggested fix:
private static object GetKey(MethodInfo serviceMethod, MethodInfo implementationMethod, MethodInfo predicateMethod)
{
return Tuple.Create(serviceMethod, implementationMethod, predicateMethod);
}And update the call site on line 58 to pass predicateMethod.
| public interface IInterceptorCollector | ||
| { | ||
| IEnumerable<IInterceptor> Collect(MethodInfo serviceMethod, MethodInfo implementationMethod); | ||
| IEnumerable<IInterceptor> Collect(MethodInfo serviceMethod, MethodInfo implementationMethod, MethodInfo predicateMethod); |
There was a problem hiding this comment.
[HIGH] Breaking change to public interface — no transition path
The signature changed from Collect(MethodInfo serviceMethod, MethodInfo implementationMethod) to Collect(MethodInfo serviceMethod, MethodInfo implementationMethod, MethodInfo predicateMethod). This is a public interface in the Abstractions assembly.
Any external consumer that implemented IInterceptorCollector (e.g., custom DI container integrations) will fail to compile.
Suggestion: Consider adding a default interface method for backward compatibility:
IEnumerable<IInterceptor> Collect(MethodInfo serviceMethod, MethodInfo implementationMethod)
=> Collect(serviceMethod, implementationMethod, serviceMethod);Or, if this is intended as a major version bump, document the breaking change clearly in release notes.
| /// <param name="a">The first object to compare.</param> | ||
| /// <param name="b">The second object to compare.</param> | ||
| /// <returns>Returns true if both parameters are null or both are not null; otherwise, false.</returns> | ||
| public static bool IsSameNullState<T>(this T? a, T? b) |
There was a problem hiding this comment.
[LOW] Dead code — IsSameNullState is defined but not used anywhere
I couldn't find any call sites for this extension method in the PR diff. Consider either using it somewhere or removing it to keep the codebase clean.
Key Changes:
AspectCore.Teststo test internal methodsWhat is Covariant Return Types
see https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/covariant-returns