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.

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

Jump to bottom

Fix MethodInfo Emit for generic interfaces #40587

Merged
merged 4 commits into from
Aug 24, 2020
Merged

Fix MethodInfo Emit for generic interfaces #40587

merged 4 commits into from
Aug 24, 2020

Conversation

wzchua
Copy link
Contributor

@wzchua wzchua commented Aug 9, 2020

Signature generation now includes CustomModifiers
MethodBuilder and ConstructorBuilder passes their SignatureHelper for
generation via GetMethodSignature()

Based on dotnet/coreclr#17881

Fixes #25958

@wzchua
Fixed Emit MethodInfo for interface generic
253b998 
Signature generation now includes CustomModifiers
MethodBuilder and ConstructorBuilder passes their SignatureHelper for
generation

Fixes dotnet#25958
@dnfadmin
Copy link

dnfadmin commented Aug 9, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@steveharter
Copy link
Member

Mono tests are failing:

    System.Reflection.Emit.Tests.ILGeneratorEmitMethodInfo.EmitMethodInfo [FAIL]
      System.InvalidProgramException : Invalid IL code in DynamicType:Call (System.Reflection.Emit.Tests.ILGeneratorEmitMethodInfo/IWithIn`1<int>): IL_0006: callvirt  0x0a000001
      
      
      Stack Trace:
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(384,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@wzchua
Copy link
Contributor Author

wzchua commented Aug 11, 2020

Hmm, it's failing the test I added. But I'm not sure how to go about debugging the reflection on mono

@steveharter
Copy link
Member

steveharter commented Aug 11, 2020 •
edited
Loading

@marek-safar can you provide pointers to @wzchua on Mono? Thanks

steveharter
steveharter reviewed Aug 11, 2020
View reviewed changes
src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs Outdated
sigHelp = GetMemberRefSignature(method, cGenericParameters);
}

if (optionalParameterTypes is {} && optionalParameterTypes.Length > 0)
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can change this to if (optionalParameterTypes?.Length)

steveharter
steveharter reviewed Aug 11, 2020
View reviewed changes
src/libraries/System.Reflection.Emit.ILGeneration/tests/ILGenerator/EmitMethodInfo.cs Outdated

public sealed class WithIn : IWithIn<int>
{
public void Method(in RuntimeMethodHandle arg)
Copy link
Member

Choose a reason for hiding this comment

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

nit: this syntax public void Method(in RuntimeMethodHandle arg) { } is a bit more common.

@steveharter
Copy link
Member

@wzchua to skip the test on Mono, your can add this to your test method:
[SkipOnMono("https://github.com/dotnet/runtime/issues/<AddNewIssueHere")]

This requires a new issue with a "runtime-mono" label.

@steveharter
Copy link
Member

steveharter commented Aug 13, 2020 •
edited
Loading

Since the 5.0 release has no more preview releases, and this is not a risk-free issue, we should consider committing this after we branch for 6.0 which is occurring August 17th.

After it is in master, it is possible to merge this into the 5.0 branch (through the "ask mode" policy) or a future servicing branch but only when we are confident that it is not breaking and it meets the bar (by unblocking important scenarios).

@wzchua
Fixed EmitMethodInfo test case
2ca8a2b 
mono has a per domain scope for methodhandles
Updated code as per review
removed while loop for explicit instance check
@wzchua
Copy link
Contributor Author

wzchua commented Aug 14, 2020

My test was wrong and I have fixed it

@steveharter steveharter added this to the 6.0.0 milestone Aug 14, 2020
jkotas
jkotas reviewed Aug 14, 2020
View reviewed changes
src/libraries/System.Reflection.Emit.ILGeneration/tests/ILGenerator/EmitMethodInfo.cs Outdated
public void EmitMethodInfo()
{
var methodType = typeof(IWithIn<int>);
var method = methodType.GetMethod("Method");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We use var only when the type is obvious from the right side.

https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/coding-style.md

jkotas
jkotas approved these changes Aug 14, 2020
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Looks good for .NET 6

@steveharter
Copy link
Member

Test failures appear unrelated: one timeout and one OSX failure in System.Net.WebSockets.Client.Tests.ConnectTest.ConnectAsync_CancellationRequestedAfterConnect_ThrowsOperationCanceledException

@steveharter steveharter changed the title Fixed Emit MethodInfo for interface generic Fix MethodInfo Emit for generic interfaces Aug 24, 2020
@steveharter steveharter merged commit 9ac61c0 into dotnet:master Aug 24, 2020
@stakx
Copy link
Contributor

stakx commented Aug 24, 2020

@wzchua, I'm glad to see this long-standing issue finally resolved. Thanks for working on it!

@dsyme dsyme mentioned this pull request Aug 25, 2020
Closed
@jkotas jkotas mentioned this pull request Nov 9, 2020
Closed
@wzchua wzchua mentioned this pull request Nov 23, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@wzchua wzchua deleted the emit-generate-incorrect-methodinfo-token branch March 5, 2021 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@steveharter steveharter steveharter left review comments

@jkotas jkotas jkotas approved these changes

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

Assignees
No one assigned
Labels
area-System.Reflection.Emit
Projects
None yet
Milestone
6.0.0
6 participants
@wzchua @dnfadmin @steveharter @stakx @jkotas @Dotnet-GitSync-Bot