Merge pull request #4131 from icsharpcode/tools/nugetfuzz-finding-identity

Make every nugetfuzz finding traceable to the assembly it came from

TestTools/README.md

@@ -37,6 +37,24 @@ net4x. Getting these right matters - binding a WPF assembly against the stub fac
 `NETCore.App.Ref` collapses whole type hierarchies to `Unknown` and invents hundreds of bogus
 warnings, so treat a sudden warning spike as a reference problem until proven otherwise.
 
+Every finding therefore carries the assembly it came from and the references that assembly was
+decompiled against - the search directories in priority order, and what each reference resolved
+to - so that judgement can be made from the report rather than from a second run, and so a
+finding can be reproduced by hand:
+
+```
+assembly: ~/.cache/nugetfuzz/fluentvalidation/12.1.1/lib/net8.0/FluentValidation.dll
+  -r ~/.cache/nugetfuzz/fluentvalidation/12.1.1/lib/net8.0
+  -r ~/.cache/nugetfuzz/microsoft.netcore.app.ref/8.0.31/ref/net8.0
+  System.Runtime, Version=8.0.0.0, ... -> ~/.cache/nugetfuzz/microsoft.netcore.app.ref/8.0.31/ref/net8.0/System.Runtime.dll
+  Some.Package, Version=1.0.0.0, ...  -> NOT FOUND
+```
+
+The reference set is recorded once the assembly is finished, not when the finding is first hit:
+the resolver keeps discovering references for as long as it decompiles. This is the same data
+`NUGETFUZZ_VERBOSE` prints, and it is the bulk of a ledger line - findings dedupe, so it is
+carried once per distinct finding, not once per hit.
+
 Environment variables: `NUGETFUZZ_VERBOSE` (per-type progress), `NUGETFUZZ_DUMP=<dir>` (write the
 decompiled C#), `NUGETFUZZ_LEDGER=<file>` (append findings as JSONL instead of writing a
 per-run HTML report), `NUGETFUZZ_HTML=<file>` (report path), `NUGET_PACKAGES` (package cache).

TestTools/nugetfuzz.cs

@@ -137,6 +137,9 @@ var downloadOnly = args.Contains("--download-only");
 var pdbMode = args.Contains("--pdb");
 // Checks the PDB the assembly already ships with. Calibration, not a sweep.
 var pdbLint = args.Contains("--pdb-lint");
+// Findings first hit by the assembly being decompiled. They get their reference context
+// once it is done, because the resolver keeps discovering references until then.
+var pendingContext = new List<string>();
 var arguments = args
 	.Where(a => !a.StartsWith("--"))
 	.SelectMany(a => a.StartsWith('@') ? File.ReadAllLines(a[1..]) : new[] { a })
@@ -554,6 +557,10 @@ async Task<string> GetPackage(string id, NuGetVersion version)
 
 async Task DecompileAssembly(string pkg, string dllPath, List<string> searchDirs, NuGetFramework matchTarget, string? fallbackDir)
 {
+	// An assembly that bails out before reporting its resolutions leaves findings behind that
+	// never received a context. Dropping them here keeps the next assembly from stamping its
+	// own references onto them, which would name the wrong reference set for the finding.
+	pendingContext.Clear();
 	var name = Path.GetFileName(dllPath);
 	PEFile module;
 	try
@@ -567,6 +574,9 @@ async Task DecompileAssembly(string pkg, string dllPath, List<string> searchDirs
 	}
 	using (module)
 	{
+		// The file name alone is ambiguous across a sweep: the same simple name ships in many
+		// packages and many TFM folders. Identify findings by full assembly name plus path.
+		name = $"{module.FullName} ({dllPath})";
 		// ".NETCoreApp,Version=v5.0" -> 5.0; null for .NET Framework / netstandard modules.
 		Version? coreVersion = null;
 		var tfmId = module.DetectTargetFrameworkId();
@@ -620,7 +630,7 @@ async Task DecompileAssembly(string pkg, string dllPath, List<string> searchDirs
 		if (pdbMode)
 		{
 			CheckGeneratedPdb(pkg, name, module, decompiler, dllPath, orderedDirs);
-			ReportResolutions(logResolver);
+			ReportResolutions(logResolver, dllPath, orderedDirs);
 			return;
 		}
 		foreach (var type in decompiler.TypeSystem.MainModule.TopLevelTypeDefinitions.ToList())
@@ -674,7 +684,7 @@ async Task DecompileAssembly(string pkg, string dllPath, List<string> searchDirs
 				Report(pkg, name, type.FullTypeName.ToString(), ex);
 			}
 		}
-		ReportResolutions(logResolver);
+		ReportResolutions(logResolver, dllPath, orderedDirs);
 	}
 }
 
@@ -685,10 +695,21 @@ string SyntaxTreeToString(SyntaxTree syntaxTree)
 	return w.ToString();
 }
 
