Replace ErrorUtilities assertion methods with Assumed API and BCL throw helpers

[DustinCampbell]

Tip

This is a huge pull request made up of mostly mechanical changes. I recommend reviewing commit-by-commit to
keep your sanity! FYI that new internal APIs are in 245052f and 0eea982. Copilot will likely decline to review this because so many files are touched.

This PR introduces an internal Assumed assertion API and systematically replaces legacy ErrorUtilities
assertion/throw methods throughout the codebase with either Assumed methods or BCL throw helpers, as appropriate.

New infrastructure

• Assumed static class (src/Framework/Assumed.cs): Provides assertion methods (Null, NotNull,
  NotNullOrEmpty, True, False, Equal, NotEqual, comparison operators, range checks, and Unreachable) that
  throw InternalErrorException on failure. Each method has two overloads: one with CallerArgumentExpression for
  automatic error messages, and one with a conditional interpolated string handler to avoid formatting allocations on
  the success path.
• InternalError helper (src/Framework/InternalError.cs): Centralized throw sites including Throw<T>
  overloads returning T for use in expression contexts.
• ArgumentException.ThrowIfNullOrEmpty<T>(IReadOnlyCollection<T>) extension method for collection emptiness
  checks.

ErrorUtilities methods replaced

Old method	Replacement
VerifyThrowInternalNull	Assumed.NotNull
VerifyThrowInternalLength (string)	Assumed.NotNullOrEmpty
VerifyThrowInternalLength<T> (collection)	Assumed.NotNullOrEmpty<T>
VerifyThrow(bool, string)	Assumed.True / Assumed.False / more specific Assumed methods
VerifyThrowInternalErrorUnreachable	Assumed.True
ThrowInternalErrorUnreachable	Assumed.Unreachable / Assumed.Unreachable<T>
ThrowInternalError	InternalError.Throw
EscapeHatches.ThrowInternalError	InternalError.Throw
VerifyThrowArgumentNull	ArgumentNullException.ThrowIfNull
VerifyThrowArgumentLength (string)	ArgumentException.ThrowIfNullOrEmpty
VerifyThrowArgumentLength<T> (collection)	ArgumentException.ThrowIfNullOrEmpty<T>
VerifyThrowArgumentOutOfRange	ArgumentOutOfRangeException.ThrowIf*
VerifyThrowObjectDisposed	ObjectDisposedException.ThrowIf

All replaced methods have been removed from ErrorUtilities. Methods that still have callers (e.g.,
VerifyThrowArgument, VerifyThrowInvalidOperation) are preserved.

Refinements during migration

• VerifyThrow → specific Assumed methods: Rather than blindly mapping to Assumed.True, conditions were
  analyzed and mapped to the most descriptive method (NotNull, Zero, NotNullOrEmpty, Equal, Positive,
  PositiveOrZero, InRange, etc.).
• Mismatched parameter names fixed: Several VerifyThrowInternalNull calls had silently wrong parameter names
  (e.g., "child" for an importedProject variable). These are now either auto-captured via
  CallerArgumentExpression or explicitly corrected.
• InternalError.Throw<T> eliminates throw-then-return-default patterns, enabling conversion of
  multi-statement method bodies to expression-bodied members.
• Null check ordering fixed in BuildRequestEngine.IssueConfigurationRequest where a null check came after a
  member access.
• VerifyCollectionCopyToArguments refactored: parameter type changed from T[] to ICollection<T>,
  reordered for CallerArgumentExpression, and resource string name typo fixed.

Scope

• 359 files changed across src/Build, src/Framework, src/Tasks, src/Utilities, and src/Shared
• MSBuildTaskHost is untouched
• No behavioral changes — all replacements throw the same exception types with equivalent messages

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[github-actions]

Expert Review Summary — PR #13790: Assumed Internal Assertion API

Overall Assessment

This is a well-executed, large-scale refactoring that introduces a modern internal assertion API (Assumed + InternalError) and systematically replaces the legacy ErrorUtilities.VerifyThrow* / ThrowInternalError pattern across ~359 files. The API design is solid and the mechanical substitutions are semantically correct.

---

✅ Strengths

Dimension	Assessment
API Surface	✅ Both Assumed and InternalError are correctly internal static — no public API impact
Performance	✅ Conditional interpolated string handlers avoid allocation on happy paths. [NoInlining] on throw paths keeps callers small
Test Coverage	✅ Assumed_Tests.cs (719 lines) comprehensively tests all methods, both overloads, positive/negative paths, and default message expression capture
Correctness	✅ Mechanical substitutions preserve semantics. ArgumentNullException.ThrowIfNull correctly replaces VerifyThrowArgumentNull with identical [CallerArgumentExpression] behavior
Backwards Compatibility	✅ No public behavior changes. Exception types remain the same (InternalErrorException, ArgumentNullException, etc.)
Build Infrastructure	✅ Files placed correctly in Framework project; accessible via InternalsVisibleTo to all consuming assemblies
Code Simplification	✅ InternalError.Throw<T> eliminates dead-code returns after unconditional throws
Scope	✅ MSBuildTaskHost (net35) is correctly excluded from the migration

---

⚠️ Concerns (Non-Blocking)

1. Naming: Assumed reads as a past participle. Assume (imperative) would be more conventional at call sites (Assume.NotNull(x) vs Assumed.NotNull(x)). Subjective — not blocking.

2. Interpolated string overload resolution: When callers pass $"..." as the 3rd argument to methods like Assumed.Equal(a, b, $"message {x}"), it's critical that the compiler resolves to the handler overload (zero-allocation) rather than the string? message overload (eager allocation). The API signature design should ensure this via overload resolution rules, but it would be worth a quick IL verification for a representative case.

3. Null-conditional chains with [NotNull]: Assumed.NotNull(instance?.RequestEntry?.Request) doesn't narrow instance itself — only the overall expression. In #nullable disable contexts this is fine, but could produce warnings if nullable is ever enabled.

4. PR size: 359 files across 17 commits is large for a single PR. The commit history is well-structured (each commit covers one type of replacement), which helps reviewability. However, if any single substitution has a subtle bug, it could be hard to catch in this volume.

---

🔍 Dimensions Verified (No Issues Found)

• ChangeWave: Not needed — no behavioral change to user builds
• String comparisons: No MSBuild name comparisons affected
• Cross-platform: No platform-specific issues introduced
• Binary log compatibility: No format changes
• Concurrency: No threading changes
• Security: N/A
• Dependencies: Using directives correctly updated

---

Verdict

COMMENT — This is a clean, well-designed refactoring. The concerns above are minor and non-blocking. The API design with conditional interpolated string handlers is particularly well done for a performance-critical codebase.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #13790 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #13790 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Expert Code Review (on open) for issue #13790 · ● 40.2M

[rainersigwald]

I am extremely on board with the interpolated-string handlers, but I am less sure about the renaming/recentralization changes. Can you pitch me on Assumed.True over VerifyThrow, and related? I know I'm poisoned by long exposure to this codebase but . . .

[DustinCampbell]

I am extremely on board with the interpolated-string handlers, but I am less sure about the renaming/recentralization changes. Can you pitch me on Assumed.True over VerifyThrow, and related? I know I'm poisoned by long exposure to this codebase but . . .

I've been trying to tease ErrorUtilities apart, since it's a bit of a web of functions that throw various exceptions with localized messages, or throw InternalErrorExceptions with non-localized messages. The method names on ErrorUtilities are built from a working vocabulary: "Verify" for assertions, always include the word "Throw", add "Internal" for InternalErrorException, etc. However, they're a bit of a mouthful and the important bits end up at the end of the method name (e.g. "Null", "Length", etc.)

First, I replaced the "Verify" functions for argument exceptions with the BCL equivalents added in modern .NET, which are polyfilled in MSBuild. Those methods, like ArgumentNullException.ThrowIfNull(...) don't use the "Verify" vocabulary, so the code base was already gravitating away from that terminology a bit.

Then, I considered all of the "VerifyThrowInternal" methods and noted that they're all for internal assertions. At first, I considered adding InternalErrorException.ThrowIf*** methods, but then I noted that many methods had comments stating that they're intended to be used when a "Debug.Assert" might be used. So, I decided on a different approach and created a separate internal API for assertions. Since most methods perform verification, it's unnecessary to have "Verify" at the start of each method name, so I baked that concept into the class name: Assumed (inspired by xUnit's Assert and Microsoft.VIsualStudio.Validation's Assumes).

Assumed ends up being much briefer, and to my eye at least, is clearer about what's meant. I much prefer this:

Assumed.NotNull(projectCache);

...over this:

ErrorUtilities.VerifyThrowInternalNull(projectChange);

In addition, Assumed is easy to add more assertion APIs on. Here's the full list of members:

Assumed.Null<T>(T?);
Assumed.NotNull<T>(T?);
Assumed.NotNullOrEmpty(string?);
Assumed.NotNullOrEmpty<T>(IReadOnlyCollection<T>?);
Assumed.True(bool);
Assumed.False(bool);
Assumed.Equal<T>(T, T);
Assumed.Equal(string?, string?, StringComparison);
Assumed.NotEqual<T>(T, T);
Assumed.NotEqual(string?, string?, StringComparison);
Assumed.GreaterThan<T>(T, T) where T : IComparable<T>;
Assumed.GreaterThanOrEqual<T>(T, T) where T : IComparable<T>;
Assumed.LessThan<T>(T, T) where T : IComparable<T>;
Assumed.LessThanOrEqual<T>(T, T) where T : IComparable<T>;
Assumed.Zero(int);
Assumed.Negative(int);
Assumed.NegativeOrZero(int);
Assumed.Positive(int);
Assumed.PositiveOrZero(int);
Assumed.InRange<T>(T, T, T) where T : IComparable<T>;
Assumed.Unreachable();
Assumed.Unreachable<T>();

These align (mostly) with the "ThrowIf***" methods on ArgumentNullException, ArgumentException, and ArgumentOutOfRangeException, and all throw InternalErrorExceptions.

Anyway, I hope that explains how I arrived at this.

[OvesN]

I will run the experimental insertion to VS on this PR

[rainersigwald]

Talked it over with the team and we’re ok with the change. I’m on point for the first review

[OvesN]

Triaged as a low-priority code-quality refactor. Plan: light review, kick off an experimental VS insertion, and if green — merge.