Consume IDynamicInterfaceCastable in the .NET 5 projection #369

jkoritzinsky · 2020-08-14T22:02:14Z

Use IDynamicInterfaceCastable to drive the managed->native calls in the .NET 5 projection.

Stop shipping the .NET Standard 2.0 projection.

TODO:

Validate that interface that inherit from collection types correctly implement all of the interface members from direct and transitive parents.
Move manually written type mappings over to IDynamicInterfaceCastable for .NET 5.
Update the RCW creation (not activation) to instantiate WinRT.IInspectable for all interface types on .NET 5.

…nterfaces.

…ories and FromAbi.

…as linked away since no members on the interface were called.

… hood via an internal IInspectable instance.

…nterfaces of runtime classes such that they can live in different projections and compile with only the public contract exposed.

… perf).

jkoritzinsky · 2020-08-17T22:45:13Z

There are a few perf losses in this PR that we should work on before merging this in:

Calls are slightly slower since they need to get the object reference from the cache instead of it just being a field on the object.
A cast that is not cached is significantly slower due to the reflection used to get a correctly typed vtable.

…ation

Scottj1s · 2020-09-23T22:35:39Z

  <group targetFramework=".NETStandard2.0" />

let's not remove just yet - there are non-Unity projects (WinML) that are evaluating cswinrt netstandard2.0

Refers to: nuget/Microsoft.Windows.CsWinRT.nuspec:17 in 471979e. [](commit_id = 471979e, deletion_comment = True)

Scottj1s · 2020-09-23T22:37:18Z

cswinrt/code_writers.h

        auto type_name = write_type_name_temp(w, type, "%", true);
+        //std::cout << type_name << std::endl << std::endl << std::endl;


jkoritzinsky · 2020-09-23T22:38:23Z

We should make sure to ship it in a separate package then. Otherwise people might get TypeLoadExceptions if they make a library targeting .NET Standard 2.0 and an application targeting .NET 5.0-Windows.

Scottj1s · 2020-09-23T22:46:40Z

#include

cruft? (std::cout)

Refers to: cswinrt/code_writers.h:6 in 471979e. [](commit_id = 471979e, deletion_comment = False)

Scottj1s · 2020-09-23T22:52:51Z

UnitTest/TestComponentCSharp_Tests.cs

-
+        }
+
+#if NETCOREAPP2_0


is the failing test? if so can you add a comment/TODO?

This is not, but this test uses the ABI type directly which is not possible with IDIC anymore so I removed it from .net5

Scottj1s · 2020-09-23T22:54:17Z

UnitTest/TestComponentCSharp_Tests.cs

