-
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
Obsolete most of the remaining legacy serialization APIs #84383
Conversation
- Move some SYSLIB0011 obsoletions to type level - Next wave of formatter obsoletions (SYSLIB0049) - Remove dead code
- Introduce SYSLIB0050 warning
- Also improve obsoletion messages
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsHere it is, the large BinaryFormatter and legacy serialization obsoletion dump for .NET 8. :) References:
(Give me a few minutes to update this text after the PR is submitted.)
|
This comment was marked as resolved.
This comment was marked as resolved.
I suggest submitting a very targeted PR that claims |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
153 / 441 files viewed. I'll resume tomorrow...
...veaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeArrayTypeInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackDecodingException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/ReflectionTypeLoadException.cs
Show resolved
Hide resolved
Per suggestions, I'll split out the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur with @stephentoub's suggestion of removing "legacy" from the description. And there's at least one ifdef that looks like it can be removed.
Aside from that, this looks good to me. Marking as approved preemptively.
@jeffhandley Per your earlier comment, I had originally used https://aka.ms/binaryformatter for these obsoletions, but they're not quite identical to BinaryFormatter. So what I was thinking was keeping a specific dotnet-warnings page for these messages, where the contents of that page could be tailored to the specific warning users are experiencing. But then the main BinaryFormatter landing page would say "BinaryFormatter has been disabled, and BTW, here's why the legacy serialization infrastructure is also not recommended going forward. If you're running into <this compiler warning> see <other link> for resolution." The benefit is that it gets people seeing these links onto a page that allows them to take immediate action, while the landing page serves as a centralized hub for all kinds of information that falls under the general serialiazation umbrella. |
The most recent commit (6e74fc2) looks large, but it's just a simple string search & replace throughout the project, tweaking the diagnostic message based on feedback. I changed the word "legacy" to "obsolete" in the |
Per offline chat, we're marking this as
NO-MERGE
|
Merge planned for 10:30pm PDT Monday night; approx. 05:30am UTC Tuesday morning. |
The PR dotnet#84383 missed one case that is now causing build errors in the Pri1 test suite. I have attempted to fix this along the lines of the above PR. Thanks Tomas
The PR #84383 missed one case that is now causing build errors in the Pri1 test suite. I have attempted to fix this along the lines of the above PR. Thanks Tomas
Note to reviewers
To keep noise to a minimum, I kindly request that discussion on this PR be scoped to the contents of the PR itself. Debate on the overall obsoletion strategy is welcome, but such discussion should take place within the obsoletion strategy doc PR, linked below.
Summary
Here it is, the large BinaryFormatter and legacy serialization obsoletion dump for .NET 8. :)
References:
(See that last link above for a full list of affected APIs.)
This PR implements the changes described at dotnet/designs#293. As of this writing, it's divided into several commits to make reasoning about the changes easier.
e7e75e4
Move the
BinaryFormatter
obsoletions (SYSLIB0011
) up from the method level to the type level; and obsolete most of the rest of the legacy serialization infrastructure under the new diagnostic codeSYSLIB0049
. (This code was later changed due to a conflict.)63e0dd6
Obsolete implementations of the legacy serialization infrastructure under the new diagnostic code
SYSLIB0050
. (This code was later changed due to a conflict.)This is the big one, largely since it involves obsoleting
Exception.GetObjectData
and dealing with the fallout. It turns out that .NET contains many Exception-derived classes.13e3ca7
Rename
SYSLIB0049 -> SYSLIB0050
andSYSLIB0050 -> SYSLIB0051
to de-conflict with a recent change in System.Text.Json.26aa7dd
Allow our unit tests to continue testing serialization-related code paths, since otherwise the SDK would prohibit exercising these code paths once the
BinaryFormatter
killbit hits.(everything after this)
Whatever random cleanup is needed to make CI happy.
Extra discussion
This exercise identified some dead code paths in WCF and OleTx and removed those code paths. Additionally, these obsoletions only affect APIs exposed in the .NET 8 targeting pack. For libraries which don't have a .NET 8-specific ref assembly, no changes were made. If a library multi-targets runtimes, the changes were scoped via
#if NET8_0_OR_GREATER
.I tested the 26aa7dd change by applying the preview SDK changes to my local machine and rerunning a clean compile + test pass.