Adding new strongly typed methods to Clipboard, DataObject and IDataObject.

[Tanya-Solyanik]

Related to ##12362
Fixes #11350

The TryGetData methods use NRBF deserializer by default and will fall back to use BinaryFormatter if the application opts into BinaryFormatter use in this context.
The GetData methods have a compatibility mode when the appropriate AppContext switches are enabled but by default they can read only known and primitive types or POCOs with primitive fields. These methods in the DataObject class are obsoleted.

Microsoft Reviewers: Open in CodeFlow

[JeremyKuhne]

Added some comments.

[JeremyKuhne]

Added some more comments. I'm a little concerned that we're using TypeName where it isn't necessary in some cases. We should optimize this before we fully validate this as changing it afterwords would be risky.

I still need to spend more time reviewing.

[JeremyKuhne]

We should not be falling into the default interface implementations for our own types that derive from IDataObject.

[codecov]

Codecov Report

Attention: Patch coverage is 85.01695% with 221 lines in your changes missing coverage. Please review.

Project coverage is 75.77604%. Comparing base (a3973fb) to head (05e9baa).
Report is 1 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11545         +/-   ##
===================================================
+ Coverage   75.71735%   75.77604%   +0.05868%     
===================================================
  Files           3152        3158          +6     
  Lines         635709      636985       +1276     
  Branches       46970       47067         +97     
===================================================
+ Hits          481342      482682       +1340     
+ Misses        150932      150838         -94     
- Partials        3435        3465         +30     

Flag	Coverage Δ
Debug	75.77604% <85.01695%> (+0.05868%)	⬆️
integration	18.23885% <12.47803%> (-0.03133%)	⬇️
production	49.40029% <78.38313%> (+0.12278%)	⬆️
test	97.03785% <89.18322%> (-0.01246%)	⬇️
unit	46.38624% <78.38313%> (+0.13154%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[lonitra]

Got through about half

[JeremyKuhne]

Why are we catching at all? Shouldn't resolvers always throw through?

[Tanya-Solyanik]

This is the case when we couldn't verify if type is assignable to T, I think we should return false. Otherwise resolver should resolve all possible types, which is impossible or we would always throw on type mismatch. Exception is more suitable for when serialization fails.

[JeremyKuhne]

I would think that TypeName is better here. @adamsitnik there might be some sharing we can do of core type handling? Would very much like your feedback on this file in any case. :)

[Tanya-Solyanik]

Why do you think that TypeName is a better option - TypeName class does not expose functions to compare TypeNames

[JeremyKuhne]

Because then you are comparing the right components- it is the whole point of TypeName. String comparison is fraught with errors.

[adamsitnik]

TypeName class does not expose functions to compare TypeNames

It does not, because different users want to compare different parts of type name. Some want to compare just the full names, some want to include assembly names, but without version. Some want to make it always a full match.

For example, SerializationRecord.TypeNameMatches ignores assembly names: https://github.com/dotnet/runtime/blob/5d69e2dca30524a93b00cd613be218144b5f95d1/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs#L48

So we decided to not include any default comparison method just to force all the users to think what they want to compare and implement it themselves.

[JeremyKuhne]

Can this be is T data? Should probably change _data to _mappedData for clarity.

[Tanya-Solyanik]

is T data would be a second declaration of data as a local variable , while it's already a function parameter

[lonitra]

When do we call this with T = NotSupportedException?

[Tanya-Solyanik]

We would want to try to read this exception in case the write for the format failed and the exception was placed on the clipboard

[lonitra]

Makes sense, but IIUC I don't think we ever call this method with T = NotSupportedException so we will never get into this if statement.

[JeremyKuhne]

More comments

[JeremyKuhne]

For anything where there is just one caller they should be an inline method or the code should just be inlined.

[Tanya-Solyanik]

I made several functions here local

[JeremyKuhne]

Where did this come from? If the code is trying to avoid allocating the full name string, ok, but it should probably use HashCode.Combine().

[Tanya-Solyanik]

I can't use the full name as an identity because it could contain assembly name in constructed types, for example, that might be type-forwarded to another assembly.

[Tanya-Solyanik]

Where did this come from? - I didn't find anything in the runtime.. Do you have suggestions where to search?

[JeremyKuhne]

Because then you are comparing the right components- it is the whole point of TypeName. String comparison is fraught with errors.

[adamsitnik]

I provided the answers to all the questions, please feel free to tag me again.

[adamsitnik]

Any thoughts on our canonical example for this??

The main gotchas here are:

• different assembly versions. Example: the user has persisted and instance of MyClass1 using an older version of the same assembly: MyClass1, MyAssembly, Version=1.0.0.0 vs MyClass1, MyAssembly, Version=2.0.0.0. Full name does not include the version for all the type names except of generic types.
• type forwarding (how to get it). I am not sure how common it is for the end users to use it, but it's quite common in runtime for the most basic types. Example: https://github.com/dotnet/runtime/blob/e5129e5323155feec25cd75e8d67986b601921d1/src/libraries/System.Private.CoreLib/src/System/DateTime.cs#L48

For both cases, it gets really tricky for generic types, where typeof(T).FullName is different than the name exported by binary formatter (because full name of the generic type includes the assembly names of generic type arguments). That is why I was pushing so hard to include SerializationRecord.TypeNameMatches in the public API (so the users would not need to worry about it).

So as long as you don't expect generic types with generic arguments, the example is fine. If you expect them to be common, you could extend the comparison to do something similar to https://github.com/dotnet/runtime/blob/5d69e2dca30524a93b00cd613be218144b5f95d1/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs#L68.

And BTW it's great that you return the type itself, as it avoids the possibility to load a hostile assembly. Example: if we have a Type: "MyClass1, MyNiceAssembly" and a type name "MyClass1, HostileAssembly", the full names match and returning the type avoids loading the HostileAssembly, which users could do with

if (type.FullName == typeName.FullName)
{
-    return type;
+    return Type.GetType(typeName.AssemblyQualifiedName);
}

is it possible to make your Matches method public for use in this meth

The method you have referenced (SerializationRecord.TypeNameMatches) is already public, but from the context I guess that you meant the helper method that it calls with TypeName and Type arguments which makes it unrelated to SerializationRecord:

https://github.com/dotnet/runtime/blob/5d69e2dca30524a93b00cd613be218144b5f95d1/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs#L68

The job it does is specific to NRBF, so I am pretty sure that we can't include it in TypeName (System.Reflection.Metadata needs to be unaware of the existence of NRBF and can't offer anything specific to NRBF). But I would not mind exposing it in System.Formats.Nrbf, I am just not sure what would be the best way to do it (we can't expose extension methods for types that we own) and if you want to have a depedency to System.Formats.Nrbf.