ApiDiff: Pass an ILog instance to the AssemblySymbolLoader constructor #45807
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
| @@ -35,7 +35,7 @@ public static void Run(ILog logger, | |||
| bool resolveAssemblyReferences = assemblyReferences?.Length > 0; | |||
|
|
|||
| // Create, configure and execute the assembly loader. | |||
| AssemblySymbolLoader loader = new(resolveAssemblyReferences, respectInternals); | |||
| AssemblySymbolLoader loader = new(logger, resolveAssemblyReferences, respectInternals); | |||
There was a problem hiding this comment.
As mentioned in the original PR, please rename this field to log. We use that name consistently in APICompat, i.e.
GenAPI was inconsistent in that regard as it was developed by multiple people from different teams and we probably didn't notice this during PR review.
Applies to all other changes in this PR as well.
| @@ -196,8 +197,10 @@ public void RetargetableFlagSet(bool strictMode) | |||
| string leftAssembly = SymbolFactory.EmitAssemblyFromSyntax(syntax, publicKey: _publicKey); | |||
| string rightAssembly = SymbolFactory.EmitAssemblyFromSyntax(syntax); | |||
|
|
|||
| IAssemblySymbol leftSymbol = new AssemblySymbolLoader().LoadAssembly(leftAssembly); | |||
| IAssemblySymbol rightSymbol = new AssemblySymbolLoader().LoadAssembly(rightAssembly); | |||
| ILog logger = new ConsoleLog(MessageImportance.High); | |||
There was a problem hiding this comment.
We already define a log instance in this file, for the rule factory:
Please use that instead.
There was a problem hiding this comment.
Seem you're talking about the new SuppressibleTestLog() being passed to AssemblyIdentityMustMatch.
I assume you mean I should extract that instance, store it in a field, and reuse that everywhere?
There was a problem hiding this comment.
If not, then I'm struggling to understand how to obtain the logger out of s_ruleFactory, if that's possible.
There was a problem hiding this comment.
I assume you mean I should extract that instance, store it in a field, and reuse that everywhere?
Yes
| using Microsoft.DotNet.Cli.Utils; | ||
|
|
||
| namespace Microsoft.DotNet.ApiSymbolExtensions.Tests | ||
| { | ||
| public class AssemblySymbolLoaderTests : SdkTest | ||
| { | ||
| private readonly ILog _logger = new ConsoleLog(MessageImportance.High); |
There was a problem hiding this comment.
These AssemblySymbolLoader unit tests should not use the real log implementation (it would log to the console). Please use a mock instead (probably via the Moq framework, I think we already do that in other places in our tests).
| @@ -54,8 +54,10 @@ private void RunTest(string original, | |||
| } | |||
| attributeDataSymbolFilter.Add(accessibilitySymbolFilter); | |||
|
|
|||
| ILog logger = new ConsoleLog(MessageImportance.Low); | |||
There was a problem hiding this comment.
This one should probably be a mock as well.
| /// <inheritdoc /> | ||
| public void LogAllDiagnostics(string? headerMessage = null) | ||
| { | ||
| if (HasRoslynDiagnostics(out IReadOnlyList<Diagnostic> roslynDiagnostics)) | ||
| { | ||
| if (!string.IsNullOrEmpty(headerMessage)) | ||
| { | ||
| _logger.LogWarning(headerMessage!); | ||
| } | ||
|
|
||
| foreach (Diagnostic warning in roslynDiagnostics) | ||
| { | ||
| _logger.LogWarning(warning.Id, warning.ToString()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void LogAllWarnings(string? headerMessage = null) | ||
| { | ||
| if (HasLoadWarnings(out IReadOnlyList<AssemblyLoadWarning> loadWarnings)) | ||
| { | ||
| if (!string.IsNullOrEmpty(headerMessage)) | ||
| { | ||
| _logger.LogWarning(headerMessage!); | ||
| } | ||
|
|
||
| foreach (AssemblyLoadWarning warning in loadWarnings) | ||
| { | ||
| _logger.LogWarning(warning.DiagnosticId, warning.Message); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think these would be more suitable as extension methods for the IAssemblySymbolLoader interface but I don't have a strong opinion. @ericstj what do you think?
There was a problem hiding this comment.
Interesting - it seems like this would be the only caller of HasRoslynDiagnostics and HasLoadWarnings -- which then makes me wonder if those should even be on the interface.
It does seem like those interfaces existed to avoid putting knowledge of the logger in IAssemblySymbolLoader. AFAICT they've been around since the start: 86b7b93#diff-925bb31f91442e16a835b4db1ae8f5e3226c2d0cbf3865f8e7122a6564080ba8R46-R56
Maybe we didn't have log abstraction at the time? I wonder if we can just rethink these methods based on usage and testing requirements, given the current app architecture.
There was a problem hiding this comment.
Maybe we didn't have log abstraction at the time? I wonder if we can just rethink these methods based on usage and testing requirements, given the current app architecture.
Yes, we only later provided log abstractions that weren't coupled to APICompat specifics (suppressions).
I wonder if we can just rethink these methods based on usage and testing requirements, given the current app architecture.
Yes, I think that would be better than the current indirection that's happening (putting an AssemblyLoadWarning on a store, then reading from it to log it).
There was a problem hiding this comment.
Can you suggest different method(s) on IAssemblySymbolLoader to replace Has* methods?
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.