ApiCompat flags introduction of a base exception type for exceptions previously inheriting from System.Exception

[JanKrivanek]

Describe the bug

ApiCompat check flags introduction of exception hierarchy:

was:
<Our Exceptions> --> System.Exception
proposed:
<Our Exceptions> --> Microsoft.Build.BackEnd.BuildExceptionBase --> System.Exception

error:

Microsoft.NET.ApiCompat.ValidatePackage.targets(39,5): error CP0007: (NETCORE_ENGINEERING_TELEMETRY=Build) Type 'Microsoft.Build.Exceptions.BuildAbortedException' does not inherit from base type 'System.Exception' on ref/net472/Microsoft.Build.dll but it does on [Baseline] ref/net472/Microsoft.Build.dll

More details on the scenario: dotnet/msbuild#8779 (comment)
Build with failure: https://dev.azure.com/dnceng-public/public/_build/results?buildId=287532&view=results

The new base was introduced in assembly Microsoft.Build.Framework. The exceptions in the same assembly that underwent same refactors are not flaged. The change is flaged only in assembly Microsoft.Build. However Microsoft.Build references Microsoft.Build.Framework and the dependency is properly captured in the packages as well:

To Reproduce

Introduce new base exception type that is defined in the referenced assembly

Further technical details

• sdk 7.0.203 (https://github.com/dotnet/msbuild/blob/main/global.json)

[JanKrivanek]

By on the linke PR @danmoseley

We generally consider this non-breaking

https://github.com/dotnet/runtime/blob/3747edb1812106f0b88a42b724c3e44b69dc79c4/docs/coding-guidelines/breaking-change-rules.md#L50 https://github.com/dotnet/runtime/blob/3747edb1812106f0b88a42b724c3e44b69dc79c4/docs/coding-guidelines/breaking-change-rules.md#L139

[ghost]

@dotnet/area-infrastructure-libraries a new issue has been filed in the ApiCompat area, please triage

[ViktorHofer]

During an APICompat baseline comparison, assembly references passed in are ignored, which causes the BaseType of the inspected type to be null:

sdk/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/CannotRemoveBaseTypeOrInterface.cs

Line 70 in e0a6431

rightBaseType = rightBaseType.BaseType;

Here's the code that disables loading assembly references for a baseline comparison:

sdk/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Runner/ApiCompatRunner.cs

Line 121 in e0a6431

resolvedExternallyProvidedAssemblyReferences = !options.IsBaselineComparison && aggregatedReferences.Length > 0;

There are two options:

1. Enable loading assembly references for the right hand side (the current package assembly) but keep it disabled for the left hand side (baseline package assembly). IIRC loading assembly references is disabled on the right hand side because member comparisons and checks could behave differently and fail between an assembly with loaded references and one without references.
2. Load the current package assembly references for the baseline package assemblies as an approximation. In some cases this might not be correct though. Also related, the current assembly loader doesn't check assembly versions:

   sdk/src/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoader.cs

   Line 374 in e0a6431

   // TODO: add version check and add a warning if it doesn't match?

Option no 2 is tracked via #18676

[ericstj]

In some cases this might not be correct though.

I don't really see it as incorrect, it is somewhat artificial, but it would be as if you were restoring the old version of the package, but applying any dependency updates that might have occurred in the new version. What's the worst issue you can imagine might happen due to this approximation?

[ViktorHofer]

What's the worst issue you can imagine might happen due to this approximation?

I could be wrong but APICompat's AssemblySymbolLoader might not be able to load some of the referenced assemblies and print a couple of assembly load errors. But as we don't validate versions, we might not see such a case. What about mixing assembly universes? I.e. the baseline package contains a netstandard2.0 asset and the current package contains only a net7.0 asset. The netstandard2.0 asset would be loaded including the assembly references from the net7.0 library. Would that cause any issues?

[ericstj]

AssemblySymbolLoader might not be able to load some of the referenced assemblies and print a couple of assembly load errors

I would expect every assembly referenced by the baseline to be present and an equal or higher version on the right. One case I can imagine is a dependency pushes a type down from A > B. Baseline referenced A, but A is no longer present in the current references. This is somewhat of an edge case and I think we'd be OK to allow it. I think we're improving this scenario by looking at references to begin with. Folks could always add the reference (which is techincally necessary for strict compatibility anyway). Concrete example of this might be a reference to Microsoft.BCL.AsyncInterfaces.

What about mixing assembly universes

I don't think moving it forward should be a problem. The framework itself is compatible. In the netstandard2.0 -> net7.0 case we'll load the netsandard2.0 asset against the net7.0 surface area and facade and it should work.