Skip to content
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

Remove the defunct DiagnosticUtility/Fx classes from DataContractSerialization #82324

Merged
merged 2 commits into from
Mar 14, 2023

Conversation

stephentoub
Copy link
Member

  • Fx.Assert just delegated to Debug.Assert: replace all the call sites with Debug.Assert/Fail.
  • DiagnosticUtility.DebugAssert just delegated to Debug.Assert: replace all the call sites with Debug.Assert/Fail.
  • DiagnosticUtility.ExceptionUtility.ThrowHelperError didn't throw anything and just returned its argument: removed all uses of it
  • DiagnosticUtility.ExceptionUtility.ThrowHelperCallback didn't throw anything and just returned its argument: removed all uses of it.
  • DiagnosticUtility.ExceptionUtility.ThrowHelperFatal didn't throw anything and just wrapped its arguments in a new Exception that was then returned: just changed the call sites to create that exception.
  • DiagnosticUtility.ExceptionUtility.ThrowHelperArgument{Null} didn't throw anything and just returned new instances of Argument{Null}Exception: just changed the call sites to create that exception.
  • Replaced some argument validation with helpers like ThrowIfNegative.
  • Fx.IsFatal: moved to ExceptionUtility class

…alization

- Fx.Assert just delegated to Debug.Assert: replace all the call sites with Debug.Assert/Fail.
- DiagnosticUtility.DebugAssert just delegated to Debug.Assert: replace all the call sites with Debug.Assert/Fail.
- DiagnosticUtility.ExceptionUtility.ThrowHelperError didn't throw anything and just returned its argument: removed all uses of it
- DiagnosticUtility.ExceptionUtility.ThrowHelperCallback didn't throw anything and just returned its argument: removed all uses of it.
- DiagnosticUtility.ExceptionUtility.ThrowHelperFatal didn't throw anything and just wrapped its arguments in a new Exception that was then returned: just changed the call sites to create that exception.
- DiagnosticUtility.ExceptionUtility.ThrowHelperArgument{Null} didn't throw anything and just returned new instances of Argument{Null}Exception: just changed the call sites to create that exception.
- Replaced some argument validation with helpers like ThrowIfNegative.
- Fx.IsFatal: moved to ExceptionUtility class
@StephenMolloy
Copy link
Member

I've been looking at this in some spare moments this week. It mostly looks fine to me, but I've dug into what the point of these utilitiy methods used to be for and noticed that it was WCF's way of generating ETW events for errors. I also noticed that the concept of creating a Fatal Exception disappeared from NetFx to .Net Core. I wanted to look more into the Fatal exception wrapper idea to see if that's something we need and have a chat with Matt about where ETW fits with WCF these days.

Copy link
Member

@StephenMolloy StephenMolloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matt says he's never found use for ETW events in DCS, unlike the ETW events in WCF which this utility originally came from in NetFx. We haven't had ETW events here in .Net Core so far, and it doesn't seem like anything we care to bring back. So...

lgtm

@StephenMolloy StephenMolloy merged commit 93ab46b into dotnet:main Mar 14, 2023
@stephentoub
Copy link
Member Author

Thanks for following up

@stephentoub stephentoub deleted the removediagnosticutility branch March 14, 2023 19:43
@sebastienros
Copy link
Member

It broke some aspnet unit tests that were checking specific exception types. Can you confirm that this change of exception type is fine here and we should just react to it? Or is that considered a non-acceptable breaking change?

93ab46b#diff-b4bbe1b5ee1bb01a618b60e8197a2f0d09e69ae4c9788f3bfd6841dac23e2da2R95

The documentation states that MaxDepth can return an ArgumentException but after this PR it returns an ArgumentOutOfRangeException. I am just not familiar with the breaking changes policy.

@sebastienros
Copy link
Member

I am realizing that ArgumentOutOfRangeException inherits from ArgumentException ... that might ease the breaking change policy quite considerably in this case.

@stephentoub
Copy link
Member Author

I am realizing that ArgumentOutOfRangeException inherits from ArgumentException ... that might ease the breaking change policy quite considerably in this case.

Right, we consider it "non-breaking" to throw more derived types than we were previously (even though that can obviously impact code doing an exact type test). It's the same as returning objects more derived than what's statically typed, e.g. if a method returns a List<T>, we don't consider it breaking to return something that derives from List<T>.

RussKie added a commit to dotnet/aspnetcore that referenced this pull request Mar 23, 2023
wtgodbe added a commit to dotnet/aspnetcore that referenced this pull request Mar 23, 2023
…/efcore (#47367)

* Update dependencies from https://github.com/dotnet/efcore build 20230322.1

dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.Design , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.Tools
 From Version 8.0.0-preview.3.23152.4 -> To Version 8.0.0-preview.3.23172.1

* Update dependencies from https://github.com/dotnet/runtime build 20230322.6

Microsoft.Bcl.AsyncInterfaces , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Configuration , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.Configuration.Xml , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.Hosting , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Http , Microsoft.Extensions.Logging , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Options , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Primitives , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.Platforms , System.Configuration.ConfigurationManager , System.Diagnostics.DiagnosticSource , System.Diagnostics.EventLog , System.DirectoryServices.Protocols , System.IO.Pipelines , System.Net.Http.Json , System.Net.Http.WinHttpHandler , System.Reflection.Metadata , System.Resources.Extensions , System.Security.Cryptography.Pkcs , System.Security.Cryptography.Xml , System.Security.Permissions , System.ServiceProcess.ServiceController , System.Text.Encodings.Web , System.Text.Json , System.Threading.AccessControl , System.Threading.Channels , System.Threading.RateLimiting
 From Version 8.0.0-preview.3.23158.1 -> To Version 8.0.0-preview.3.23172.6

* Update dependencies from https://github.com/dotnet/efcore build 20230322.3

dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.Design , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.Tools
 From Version 8.0.0-preview.3.23152.4 -> To Version 8.0.0-preview.3.23172.3

* Update dependencies from https://github.com/dotnet/runtime build 20230322.11

Microsoft.Bcl.AsyncInterfaces , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Configuration , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.Configuration.Xml , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.Hosting , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Http , Microsoft.Extensions.Logging , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Options , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Primitives , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.Platforms , System.Configuration.ConfigurationManager , System.Diagnostics.DiagnosticSource , System.Diagnostics.EventLog , System.DirectoryServices.Protocols , System.IO.Pipelines , System.Net.Http.Json , System.Net.Http.WinHttpHandler , System.Reflection.Metadata , System.Resources.Extensions , System.Security.Cryptography.Pkcs , System.Security.Cryptography.Xml , System.Security.Permissions , System.ServiceProcess.ServiceController , System.Text.Encodings.Web , System.Text.Json , System.Threading.AccessControl , System.Threading.Channels , System.Threading.RateLimiting
 From Version 8.0.0-preview.3.23158.1 -> To Version 8.0.0-preview.3.23172.11

* Update dependencies from https://github.com/dotnet/efcore build 20230322.4

dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.Design , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.Tools
 From Version 8.0.0-preview.3.23152.4 -> To Version 8.0.0-preview.3.23172.4

* Fix test per dotnet/runtime#82324

* Set RequestServices in tests

* Fix tests

---------

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Igor Velikorossov <[email protected]>
Co-authored-by: Chris R <[email protected]>
Co-authored-by: wtgodbe <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants