ApiDiff: Pass an ILog instance to the AssemblySymbolLoader constructor

[carlossanlop]

This PR is part of the work needed to create an ApiDiff tool that reuses some of the code from Microsoft.DotNet.GenAPI. The idea is to make the larger PR smaller and make it easier to review: #45389

The purpose of this change is to construct AssemblySymbolLoader instances passing a predefined ILog instance to its constructor which will be necessary later.

[ViktorHofer]

We need to be careful about not adding new warnings to APICompat that didn't exist before. More precisely, APICompat didn't emit diagnostics or warnings, only GenAPI. You might want to make this configurable in the AssemblySymbolLoader ctor. But I would first wait for Eric to reply to the below.

@ericstj I just noticed that we have this related code path in APICompat:

sdk/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Mapping/AssemblyMapper.cs

Lines 126 to 137 in da12057

	// If we should warn on missing references and we are unable to resolve the type forward, then we should log a diagnostic
	if (Settings.WithReferences)
	{
	_assemblyLoadErrors.Add(new CompatDifference(
	side == ElementSide.Left ? assembly.MetadataInformation : MetadataInformation.DefaultLeft,
	side == ElementSide.Right ? assembly.MetadataInformation : MetadataInformation.DefaultRight,
	DiagnosticIds.AssemblyReferenceNotFound,
	string.Format(Resources.MatchingAssemblyNotFound, $"{symbol.ContainingAssembly.Name}.dll"),
	DifferenceType.Changed,
	symbol.ContainingAssembly.Identity.GetDisplayName()));
	}
	}

This goes into the suppression file, i.e. https://github.com/dotnet/aspnetcore/blob/e0e0224d744e6219849538cf9ce5dd3db1ccd658/src/Caching/SqlServer/src/CompatibilitySuppressions.xml#L3-L6 (with a Left and Right metadata with a > 2022 APICompat version). Back when this was implemented, we didn't have a way to emit a warning to the log interface. Do you think the suppression file is the correct destination for the "warning"? Asking as this might change my first sentence in this comment.

[ericstj]

Do you think the suppression file is the correct destination for the "warning"? Asking as this might change my first sentence in this comment.

It doesn't feel like it should be in a suppression file, no. Not loading a dependency isn't a problem with the comparison/compatibility, it's a problem with the input to the tool. That said, we do get more granular suppression in the file so it could be more handy in suppressing a particular missing dependency while not suppressing all missing dependencies.

[ViktorHofer]

could be more handy in suppressing a particular missing dependency while not suppressing all missing dependencies

I wonder if emitting a warning is the right level for missing assemblies. Maybe it would make more sense to treat them as diagnostics? Unsure.

[ViktorHofer]

Looks good but we should first reach consensus regarding the missing assemblies logging when using APICompat. This PR changes that by introducing new warnings, i.e. when doing baseline package validation with missing dependencies on the left side.

The more I think about it, the more I like the idea of treating missing assemblies as diagnostic information.

[ericstj]

The more I think about it, the more I like the idea of treating missing assemblies as diagnostic information.

Would it ever cause us to miss a compatibility check? Suppose the target of a typeforward was missing, and that type broke compat?

[ViktorHofer]

Would it ever cause us to miss a compatibility check? Suppose the target of a typeforward was missing, and that type broke compat?

Yes, we had such cases. But if we would now start emitting these as warnings that don't go to the suppression file, there's potential to break repositories that have missing assemblies (i.e. during baseline package validation) and use /warnaserror. But, we only emit the compatibility difference when we perform validation with references. Otherwise, we don't emit the compat difference. If we keep that characteristic then I'm less concerned about the behavioral change.

The more I think about, they really shouldn't go into the suppression file. These have been a factor of confusion for others and even me as the owner of the tool. But maybe emitting them as warnings is indeed the right thing to do. Unsure if we ever need a fine-grained suppression mechanism per assembly for these.

[carlossanlop]

I found 3 more tests that were still expecting CP1002 to exist, and now they're failing (as expected). I need to delete those tests.