Fix missing assemblyref for typeref only kept for debug info in illink

src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

@@ -1452,7 +1452,7 @@ sealed class TypeReferenceMarker : TypeReferenceWalker
 			readonly MarkingHelpers markingHelpers;
 
 			TypeReferenceMarker (AssemblyDefinition assembly, MarkingHelpers markingHelpers)
-				: base (assembly)
+				: base (assembly, walkSymbols: false)
 			{
 				this.markingHelpers = markingHelpers;
 			}

src/tools/illink/src/linker/Linker.Steps/SweepStep.cs

@@ -58,7 +58,7 @@ protected override void Process ()
 			foreach (var assembly in assemblies)
 				RemoveUnmarkedAssembly (assembly);
 
-			// Look for references (included to previously unresolved assemblies) marked for deletion
+			// Look for references (including to previously unresolved assemblies) marked for deletion
 			foreach (var assembly in assemblies)
 				UpdateAssemblyReferencesToRemovedAssemblies (assembly);
 
@@ -69,7 +69,7 @@ protected override void Process ()
 				case AssemblyAction.CopyUsed:
 				case AssemblyAction.Link:
 				case AssemblyAction.Save:
-					bool changed = AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+					bool changed = SweepAssemblyReferences (assembly);
 					if (changed && action == AssemblyAction.CopyUsed)
 						Annotations.SetAction (assembly, AssemblyAction.Save);
 					break;
@@ -174,7 +174,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly)
 				AssemblyAction assemblyAction = AssemblyAction.Copy;
 				if (SweepTypeForwarders (assembly)) {
 					// Need to sweep references, in case sweeping type forwarders removed any
-					AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+					SweepAssemblyReferences (assembly);
 					assemblyAction = AssemblyAction.Save;
 				}
 
@@ -194,7 +194,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly)
 			case AssemblyAction.Save:
 				if (SweepTypeForwarders (assembly)) {
 					// Need to sweep references, in case sweeping type forwarders removed any
-					AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+					SweepAssemblyReferences (assembly);
 				}
 				break;
 			}
@@ -242,7 +242,7 @@ protected virtual void SweepAssembly (AssemblyDefinition assembly)
 			}
 
 			if (SweepTypeForwarders (assembly) || updateScopes)
-				AssemblyReferencesCorrector.SweepAssemblyReferences (assembly);
+				SweepAssemblyReferences (assembly);
 		}
 
 		bool IsMarkedAssembly (AssemblyDefinition assembly)
@@ -568,32 +568,32 @@ protected virtual void CustomAttributeUsageRemoved (ICustomAttributeProvider pro
 		{
 		}
 
+		bool SweepAssemblyReferences (AssemblyDefinition assembly)
+		{
+			//
+			// We used to run over list returned by GetTypeReferences but
+			// that returns typeref(s) of original assembly and we don't track
+			// which types are needed for which assembly which left us
+			// with dangling assembly references
+			//
+			assembly.MainModule.AssemblyReferences.Clear ();
+
+			var arc = new AssemblyReferencesCorrector (assembly, walkSymbols: Context.LinkSymbols);
+			arc.Process ();
+
+			return arc.ChangedAnyScopes;
+		}
+
 		sealed class AssemblyReferencesCorrector : TypeReferenceWalker
 		{
 			readonly DefaultMetadataImporter importer;
 
-			bool changedAnyScopes;
+			public bool ChangedAnyScopes { get; private set; }
 
-			AssemblyReferencesCorrector (AssemblyDefinition assembly) : base (assembly)
+			public AssemblyReferencesCorrector (AssemblyDefinition assembly, bool walkSymbols) : base (assembly, walkSymbols)
 			{
 				this.importer = new DefaultMetadataImporter (assembly.MainModule);
-				changedAnyScopes = false;
-			}
-
-			public static bool SweepAssemblyReferences (AssemblyDefinition assembly)
-			{
-				//
-				// We used to run over list returned by GetTypeReferences but
-				// that returns typeref(s) of original assembly and we don't track
-				// which types are needed for which assembly which left us
-				// with dangling assembly references
-				//
-				assembly.MainModule.AssemblyReferences.Clear ();
-
-				var arc = new AssemblyReferencesCorrector (assembly);
-				arc.Process ();
-
-				return arc.changedAnyScopes;
+				ChangedAnyScopes = false;
 			}
 
 			protected override void ProcessTypeReference (TypeReference type)
@@ -626,7 +626,7 @@ protected override void ProcessTypeReference (TypeReference type)
 					return;
 
 				type.Scope = tr.Scope;
-				changedAnyScopes = true;
+				ChangedAnyScopes = true;
 			}
 
 			protected override void ProcessExportedType (ExportedType exportedType)
@@ -647,7 +647,7 @@ protected override void ProcessExportedType (ExportedType exportedType)
 					return;
 
 				exportedType.Scope = tr.Scope;
-				changedAnyScopes = true;
+				ChangedAnyScopes = true;
 			}
 		}
 	}