-void ReportResolutions(LoggingResolver logResolver)
+void ReportResolutions(LoggingResolver logResolver, string dllPath, List<string> orderedDirs)
 {
 	var resolutions = logResolver.Resolutions;
 	var unresolved = resolutions.Where(kv => kv.Value == null).Select(kv => kv.Key).OrderBy(k => k).ToList();
+	// What it takes to reproduce a finding by hand: the assembly it came from and the
+	// references it was decompiled against, which are what the report is read for once
+	// a warning turns out to be a reference problem rather than a decompiler defect.
+	var context = new StringBuilder($"assembly: {dllPath}");
+	foreach (var dir in orderedDirs)
+		context.Append($"{Environment.NewLine}  -r {dir}");
+	foreach (var (refName, refPath) in resolutions.OrderBy(kv => kv.Key, StringComparer.Ordinal))
+		context.Append($"{Environment.NewLine}  {refName} -> {refPath ?? "NOT FOUND"}");
+	foreach (var key in pendingContext)
+		failures[key] = failures[key] with { Context = context.ToString() };
+	pendingContext.Clear();
 	refsTotal += resolutions.Count;
 	refsResolved += resolutions.Count - unresolved.Count;
 	Console.WriteLine($"    refs: {resolutions.Count - unresolved.Count}/{resolutions.Count} resolved");
@@ -768,7 +789,6 @@ void CheckGeneratedPdb(string pkg, string asm, PEFile module, CSharpDecompiler d
 // reported here is a defect in the lint rather than in ILSpy.
 void LintExistingPdb(string dllPath)
 {
-	var asm = Path.GetFileName(dllPath);
 	using var peStream = File.OpenRead(dllPath);
 	PEReader peReader;
 	try
@@ -783,6 +803,7 @@ void LintExistingPdb(string dllPath)
 	}
 	using (peReader)
 	{
+		var asm = $"{peReader.GetMetadataReader().GetFullAssemblyName()} ({dllPath})";
 		// A PDB next to the assembly if there is one, otherwise the one embedded in the PE.
 		MemoryStream? pdbStream = null;
 		MetadataReaderProvider? provider = null;
@@ -1304,6 +1325,7 @@ void Report(string pkg, string asm, string type, Exception ex)
 	{
 		failures[key] = new Finding(kind, inner.GetType().Name, FirstLine(inner.Message),
 			FirstLine(topFrame), location, ex.ToString(), 1);
+		pendingContext.Add(key);
 		Console.WriteLine($"  [{kind}] {location}");
 		foreach (var line in ex.ToString().Split('\n').Take(30))
 			Console.WriteLine("      " + line.TrimEnd());
@@ -1324,7 +1346,7 @@ static void AppendToLedger(string path, IEnumerable<Finding> findings, int assem
 		// often than a decompiler defect ("might be due to ... missing references" is what
 		// the warning itself says), and the report separates the two on this basis.
 		lines.Add(JsonSerializer.Serialize(new LedgerEntry("finding", f.Kind, f.ExceptionType, f.Message,
-			f.Frame, f.FirstLocation, f.Detail, f.Count, 0, 0, refsResolved, refsTotal)));
+			f.Frame, f.FirstLocation, f.Detail, f.Count, 0, 0, refsResolved, refsTotal, f.Context)));
 	}
 	lines.Add(JsonSerializer.Serialize(new LedgerEntry("totals", "", "", "", "", "", "", 0,
 		assemblies, types, refsResolved, refsTotal)));
@@ -1385,7 +1407,7 @@ static void RenderLedger(string ledgerPath, string outPath)
 		merged[key] = merged.TryGetValue(key, out var existing)
 			? existing with { Count = existing.Count + entry.Count }
 			: new Finding(entry.Kind, entry.ExceptionType, entry.Message, entry.Frame,
-				entry.FirstLocation, entry.Detail, entry.Count);
+				entry.FirstLocation, entry.Detail, entry.Count, entry.Context);
 		// Ledger lines written before this attribution existed carry 0/0; treat those as
 		// unknown rather than clean, so they are never presented as confirmed defects.
 		(entry.RefsTotal > 0 && entry.RefsResolved == entry.RefsTotal ? clean : degraded).Add(key);
@@ -1450,7 +1472,8 @@ static void WriteHtmlReport(string path, List<Finding> findings, int assemblies,
 				: "";
 			html.AppendLine($"<details class={kind}><summary><span class=count>{f.Count}x</span> "
 				+ $"{Esc(f.ExceptionType)}: {Esc(f.Message)}{suspect}</summary>");
-			html.AppendLine($"<pre>first: {Esc(f.FirstLocation)}\nframe: {Esc(f.Frame)}\n\n{Esc(f.Detail)}</pre></details>");
+			var context = f.Context.Length > 0 ? $"{Esc(f.Context)}\n" : "";
+			html.AppendLine($"<pre>first: {Esc(f.FirstLocation)}\nframe: {Esc(f.Frame)}\n{context}\n{Esc(f.Detail)}</pre></details>");
 		}
 	}
 	html.AppendLine("""
@@ -1479,7 +1502,7 @@ static string FirstLine(string s)
 // One deduplicated defect: Count counts every location that hit it, Detail keeps the
 // full exception text of the first one for triage.
 record Finding(string Kind, string ExceptionType, string Message, string Frame,
-	string FirstLocation, string Detail, int Count)
+	string FirstLocation, string Detail, int Count, string Context = "")
 {
 	public string Describe()
 		=> $"[{Kind}] {ExceptionType}: {Message} @ {Frame}  (first: {FirstLocation})";
@@ -1488,7 +1511,7 @@ record Finding(string Kind, string ExceptionType, string Message, string Frame,
 // One line of the sweep ledger: either a deduplicated finding or a per-run totals record.
 record LedgerEntry(string Record, string Kind, string ExceptionType, string Message, string Frame,
 	string FirstLocation, string Detail, int Count, int Assemblies, int Types,
-	long RefsResolved, long RefsTotal);
+	long RefsResolved, long RefsTotal, string Context = "");
 
 record VersionIndex(string[] versions);