Skip to content
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.

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

Add BindingFlags.DoNotWrapExceptions #7863

Merged
merged 1 commit into from Mar 29, 2018

Conversation

Projects
None yet
5 participants
@AustinWise
Copy link
Contributor

AustinWise commented Mar 28, 2018

The original design review is here: dotnet/corefx#22866 . To summarize, this adds a new flag to BindingFlags called DoNotWrapExceptions. When this flag is used to invoke a method using reflection, any exceptions thrown are not wrapped with a TargetInvocationException.

It has already been implemented in some other version of .NET:

I would be delighted if this handy feature was also available in Mono.

I have in part based my changes on the CoreCLR implementation. There all already tests for this feature in corefx, so I have added those to the corlib_xtest.dll.sources file.

I have a couple of concerns about my implementation:

  • I notice that DynamicMethod.Invoke was ignoring the BindingFlags and other arguments. I changed this method to pass along all the arguments. For what it's worth, it appears that CoreCLR respects these arguments.
@vargaz

This comment has been minimized.

Copy link
Member

vargaz commented Mar 28, 2018

Support for filter clauses is not perfect, it would be better to simply duplicate the body:
if (do-not-wrap) {
call
} else {
try {
call
} catch (...) {
...
}
}

@AustinWise

This comment has been minimized.

Copy link
Contributor Author

AustinWise commented Mar 28, 2018

I have update my pull request to not use exception filter clauses.

@marek-safar

This comment has been minimized.

Copy link
Member

marek-safar commented Mar 28, 2018

approve

@@ -64,5 +64,6 @@ public enum BindingFlags

// These are a couple of misc attributes used
IgnoreReturn = 0x01000000, // This is used in COM Interop
DoNotWrapExceptions = 0x02000000, // Disables wrapping exceptions in TargetInvocationException

This comment has been minimized.

@marek-safar

marek-safar Mar 28, 2018

Member

Please instead of changing this file replace it in corlib.dll.sources with the version from external/corefx/src/Common/src/CoreLib/System/Reflection/BindingFlags.cs

This comment has been minimized.

@AustinWise

AustinWise Mar 28, 2018

Author Contributor

Done

[MethodImplAttribute(MethodImplOptions.NoInlining)] // Methods containing StackCrawlMark local var has to be marked non-inlineable
static public Object CreateInstance(Type type, bool nonPublic)
internal static object CreateInstance(Type type, bool nonPublic, bool wrapExceptions)

This comment has been minimized.

@marek-safar

marek-safar Mar 28, 2018

Member

Should be just private

This comment has been minimized.

@AustinWise

AustinWise Mar 28, 2018

Author Contributor

Before this change RuntimeType called Activator.CreateInstance(Type type, bool nonPublic) when a constructor has no arguments. After this change I needed a way to propagate the DoNotWrapExceptions flag, so I added this internal overload.

This comment has been minimized.

@marek-safar

marek-safar Mar 28, 2018

Member

As it's used by CreateInstance only, it does have to be internal

@marek-safar

This comment has been minimized.

Copy link
Member

marek-safar commented Mar 28, 2018

approve

@AustinWise

This comment has been minimized.

Copy link
Contributor Author

AustinWise commented Mar 28, 2018

I noticed that API Diff check failed because of the addition of the new flag. Is that something I should fix in this pull request?

@marek-safar

This comment has been minimized.

Copy link
Member

marek-safar commented Mar 28, 2018

We'll take care of apidiff handling

@@ -68,6 +68,7 @@
<Compile Include="..\..\..\external\corefx\src\Common\src\CoreLib\System\Memory.cs" />
<Compile Include="..\..\..\external\corefx\src\Common\src\CoreLib\System\MemoryDebugView.cs" />
<Compile Include="..\..\..\external\corefx\src\Common\src\CoreLib\System\ReadOnlyMemory.cs" />
<Compile Include="..\..\..\external\corefx\src\Common\src\CoreLib\System\Reflection\BindingFlags.cs" />

This comment has been minimized.

@luhenry

luhenry Mar 28, 2018

Member

You should do these 2 modifications to https://github.com/mono/mono/blob/master/mcs/class/corlib/corlib.dll.sources instead. This corlib-net_4_x.csproj is autogenerated from the mcs/class/corlib/*.sources files

This comment has been minimized.

@AustinWise

AustinWise Mar 28, 2018

Author Contributor

I have removed my changes from the generated csproj file.

@AustinWise

This comment has been minimized.

Copy link
Contributor Author

AustinWise commented Mar 28, 2018

@luhenry It appears that a bot periodically regenerates the csproj file so I have removed my changes to that file. Is that what you had in mind?

@marek-safar

This comment has been minimized.

Copy link
Member

marek-safar commented Mar 29, 2018

@monojenkins commit apidiff

monojenkins added a commit to mono/api-snapshot that referenced this pull request Mar 29, 2018

@akoeplinger akoeplinger merged commit 77b2db7 into mono:master Mar 29, 2018

19 of 23 checks passed

API Diff The public API changed.
Details
API Diff Update Update failed.
Details
Linux x64 Build finished. 121004 tests run, 1442 skipped, 0 failed.
Details
OS X i386 Build finished. 119852 tests run, 1326 skipped, 0 failed.
Details
Linux AArch64 Build finished. 120998 tests run, 1445 skipped, 0 failed.
Details
Linux AArch64 Interpreter Build finished. 11221 tests run, 97 skipped, 0 failed.
Details
Linux ARMv5 Build finished. 120958 tests run, 1443 skipped, 0 failed.
Details
Linux ARMv7 Build finished. 78320 tests run, 1441 skipped, 0 failed.
Details
Linux ARMv7 Interpreter Build finished. 11174 tests run, 97 skipped, 0 failed.
Details
Linux i386 Build finished. 121002 tests run, 1440 skipped, 0 failed.
Details
Linux i386 Coop GC Build finished. 120993 tests run, 1440 skipped, 0 failed.
Details
Linux x64 Acceptance Tests Build finished. 6681 tests run, 6 skipped, 0 failed.
Details
Linux x64 Checked Private Types Build Build finished. No test results found.
Details
Linux x64 Coop GC Build finished. 120995 tests run, 1442 skipped, 0 failed.
Details
Linux x64 FullAOT Build finished. 21730 tests run, 553 skipped, 0 failed.
Details
Linux x64 Interpreter Build finished. 11221 tests run, 97 skipped, 0 failed.
Details
Linux x64 mcs Build finished.
Details
OS X x64 Build finished. 120155 tests run, 1328 skipped, 0 failed.
Details
PR Trigger Docs Click to view available PR triggers (Xamarin internal).
Details
Test Result Viewer Click to view aggregated test results (Xamarin internal).
Details
Windows i386 Build finished. 116147 tests run, 1155 skipped, 0 failed.
Details
Windows x64 Build finished. 116171 tests run, 1157 skipped, 0 failed.
Details
license/cla All CLA requirements met.
Details
@luhenry

This comment has been minimized.

Copy link
Member

luhenry commented Mar 29, 2018

@AustinWise yes we do regenerate periodically the csproj files from the *.dll.sources files, so modifying the former will just be overridden by the automatic generation, while modifying the latter will make it happen in the former.

@AustinWise AustinWise deleted the AustinWise:austin/DoNotWrapExceptions branch Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.