Improve exception message for empty assembly loading#124301
Improve exception message for empty assembly loading#124301
Conversation
- Added new resource string BadImageFormat_EmptyAssembly - Updated Assembly.Load and AssemblyLoadContext.LoadFromStream to use the new message - Updated tests to verify the new error message contains "empty" Co-authored-by: elinor-fung <47805090+elinor-fung@users.noreply.github.com>
|
@copilot please invoke the code-review skill and post the analysis/results as a comment on this PR |
🤖 Copilot Code Review — PR #124517Holistic AssessmentMotivation: ✅ Justified. Loading empty assemblies (0-byte streams/arrays) is a real scenario that users encounter when working with dynamically generated assemblies, and the current generic "Bad IL format" message provides no actionable guidance. The linked Roslyn issue confirms this is a genuine pain point. Approach: ✅ Sound. The fix is surgical and targeted—it introduces a new, specific resource string for the empty assembly case while preserving the generic message for other BadImageFormat scenarios. The change is behavior-preserving (exception type and throwing conditions unchanged) and only improves diagnostic quality. Summary: ✅ LGTM. This is a focused, low-risk improvement that makes an error message more actionable. The code changes are minimal, the tests validate the new behavior appropriately, and the approach follows established patterns for error message improvements in the codebase. Detailed Findings✅ Core Implementation — Correctly ScopedThe change appropriately:
This is the correct approach—specific messages for diagnosable cases, generic messages for others. ✅ Test Quality — Appropriate Validation StrategyThe test assertions use
Both empty-byte-array and empty-stream code paths are now covered by tests. 💡 Message Wording — Minor Clarity ConsiderationThe message reads: "Assembly image is empty. The stream or byte array must contain a valid PE file." The second sentence could be read as "the problem is the PE file isn't valid" rather than "the problem is there's no data at all." Consider whether a slight rephrasing would be clearer, e.g.:
However, this is a minor point—the current message is substantially better than "Bad IL format" and clearly mentions "empty," which is the key diagnostic information. This is purely a suggestion for future refinement if desired. ✅ Cross-Cutting Consistency — Other Load Paths ConsideredI verified whether file-based loading paths ( Verdict: This is a well-scoped, low-risk improvement that makes a common error scenario easier to diagnose. No blocking issues identified. |
There was a problem hiding this comment.
Pull request overview
This PR improves the exception message when attempting to load an empty assembly (0-length byte array or stream). Previously, users received the generic "Bad IL format." message, which didn't provide actionable information for a common scenario when working with dynamically generated assemblies.
Changes:
- Added a new resource string
BadImageFormat_EmptyAssemblywith a descriptive error message - Updated
Assembly.Load(byte[])andAssemblyLoadContext.LoadFromStream()to throwBadImageFormatExceptionwith the specific message when input is empty - Added test coverage to verify the improved error message contains "empty"
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/Resources/Strings.resx | Added new resource string BadImageFormat_EmptyAssembly with descriptive message for empty assembly input |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs | Updated Assembly.Load(byte[]) to use the new specific error message for empty byte arrays |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs | Updated LoadFromStream() to use the new specific error message for empty streams |
| src/libraries/System.Runtime/tests/System.Reflection.Tests/AssemblyTests.cs | Updated test to verify the new error message contains "empty" |
| src/libraries/System.Runtime/tests/System.Reflection.Tests/CoreCLR/AssemblyTests.cs | Added new test to verify empty stream throws with correct error message |
Description
Loading an empty assembly (0-length byte array or stream) throws
BadImageFormatExceptionwith the generic message "Bad IL format." This is a common scenario with bugs in dynamically generated assemblies and provides no actionable information.Changes
Before:
After:
Implementation
BadImageFormat_EmptyAssemblyresource stringAssembly.Load(byte[])andAssemblyLoadContext.LoadFromStream()to use specific message when input is emptyThe change is scoped to explicit empty checks in managed code. Native code paths (
AssemblyNative_LoadFromStream) already assert non-empty input via managed validation.cc @dotnet/appmodel @AaronRobinsonMSFT
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.