Reduce build warnings: lift IDisposable, fix CS9336 precedence bug, dispose evicted assemblies#3732
Merged
siegfriedpammer merged 6 commits intoicsharpcode:masterfrom May 8, 2026
Conversation
5dd76e3 to
ae1ce27
Compare
MetadataFile now declares IDisposable using the canonical pattern (public non-virtual Dispose() + protected virtual Dispose(bool)). PEFile and WebCilFile become sealed and override Dispose(bool) to release the PEReader and MemoryMappedViewAccessor they own; ResourcesFile is also sealed. PortableDebugInfoProvider disposes the MetadataReaderProvider it owns. LoadedAssembly implements IDisposable and disposes both the loaded MetadataFile and the debug-info provider. AssemblyList.Unload / Clear / ReloadAssembly / HotReplaceAssembly now dispose the LoadedAssembly instances they evict, fixing a resource leak where every "Reload Assembly" held the previous PEReader (and the underlying file handle / memory-mapped view) alive until GC eventually finalized it. The disposal contract terminates at the AssemblyList tier: downstream holders of MetadataFile (MetadataModule, DecompilerTypeSystem, AssemblyListSnapshot, ...) hold borrowed references rather than owned ones, so making the base IDisposable does not cascade into CA1001 / CA2213 warnings elsewhere. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four cases where the analyzer rule conflicts with intentional design: * EmptyList<T>.IDisposable.Dispose (CA1063) — explicit IDisposable on IEnumerator<T>; making it public would conflict with the rest of the IList<T> / IEnumerator<T> surface. * MetadataFile.SectionHeaders (CA1065) — throw documents that this MetadataFileKind has no PE sections; PE-like derived kinds override. * LongSet.GetHashCode + LongSet itself (CA1065 + CA2231) — explicit guards against using LongSet in hash containers / via equality operators; SetEquals is the supported comparison and IEquatable<LongSet>.Equals is itself [Obsolete]. * AnnotationList.Clone (CA2002) — AnnotationList is a private nested type; the surrounding Annotatable class deliberately locks on the AnnotationList instance to serialize annotation reads/writes, and external code cannot obtain a reference to it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ExtensionDeclaration.SymbolKind (CA1065) — was throwing
NotImplementedException; return SymbolKind.TypeDefinition to match
TypeDeclaration / DelegateDeclaration, since `extension` declarations
are type-level.
* CustomAttribute.DecodeValue (CA2002) — replace `lock(this)` on the
sealed-but-internal class with a private syncRoot field.
* PlainTextOutput (CA1001) — implement IDisposable; track an
ownsWriter flag so we only dispose the underlying TextWriter when
the parameterless constructor created its own StringWriter.
* DotNetCorePathFinder (CA1060) — move the libc realpath / free
PInvokes into a private nested NativeMethods class.
* ILSpy.ReadyToRun (CA1016) — add [assembly: AssemblyVersion("1.0.0.0")]
in Properties/AssemblyInfo.cs to match the BamlDecompiler plugin.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The C# 9 IntPtr / UIntPtr guard in IsBinaryCompatibleWithType read
type.Kind is not TypeKind.NInt or TypeKind.NUInt
which parses as `(is not NInt) or (is NUInt)` — true unless
Kind == NInt. The intent (per the surrounding comment "but not nint
or C# 11 IntPtr") is "Kind is neither NInt nor NUInt", which needs
parentheses around the alternation:
type.Kind is not (TypeKind.NInt or TypeKind.NUInt)
Effect: when Kind == NUInt the branch no longer mistakenly applies
the C# 9 IntPtr-only restrictions (suppressing compound assignment
without nint, disallowing shifts).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ResXResourceWriter is a verbatim port of the Mono implementation (see file header). Both warnings flag deliberate decisions in the upstream port that we preserve for fidelity: * CA1063 — Dispose() is virtual and the protected Dispose(bool) is not, the inverse of the canonical pattern; keeping the Mono shape. * CA2213 — the writer's stream / textwriter fields are caller-owned and intentionally not disposed by the writer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ae1ce27 to
fd6a81e
Compare
The helper used to catch only FormatException, but the WPF type converters (e.g. RectConverter for the saved window placement) raise InvalidOperationException on malformed input — for instance, an empty string from a partially-written config file. That was enough to propagate up through SessionSettings.LoadFromXml and crash startup before the main window could even open, with no recovery path other than manually editing the on-disk ILSpy.xml. Treat any conversion failure as "use the default" instead, and treat empty strings the same as null at the entry. Effect: a single bad saved value falls back silently and the application starts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Walks through the build-warning noise in the solution and fixes / suppresses each one with a recorded justification, plus a couple of related findings the warnings exposed. Net effect: all 42 code-level CA + CS warnings silenced (only NU1901 NuGet-vuln advisories and one MSB3277 binding-conflict in the PowerShell project remain — both project-config concerns).
What's in the six commits
Lift IDisposable to MetadataFile and dispose evicted assemblies. The base class now declares
IDisposablewith the canonical pattern;PEFile/WebCilFileare sealed and overrideDispose(bool)to release their ownedPEReader/MemoryMappedViewAccessor;LoadedAssemblybecomesIDisposableand disposes both theMetadataFileand thePortableDebugInfoProviderit owns;AssemblyList.Unload/Clear/ReloadAssembly/HotReplaceAssemblynow dispose evicted assemblies. Side benefit: fixes a real resource leak — every "Reload Assembly" was previously holding the previousPEReader(and its file / MMF handles) open until GC eventually finalized the chain.Annotate intentional analyzer-violations with
[SuppressMessage]. Four cases where the analyzer rule conflicts with intentional design (EmptyList<T>explicit IDisposable,MetadataFile.SectionHeadersdocumenting "no PE sections",LongSetexplicit hash/equality guards,AnnotationListprivate-nested lock target).Fix five small analyzer warnings.
ExtensionDeclaration.SymbolKindreturnsTypeDefinition(wasNotImplementedException);CustomAttributeswitches to a privatesyncRoot;PlainTextOutputimplementsIDisposablewith anownsWriterflag;DotNetCorePathFinderPInvokes nested into aNativeMethodsclass;ILSpy.ReadyToRungets[AssemblyVersion("1.0.0.0")].Fix CS9336 precedence bug in
CompoundAssignmentInstruction. The C# 9 IntPtr/UIntPtr guard readtype.Kind is not TypeKind.NInt or TypeKind.NUInt, which parses as(is not NInt) or (is NUInt)— true for everything exceptNInt. The intent (per the comment "but not nint or C# 11 IntPtr") needed parens:is not (NInt or NUInt). Effect: whenKind == NUIntthe branch no longer mistakenly applies the C# 9 IntPtr-only restrictions.Suppress CA1063 / CA2213 on the ported
ResXResourceWriter. Verbatim port of the Mono implementation (per file header); the deviations from the canonical Dispose pattern are intentional fidelity to upstream.Make
SessionSettings.FromStringrobust against malformed saved values. Was catching onlyFormatException, but the WPF type converters (e.g.RectConverterfor window placement) throwInvalidOperationExceptionon malformed input. A partially-written config file would crash startup before the main window could open, with no recovery other than manually editingILSpy.xml. Now any conversion failure (and empty input) falls back to the default.Warning delta
Test plan
dotnet build ILSpy.sln -c Debug— 0 errors, 0 code-level warnings remainICSharpCode.Decompiler.Testssuite — passes (3284 / 0 / 15 skipped) on the original branch base; no compiler-touching changes sinceSessionSettingsfix exercised against a config file that previously crashed at startupNotes
type.Kind == NUInt. The new code correctly excludes bothNIntandNUInt. All compound-assignment tests pass.LoadedAssemblybecomingIDisposabledid not trigger a CA1001 cascade in downstream consumers —MetadataModule,DecompilerTypeSystem,AssemblyListSnapshotetc. hold borrowed references, not owned ones, so the disposal contract terminates cleanly at theAssemblyListtier.ICSharpCode.Decompiler.PowerShell). Both are project-config concerns and would be cleanest as separate PRs.🤖 Generated with Claude Code