Enable nullable for the mapper types and code clean-up #26989
Conversation
| @@ -9,17 +9,17 @@ namespace Microsoft.DotNet.ApiCompatibility.Abstractions | |||
| /// Class to wrap an Element of T with it's <see cref="MetadataInformation"/>. | |||
| /// </summary> | |||
| /// <typeparam name="T">The type of the Element that is holded</typeparam> | |||
| public class ElementContainer<T> where T : ISymbol | |||
| public readonly struct ElementContainer<T> where T : ISymbol | |||
There was a problem hiding this comment.
Changes in this file are unrelated but are an optimization (using a readonly struct over class reduces run time costs) that I felt isn't worth putting into a separate PR.
| /// <summary> | ||
| /// Gets the assembly load errors that happened when trying to follow type forwards. | ||
| /// </summary> | ||
| public IReadOnlyList<IReadOnlyList<CompatDifference>> AssemblyLoadErrors => _assemblyLoadErrors; |
There was a problem hiding this comment.
Moving the AssemblyLoadErrors from the the base ElementMapper type into the AssemblyMapper as that's the only mapper that deals with resolving external assemblies (via type forwards). This reduces the instantiation costs of the other mapper types.
There was a problem hiding this comment.
That seems fine by me. One small comment is that I'm not sure I understand why is that relevant in the nullability changes.
There was a problem hiding this comment.
Yes, this isn't related to the nullable reference type feature. This is general code clean-up (same as for the other comments that I left on this PR) that I bundled into this PR as I already touched the files.
| for (int i = 0; i < Right.Length; i++) | ||
| { | ||
| for (int i = 0; i < Right.Length; i++) | ||
| { | ||
| AddOrCreateMappers(Right[i], ElementSide.Right, i); | ||
| } | ||
| AddOrCreateMappers(Right[i], ElementSide.Right, i); |
There was a problem hiding this comment.
Same clean-up as in 7304516. The if/else isn't required here.
| for (int i = 0; i < Right.Length; i++) | ||
| { | ||
| AddOrCreateMappers(Right[0], ElementSide.Right); | ||
| } | ||
| else | ||
| { | ||
| for (int i = 0; i < Right.Length; i++) | ||
| { | ||
| AddOrCreateMappers(Right[i], ElementSide.Right, i); | ||
| } | ||
| AddOrCreateMappers(Right[i], ElementSide.Right, i); |
There was a problem hiding this comment.
Same as above. The if/else isn't required here.
| /// <summary> | ||
| /// The containg type of this member. | ||
| /// </summary> | ||
| internal TypeMapper ContainingType { get; } |
There was a problem hiding this comment.
style nit: placing the property before the ctor.
| // Retrieve the right names | ||
| string[] rightNames = new string[rightCount]; | ||
| for (int i = 0; i < rightCount; i++) | ||
| { | ||
| ElementContainer<IAssemblySymbol> element = right[i]; | ||
| rightNames[i] = element.MetadataInformation.DisplayString; | ||
| mapper.AddElement(element.Element, ElementSide.Right, i); | ||
| rightNames[i] = right[i].MetadataInformation.DisplayString; |
There was a problem hiding this comment.
Constructing the rightNames before initializing the AssemblyMapper to avoid instantiating a throw-away ComparingSettings object.
| AssemblyMapper mapper = new(GetComparingSettingsCore(leftName, rightName != null ? new[] { rightName } : null)); | ||
| mapper.AddElement(left, ElementSide.Left); | ||
| mapper.AddElement(right, ElementSide.Right); | ||
|
|
||
| DifferenceVisitor visitor = new(); | ||
| visitor.Visit(mapper); | ||
| return visitor.DiagnosticCollections.First(); | ||
| return visitor.DiagnosticCollections[0]; |
There was a problem hiding this comment.
Avoiding Linq as the DiagnosticCollections are now exposed as a IReadOnlyList which allows to index on it.
src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Abstractions/Mappers/TypeMapper.cs
Outdated
Show resolved
Hide resolved
|
After this, is there anything left to annotate or is this it? |
| @@ -38,7 +37,7 @@ public class ApiComparer : IApiComparer | |||
|
|
|||
| DifferenceVisitor visitor = new(); | |||
| visitor.Visit(mapper); | |||
| return visitor.DiagnosticCollections.First(); | |||
| return visitor.DiagnosticCollections[0]; | |||
There was a problem hiding this comment.
I know this is not new from your work, but do we have to check if this is null? or if the size of the DiagnosticCollection has at least 1 elemnt?
There was a problem hiding this comment.
The size of the DiagnosticCollection is governed by the rightCount ctor parameterof the DifferenceVisitor type and that value defaults to 1:
Also, the individual values in the collection are already initialized to an empty HashSet<CompatDifference>, so this could never be null.
The underlying issue will get closed when this gets merged. All the shipping code under src/ApiCompat now use the nullable reference type compiler feature (except for the underlying msbuild task infrastructure that comes via the sdk). Also, the feature switch is already enabled for all product code libraries: |
ViktorHofer
changed the title
Fixes #25760
I left out the mapper types in my previous changes as I haven't yet looked into them more closely. I just spent several hours drawing a call stack and trying to understand the mapping infrastructure and I fell much more enlightened now 😁.