Merge pull request #4128 from icsharpcode/tests/nugetfuzz-leaked-names

Report compiler-generated names leaking into decompiled output

TestTools/decompdiff.cs

@@ -255,7 +255,7 @@ summary.AppendLine($"|---|---|---|---|");
 summary.AppendLine(MetricRow("lines", oldTotals.Lines, newTotals.Lines));
 summary.AppendLine(MetricRow("goto statements", oldTotals.Gotos, newTotals.Gotos));
 summary.AppendLine(MetricRow("//IL_ warning comments", oldTotals.IlWarnings, newTotals.IlWarnings));
-summary.AppendLine(MetricRow("compiler-generated name leaks (<>)", oldTotals.GeneratedNames, newTotals.GeneratedNames));
+summary.AppendLine(MetricRow("compiler-generated name leaks", oldTotals.GeneratedNames, newTotals.GeneratedNames));
 summary.AppendLine();
 summary.AppendLine($"types: {unchanged} unchanged, {changed.Count} changed, {newErrors} NEW errors, {fixedErrors} fixed errors, {bothErrors} errored in both");
 summary.AppendLine();
@@ -832,11 +832,21 @@ record ChangedType(string Location, Metrics Old, Metrics New);
 
 record struct Metrics(int Lines, int Gotos, int IlWarnings, int GeneratedNames)
 {
+	// Names that the decompiler's own identifier rules reject: every character of an
+	// identifier must be a letter, digit or '_' (EscapeInvalidIdentifiers.IsValid), and the
+	// mangled ones start with '<' or contain '$' (SRMExtensions.IsGeneratedName). The input
+	// is output text rather than identifiers, so the shapes are matched lexically: a
+	// bracketed part followed by an identifier character ("<>c", "<P>k__BackingField",
+	// "<Main>$"), or a '$' between identifier characters ("VB$AnonymousType_0"). Real C#
+	// never puts an identifier character directly after a generic argument list's '>', so
+	// generic instantiations do not match.
+	const string GeneratedNamePattern = @"<[A-Za-z0-9_.,<> ]*>[A-Za-z0-9_$]|[A-Za-z0-9_]\$[A-Za-z0-9_$]";
+
 	public static Metrics Measure(string code) => new(
 		code.Count(c => c == '\n') + 1,
 		Regex.Matches(code, @"\bgoto ").Count,
 		Regex.Matches(code, @"//IL_[0-9a-fA-F]+:").Count,
-		Regex.Matches(code, @"<>").Count);
+		Regex.Matches(code, GeneratedNamePattern).Count);
 
 	public static Metrics operator +(Metrics a, Metrics b)
 		=> new(a.Lines + b.Lines, a.Gotos + b.Gotos, a.IlWarnings + b.IlWarnings, a.GeneratedNames + b.GeneratedNames);

TestTools/nugetfuzz.cs

@@ -47,6 +47,8 @@ using System.Text.RegularExpressions;
 
 using ICSharpCode.Decompiler;
 using ICSharpCode.Decompiler.CSharp;
+using ICSharpCode.Decompiler.CSharp.OutputVisitor;
+using ICSharpCode.Decompiler.CSharp.Syntax;
 using ICSharpCode.Decompiler.DebugInfo;
 using ICSharpCode.Decompiler.Metadata;
 
