Skip to content

[ci-fix] Use DoNotWrapExceptions across ReflectionMemberAccessor Invoke paths #129212

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

Draft
github-actions wants to merge 9 commits into
base: main
Choose a base branch
from
+119 −7
Draft

[ci-fix] Use DoNotWrapExceptions across ReflectionMemberAccessor Invoke paths #129212

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5281736
Unwrap TargetInvocationException in ReflectionMemberAccessor property…
github-actions[bot] Jun 10, 2026
9579fbe
ci: trigger checks
github-actions[bot] Jun 10, 2026
c357fbb
Use DoNotWrapExceptions for ReflectionMemberAccessor property accessors
Copilot Jun 16, 2026
b6b94f7
Apply DoNotWrapExceptions to remaining reflection Invoke calls
Copilot Jun 17, 2026
5c1d406
Adjust constructor invoke paths for DoNotWrapExceptions on NET only
Copilot Jun 17, 2026
91768ba
Normalize Invoke overload usage in ReflectionMemberAccessor
Copilot Jun 17, 2026
8a0b1d0
Clarify add-method exception semantics and remove null-forgiving cast
Copilot Jun 17, 2026
102e407
Add context to union accessor creation failure
Copilot Jun 17, 2026
6e9eede
Refine union accessor Invoke null handling
Copilot Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 119 additions & 7 deletions .../System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.ExceptionServices;

namespace System.Text.Json.Serialization.Metadata
{
Expand Down Expand Up @@ -33,7 +34,16 @@ public ReflectionMemberAccessor()
: null;
}

return () => ctorInfo.Invoke(null);
return () =>
#if NET
ctorInfo.Invoke(
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: null,
culture: null);
#else
ctorInfo.Invoke(null);
#endif
}

public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor)
Expand All @@ -56,6 +66,13 @@ public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorI
argsToPass[i] = arguments[i];
}

#if NET
return (T)constructor.Invoke(
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: argsToPass,
culture: null);
#else
try
{
return (T)constructor.Invoke(argsToPass);
Expand All @@ -67,6 +84,7 @@ public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorI
// This doesn't apply to the method below as it supports a max of 4 constructor params.
throw e.InnerException ?? e;
}
#endif
};
}

Expand Down Expand Up @@ -108,7 +126,16 @@ public override JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, T
}
}

return (T)constructor.Invoke(arguments);
return
#if NET
(T)constructor.Invoke(
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: arguments,
culture: null);
#else
(T)constructor.Invoke(arguments);
#endif
};
}

Expand All @@ -122,6 +149,13 @@ public override JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, T

return value =>
{
#if NET
return (T)constructor.Invoke(
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: new object?[] { value },
culture: null);
#else
try
{
return (T)constructor.Invoke(new object?[] { value });
Expand All @@ -130,6 +164,7 @@ public override JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, T
{
throw e.InnerException ?? e;

@jkotas jkotas Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the pattern for catch and rethrow be consistent across all cases?

}
#endif
};
}

Expand All @@ -143,7 +178,17 @@ public override JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, T

return delegate (TCollection collection, object? element)
{
#if NET
// Keep exception propagation aligned with ReflectionEmitMemberAccessor.
addMethod.Invoke(
collection,
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: new object[] { element! },
culture: null);
#else
addMethod.Invoke(collection, new object[] { element! });

@jkotas jkotas Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this catch and rethrow the exception like the other cases?

#endif
};
}

Expand All @@ -167,7 +212,24 @@ public override Func<object, TProperty> CreatePropertyGetter<TProperty>(Property

return delegate (object obj)
{
return (TProperty)getMethodInfo.Invoke(obj, null)!;
#if NET
return (TProperty)getMethodInfo.Invoke(

@jkotas jkotas Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are other Invoke method calls in this file. Do all of them need the same treatment?

@kotlarmilos kotlarmilos Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot resolve this comment

Copilot AI Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Addressed in 6e9eede. I audited the other Invoke call sites in ReflectionMemberAccessor and updated them to use BindingFlags.DoNotWrapExceptions on NET (with existing non-NET compatibility paths preserved).

obj,
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: null,
culture: null)!;
#else
try
{
return (TProperty)getMethodInfo.Invoke(obj, null)!;
}
catch (TargetInvocationException e) when (e.InnerException is not null)
Comment thread
jkotas marked this conversation as resolved.
{
ExceptionDispatchInfo.Capture(e.InnerException).Throw();
throw; // unreachable
}
#endif
};
}

Expand All @@ -177,7 +239,24 @@ public override Func<TDeclaringType, TProperty> CreatePropertyGetter<TDeclaringT

return delegate (TDeclaringType obj)
{
return (TProperty)getMethodInfo.Invoke(obj, null)!;
#if NET
return (TProperty)getMethodInfo.Invoke(
obj,
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: null,
culture: null)!;
#else
try
{
return (TProperty)getMethodInfo.Invoke(obj, null)!;
}
catch (TargetInvocationException e) when (e.InnerException is not null)
{
ExceptionDispatchInfo.Capture(e.InnerException).Throw();
throw; // unreachable
}
#endif
};
}

Expand All @@ -187,7 +266,23 @@ public override Action<object, TProperty> CreatePropertySetter<TProperty>(Proper

return delegate (object obj, TProperty value)
{
setMethodInfo.Invoke(obj, new object[] { value! });
#if NET
setMethodInfo.Invoke(
obj,
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: new object[] { value! },
culture: null);
#else
try
{
setMethodInfo.Invoke(obj, new object[] { value! });
}
catch (TargetInvocationException e) when (e.InnerException is not null)
{
ExceptionDispatchInfo.Capture(e.InnerException).Throw();
}
#endif
};
}

Expand Down Expand Up @@ -216,10 +311,27 @@ public override UnionTryGetValueAccessor<TUnion> CreateUnionTryGetValueAccessor<
{
KeyValuePair<Type, MethodInfo> entry = entries[i];
caseTypes[i] = entry.Key;
chain[i] = (UnionTryGetValueAccessor<TUnion>)typeof(ReflectionMemberAccessor)
object? accessor = typeof(ReflectionMemberAccessor)
.GetMethod(nameof(CreateUnionTryGetValueAccessorCore), BindingFlags.NonPublic | BindingFlags.Static)!
.MakeGenericMethod(typeof(TUnion), entry.Key)
.Invoke(null, new object[] { entry.Value })!;
#if NET
.Invoke(
null,
BindingFlags.DoNotWrapExceptions,
binder: null,
parameters: new object[] { entry.Value },
culture: null);
#else
.Invoke(null, new object[] { entry.Value });
#endif

if (accessor is null)
{
throw new InvalidOperationException(
$"Failed to create union accessor for type '{entry.Key}' using method '{entry.Value}'.");

@jkotas jkotas Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This unrelated change. It should be left for separate PR (and tests should be added to cover it in that PR)

}

chain[i] = (UnionTryGetValueAccessor<TUnion>)accessor;
}

return (TUnion union, out Type? caseType, out object? value) =>
Expand Down
Loading