-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
IDynamicInterfaceCastable interface #36654
Comments
I dislike the lack of type-safety around the |
It does have a similar issue, but no where near as dangerous. The distinction here is the limiting factor of interfaces. There are no fields access issues here because there are no fields only method dispatch issues. Those need to be considered, but the default interface approach limits a large vector of the worst kind of issues in The |
Also, note that |
So whats the worst that can happen here? What would happen if you call another interface method that doesn't have a default implementation? If it just throws an exception and doesn't corrupt runtime state that sounds fine to me. |
@Scottj1s from a CSWinRT perspective, this will require a lot of additional code generation and an introduction of a "target" switch for code-gen since it's so heavily based on default interface methods, which aren't valid in the .NET standard target. |
If this is asking must a cast be performed because
This question is implied in the design notes and we need to understand how/if we can fail "gracefully". At a minimum we need to not corrupt the runtime so a fail fast might be what we get, but that is something we need to investigate. |
@jkoritzinsky, as I understand the spec, this would require a parallel implementation of each interface (IFooImpl) to address the casting and dispatching of the smuggled 'this' pointer. In that regard, it add some significant bloat. I was hoping there might be a way to somehow fold ABI::IFoo and IFooImpls together. Can you elaborate on the "target" switch? |
@AaronRobinsonMSFT, I think we need to address support for classes as well as interfaces. I'm concerned that this design, which allows an interface cast to be implemented with little overhead, is imposing the constraint that classes can't be supported. But that will be a common use case for C#/WinRT. |
@Scottj1s If that is truly needed then we would need to think about actually going the |
@AaronRobinsonMSFT, here's an example: |
Another example (although more theoretical): WinUI specifies a custom runtime class name for most every type they define in native code, even if it isn't exposed in managed code. Let's say that they introduce a new implementation of The impetus for requesting ICastableObject from WinUI was this exact scenario with interfaces, so I wouldn't be amazed if they would also hit it with classes. |
DeviceInformationCollection is a bad example - that's a C#/WinRT code-gen isssue. Jeremy's example is exactly what I was trying to describe: code-based and not metadata-based. Most C++/WinRT implementations will behave this way. |
I would consider that a bug though. Right? If so I don't think we can design anything that will mitigate these kind of bugs.
That isn't entirely accurate. The |
I do not see where this bloat would come from. Can you elaborate?
For classes, the projections have to create the right wrapper type from the start (e.g. using
We stopped designing the platform features for .NET standard first several years ago. The problem with designing for .NET standard first is that it makes the design optimized for the past, not for the future, that is a sure way to run the platform into the ground. We design for future (ie the latest .NET version) first. If there is a strong reason to have the feature available for netstandard too, we look at the best way to make that happen in some form - it is typically degraded experience and requires extra work. We do not let the netstandard constrains to leak into the forward looking design. |
I think I'm persuaded. A runtimeclass is a convenient interface container. If a type hasn't been modeled that way, the user will have to cast per-interface, which is fine. All members must be exposed via interfaces anyway. |
In a design more similar to CoreRT's CastableObject, C#/WinRT could directly re-use their current "ABI" types that implement the .NET definition of the interface over an IntPtr. With this design, C#/WinRT will have to provide both the current ABI type implementation, as well as a new implementation that ICastableObject will return that unwraps the object and either constructs the current ABI type and calls that to do the work, or emits the implementation again to do all of the work inline.
The netstandard mention was more for @Scottj1s since C#/WinRT will have to think about it for their usage of this feature. |
Is there any story here for netstandard? I was hoping we could bury the details in winrt.runtime.dll, so an explicit target switch wouldn't be necessary. Beyond that, would netstandard just not have any dynamic casting support? That's certainly acceptable, if we have no choice (as with reference tracking). |
@Scottj1s unfortunately this entire path would require .NET5. The default interface support required some runtime features. |
Cc @vitek-karas @marek-safar since this affects interface sweeping in Linker ( Also, Marek, could you add relevant folks from Mono side? |
What kind of code do we expect to see in Making this declarative would be more friendly to static analysis that is necessary to make this work with linking and AOT. |
Please don't constrain the implementation for non-AOT scenarios too much, we'll probably have to roll our own RCW implementation since I suspect our scenarios will not to be supported out of the box (we use custom TLB generation since TLBs have been dropped from .NET Core, but they do work if implemented manually), so if the low level support is too restrictive this would prevent us from adapting ComWrapper. The new implementation should be fully capable to replace the current RCW/CCW system with a custom one, not build a new locked-in box that you have to work around. (It sounds like we'd have to fork the runtime to extend a declarative system if it turns out to be short sighted, which is exactly the same problem the old COM interop had and ComWrappers tries to solve by moving implementations to user code, so being too declarative has me worried of repeating the same mistakes again.) |
@weltkante Are you expecting to be using something like TypeBuilder to construct interface types at runtime? I wouldn't be opposed to an extra API to allow those scenarios, but those will be marked IL Linker unsafe and one will not be able to use IL Linker with that API (without seeing a bunch of warnings, that is). If you're not using |
During the API review we discussed whether we should allow leaking out arbitrary exceptions from the One thing occurred to me - should we also loop in the codegen team on this?
|
namespace System.Runtime.InteropServices
{
public interface IDynamicInterfaceCastable
{
RuntimeTypeHandle GetInterfaceImplementation(RuntimeTypeHandle interfaceType, bool throwIfNotFound);
}
[AttributeUsage(AttributeTargets.Interface,
AllowMultiple = false,
Inherited = false)]
public sealed class DynamicInterfaceCastableImplementationAttribute : Attribute
{
}
} |
@dotnet/jit-contrib @AndyAyersMS @BruceForstall @davidwrighton Any thoughts on @MichalStrehovsky's comment at #36654 (comment). |
The transient exceptions like OutOfMemoryException or certain FileLoadExceptions can leak from pretty much everywhere. The codegen makes simplifying assumption that optimizing out these exceptions or moving them around is acceptable. |
I think we should allow leaking arbitrary exceptions. There is no good way to tell whether the exception is one of these transient exceptions that are ok to leak; or whether the implementation is misbehaving. The documentation should describe the contract that well-behaved implementation |
The question is how callers are supposed to reason about this behavior. I guess I buy that catching |
Pretty much anything in .NET can throw For example, For callers, it means to only catch exceptions that they expect and understand. Leave all other exceptions alone as they are most likely |
|
There are actually 3 different circumstances in which the method on the new interface is called. There is the hard cast ( The approved api allows the implementation to distinguish between the first two, but it cannot tell the difference between the first and the third. This seems like it could be useful. For example, if you are checking the instances during cast operations, this might be somewhat costly. One may want to skip those checks during virtual calls that are taking the slow path in the VSD. Skipping those checks would normally be safe for two reasons. First, if the calling code is verifiable, then it must have already done one of the cast operations, which must have returned success. Second, if the calling code was unsafe (not verifiable) and made the interface call without the cast operation, it could just as easily have passed the object to some other function that takes the target interface, and makes the virtual interface call on it. In that case, if the VSD already has the virtual call cached, then it would bypass IDynamicInterfaceCastable entirely. So if unsafe code accidentally calling the DIMs without running a cast opcode first is a concern for somebody, they already would need to make the DIMs resilient to this scenario. And of course, this is only one possible reason for wanting to know the that the Interface is being invoked for virtual call reasons, as opposed to casting reasons. There may be other reasons for wanting to know the difference. I’m not thinking of any off the top of my head, but that does not mean there are no others. So it might make sense to tweak the api slightly. Perhaps the bool argument should become a three way enum? |
Another observation is that the implementation interface is not needed for casting. If we wanted to allow the callbacks to do the minimal amount of work possible, we may want to split it into two methods: RuntimeTypeHandle GetInterfaceImplementation(RuntimeTypeHandle interfaceType);
bool IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented); |
I like splitting into two. namespace System.Runtime.InteropServices
{
public interface IDynamicInterfaceCastable
{
/// <summary>
/// Called when an implementing class instance is cast to an interface type that
/// is not contained in the class's metadata.
/// </summary>
/// <param name="interfaceType">The interface type.</param>
/// <param name="throwIfNotImplemented">Indicates if the function should throw an exception instead of returning false.</param>
/// <returns>Whether or not this object can be cast to the given interface</returns>
/// <remarks>
/// This is called if casting this object to the given interface type would
/// otherwise fail. Casting here means the IL isinst and castclass instructions
/// in the case where they are given an interface type as the target type.
///
/// If <paramref name="throwIfNotImplemented" /> is true and an exception
/// is not thrown by the implementation, but false is returned, then
/// <see cref="System.InvalidCastException" /> will be thrown.
/// </remarks>
bool IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented);
/// <summary>
/// Called during interface dispatch when the given interface type cannot be found
/// in the class's metadata.
/// </summary>
/// <param name="interfaceType">The interface type.</param>
/// <returns>The type that should be used to dispatch for <paramref name="interfaceType"/>.</returns>
/// <remarks>
/// When this function is called, the cast of this object to the given interface
/// should already have been verified through the castclass/isinst instructions.
///
/// The returned type must be an interface type marked with the
/// <see cref="DynamicInterfaceCastableImplementationAttribute"/>, otherwise
/// <see cref="System.InvalidOperationException" /> will be thrown.
/// </remarks>
RuntimeTypeHandle GetInterfaceImplementation(RuntimeTypeHandle interfaceType);
}
} |
Just in case this is related and no-one had considered it, I thought I'd mention a source of crashes with the existing implementation in a C# UWP app. Implicitly casting a list of class instances to a list of interfaces that they implement frequently results in an app crash when the list is used later on. For example, if I do this: var myList = new List<StorageFile>();
//add some storage files to list
var dataPackage = new DataPackage();
await dataPackage.SetStorageItemsAsync(myList); Then the app crashes (well it did last time I checked, but I haven't tested recently). Note that the definition of public void SetStorageItems(IEnumerable<IStorageItem> value); so the list is implicitly cast from This is not the only case where I have experienced problems before. In one case the crash actually went away by looping through the list using an iterator with empty body, like foreach(item in list){
//Do nothing!!
} This was terribly confusing as I only added the loop to try to debug the issue (I had a Debug.WriteLine in it to start with) and it turned out that my debugging code fixed the problem!! I hope that whatever solution you come up with can handle this implicit casting of lists correctly. |
@benstevens48 Thanks for letting us know. This API will be leveraged by the C#/WinRT tool chain and that would be responsible for handling a majority of these kind of UX cases. If possible it might be interesting to try the above now using the new tool chain and check the behavior. If this isn't possible, creating an issue at https://github.com/microsoft/CsWinRT would put the behavior on their radar for future validation. |
Implemented in #37042 |
Background and Motivation
In the .NET Native runtime support existed for a .NET class to participate in a C-style cast when that class didn't support the cast-to type. The COM
QueryInterface()
is an example where this kind of scenario exists. In COM, the instance is represented by an opaqueIUnknown
pointer and the only way to discover what is supported by the instance is to callQueryInterface()
with aGUID
representing the question of "Does this instance support this type?". The answer is typically a simple "yes" (S_OK
) or "no" (E_NOINTERFACE
). If the answer is "yes", a casted to instance of the type is returned, otherwisenull
. There are scenarios where the current instance may not have implemented this type but can provide an instance that does - this is called a tear-off.In .NET, the metadata for a type is static and therefore if a type can't be cast to another type because it isn't in metadata that is correct. This means that a type has no reason to participate in the casting question. However, when implementing support for a COM scenario this rigidity isn't as beneficial since it may not be possible to know all the supported types on an instance. This proposal provides a way to let types provide an answer and an
object
instance that does satisfy the requested type eventhough the original instance does not and still adhere to the static metadata constraints of .NET.In .NET Native there are two mechanisms to address this problem. The first,
ICastable
interface, proved to have usability issues. Usage of theICastable
API was error prone and had a potentially catastrophic consequence if used incorrectly - silent success or an unstable runtime. TheICastable
API exists in the CoreCLR runtime but is not publicly exposed and exists only to support MCG scenarios.The second approach was
CastableObject
. This approach didn't return a type but instead returned an actualobject
instance to dispatch on. TheCastableObject
type is anabstract
type that contained some minor state for caching purposes. This approach did require inserting a new type into the user's type hierarchy. Updating the type hierarchy and the stored state of theabstract
type made this solution more reliable, but less performant thanICastable
.For CoreCLR, the following proposal is based on lessons learned from .NET Native along with a recent C# language feature, default interfaces, that make a modified version of the
ICastable
approach easier to implement with confidence. The proposed interface followed by a usage example are described below.Goals
ComWrappers
API in creating C# friendly wrappers for externalIUnknown
based objects.QueryInterface()
for all possible supported types when an externalIUnknown
based object enters the runtime.Non-Goals
ICastable
scenarios.Proposed API
Design notes
this
pointer that by definition should implement the enclosing type (e.g.IFooImpl
) as well as any implementing types (e.g.IFoo
). We will need to define the exact semantics and requirements here.ICastableObject
could control casting on a per object basis, as we can easily callGetInterfaceImplementation()
at each cast opportunity.ICastableObject
would control the result of a dispatching on an interface at a per TYPE level. So, a given type could not use different default interface type (e.g.IFooImpl
) for different instances. This would be sufficient for any plausible use of this feature for interop, but it might impact useablility for aspect oriented programming, etc.Usage Examples
Consider the following interface and class.
The following is an example of usage.
Community impact
ICastable
request: Proposal for System.Runtime.CompilerServices.ICastable .NET Core public API #23727ICastable
: COM Interop Guidance corert#4219 (comment)ComWrappers
API: ComWrapper RCW Design question #35929The text was updated successfully, but these errors were encountered: