IL2121 warnings point to XML files correctly (#2965)

Warnings indicating an unnecessary suppression produced by XML are now pointing to the xml location instead of being generated on the member the xml targets.

7 files changed, 89 insertions, 42 deletions

diff --git a/src/linker/Linker.Steps/CheckSuppressionsDispatcher.cs b/src/linker/Linker.Steps/CheckSuppressionsDispatcher.cs
index ade9de9d9..4bb15635c 100644
--- a/src/linker/Linker.Steps/CheckSuppressionsDispatcher.cs
+++ b/src/linker/Linker.Steps/CheckSuppressionsDispatcher.cs
@@ -4,7 +4,6 @@
 using System.Collections.Generic;
 using System.Linq;
 using ILLink.Shared;
-using Mono.Cecil;
 
 namespace Mono.Linker.Steps
 {
@@ -23,16 +22,14 @@ namespace Mono.Linker.Steps
 			// Suppressions targeting warning caused by anything but the linker should not be reported.
 			// Suppressions targeting RedundantSuppression warning should not be reported.
 			redundantSuppressions = redundantSuppressions
-				.Where (suppression => ((DiagnosticId) suppression.suppressMessageInfo.Id).GetDiagnosticCategory () == DiagnosticCategory.Trimming)
-				.Where (suppression => ((DiagnosticId) suppression.suppressMessageInfo.Id) != DiagnosticId.RedundantSuppression);
+				.Where (suppression => ((DiagnosticId) suppression.SuppressMessageInfo.Id).GetDiagnosticCategory () == DiagnosticCategory.Trimming)
+				.Where (suppression => ((DiagnosticId) suppression.SuppressMessageInfo.Id) != DiagnosticId.RedundantSuppression);
 
-			foreach (var (provider, suppressMessageInfo) in redundantSuppressions) {
-				var source = GetSuppresionProvider (provider);
+			foreach (var suppression in redundantSuppressions) {
+				var source = context.Suppressions.GetSuppressionOrigin (suppression);
 
-				context.LogWarning (new MessageOrigin (source), DiagnosticId.RedundantSuppression, $"IL{suppressMessageInfo.Id:0000}");
+				context.LogWarning (new MessageOrigin (source), DiagnosticId.RedundantSuppression, $"IL{suppression.SuppressMessageInfo.Id:0000}");
 			}
 		}
-
-		private static ICustomAttributeProvider GetSuppresionProvider (ICustomAttributeProvider provider) => provider is ModuleDefinition module ? module.Assembly : provider;
 	}
 }
diff --git a/src/linker/Linker.Steps/LinkAttributesParser.cs b/src/linker/Linker.Steps/LinkAttributesParser.cs
index d297ab5b7..81b89e292 100644
--- a/src/linker/Linker.Steps/LinkAttributesParser.cs
+++ b/src/linker/Linker.Steps/LinkAttributesParser.cs
@@ -38,9 +38,10 @@ namespace Mono.Linker.Steps
 
 		static bool IsRemoveAttributeInstances (string attributeName) => attributeName == "RemoveAttributeInstances" || attributeName == "RemoveAttributeInstancesAttribute";
 
