Improve DisplayNameHelpers for NativeAOT (dotnet#70084)

These helpers are used to report names of things in warnings. The functional changes are: * For method parameters, use the parameter name if available (and only if not fallback to the #1 notation) * For property accessor methods, use the C# naming scheme, so for example Type.Property.get instead of Type.get_Property. Both of these changes are in preparation to bring NativeAOT closer in behavior to ILLink and the trim analyzers. For this I moved some of the helpers to the common shared code. Some unrelated code cleanup as well. Co-authored-by: Michal Strehovský <[email protected]>
gfoidl · Jun 2, 2022 · 1b4a9c4 · 1b4a9c4
1 parent 655bc1d
commit 1b4a9c4
Show file tree

Hide file tree

Showing 11 changed files with 58 additions and 54 deletions.
diff --git a/src/coreclr/tools/Common/Compiler/DisplayNameHelpers.cs b/src/coreclr/tools/Common/Compiler/DisplayNameHelpers.cs
@@ -2,9 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Reflection.Metadata;
 using System.Text;
 
 using Internal.TypeSystem;
+using Internal.TypeSystem.Ecma;
 
 using Debug = System.Diagnostics.Debug;
 
@@ -38,6 +40,12 @@ public static string GetDisplayName(this MethodDesc method)
             {
                 sb.Append(method.OwningType.GetDisplayNameWithoutNamespace());
             }
+            else if (method.GetPropertyForAccessor() is PropertyPseudoDesc property)
+            {
+                sb.Append(property.Name);
+                sb.Append('.');
+                sb.Append(property.GetMethod == method ? "get" : "set");
+            }
             else
             {
                 sb.Append(method.Name);
@@ -68,6 +76,20 @@ public static string GetDisplayName(this MethodDesc method)
             return sb.ToString();
         }
 
+        public static string GetParameterDisplayName(this EcmaMethod method, int parameterIndex)
+        {
+            var reader = method.MetadataReader;
+            var methodDefinition = reader.GetMethodDefinition(method.Handle);
+            foreach (var parameterHandle in methodDefinition.GetParameters())
+            {
+                var parameter = reader.GetParameter(parameterHandle);
+                if (parameter.SequenceNumber == parameterIndex + 1)
+                    return reader.GetString(parameter.Name);
+            }
+
+            return $"#{parameterIndex}";
+        }
+
         public static string GetDisplayName(this FieldDesc field)
         {
             return new StringBuilder(field.OwningType.GetDisplayName())

diff --git a/...ler.Compiler/Compiler/MethodExtensions.cs → ...tools/Common/Compiler/MethodExtensions.cs b/...ler.Compiler/Compiler/MethodExtensions.cs → ...tools/Common/Compiler/MethodExtensions.cs
@@ -103,5 +103,25 @@ public static bool NotCallableWithoutOwningEEType(this MethodDesc method)
                 (owningType is not MetadataType mdType || !mdType.IsModuleType) && /* Compiler parks some instance methods on the <Module> type */
                 !method.IsSharedByGenericInstantiations; /* Current impl limitation; can be lifted */
         }
+
+        public static PropertyPseudoDesc GetPropertyForAccessor(this MethodDesc accessor)
+        {
+            if (accessor.GetTypicalMethodDefinition() is not EcmaMethod ecmaAccessor)
+                return null;
+
+            var type = (EcmaType)ecmaAccessor.OwningType;
+            var reader = type.MetadataReader;
+            foreach (var propertyHandle in reader.GetTypeDefinition(type.Handle).GetProperties())
+            {
+                var accessors = reader.GetPropertyDefinition(propertyHandle).GetAccessors();
+                if (ecmaAccessor.Handle == accessors.Getter
+                    || ecmaAccessor.Handle == accessors.Setter)
+                {
+                    return new PropertyPseudoDesc(type, propertyHandle);
+                }
+            }
+
+            return null;
+        }
     }
 }
diff --git a/...r.Compiler/Compiler/PropertyPseudoDesc.cs → ...ols/Common/Compiler/PropertyPseudoDesc.cs b/...r.Compiler/Compiler/PropertyPseudoDesc.cs → ...ols/Common/Compiler/PropertyPseudoDesc.cs
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DiagnosticUtilities.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DiagnosticUtilities.cs
@@ -32,7 +32,15 @@ internal static Origin GetMethodParameterFromIndex(MethodDesc method, int parame
 
         internal static string GetParameterNameForErrorMessage(ParameterOrigin origin)
         {
-            return $"#{origin.Index}";
+            return GetParameterNameForErrorMessage(origin.Method, origin.Index);
+        }
+
+        internal static string GetParameterNameForErrorMessage(MethodDesc method, int parameterIndex)
+        {
+            if (method.GetTypicalMethodDefinition() is EcmaMethod ecmaMethod)
+                return ecmaMethod.GetParameterDisplayName(parameterIndex);
+
+            return $"#{parameterIndex}";
         }
 
         internal static string GetMethodSignatureDisplayName(MethodDesc method)

diff --git a/...reclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs b/...reclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
@@ -513,26 +513,4 @@ private static DefType[] TryGetExplicitlyImplementedInterfaces(this TypeDesc typ
             return Array.Empty<DefType>();
         }
     }