src/tools/illink/src/linker/Linker/TypeReferenceWalker.cs

@@ -15,15 +15,18 @@ abstract class TypeReferenceWalker
 
 		protected HashSet<TypeReference> Visited { get; } = new HashSet<TypeReference> ();
 
-		public TypeReferenceWalker (AssemblyDefinition assembly)
+		readonly bool walkSymbols;
+
+		public TypeReferenceWalker (AssemblyDefinition assembly, bool walkSymbols)
 		{
 			this.assembly = assembly;
+			this.walkSymbols = walkSymbols;
 		}
 
 		// Traverse the assembly and mark the scopes of discovered type references (but not exported types).
 		// This includes scopes referenced by Cecil TypeReference objects that don't represent rows in the typeref table,
 		// such as references to built-in types, or attribute arguments which encode type references as strings.
-		protected virtual void Process ()
+		public virtual void Process ()
 		{
 			if (Visited.Count > 0)
 				throw new InvalidOperationException ();
@@ -103,6 +106,9 @@ void WalkScopes (TypeDefinition typeDefinition)
 
 					if (m.HasBody)
 						WalkTypeScope (m.Body);
+
+					if (walkSymbols && m.DebugInformation?.Scope?.Import is ImportDebugInformation import)
+						WalkDebugInfoImportScope (import);
 				}
 			}
 
@@ -315,6 +321,17 @@ void WalkForwardedTypesScope (CustomAttributeArgument attributeArgument)
 			}
 		}
 
+		void WalkDebugInfoImportScope (ImportDebugInformation import)
+		{
+			if (import.HasTargets) {
+				foreach (var target in import.Targets)
+					WalkScopeOfTypeReference (target.Type);
+			}
+
+			if (import.Parent is ImportDebugInformation parent)
+				WalkDebugInfoImportScope (parent);
+		}
+
 		void WalkScopeOfTypeReference (TypeReference type)
 		{
 			if (type == null)

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs

@@ -11,7 +11,7 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 	[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
 	public class SetupCompileBeforeAttribute : BaseMetadataAttribute
 	{
-		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false, string outputSubFolder = null)
+		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string[] additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false, string outputSubFolder = null)
 		{
 			ArgumentNullException.ThrowIfNull (sourceFiles);
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj

@@ -16,6 +16,7 @@
     <Compile Remove="LinkXml\Dependencies\CanPreserveAnExportedType_Forwarder.cs" />
     <Compile Remove="LinkXml\Dependencies\UsedNonRequiredExportedTypeIsKept_fwd.cs" />
     <Compile Remove="LinkXml\Dependencies\UsedNonRequiredExportedTypeIsKeptWhenRooted_fwd.cs" />
+    <Compile Remove="References\Dependencies\CustomMarkHandlerSaveAssembly.cs" />
     <Compile Remove="TypeForwarding\Dependencies\AnotherLibraryForwarder.cs" />
     <Compile Remove="TypeForwarding\Dependencies\AssemblyReferenceIsRemovedWhenUnused_Library.cs" />
     <Compile Remove="TypeForwarding\Dependencies\ForwarderLibrary.cs" />

src/tools/illink/test/Mono.Linker.Tests.Cases/References/AssemblyOnlyUsedByUsingSaveAction.cs

@@ -0,0 +1,42 @@
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Helpers;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+using Mono.Linker.Tests.Cases.References.Dependencies;
+
+namespace Mono.Linker.Tests.Cases.References
+{
+	/// <summary>
+	/// We can't detect the using usage in the assembly.  As a result, nothing in `library` is going to be marked and that assembly will be deleted.
+	/// With the assembly action of `save`, we remove unused assembly references from the assembly and rewrite it.
+	/// When cecil writes the assembly, the unused typeref is not written out.
+	/// </summary>
+
+	// Add a custom step which sets the assembly action of the test to "save"    
+    [SetupCompileBefore ("SetSaveAction.dll", new[] { "Dependencies/CustomMarkHandlerSaveAssembly.cs" },
+		new[] { "illink.dll", "Mono.Cecil.dll", "netstandard.dll" })]
+    [SetupLinkerArgument ("--custom-step", "CustomMarkHandlerSaveAssembly,SetSaveAction.dll")]
+
+	// When csc is used, `saved.dll` will have a reference to `library.dll`
+	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
+	[SetupCompileBefore ("saved.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs" },
+		// Even though this testcase doesn't link symbols, tell the compiler to produce symbols to confirm that the behavior
+		// isn't affected by the presence of symbols when not passing '-b'.
+		new[] { "library.dll" }, additionalArguments: new string[] { "/debug:portable" }, compilerToUse: "csc")]
+
+	// Here to assert that the test is setup correctly to preserve unused code in the saved assembly.  This is an important aspect of the bug
+	[KeptMemberInAssembly ("saved.dll", typeof (AssemblyOnlyUsedByUsing_UnusedUsing), "Unused()")]
+
+	// The library should be gone. The `using` statement leaves no traces in the IL so nothing in `library` will be marked
+	[RemovedAssembly ("library.dll")]
+	// The `save` action results in the reference to System.Runtime being resolved into a reference directly to System.Private.CoreLib.
+	// The reference to `library` is removed.
+	[KeptReferencesInAssembly ("saved.dll", new[] { "System.Private.CoreLib" })]
+	public class AssemblyOnlyUsedByUsingSaveAction
+	{
+		public static void Main ()
+		{
+			// Use something to keep the reference at compile time
+			AssemblyOnlyUsedByUsing_UnusedUsing.UsedToKeepReference ();
+		}
+	}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/AssemblyOnlyUsedByUsingSaveActionWithSymbols.cs

@@ -0,0 +1,45 @@
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Helpers;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+using Mono.Linker.Tests.Cases.References.Dependencies;
+
+namespace Mono.Linker.Tests.Cases.References
+{
+	/// <summary>
+	/// We can't detect the using usage in the assembly.  As a result, nothing in `library` is going to be marked and that assembly will be deleted.
+	/// With the assembly action of `save`, we remove unused assembly references from the assembly and rewrite it, preserving all methods.
+	/// When debug symbols are present for the `save` assembly, we do not trim debug symbols, so cecil keeps all type references in the
+	/// save assembly that are referenced by the debug info. This includes the unused typeref, so the assemblyref referenced by the typeref
+	/// must also be preserved.
+	/// </summary>
+
+	// Add a custom step which sets the assembly action of the test to "save"    
+    [SetupCompileBefore ("SetSaveAction_Symbols.dll", new[] { "Dependencies/CustomMarkHandlerSaveAssembly.cs" },
+		new[] { "illink.dll", "Mono.Cecil.dll", "netstandard.dll" })]
+    [SetupLinkerArgument ("--custom-step", "CustomMarkHandlerSaveAssembly,SetSaveAction_Symbols.dll")]
+
+	// When csc is used, `saved.dll` will have a reference to `library.dll`
+	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
+	[SetupCompileBefore ("saved.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs" },
+		new[] { "library.dll" }, additionalArguments: new string[] { "/debug:portable" }, compilerToUse: "csc")]
+
+	// Here to assert that the test is setup correctly to preserve unused code in the saved assembly.  This is an important aspect of the bug
+	[KeptMemberInAssembly ("saved.dll", typeof (AssemblyOnlyUsedByUsing_UnusedUsing), "Unused()")]
+
+	// The library should be gone. The `using` statement leaves no traces in the IL so nothing in `library` will be marked
+	[RemovedAssembly ("library.dll")]
+	// The `save` action results in the reference to System.Runtime being resolved into a reference directly to System.Private.CoreLib.
+	// The reference to `library` is kept, because it is referenced from a typeref that is referenced from the debug info.
+	[KeptReferencesInAssembly ("saved.dll", new[] { "System.Private.CoreLib", "library" })]
+
+	// Linking debug symbols is required for cecil not to remove the typeref from the assembly, because it is only referenced from the debug info.
+	[SetupLinkerLinkSymbols ("true")]
+	public class AssemblyOnlyUsedByUsingSaveActionWithSymbols
+	{
+		public static void Main ()
+		{
+			// Use something to keep the reference at compile time
+			AssemblyOnlyUsedByUsing_UnusedUsing.UsedToKeepReference ();
+		}
+	}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/AssemblyOnlyUsedByUsingWithCsc.cs

@@ -7,26 +7,26 @@ namespace Mono.Linker.Tests.Cases.References
 {
 	/// <summary>
 	/// We can't detect the using usage in the assembly.  As a result, nothing in `library` is going to be marked and that assembly will be deleted.
-	/// Because of that, `copied` needs to have it's reference to `library` removed even though we specified an assembly action of `copy`
+	/// However, because we specified an assembly action of `copy`, we do not rewrite `copied`, and it ends up with an unused reference to the removed `library`.
 	/// </summary>
 	[SetupLinkerAction ("copy", "copied")]
-	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
 
 	// When csc is used, `copied.dll` will have a reference to `library.dll`
-	[SetupCompileBefore ("copied.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Copied.cs" }, new[] { "library.dll" }, compilerToUse: "csc")]
+	[SetupCompileBefore ("library.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_Lib.cs" })]
+	[SetupCompileBefore ("copied.dll", new[] { "Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs" }, new[] { "library.dll" }, compilerToUse: "csc")]
 
 	// Here to assert that the test is setup correctly to copy the copied assembly.  This is an important aspect of the bug
-	[KeptMemberInAssembly ("copied.dll", typeof (AssemblyOnlyUsedByUsing_Copied), "Unused()")]
+	[KeptMemberInAssembly ("copied.dll", typeof (AssemblyOnlyUsedByUsing_UnusedUsing), "Unused()")]
 
-	// We library should be gone.  The `using` statement leaves no traces in the IL so nothing in `library` will be marked
+	// The library should be gone. The `using` statement leaves no traces in the IL so nothing in `library` will be marked
 	[RemovedAssembly ("library.dll")]
 	[KeptReferencesInAssembly ("copied.dll", new[] { "System.Runtime", "library" })]
 	public class AssemblyOnlyUsedByUsingWithCsc
 	{
 		public static void Main ()
 		{
 			// Use something to keep the reference at compile time
-			AssemblyOnlyUsedByUsing_Copied.UsedToKeepReference ();
+			AssemblyOnlyUsedByUsing_UnusedUsing.UsedToKeepReference ();
 		}
 	}
-}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/Dependencies/AssemblyOnlyUsedByUsing_Copied.cs → src/tools/illink/test/Mono.Linker.Tests.Cases/References/Dependencies/AssemblyOnlyUsedByUsing_UnusedUsing.cs

@@ -1,11 +1,10 @@
-
 // This is what triggers the behavior difference between Roslyn and mcs.  Roslyn will keep the reference
 // to this assembly because of this whereas mcs will not
 using ImportantForBug = Mono.Linker.Tests.Cases.References.Dependencies.AssemblyOnlyUsedByUsing_Lib;
 
 namespace Mono.Linker.Tests.Cases.References.Dependencies
 {
-	public class AssemblyOnlyUsedByUsing_Copied
+	public class AssemblyOnlyUsedByUsing_UnusedUsing
 	{
 		public static void UsedToKeepReference ()
 		{
@@ -15,4 +14,4 @@ private static void Unused ()
 		{
 		}
 	}
-}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/References/Dependencies/CustomMarkHandlerSaveAssembly.cs

@@ -0,0 +1,15 @@
+using System;
+using Mono.Cecil;
+using Mono.Linker;
+using Mono.Linker.Steps;
+
+public class CustomMarkHandlerSaveAssembly : IMarkHandler
+{
+    public void Initialize (LinkContext context, MarkContext markContext)
+    {
+        markContext.RegisterMarkAssemblyAction (assembly => {
+            if (assembly.Name.Name == "saved")
+                context.Annotations.SetAction (assembly, AssemblyAction.Save);
+        });
+    }
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdb.cs

@@ -5,7 +5,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("false")]
 
 	[RemovedSymbols ("LibraryWithEmbeddedPdbSymbols.dll")]

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbAndSymbolLinkingEnabled.cs

@@ -5,7 +5,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("true")]
 
 	[KeptSymbols ("LibraryWithEmbeddedPdbSymbols.dll")]

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbAndSymbolLinkingEnabledAndDeterministicMvid.cs

@@ -4,7 +4,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("true")]
 	[SetupLinkerArgument ("--deterministic", "true")]
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbAndSymbolLinkingEnabledAndNewMvid.cs

@@ -4,7 +4,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("true")]
 	[SetupLinkerArgument ("--deterministic", "true")]
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/ReferenceWithEmbeddedPdbCopyAction.cs

@@ -4,7 +4,7 @@
 
 namespace Mono.Linker.Tests.Cases.Symbols
 {
-	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: "/debug:embedded", compilerToUse: "csc")]
+	[SetupCompileBefore ("LibraryWithEmbeddedPdbSymbols.dll", new[] { "Dependencies/LibraryWithEmbeddedPdbSymbols.cs" }, additionalArguments: new[] { "/debug:embedded" }, compilerToUse: "csc")]
 	[SetupLinkerLinkSymbols ("false")]
 	[SetupLinkerAction ("copy", "LibraryWithEmbeddedPdbSymbols")]