Fix #3510: keep exporting a project when a member cannot be decompiled - #3971
Conversation
christophwille
left a comment
There was a problem hiding this comment.
Code review (high effort, multi-agent, Claude Code)
The PR's error-recovery machinery has structural gaps that defeat its own goal: RecordingErrors wraps an eager method so code-file failures still abort the whole export, CreateFile was hoisted out of the per-file try so one I/O failure kills the entire run, the catch handler writes to the possibly-failed writer, and the yield-break wrapper silently drops all resources after the first failure.
Additionally, the removed rethrow silently changes exit-code behavior for ilspycmd and PowerShell consumers, CSharpDecompiler.Errors accumulates across calls without a reset, and the UI exporter drops recovered errors when the export later throws. A pre-existing File.OpenWrite truncation bug and two minor cleanups (duplicated summary phrasing, a history-referencing comment) round out the findings.
10 findings (8 correctness, 2 cleanup) posted as inline comments; all were independently verified against the PR head.
Automated review run via Claude Code (claude-fable-5); findings verified by independent verifier agents.
christophwille
left a comment
There was a problem hiding this comment.
Re-review (high effort, multi-agent, Claude Code)
Re-reviewed at head 9874542 after the updates. The earlier round's structural issues (eager RecordingErrors wrap, CreateFile outside the per-file try, catch writing to the failed writer, yield-break dropping items) are addressed. This round surfaced 10 findings (8 correctness, 2 cleanup), all independently verified, posted as inline comments.
The most severe are correctness gaps in the recovery itself:
- the non-SDK .csproj still references source files whose creation failed (a broken deliverable the old abort never produced),
- the PowerShell cmdlets were never taught to read the new
Errorslists, so failures become fully silent, - one mid-iterator failure in
WriteMiscellaneousFilesInProjectsilently drops app.manifest/app.config, - recovered errors are lost from the UI report when the export later throws, and
- an unguarded writer
Disposein the finally block can re-introduce the whole-export abort on disk-full.
Remaining findings cover truncated files contradicting the "replaced by error text" message, RecordingErrors' unbounded retry against non-compiler-iterator overrides, a stderr/exit-code documentation contradiction in ilspycmd, lost regression coverage in the round-trip suite, and a duplicated per-error headline format.
Automated review run via Claude Code (claude-fable-5); findings verified by independent verifier agents.
7ce298e to
678ee32
Compare
One member the decompiler could not handle aborted the whole export, so a single unsupported method in a large assembly left the user with nothing: no sources, no .csproj, no way around it. Recovering silently would trade that for a worse outcome - broken output nobody knows is broken - so every failure is recorded, written where the content would have gone, and pointed at the issue tracker. The recovery has to hold for anything the export touches, not just method bodies: a file that cannot be created, a resource that cannot be decoded, an output visitor that throws mid-type. Each of those costs its own unit and nothing else, and the units behind a failure are still produced - dropping them would make the export look complete when it is not. Consumers that relied on the exception keep their failure signal: ilspycmd exits non-zero and lists the failures, the PowerShell cmdlets raise an error record per failure, and the round-trip suite asserts the export reported none - otherwise a crash on a method its own tests never call would ship green. Assisted-by: Claude:claude-opus-5[1m]:Claude Code
An export's ITextOutput goes nowhere: ProjectExporter and SolutionWriter both hand the language a throwaway PlainTextOutput and build their own status report, so a language writing failures into that output is invisible. The failures travel on DecompilationOptions instead and are rendered by the caller that owns the report - each one with its full exception in a collapsed fold, which is what makes a bug report actionable. Drive-by: WriteExceptionDetails split the exception text without trimming, so for exceptions rendering a trailing newline the fold reached one line past the last frame and swallowed the line behind it; and the tab's own decompilation- failure path had regressed to dumping a raw stack trace instead of using that helper. Assisted-by: Claude:claude-opus-5[1m]:Claude Code
678ee32 to
3b36efb
Compare
Problem
Save Code /
ilspycmd -paborted the entire export on the first member it could not decompile. One unsupported method in a large assembly left the user with nothing: no sources, no.csproj, and no way to work around it (#3510).What changed
CSharpDecompiler.DecompileBodyrecords the failure instead of throwing. The member keeps its signature and the exception takes the place of its body - a comment with the full trace and where to report it, in the same spot warnings about the code go. The remaining members decompile normally, and the failures are exposed asCSharpDecompiler.Errors(reset per decompilation).WholeProjectDecompilerapplies the same rule per file, per resource and around the assembly-info file, so an export always runs to completion. What it recovered from is onWholeProjectDecompiler.Errors.ilspycmd. TheITextOutputhanded to the language is discarded by both export callers, so the errors travel onDecompilationOptions.DecompilationErrorsand are rendered by the caller that owns the report.ErrorTolerantOutputVisitorkeeps a file well-formed when writing a member throws: the error takes that member's place, the braces it opened are closed, and the following members are still written.RoundtripAssemblyasserts the export reported none - otherwise a decompiler crash on a method the round-trip's own tests never call would ship green.ilspycmdkeeps its failure signal: it lists the failures on stderr and exits non-zero, as it did when the decompiler threw. The new--ignore-decompilation-errorsflag exits with success instead, for automation that wants the partial output to count as a success; the failures are still listed either way.Recovering silently would trade one bad outcome for a worse one, so the inline comment names
https://github.com/icsharpcode/ILSpy/issues/newand every summary repeats it.This also improves the text view: a single bad method used to replace the whole type with an error page; the type is now shown with the error in place of that one member.
Release notes
Drive-by fixes
ilspycmdwrote the.csprojthroughFile.OpenWrite, which does not truncate: re-exporting into the same directory could leave a shorter project file with trailing bytes from the previous run.SmartTextOutputExtensions.WriteExceptionDetailssplit the exception text without trimming, so for exceptions that render a trailing newline (DecompilerExceptiondoes) the fold reached one line past the last frame and a collapsed fold swallowed the line behind it. That affected the PDB generator and the assembly tree node too.DecompilerTabPageModel's decompilation-failure path dumped the raw stack trace into the text view instead of using that folded helper.Tests
DecompilationErrorRecoveryTests- a failing method body keeps the rest of the type, is recorded as an error, and does not carry over into the next decompilation.WholeProjectDecompilerTests.FailuresDoNotAbortTheExport- a failing decompile, two failing file creations (a type file and the assembly-info file) and a failing resource enumeration are all reported, and every other file is still written.WholeProjectDecompilerTests.OneFailingResourceDoesNotDropTheOthers- a resource that cannot be written costs that resource alone.ProjectExportTests.Export_Report_Names_The_Failures_And_Where_To_Report_Them- the export report names the failure and the report URL, and the export still counts as successful.DecompilationErrorRecoveryTests.FailingOutputKeepsTheFileWellFormed- a member whose output throws is replaced in place, the file closes every brace it opens, and the members after it are still written.SmartTextOutputExtensionsTests.Exception_Fold_Stops_At_The_Last_Frame- the fold does not extend past the last frame.Failures are injected with a throwing
IILTransform/IAstTransformand an overriddenCreateFile, so the tests do not depend on a decompiler bug that may get fixed.Decompiler suite green (3368 tests);
ilspycmdtests and the affected UI tests green.Written by an AI agent (Claude) on Siegfried's behalf.