-
-    // Temporary local copy of the enum because the enum we're compiling against
-    // doesn't define all the values. Can be removed once we update to .NET 6.
-    public enum DynamicallyAccessedMemberTypes
-    {
-        None = 0,
-        PublicParameterlessConstructor = 0x0001,
-        PublicConstructors = 0x0002 | PublicParameterlessConstructor,
-        NonPublicConstructors = 0x0004,
-        PublicMethods = 0x0008,
-        NonPublicMethods = 0x0010,
-        PublicFields = 0x0020,
-        NonPublicFields = 0x0040,
-        PublicNestedTypes = 0x0080,
-        NonPublicNestedTypes = 0x0100,
-        PublicProperties = 0x0200,
-        NonPublicProperties = 0x0400,
-        PublicEvents = 0x0800,
-        NonPublicEvents = 0x1000,
-        Interfaces = 0x2000,
-        All = ~None
-    }
 }
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/EcmaExtensions.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/EcmaExtensions.cs
@@ -63,25 +63,5 @@ public static PropertyPseudoDesc GetProperty(this MetadataType mdType, string na
 
             return null;
         }
-
-        public static PropertyPseudoDesc GetPropertyForAccessor(this MethodDesc accessor)
-        {
-            if (accessor.GetTypicalMethodDefinition() is not EcmaMethod ecmaAccessor)
-                return null;
-            var type = (EcmaType)ecmaAccessor.OwningType;
-            var reader = type.MetadataReader;
-            var module = type.EcmaModule;
-            foreach (var propertyHandle in reader.GetTypeDefinition(type.Handle).GetProperties())
-            {
-                var accessors = reader.GetPropertyDefinition(propertyHandle).GetAccessors();
-                if (ecmaAccessor.Handle == accessors.Getter
-                    || ecmaAccessor.Handle == accessors.Setter)
-                {
-                    return new PropertyPseudoDesc(type, propertyHandle);
-                }
-            }
-
-            return null;
-        }
     }
 }
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
@@ -371,7 +371,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
 
                             if (!IsTypeInterestingForDataflow(signature[parameter.SequenceNumber - 1]))
                             {
-                                _logger.LogWarning(method, DiagnosticId.DynamicallyAccessedMembersOnMethodParameterCanOnlyApplyToTypesOrStrings, $"#{parameter.SequenceNumber}", method.GetDisplayName());
+                                _logger.LogWarning(method, DiagnosticId.DynamicallyAccessedMembersOnMethodParameterCanOnlyApplyToTypesOrStrings, DiagnosticUtilities.GetParameterNameForErrorMessage(method, parameter.SequenceNumber - 1), method.GetDisplayName());
                                 continue;
                             }
 