-		CustomAttribute[]? ProcessAttributes (XPathNavigator nav, ICustomAttributeProvider provider)
+		(CustomAttribute[]? customAttributes, MessageOrigin[]? origins) ProcessAttributes (XPathNavigator nav, ICustomAttributeProvider provider)
 		{
-			var builder = new ArrayBuilder<CustomAttribute> ();
+			var customAttributesBuilder = new ArrayBuilder<CustomAttribute> ();
+			var originsBuilder = new ArrayBuilder<MessageOrigin> ();
 			foreach (XPathNavigator attributeNav in nav.SelectChildren ("attribute", string.Empty)) {
 				if (!ShouldProcessElement (attributeNav))
 					continue;
@@ -74,11 +75,12 @@ namespace Mono.Linker.Steps
 				CustomAttribute? customAttribute = CreateCustomAttribute (attributeNav, attributeType);
 				if (customAttribute != null) {
 					_context.LogMessage ($"Assigning external custom attribute '{FormatCustomAttribute (customAttribute)}' instance to '{provider}'.");
-					builder.Add (customAttribute);
+					customAttributesBuilder.Add (customAttribute);
+					originsBuilder.Add (GetMessageOriginForPosition (attributeNav));
 				}
 			}
 
-			return builder.ToArray ();
+			return (customAttributesBuilder.ToArray (), originsBuilder.ToArray ());
 
 			static string FormatCustomAttribute (CustomAttribute ca)
 			{
@@ -491,14 +493,14 @@ namespace Mono.Linker.Steps
 		{
 			Debug.Assert (_attributeInfo != null);
 			foreach (XPathNavigator parameterNav in nav.SelectChildren ("parameter", string.Empty)) {
-				var attributes = ProcessAttributes (parameterNav, method);
-				if (attributes != null) {
+				var (attributes, origins) = ProcessAttributes (parameterNav, method);
+				if (attributes != null && origins != null) {
 					string paramName = GetAttribute (parameterNav, "name");
 					foreach (ParameterDefinition parameter in method.Parameters) {
 						if (paramName == parameter.Name) {
 							if (parameter.HasCustomAttributes || _attributeInfo.CustomAttributes.ContainsKey (parameter))
 								LogWarning (parameterNav, DiagnosticId.XmlMoreThanOneValyForParameterOfMethod, paramName, method.GetDisplayName ());
-							_attributeInfo.AddCustomAttributes (parameter, attributes);
+							_attributeInfo.AddCustomAttributes (parameter, attributes, origins);
 							break;
 						}
 					}
@@ -572,9 +574,9 @@ namespace Mono.Linker.Steps
 		void PopulateAttributeInfo (ICustomAttributeProvider provider, XPathNavigator nav)
 		{
 			Debug.Assert (_attributeInfo != null);
-			var attributes = ProcessAttributes (nav, provider);
-			if (attributes != null)
-				_attributeInfo.AddCustomAttributes (provider, attributes);
+			var (attributes, origins) = ProcessAttributes (nav, provider);
+			if (attributes != null && origins != null)
+				_attributeInfo.AddCustomAttributes (provider, attributes, origins);
 		}
 	}
 }
diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs
index 81b4b5909..7dbede11d 100644
--- a/src/linker/Linker.Steps/MarkStep.cs
+++ b/src/linker/Linker.Steps/MarkStep.cs
@@ -281,6 +281,9 @@ namespace Mono.Linker.Steps
 			// instead of the per-assembly stores.
 			foreach (var (provider, annotations) in xmlInfo.CustomAttributes)
 				Context.CustomAttributes.PrimaryAttributeInfo.AddCustomAttributes (provider, annotations);
+
+			foreach (var (ca, origin) in xmlInfo.CustomAttributesOrigins)
+				Context.CustomAttributes.PrimaryAttributeInfo.CustomAttributesOrigins.Add (ca, origin);
 		}
 
 		void Complete ()
diff --git a/src/linker/Linker/AttributeInfo.cs b/src/linker/Linker/AttributeInfo.cs
index fec37d51e..876dfcfaa 100644
--- a/src/linker/Linker/AttributeInfo.cs
+++ b/src/linker/Linker/AttributeInfo.cs
@@ -3,6 +3,8 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
+using System.Linq;
 using Mono.Cecil;
 
 namespace Mono.Linker
@@ -11,9 +13,23 @@ namespace Mono.Linker
 	{
 		public Dictionary<ICustomAttributeProvider, CustomAttribute[]> CustomAttributes { get; }
 
+		public Dictionary<CustomAttribute, MessageOrigin> CustomAttributesOrigins { get; }
+
 		public AttributeInfo ()
 		{
 			CustomAttributes = new Dictionary<ICustomAttributeProvider, CustomAttribute[]> ();
+			CustomAttributesOrigins = new Dictionary<CustomAttribute, MessageOrigin> ();
+		}
+
+		public void AddCustomAttributes (ICustomAttributeProvider provider, CustomAttribute[] customAttributes, MessageOrigin[] origins)
+		{
+			Debug.Assert (customAttributes.Length == origins.Length);
+
+			AddCustomAttributes (provider, customAttributes);
+
+			foreach (var (customAttribute, origin) in customAttributes.Zip (origins)) {
+				CustomAttributesOrigins.Add (customAttribute, origin);
+			}
 		}
 
 		public void AddCustomAttributes (ICustomAttributeProvider provider, CustomAttribute[] customAttributes)
diff --git a/src/linker/Linker/CustomAttributeSource.cs b/src/linker/Linker/CustomAttributeSource.cs
index ebf2efd2d..f22802da1 100644
--- a/src/linker/Linker/CustomAttributeSource.cs
+++ b/src/linker/Linker/CustomAttributeSource.cs
@@ -73,6 +73,17 @@ namespace Mono.Linker
 			}
 		}
 
+		public bool TryGetCustomAttributeOrigin (ICustomAttributeProvider provider, CustomAttribute customAttribute, out MessageOrigin origin)
+		{
+			if (PrimaryAttributeInfo.CustomAttributesOrigins.TryGetValue (customAttribute, out origin))
+				return true;
+
+			if (!TryGetEmbeddedXmlInfo (provider, out var embeddedXml))
+				return false;
+
+			return embeddedXml.CustomAttributesOrigins.TryGetValue (customAttribute, out origin);
+		}
+
 		public bool HasAny (ICustomAttributeProvider provider)
 		{
 			if (provider.HasCustomAttributes)
diff --git a/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs b/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs
index c83039c56..168b1ef72 100644
--- a/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs
+++ b/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs
@@ -18,8 +18,18 @@ namespace Mono.Linker
 
 		public class Suppression
 		{
-			public SuppressMessageInfo SuppressMessageInfo { get; set; }
+			public SuppressMessageInfo SuppressMessageInfo { get; }
 			public bool Used { get; set; }
+			public CustomAttribute OriginAttribute { get; }
+			public ICustomAttributeProvider Provider { get; }
+
+			public Suppression (SuppressMessageInfo suppressMessageInfo, bool used, CustomAttribute originAttribute, ICustomAttributeProvider provider)
+			{
+				SuppressMessageInfo = suppressMessageInfo;
+				Used = used;
+				OriginAttribute = originAttribute;
+				Provider = provider;
+			}
 		}
 
 		readonly LinkContext _context;
@@ -33,19 +43,20 @@ namespace Mono.Linker
 			InitializedAssemblies = new HashSet<AssemblyDefinition> ();
 		}
 
-		void AddSuppression (SuppressMessageInfo info, ICustomAttributeProvider provider)
+		void AddSuppression (Suppression suppression)
 		{
 			var used = false;
-			if (!_suppressions.TryGetValue (provider, out var suppressions)) {
+			if (!_suppressions.TryGetValue (suppression.Provider, out var suppressions)) {
 				suppressions = new Dictionary<int, Suppression> ();
-				_suppressions.Add (provider, suppressions);
-			} else if (suppressions.TryGetValue (info.Id, out Suppression? value)) {
+				_suppressions.Add (suppression.Provider, suppressions);
+			} else if (suppressions.TryGetValue (suppression.SuppressMessageInfo.Id, out Suppression? value)) {
 				used = value.Used;
-				string? elementName = provider is MemberReference memberRef ? memberRef.GetDisplayName () : provider.ToString ();
+				string? elementName = suppression.Provider is MemberReference memberRef ? memberRef.GetDisplayName () : suppression.Provider.ToString ();
 				_context.LogMessage ($"Element '{elementName}' has more than one unconditional suppression. Note that only the last one is used.");
 			}
 
-			suppressions[info.Id] = new Suppression { SuppressMessageInfo = info, Used = used };
+			suppression.Used = used;
+			suppressions[suppression.SuppressMessageInfo.Id] = suppression;
 		}
 
 		public bool IsSuppressed (int id, MessageOrigin warningOrigin, out SuppressMessageInfo info)
@@ -82,12 +93,12 @@ namespace Mono.Linker
 			TryGetSuppressionsForProvider (provider, out _);
 		}
 
-		public IEnumerable<(ICustomAttributeProvider provider, SuppressMessageInfo suppressMessageInfo)> GetUnusedSuppressions ()
+		public IEnumerable<Suppression> GetUnusedSuppressions ()
 		{
 			foreach (var (provider, suppressions) in _suppressions) {
 				foreach (var (_, suppression) in suppressions) {
 					if (!suppression.Used)
-						yield return (provider, suppression.SuppressMessageInfo);
+						yield return suppression;
 				}
 			}
 		}
@@ -166,8 +177,8 @@ namespace Mono.Linker
 				var assembly = module.Assembly;
 				if (InitializedAssemblies.Add (assembly)) {
 					foreach (var suppression in DecodeAssemblyAndModuleSuppressions (module)) {
-						AddSuppression (suppression.Info, suppression.Target);
-						membersToScan.Add (suppression.Target);
+						AddSuppression (suppression);
+						membersToScan.Add (suppression.Provider);
 					}
 				}
 			}
@@ -177,8 +188,8 @@ namespace Mono.Linker
 			foreach (var member in membersToScan) {
 				if (member is ModuleDefinition or AssemblyDefinition)
 					continue;
-				foreach (var suppressionInfo in DecodeSuppressions (member))
-					AddSuppression (suppressionInfo, member);
+				foreach (var suppression in DecodeSuppressions (member))
+					AddSuppression (suppression);
 			}
 
 			return _suppressions.TryGetValue (provider, out suppressions);
@@ -245,7 +256,7 @@ namespace Mono.Linker
 			}
 		}
 
