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

[API Proposal]: DispatchProxy.Create<TProxy>(Type interfaceType) #67444

Closed
AndreasHoffmann2 opened this issue Apr 1, 2022 · 11 comments
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@AndreasHoffmann2
Copy link

AndreasHoffmann2 commented Apr 1, 2022

Background and motivation

I just started using the DispatchProxy. Unfortunately, my code does not have a generic type-parameter for the interface type, since I determine the interface I want to generate at runtime.
That's why I would like to use to a method that accepts at least the interface as normal parameter:
DispatchProxy.Create<TProxy>(Type interfaceType)
The class does convert the generic parameter to the type immediately anyway.

My current workaround is rather ugly:
object myInstance = typeof(DispatchProxy).GetMethod("Create").MakeGenericMethod(interfaceType, typeof(MyDispatchProxy)).Invoke(null, Type.EmptyTypes);

Perhaps, for completeness' sake, a non-generic method would make sense well:
DispatchProxy.Create(Type interfaceType, Type proxyType)

API Proposal

namespace System.Reflection
{
    public class DispatchProxy
    {
        // existing
        public static T Create<T, TProxy>() where TProxy : DispatchProxy { ... }

+       [RequiresDynamicCode("The native code for the method requested might not be available at runtime.")]
+       public static object Create<TProxy>(Type interfaceType) where TProxy : DispatchProxy { ... }
        
         // Maybe also:
+       [RequiresDynamicCode("The native code for the method requested might not be available at runtime.")]
+       public static object Create(Type interfaceType, Type proxyType) { ... }
    }
}

API Usage

object myInstance = DispatchProxy.Create<MyDispatchProxy>(interfaceType);
//instead of:
object myInstance = typeof(DispatchProxy).GetMethod("Create").MakeGenericMethod(interfaceType, typeof(MyDispatchProxy)).Invoke(null, Type.EmptyTypes);

### Alternative Designs

_No response_

### Risks

_No response_
@AndreasHoffmann2 AndreasHoffmann2 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 1, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Apr 1, 2022
@ghost
Copy link

ghost commented Apr 1, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

I just started using the DispatchProxy. Unfortunately, my code does not have a generic type-parameter for the interface type, since I determine the interface I want to generate at runtime.
That's why I would like to use to a method that accepts at least the interface as normal parameter:
DispatchProxy.Create(Type interfaceType)
The class does convert the generic parameter to the type immediately anyway.

My current workaround is rather ugly:
object myInstance = typeof(DispatchProxy).GetMethod("Create").MakeGenericMethod(interfaceType, typeof(MyDispatchProxy)).Invoke(null, Type.EmptyTypes);

Perhaps, for completeness' sake, a non-generic method would make sense well:
DispatchProxy.Create(Type interfaceType, Type proxyType)

API Proposal

namespace System.Reflection
{
    public class DispatchProxy
    {
        public static object Create<TProxy>(Type interfaceType) where TProxy : DispatchProxy { ... }
        //Maybe also:
       public static object Create(Type interfaceType, Type proxyType) { ... }
    }
}

API Usage

object myInstance = DispatchProxy.Create<MyDispatchProxy>(interfaceType);
//instead of:
object myInstance = typeof(DispatchProxy).GetMethod("Create").MakeGenericMethod(interfaceType, typeof(MyDispatchProxy)).Invoke(null, Type.EmptyTypes);

### Alternative Designs

_No response_

### Risks

_No response_

<table>
  <tr>
    <th align="left">Author:</th>
    <td>AndreasHoffmann2</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`api-suggestion`, `area-System.Reflection`, `untriaged`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@MichalStrehovsky
Copy link
Member

Looks the same as #65761.

@AndreasHoffmann2
Copy link
Author

AndreasHoffmann2 commented Apr 2, 2022

Yes it is. I did not find the old one. Sorry.

I do not agree with the arguments there and in #28419.

Using reflection is normally considered a last resort to work around an API that does not offer methods for otherwise valid requirements. Detecting a type at runtime and creating a proxy is such a valid requirement.

An API should never be designed in a way that valid requirements require the use of reflection.

A quote from a C#-language-design-meeting:
"We can and do allow users to shoot their feet off. However, lets not give them something capable of doing nothing else."

Thus it would be totally in line with the concept of C# itself to offer a function that has many valid use cases and some minor drawbacks if used incorrectly. We are talking about a library for professionals here. Every single one should know that using generics is always preferable if possible. If he doesn't know the obvious, then he needs to learn it. By trial and error if necessary.