@@ -714,8 +714,8 @@ void LogValidationWarning(object provider, object baseProvider, MethodDesc origi
             {
                 case int parameterNumber:
                     _logger.LogWarning(origin, DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodParameterBetweenOverrides,
-                        $"#{parameterNumber}", DiagnosticUtilities.GetMethodSignatureDisplayName(origin),
-                        $"#{parameterNumber}", DiagnosticUtilities.GetMethodSignatureDisplayName((MethodDesc)baseProvider));
+                        DiagnosticUtilities.GetParameterNameForErrorMessage(origin, parameterNumber), DiagnosticUtilities.GetMethodSignatureDisplayName(origin),
+                        DiagnosticUtilities.GetParameterNameForErrorMessage((MethodDesc)baseProvider, parameterNumber), DiagnosticUtilities.GetMethodSignatureDisplayName((MethodDesc)baseProvider));
                     break;
                 case GenericParameterDesc genericParameterOverride:
                     _logger.LogWarning(origin, DiagnosticId.DynamicallyAccessedMembersMismatchOnGenericParameterBetweenOverrides,

diff --git a/...ot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DynamicDependencyAttributeAlgorithm.cs b/...ot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DynamicDependencyAttributeAlgorithm.cs
@@ -16,7 +16,6 @@
 
 using DependencyList = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore<ILCompiler.DependencyAnalysis.NodeFactory>.DependencyList;
 using MethodAttributes = System.Reflection.MethodAttributes;
-using DynamicallyAccessedMemberTypes = ILCompiler.Dataflow.DynamicallyAccessedMemberTypes;
 
 namespace ILCompiler.DependencyAnalysis
 {

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj b/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj
@@ -290,7 +290,9 @@
     <Compile Include="..\..\Common\Compiler\InstructionSetSupport.cs" Link="Compiler\InstructionSetSupport.cs" />
     <Compile Include="..\..\Common\Compiler\Int128FieldLayoutAlgorithm.cs" Link="Compiler\Int128FieldLayoutAlgorithm.cs" />
     <Compile Include="..\..\Common\Compiler\InternalCompilerErrorException.cs" Link="Compiler\InternalCompilerErrorException.cs" />
+    <Compile Include="..\..\Common\Compiler\MethodExtensions.cs" Link="Compiler\MethodExtensions.cs" />
     <Compile Include="..\..\Common\Compiler\NameMangler.cs" Link="Compiler\NameMangler.cs" />
+    <Compile Include="..\..\Common\Compiler\PropertyPseudoDesc.cs" Link="Compiler\PropertyPseudoDesc.cs" />
     <Compile Include="..\..\Common\Compiler\SingleMethodRootProvider.cs" Link="Compiler\SingleMethodRootProvider.cs" />
     <Compile Include="..\..\Common\Compiler\TypeExtensions.cs" Link="Compiler\TypeExtensions.cs" />
     <Compile Include="..\..\Common\Compiler\VectorFieldLayoutAlgorithm.cs" Link="Compiler\VectorFieldLayoutAlgorithm.cs" />
@@ -506,15 +508,13 @@
     <Compile Include="Compiler\ManagedBinaryEmitter.cs" />
     <Compile Include="Compiler\MetadataManager.cs" />
     <Compile Include="Compiler\InteropStubManager.cs" />
-    <Compile Include="Compiler\MethodExtensions.cs" />
     <Compile Include="Compiler\MultiFileCompilationModuleGroup.cs" />
     <Compile Include="Compiler\NativeLibraryInitializerRootProvider.cs" />
     <Compile Include="Compiler\NodeMangler.cs" />
     <Compile Include="Compiler\ObjectDumper.cs" />
     <Compile Include="Compiler\ExportsFileWriter.cs" />
     <Compile Include="Compiler\ProcessLinkerXmlBase.cs" />
     <Compile Include="Compiler\ProcessXmlBase.cs" />
-    <Compile Include="Compiler\PropertyPseudoDesc.cs" />
     <Compile Include="Compiler\RootingHelpers.cs" />
     <Compile Include="Compiler\RootingServiceProvider.cs" />
     <Compile Include="Compiler\RuntimeConfigurationRootProvider.cs" />

diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj
@@ -93,8 +93,10 @@
     <Compile Include="..\..\Common\Compiler\InstructionSetSupport.cs" Link="Compiler\InstructionSetSupport.cs" />
     <Compile Include="..\..\Common\Compiler\Int128FieldLayoutAlgorithm.cs" Link="Compiler\Int128FieldLayoutAlgorithm.cs" />
     <Compile Include="..\..\Common\Compiler\InternalCompilerErrorException.cs" Link="Compiler\InternalCompilerErrorException.cs" />
+    <Compile Include="..\..\Common\Compiler\MethodExtensions.cs" Link="Compiler\MethodExtensions.cs" />
     <Compile Include="..\..\Common\Compiler\NameMangler.cs" Link="Compiler\NameMangler.cs" />
     <Compile Include="..\..\Common\Compiler\SingleMethodRootProvider.cs" Link="Compiler\SingleMethodRootProvider.cs" />
+    <Compile Include="..\..\Common\Compiler\PropertyPseudoDesc.cs" Link="Compiler\PropertyPseudoDesc.cs" />
     <Compile Include="..\..\Common\Compiler\TypeExtensions.cs" Link="Compiler\TypeExtensions.cs" />
     <Compile Include="..\..\Common\Compiler\VectorFieldLayoutAlgorithm.cs" Link="Compiler\VectorFieldLayoutAlgorithm.cs" />
     <Compile Include="..\..\Common\JitInterface\CorInfoTypes.VarInfo.cs" Link="JitInterface\CorInfoTypes.VarInfo.cs" />
@@ -278,6 +280,4 @@
       <Link>JitInterface\UnboxingMethodDesc.cs</Link>
     </Compile>
   </ItemGroup>
-
-  <Import Project="..\ILLink.Shared\ILLink.Shared.projitems" Label="Shared" />
 </Project>
diff --git a/src/coreclr/tools/aot/crossgen2.sln b/src/coreclr/tools/aot/crossgen2.sln
@@ -17,9 +17,6 @@ EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ILCompiler.TypeSystem.Tests", "ILCompiler.TypeSystem.Tests\ILCompiler.TypeSystem.Tests.csproj", "{9E65EC58-B500-4C4A-B57D-BF242129A3C6}"
 EndProject
 Global
-	GlobalSection(SharedMSBuildProjectFiles) = preSolution
-		ILLink.Shared\ILLink.Shared.projitems*{83a832de-bf4a-44c4-b361-90f5f88b979b}*SharedItemsImports = 5
-	EndGlobalSection
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Checked|Any CPU = Checked|Any CPU
 		Checked|x64 = Checked|x64