-		IEnumerable<SuppressMessageInfo> DecodeSuppressions (ICustomAttributeProvider provider)
+		IEnumerable<Suppression> DecodeSuppressions (ICustomAttributeProvider provider)
 		{
 			Debug.Assert (provider is not ModuleDefinition or AssemblyDefinition);
 
@@ -259,11 +270,11 @@ namespace Mono.Linker
 				if (!TryDecodeSuppressMessageAttributeData (ca, out var info))
 					continue;
 
-				yield return info;
+				yield return new Suppression (info, used: false, originAttribute: ca, provider);
 			}
 		}
 
-		IEnumerable<(SuppressMessageInfo Info, ICustomAttributeProvider Target)> DecodeAssemblyAndModuleSuppressions (ModuleDefinition module)
+		IEnumerable<Suppression> DecodeAssemblyAndModuleSuppressions (ModuleDefinition module)
 		{
 			AssemblyDefinition assembly = module.Assembly;
 			foreach (var suppression in DecodeGlobalSuppressions (module, assembly))
@@ -275,7 +286,7 @@ namespace Mono.Linker
 			}
 		}
 
-		IEnumerable<(SuppressMessageInfo Info, ICustomAttributeProvider Target)> DecodeGlobalSuppressions (ModuleDefinition module, ICustomAttributeProvider provider)
+		IEnumerable<Suppression> DecodeGlobalSuppressions (ModuleDefinition module, ICustomAttributeProvider provider)
 		{
 			var attributes = _context.CustomAttributes.GetCustomAttributes (provider).
 					Where (a => TypeRefHasUnconditionalSuppressions (a.AttributeType));
@@ -286,13 +297,13 @@ namespace Mono.Linker
 
 				var scope = info.Scope?.ToLower ();
 				if (info.Target == null && (scope == "module" || scope == null)) {
-					yield return (info, provider);
+					yield return new Suppression (info, used: false, originAttribute: instance, provider);
 					continue;
 				}
 
 				switch (scope) {
 				case "module":
-					yield return (info, provider);
+					yield return new Suppression (info, used: false, originAttribute: instance, provider);
 					break;
 
 				case "type":
@@ -301,7 +312,7 @@ namespace Mono.Linker
 						break;
 
 					foreach (var result in DocumentationSignatureParser.GetMembersForDocumentationSignature (info.Target, module, _context))
-						yield return (info, result);
+						yield return new Suppression (info, used: false, originAttribute: instance, result);
 
 					break;
 				default:
@@ -316,5 +327,14 @@ namespace Mono.Linker
 			return typeRef.Name == "UnconditionalSuppressMessageAttribute" &&
 				typeRef.Namespace == "System.Diagnostics.CodeAnalysis";
 		}
+
+		public MessageOrigin GetSuppressionOrigin (Suppression suppression)
+		{
+			if (_context.CustomAttributes.TryGetCustomAttributeOrigin (suppression.Provider, suppression.OriginAttribute, out MessageOrigin origin))
+				return origin;
+			if (suppression.Provider is ModuleDefinition module)
+				return new MessageOrigin (module.Assembly);
+			return new MessageOrigin (suppression.Provider);
+		}
 	}
 }
