Scale back [Serializable] on CoreCLR types #11765

morganbr · 2017-05-20T03:06:04Z

Removes SerializableAttribute from CoreCLR types not intended to be serializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime).

This is the first CoreCLR portion of https://github.com/dotnet/corefx/issues/19119.

Please do not merge until dotnet/corefx#20035 is merged since those are the test changes that match this change.

…erializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime).

morganbr · 2017-05-20T03:06:22Z

CC @dotnet/corert-contrib

danmoseley

LGTM. I checked the types you removed it from but not which ones you didn't.

jkotas · 2017-05-20T05:09:44Z

src/mscorlib/shared/System/DefaultBinder.cs

@@ -9,7 +9,6 @@
 namespace System
 {
    //Marked serializable even though it has no state.


Comment is no longer applicable.

jkotas · 2017-05-20T05:12:05Z

src/mscorlib/shared/System/Diagnostics/Tracing/EventSourceException.cs

@@ -18,9 +18,6 @@ namespace System.Diagnostics.Tracing
    /// <summary>
    /// Exception that is thrown when an error occurs during EventSource operation.
    /// </summary>
-#if !ES_BUILD_PCL
-    [Serializable]


I would think that this should stay, but under different ifdef. @brianrob ?

I believe that the EventSourceException is Serializable because we require all Exceptions to be serializable on the Desktop runtime to support Exceptions traveling across AppDomain boundaries.

EventSource is unusual, in that at least at one time we tried to keep the source code the same on Desktop and CoreCLR (however because shiproom would wanted only changes that were in response to a particular request to change the desktop, the code bases are NOT in sync today). Still it may still be useful to keep open the option of a 'bulk update' (I think it is still true that we COULD simply copy the contents to the desktop and it would be 'correct'. Removing [Serializable] for the desktop build would break that.

I looked and we don't have a #ifdef that means' Desktop. The closest thing we have is 'not coreCLR or Project N or not Portable'

#if !CORECLR && !ES_BUILD_PN && !ES_BUILD_PCL

This is my recommendation. Simply surround the [serialable] with this instead.

A couple of things to be aware of:

This is shared source so it will sync to CoreRT and ProjectN as well. You'll want to make sure that you have the proper !CORERT ifdef as well if you care about CoreRT.

EventSource tests are located in CoreFx. Before you submit, you should request that CoreFX tests be run against this change. These are the trigger phrases you want to use:

@dotnet-bot test Ubuntu x64 corefx_baseline
@dotnet-bot test Windows_NT x64 corefx_baseline

Thanks. I've added @vancem 's suggested ifdef along with !CORERT.

@brianrob , I've been testing this against CoreFX and modified tests appropriately so that it works. That change was dotnet/corefx#20035.

Great. Thanks.

jkotas · 2017-05-20T05:19:25Z

src/mscorlib/src/System/Collections/Generic/Comparer.cs

@@ -134,7 +132,6 @@ public override int Compare(T x, T y)
    // since we want to serialize as ObjectComparer for
    // back-compat reasons (see below).

-    [Serializable]
    internal sealed class Int32EnumComparer<T> : Comparer<T>, ISerializable where T : struct


Do these need to remain serializable to make the Comparer<T> serialization work?

I would expect so. I seem to remember adding these explicitly in response to a bug, and adding tests for them in corefx.

Looks plausible. I've added them.

jkotas · 2017-05-20T05:20:57Z

src/mscorlib/shared/System/UnitySerializationHolder.cs

@@ -12,7 +12,6 @@

 namespace System
 {
-    [Serializable]
    // Holds classes (Empty, Null, Missing) for which we guarantee that there is only ever one instance of.


This whole file should be deleted.

I'll include that with general ISerializable cleanup (I've updated https://github.com/dotnet/corefx/issues/19119 to include that)

jkotas · 2017-05-20T05:21:45Z

src/mscorlib/shared/System/Reflection/MemberInfoSerializationHolder.cs

@@ -9,7 +9,6 @@

 namespace System.Reflection
 {
-    [Serializable]
 #if CORECLR


This whole file should be deleted.

I'll include that with general ISerializable cleanup (I've updated dotnet/corefx#19119 to include that)

If you get rid of this file, you can get rid of

src\System\Reflection\MemberSerializationStringGenerator.cs

too. There are versions of this file in both CoreCLR and CoreRT.

ViktorHofer

I haven't looked into all changes - LGTM

morganbr · 2017-05-23T23:32:29Z

@dotnet-bot test Windows_NT x64 CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x64 CoreCLR Perf Tests Correctness

brianrob · 2017-05-24T00:07:11Z

FYI, the perf correctness failures are tracked by https://github.com/dotnet/coreclr/issues/11856.

morganbr · 2017-05-24T00:10:56Z

@brianrob , thanks. I'll go ahead and merge then since previous CIs succeeded and #11856 hasn't been fixed yet.

Removes SerializableAttribute from CoreCLR types not intended to be serializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime). Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Removes SerializableAttribute from CoreCLR types not intended to be serializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime).

* Scale back [Serializable] on CoreCLR types (#11765) Removes SerializableAttribute from CoreCLR types not intended to be serializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime). * ISerializable cleanup (#11873) Changes to throw PlatformNotSupportedException from ISerializable.GetObjectData and serialization constructors on non-Serializable types. Also removes private serialization constructors and some unneeded code that was used to support serializing non-serializable types. A few tests testing GetObjectData implementations are also removed. For Exception types, we call base instead of throwing from GetObjectData to be consistent with the many Exceptions that don't override GetObjectData * IDeserializationCallback cleanup Throws PlatformNotSupportedException from IDeserializationCallback and IObjectReference implementations on types that are no longer serializable. * Coretype variables renamed back to netfx counterpart for reflection based serialization (#11910) * Variables renamed for reflection based serialization * Make EqualityComparers serialize like desktop * add missing interfaces * TimeZone serializable added * Internal hashtable serializable * Removed TimeZone as serializable type * Remove Lazy<T>'s [Serializable] attribute for 2.0 For performance, Lazy was completely rewritten for .NET Core 2.0 and has an entirely different format than desktop; trying to get it to match the desktop serialization format would require either reverting or providing a complicated custom serialization/deserialization implementation to try to match. Lazy can also wrap an Exception that occurred from trying to instantiate the object, and the only exception types that are serializable as of now in core are the base Exception and AggregateException. As such, we're cutting it from the list of supported types in 2.0. An easy workaround is simply to do what the implementation does: serialize lazy.Value instead of lazy. * tiny fixes to equalitycomparer.cs * Merge pull request #11995 from ViktorHofer/SerializationFollowUp Internal hashtable serialization attribute removed * Fixes deserializaing TimeZoneInfo by putting back the IDeserializationCallback.OnDeserialization method. (#12024)

Apparently, the serialization support for `System.Random` is not present in `netstandard1.3` and even though it was added at some point, it is again removed from the future `netstandard` versions. See the following PR for more info: dotnet/coreclr#11765

karelz · 2017-08-28T18:48:21Z

Ported to 2.0.0 in #12020

Apparently, the serialization support for `System.Random` is not present in `netstandard1.3` and even though it was added at some point, it is again removed from the future `netstandard` versions. See the following PR for more info: dotnet/coreclr#11765

Removes SerializableAttribute from CoreCLR types not intended to be s…

6ba890b

…erializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime).

morganbr added this to the 2.0.0 milestone May 20, 2017

morganbr self-assigned this May 20, 2017

morganbr requested review from stephentoub, danmoseley, jkotas, ViktorHofer and zhenlan May 20, 2017 03:06

dnfclas added the cla-already-signed label May 20, 2017

danmoseley approved these changes May 20, 2017

View reviewed changes

jkotas reviewed May 20, 2017

View reviewed changes

stephentoub approved these changes May 20, 2017

View reviewed changes

ViktorHofer approved these changes May 23, 2017

View reviewed changes

Address PR comments

969a8c4

morganbr merged commit ebda0e6 into dotnet:master May 24, 2017

aignas mentioned this pull request Aug 25, 2017

.NET Standard support for new VS2017 project format mathnet/mathnet-numerics#499

Closed

karelz modified the milestones: 2.1.0, 2.0.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scale back [Serializable] on CoreCLR types #11765

Scale back [Serializable] on CoreCLR types #11765

morganbr commented May 20, 2017

morganbr commented May 20, 2017

danmoseley left a comment

jkotas May 20, 2017

morganbr May 23, 2017

jkotas May 20, 2017

vancem May 23, 2017

brianrob May 23, 2017 •

edited

Loading

morganbr May 23, 2017

brianrob May 24, 2017

jkotas May 20, 2017

stephentoub May 20, 2017

morganbr May 23, 2017

jkotas May 20, 2017

morganbr May 23, 2017

jkotas May 20, 2017

morganbr May 23, 2017

ghost May 23, 2017

ViktorHofer left a comment •

edited

Loading

morganbr commented May 23, 2017

brianrob commented May 24, 2017

morganbr commented May 24, 2017

karelz commented Aug 28, 2017

Scale back [Serializable] on CoreCLR types #11765

Scale back [Serializable] on CoreCLR types #11765

Conversation

morganbr commented May 20, 2017

morganbr commented May 20, 2017

danmoseley 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

brianrob May 23, 2017 • edited Loading

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

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

ViktorHofer left a comment • edited Loading

Choose a reason for hiding this comment

morganbr commented May 23, 2017

brianrob commented May 24, 2017

morganbr commented May 24, 2017

karelz commented Aug 28, 2017

brianrob May 23, 2017 •

edited

Loading

ViktorHofer left a comment •

edited

Loading