But we should not cripple the API with respect to developers who do not read the docs.

@MichalStrehovsky
Copy link
Member

We can now mark AOT unfriendly APIs with the RequiresDynamicCode attribute so if we were to add it, we can at least annotate it as such.

That said, the problem of people using a System.Type overload instead of the generic method is real; another example in addition to the ones in #28419: Marshal.SizeOf has a generic overload that works across all form factors of .NET, similar to Marshal.GetDelegateToFunctionPointer. The problem with people using the System.Type overload where the generic one would be better is absolutely real and the number of libraries where this is the only AOT problem is big.

@AndreasHoffmann2
Copy link
Author

AndreasHoffmann2 commented Apr 4, 2022

Thank you for your effort and the interesting read I had afterwards about AOT :-)

That's 3,500 typeof-usages vs. 1,200 generic ones. I really don't like to think that other developers are just not competent enough to use the generic method. Thus I kind of hope that most of the typeof-usages are older than the generic function itself, which was introduced 2013 with .NET 4.5.1. And it probably took some time until people really started using it.

In my head, each usage of reflection does actually hurt. And I like to think that this is the case for most developers.

But there are different levels of reflection:
Passing a Type to an API is bad. But there is something even worse: Fetch a method by name, stuff in generic parameters, invoke it and hope for the best.

The latter one should never be encouraged by anybody IMO. And in particular not by any project that has "dotnet" as first portion of it's name.

So adding the method would be the lesser of two evils.

And perhaps some other team will come up with a code-warning in the future. It does not seem too hard to me to detect this kind of abuse of typeof and show a warning in code.

@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 13, 2022

We can now mark AOT unfriendly APIs with the RequiresDynamicCode attribute so if we were to add it, we can at least annotate it as such.

That is great to hear, except the AOT compatibility issue I am supportive with this and previous similar API proposals, updated the proposal with the attribute. For the problem of people using a System.Type overload instead of the generic method as @AndreasHoffmann2 mentioned we can introduce a roslyn analyzer to detect such of usage of typeof and show a warning.

I wonder how useful will be the public static object Create(Type interfaceType, Type proxyType) { ... } overload. @AndreasHoffmann2 do you have/know a real scenario that would need/require a dynamic proxyType?

@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Apr 13, 2022
@buyaa-n buyaa-n added this to the 7.0.0 milestone Apr 13, 2022
@AndreasHoffmann2
Copy link
Author

I don't have any real world scenarios for the Type-Type-overloading. Only speculation of a case, where this might be necessary:
I could think of some configurable interface-decorator-pattern. Like one proxy that counts the calls, another one that logs the calls and one more to measure the performance of a call. And perhaps even more fancy stuff I cannot currently think of. This might be configured in some xml-file and stacked together at runtime.

But this is only speculation. In my case, I do know the proxy at compile-time.

@buyaa-n buyaa-n added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 26, 2022
@terrajobst
Copy link
Member

terrajobst commented Jun 23, 2022

Video

  • Makes sense
    • Let's skip the second one as it seems unfortunate to reverse the order if interface type and proxy type.
namespace System.Reflection;

public class DispatchProxy
{
    // Existing
    // public static T Create<T, TProxy>() where TProxy : DispatchProxy;
    [RequiresDynamicCode("The native code for the method requested might not be available at runtime.")]
    public static object Create(Type interfaceType, Type proxyType);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 23, 2022
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Jun 24, 2022
@buyaa-n buyaa-n modified the milestones: 7.0.0, 8.0.0 Jul 13, 2022
@buyaa-n buyaa-n closed this as completed Oct 11, 2022
@MichalStrehovsky
Copy link
Member

we can introduce a roslyn analyzer to detect such of usage of typeof and show a warning.

@buyaa-n is there an issue tracking adding the analyzer rule?

@MichalStrehovsky
Copy link
Member

we can introduce a roslyn analyzer to detect such of usage of typeof and show a warning.

@buyaa-n is there an issue tracking adding the analyzer rule?

...maybe it should just be tracked as part of #73136.

@buyaa-n
Copy link
Contributor

buyaa-n commented Oct 25, 2022

@buyaa-n is there an issue tracking adding the analyzer rule?

Yes, Analyzer: Detect the problem of using a System.Type overload instead of the generic overload

@ghost ghost locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

4 participants