diff --git a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFromXML.cs b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFromXML.cs
index 625817372..274eb8119 100644
--- a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFromXML.cs
+++ b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFromXML.cs
@@ -12,6 +12,8 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 {
 	[SkipKeptItemsValidation]
 	[ExpectedNoWarnings]
+	[ExpectedWarning ("IL2121", "IL2026", ProducedBy = ProducedBy.Trimmer, FileName = "DetectRedundantSuppressionsFromXML.xml", SourceLine = 7)]
+	[ExpectedWarning ("IL2121", "IL2109", ProducedBy = ProducedBy.Trimmer, FileName = "DetectRedundantSuppressionsFromXML.xml", SourceLine = 12)]
 	[SetupLinkAttributesFile ("DetectRedundantSuppressionsFromXML.xml")]
 	public class DetectRedundantSuppressionsFromXML
 	{
@@ -20,12 +22,8 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 			DetectRedundantSuppressions.Test ();
 		}
 
-		[ExpectedWarning ("IL2121", "IL2109", ProducedBy = ProducedBy.Trimmer)]
 		public class DetectRedundantSuppressions
 		{
-			// The warning should ideally point to XML.
-			// https://github.com/dotnet/linker/issues/2923
-			[ExpectedWarning ("IL2121", "IL2026", ProducedBy = ProducedBy.Trimmer)]
 			public static void Test ()
 			{
 				DoNotTriggerWarning ();