@@ -81,6 +83,7 @@ var installedTfm = NuGetFramework.Parse($"net{Environment.Version.Major}.{Enviro
 var net48 = NuGetFramework.Parse("net48");
 var reducer = new FrameworkReducer();
 var failures = new Dictionary<string, Finding>();
+var formatting = new DecompilerSettings().CSharpFormattingOptions;
 int assemblyCount = 0, typeCount = 0, pdbChecked = 0, pdbSkipped = 0;
 long charCount = 0, refsResolved = 0, refsTotal = 0;
 bool verbose = Environment.GetEnvironmentVariable("NUGETFUZZ_VERBOSE") != null;
@@ -626,17 +629,34 @@ async Task DecompileAssembly(string pkg, string dllPath, List<string> searchDirs
 			decompiler.CancellationToken = cts.Token;
 			try
 			{
-				var code = decompiler.DecompileTypeAsString(type.FullTypeName);
+				var tree = decompiler.DecompileType(type.FullTypeName);
+				var code = SyntaxTreeToString(tree);
 				typeCount++;
 				charCount += code.Length;
 				// Compiler-generated types (<Module>, <PrivateImplementationDetails>,
 				// VB$AnonymousType_*, ...) are still decompiled to shake out edge cases,
 				// but empty output is normal for them ('<' and '$' match the decompiler's
 				// own generated-name detection in SRMExtensions.IsGeneratedName).
-				if (string.IsNullOrWhiteSpace(code) && !type.Name.StartsWith('<') && !type.Name.Contains('$'))
+				bool generatedType = type.Name.StartsWith('<') || type.Name.Contains('$');
+				if (string.IsNullOrWhiteSpace(code) && !generatedType)
 					Report(pkg, name, type.FullTypeName.ToString(), new InvalidDataException("empty decompilation output"));
 				else if (dumpDir != null)
 					File.WriteAllText(Path.Combine(dumpDir, SanitizeFileName($"{pkg}.{type.FullTypeName}.cs")), code);
+				// An identifier may only contain letters, digits and '_' (the rule
+				// EscapeInvalidIdentifiers.IsValid applies for project output). Any other name in
+				// the tree is a compiler-generated entity the decompiler failed to fold away, and
+				// the output does not compile. Inside a generated type everything is mangled by
+				// definition, so only user-written types are checked.
+				if (!generatedType)
+				{
+					foreach (var leak in tree.DescendantsAndSelf.OfType<Identifier>()
+						.Select(i => i.Name)
+						.Where(n => !n.All(ch => char.IsLetterOrDigit(ch) || ch == '_'))
+						.Distinct())
+					{
+						Report(pkg, name, type.FullTypeName.ToString(), new LeakedName(leak));
+					}
+				}
 				// ILFunction warnings (unknown result types, stack type mismatches, invalid IL)
 				// surface in the output as "//IL_xxxx: <message>" comments.
 				foreach (var warning in Regex.Matches(code, @"//IL_[0-9a-fA-F]+: (.*)")
@@ -658,6 +678,13 @@ async Task DecompileAssembly(string pkg, string dllPath, List<string> searchDirs
 	}
 }
 
+string SyntaxTreeToString(SyntaxTree syntaxTree)
+{
+	var w = new StringWriter();
+	syntaxTree.AcceptVisitor(new CSharpOutputVisitor(w, formatting));
+	return w.ToString();
+}
+
 void ReportResolutions(LoggingResolver logResolver)
 {
 	var resolutions = logResolver.Resolutions;
@@ -1264,6 +1291,7 @@ void Report(string pkg, string asm, string type, Exception ex)
 	var kind = inner is AssertionFailedException ? "ASSERT"
 		: inner is TimeoutException ? "TIMEOUT"
 		: inner is DecompilerWarning ? "WARNING"
+		: inner is LeakedName ? "LEAK"
 		: inner is PdbFinding ? "PDB" : "EXCEPTION";
 	var key = $"{kind}|{inner.GetType().Name}|{inner.Message}|{topFrame}";
 	var location = $"{pkg} / {asm} / {type}";
@@ -1396,7 +1424,7 @@ static void WriteHtmlReport(string path, List<Finding> findings, int assemblies,
 		           color:#a8791f; border:1px solid #a8791f55; margin-left:6px; }
 		.ASSERT { border-left:4px solid #d97706; } .EXCEPTION { border-left:4px solid #dc2626; }
 		.TIMEOUT { border-left:4px solid #7c3aed; } .WARNING { border-left:4px solid #2563eb; }
-		.PDB { border-left:4px solid #0d9488; }
+		.PDB { border-left:4px solid #0d9488; } .LEAK { border-left:4px solid #db2777; }
 		#filter { width:100%; padding:8px; margin:8px 0; border:1px solid var(--line); border-radius:6px;
 		          background:var(--bg); color:var(--fg); font:13px ui-monospace,monospace; }
 		</style></head><body>
@@ -1407,7 +1435,7 @@ static void WriteHtmlReport(string path, List<Finding> findings, int assemblies,
 		+ $"({findings.Sum(f => f.Count)} total)"
 		+ (dumpDir != null ? $"<br>decompiled sources dumped to {Esc(dumpDir)}" : "") + "</div>");
 	html.AppendLine("<input id=filter placeholder='filter by message, type, package or frame'>");
-	foreach (var kind in new[] { "ASSERT", "EXCEPTION", "TIMEOUT", "WARNING" })
+	foreach (var kind in new[] { "ASSERT", "EXCEPTION", "TIMEOUT", "LEAK", "WARNING", "PDB" })
 	{
 		var group = findings.Where(f => f.Kind == kind).OrderByDescending(f => f.Count).ToList();
 		if (group.Count == 0)
@@ -1471,6 +1499,13 @@ class AssertionFailedException(string message) : Exception(message);
 
 class DecompilerWarning(string message) : Exception(message);
 
+// A compiler-generated name that reached the output. The message keeps only the shape of
+// the name - the part in angle brackets is the enclosing member and the digits are per
+// occurrence - so that one bucket collects every hit of the same unfolded construct.
+class LeakedName(string name)
+	: Exception("leaked compiler-generated name: "
+		+ Regex.Replace(Regex.Replace(name, "<[^<>]*>", "<>"), "[0-9]+", "N"));
+
 // A defect in a generated PDB. The message describes the shape of the defect and never the
 // instance that hit it - Report() dedupes on the message, so naming a method or an offset in it
 // would turn one bug into one finding per method.