@@ -775,7 +777,7 @@ public void TestClassGeneric()
            {
                var obj = objs[i];
                Assert.Same(obj, TestObject);
-                Assert.Equal(TestObject.ThisPtr, objs[i].ThisPtr);
+                Assert.Equal(TestObject, objs[i]);


how come the ThisPtr check was removed?

There's no longer a ThisPtr property exposed because it makes the public surface area not match the WinMD contract and it isn't entirely accurate for composed types. There's no good reason to use the ThisPtr property, so I removed it early in the design.

Scottj1s · 2020-09-23T22:56:52Z

using System;

I don't see any IDynamicInterfaceCastable tests - can we add a few?

Refers to: UnitTest/TestComponentCSharp_Tests.cs:1 in 471979e. [](commit_id = 471979e, deletion_comment = False)

Scottj1s · 2020-09-23T23:00:53Z

WinRT.Runtime/IInspectable.cs

@@ -16,7 +16,7 @@ public enum TrustLevel
    // IInspectable
    [ObjectReferenceWrapper(nameof(_obj))]
    [Guid("AF86E2E0-B12D-4c6a-9C5A-D7AA65101E90")]
-    public class IInspectable
+    public partial class IInspectable


partial only to combine with IInspectable.net5.cs? #if NET5_0 might be simpler

Given that IInspectable implements an extra interface in .NET 5.0, we'd have to add quite a few #if NET5_0 lines throughout the file. It's easier IMO to have two separate files.

Scottj1s · 2020-09-23T23:05:29Z

WinRT.Runtime/Interop/IAgileReference.netstandard2.0.cs

+            public static readonly Vftbl AbiToProjectionVftable;
+            public static readonly IntPtr AbiToProjectionVftablePtr;
+
+#if NETSTANDARD2_0


cruft: everywhere we've forked sources, we can get rid of conditional compilation

Scottj1s · 2020-09-23T23:09:21Z

many of the .net5.cs/.netstandard2.0.cs splits seem unnecessary (the sources are identical). are we expecting them to diverge? if not, can we remove the duplication?

Scottj1s · 2020-09-24T03:32:06Z

We should make sure to ship it in a separate package then. Otherwise people might get TypeLoadExceptions if they make a library targeting .NET Standard 2.0 and an application targeting .NET 5.0-Windows.

I'm fine with that for RTM - we have a few validation scenarios that are targeting netstandard2.0 that I want to keep enabled in the meantime.

Scottj1s

manodasanW · 2020-09-24T21:15:58Z

WinRT.Runtime/Interop/IAgileReference.net5.cs

+            public static readonly Vftbl AbiToProjectionVftable;
+            public static readonly IntPtr AbiToProjectionVftablePtr;
+
+#if NETSTANDARD2_0


Do we need netstandard compat in a net5 file?

How much diff is there to merge both files?

I think I fixed it in a newer commit.

WinRT.Runtime/Projections/Bindable.net5.cs

manodasanW · 2020-09-24T22:01:36Z

WinRT.Runtime/Projections/IEnumerable.net5.cs

+            }
+
+            public bool _MoveNext()
+            {


Is it possible for someone to call movenext first without calling current or hascurrent or is it just us calling it? If it is possible, would that make m_firstItem wrong?

manodasanW · 2020-09-24T22:27:32Z

WinRT.Runtime/Projections/IPropertyValue.net5.cs

+                }
+                catch (InvalidCastException elementCastException)
+                {
+                    //global::System.Exception e = new InvalidCastException(string.Format(SR.InvalidCast_WinRTIPropertyValueArrayCoersion, value.GetType(), typeof(T[]).Name, i, elementCastException.Message), elementCastException.HResult);


is commented code intentional?

jkoritzinsky added 16 commits August 11, 2020 15:27

Don't ship the netstandard2.0 projection support.

4be3620

Start refactoring for implementing IDynamicInterfaceCastable.

7ad98c8

Add IWinRTObject interface and continue implementing the updates to i…

58c590a

…nterfaces.

Implement IDynamicInterfaceCastable for generated code excluding fact…

6e455f3

…ories and FromAbi.

Add fallback for when an interface is kept alive but its Vftbl type w…

686cead

…as linked away since no members on the interface were called.

Move statics and factory caches to the IDynamicInterfaceCastable plan.

4fd093b

Use the QI cache when checking if an interface is implemented.

89a83ee

Move activation/composition to work with IDynamicInterfaceCastable.

6511ae6

Use equality functions instead of ThisPtr.

bcaf88e

Implement static interface support by using dynamic casting under the…

cc30448

… hood via an internal IInspectable instance.

Update to LangVersion preview

1f2efee

Fix projected required interface implementations and handle default i…

1f4d869

…nterfaces of runtime classes such that they can live in different projections and compile with only the public contract exposed.

Use Spans instead of LINQ to improve perf in Guid generation (startup…

25e4e4b

… perf).

Get WinUI projection compiling with no new warnings.

b58f37d

Fix more collections so the Windows projection also builds.

a02a72c

Cache cast results in .NET 5 projection to regain some perf losses.

b5c3c31

Add support for IDynamicInterfaceCastable-based interfaces in RCW cre…

597a024

…ation

jkoritzinsky mentioned this pull request Aug 27, 2020

Adding a specific WPF control to the UI causes WinRT API to stop working #381

Closed

Merge remote-tracking branch 'origin' into IDynamicInterfaceCastable

229fead

jkoritzinsky force-pushed the IDynamicInterfaceCastable branch from 049eb26 to 229fead Compare September 10, 2020 23:02

ujjwalchadha added 2 commits September 16, 2020 11:55

Add field access optimization for default interfaces

c38b4a2

Added manual type mappings for IDynamicInterfaceCastable

471979e

ujjwalchadha requested a review from Scottj1s September 23, 2020 20:32

Scottj1s changed the title ~~WIP: Consume IDynamicInterfaceCastable in the .NET 5 projection~~ Consume IDynamicInterfaceCastable in the .NET 5 projection Sep 23, 2020

Scottj1s requested a review from manodasanW September 23, 2020 22:16

Scottj1s reviewed Sep 23, 2020

View reviewed changes

ujjwalchadha added 2 commits September 24, 2020 00:50

Code cleanup

ba117da

Add tests for IDynamicInterfaceCastable

1cb8c01

Scottj1s approved these changes Sep 24, 2020

View reviewed changes

manodasanW reviewed Sep 24, 2020

View reviewed changes

WinRT.Runtime/Projections/Bindable.net5.cs Show resolved Hide resolved

manodasanW reviewed Sep 24, 2020

View reviewed changes

jkoritzinsky mentioned this pull request Sep 24, 2020

Performance of TryUnwrapObject can be improved by caching attributes. #437

Closed

manodasanW reviewed Sep 24, 2020

View reviewed changes

manodasanW approved these changes Sep 24, 2020

View reviewed changes

ujjwalchadha and others added 4 commits September 24, 2020 20:48

Merged Master into IDynamicInterfaceCastable

4418c1b

Merge branch 'master' into IDynamicInterfaceCastable

80ccff5

Fix build errors

94294d2

Fix authoring

b06d2d3

ujjwalchadha merged commit 17687ff into master Sep 26, 2020

manodasanW deleted the IDynamicInterfaceCastable branch November 11, 2020 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consume IDynamicInterfaceCastable in the .NET 5 projection #369

Consume IDynamicInterfaceCastable in the .NET 5 projection #369

jkoritzinsky commented Aug 14, 2020 •

edited

Loading

jkoritzinsky commented Aug 17, 2020

Scottj1s commented Sep 23, 2020

Scottj1s Sep 23, 2020

jkoritzinsky commented Sep 23, 2020

Scottj1s commented Sep 23, 2020

Scottj1s Sep 23, 2020

ujjwalchadha Sep 23, 2020

Scottj1s Sep 23, 2020

jkoritzinsky Sep 24, 2020

Scottj1s commented Sep 23, 2020

Scottj1s Sep 23, 2020

jkoritzinsky Sep 24, 2020

Scottj1s Sep 23, 2020

Scottj1s commented Sep 23, 2020

Scottj1s commented Sep 24, 2020

Scottj1s left a comment

manodasanW Sep 24, 2020

manodasanW Sep 24, 2020

ujjwalchadha Sep 24, 2020

manodasanW Sep 24, 2020

manodasanW Sep 24, 2020

		auto type_name = write_type_name_temp(w, type, "%", true);
		//std::cout << type_name << std::endl << std::endl << std::endl;

Consume IDynamicInterfaceCastable in the .NET 5 projection #369

Consume IDynamicInterfaceCastable in the .NET 5 projection #369

Conversation

jkoritzinsky commented Aug 14, 2020 • edited Loading

jkoritzinsky commented Aug 17, 2020

Scottj1s commented Sep 23, 2020

Choose a reason for hiding this comment

jkoritzinsky commented Sep 23, 2020

Scottj1s commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottj1s commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottj1s commented Sep 23, 2020

Scottj1s commented Sep 24, 2020

Scottj1s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkoritzinsky commented Aug 14, 2020 •

edited

Loading