Update MicrosoftCodeAnalysisVersion_LatestVS

[sbomer]

Similar to #123509, this updates the MicrosoftCodeAnalysisVersion_LatestVS to match what is included in the SDK from global.json.

It adds a new property that has the 10.0 version (from #123509).

[Copilot]

Pull request overview

This pull request updates the Microsoft.CodeAnalysis (Roslyn) version to match what's referenced in the .NET SDK 10.0.1xx, and addresses code analysis warnings introduced by the newer Roslyn version.

Changes:

• Updated MicrosoftCodeAnalysisVersion_LatestVS from 4.14.0 to 5.4.0-2.26060.102
• Added MicrosoftCodeAnalysisVersion_5_0 for VS 18.0/.NET SDK 10.0.1xx compatibility
• Suppressed IDE0071 warning for a necessary .ToString() call in TypeNameParser
• Removed unnecessary .ToString() calls in MLDsa cryptography implementations

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
eng/Versions.props	Updates Roslyn/Code Analysis versions to align with SDK 10.0.1xx and adds new version property for VS 18.0 compatibility
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs	Adds pragma warning disable/restore for IDE0071 around Debug.Assert with ReadOnlySpan.ToString()
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.Windows.cs	Removes unnecessary ToString() call on ReadOnlySpan in Debug.Fail message
src/libraries/Common/src/System/Security/Cryptography/MLDsaCng.Windows.cs	Removes unnecessary ToString() call on ReadOnlySpan in Debug.Fail message

[agocke]

This is probably superseded by #123527

[sbomer]

Yup, that should unblock flow. I'll leave this open for the style fixes and #123509 (comment).

[agocke]

Not sure we should take these style changes. Seem more like suggestions than real improvements.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

[stephentoub]

🤖 Copilot Code Review — PR #123514

Holistic Assessment

Motivation: The version update portion of this PR is justified — it aligns MicrosoftCodeAnalysisVersion_LatestVS with what's shipped in the 10.0.1xx SDK, addressing analyzer compatibility issues. However, the extensive style changes (~85 files) appear to be opportunistic cleanup triggered by newer analyzer rules, without independent justification.

Approach: The version bump takes the right approach. The style changes, while individually correct, represent a large-scale refactoring that creates git history noise and increases review burden without functional benefit.

Net positive/negative: Mixed. The version update is necessary and valuable, but combining it with 85 files of mechanical style changes makes review harder and is already raising maintainer concerns.

---

Detailed Findings

✅ Version Updates — Core changes are technically correct

The version updates in eng/Versions.props are appropriate:

• Adding MicrosoftCodeAnalysisVersion_5_0 with value 5.0.0-2.26070.104 follows the established pattern
• Updating MicrosoftCodeAnalysisVersion_LatestVS from 4.14.0 to 5.4.0-2.26060.102 aligns with the 10.0.1xx SDK
• The existing comments correctly explain compatibility requirements with VS Preview and SDK versions

Flagged by: Claude, GPT

---

⚠️ PR Scope — Large-scale style changes mixed with infrastructure fix

The PR mixes a necessary version update with ~85 files of mechanical style changes:

• Converts if (x != null) { x.Dispose(); x = null; } → x?.Dispose(); x = null;
• Converts if (x != null) x.Event -= handler; → x?.Event -= handler;
• Converts += 1 → ++
• Uses pattern matching if (x is T y) instead of if (x != null)
• Removes BOM from two files

While these changes are individually semantically correct, they:

• Create noise in git history for files unrelated to the core fix
• Increase review surface area unnecessarily
• Already prompted maintainer skepticism: "Not sure we should take these style changes. Seem more like suggestions than real improvements."

Recommendation: Consider splitting this into two PRs — one for the version update (essential) and one for the style cleanup (optional/deferrable).

Flagged by: Claude, GPT

---

⚠️ IDE0071 Warning Suppressions — May conflict with PR #123527

The PR adds #pragma warning disable IDE0071 around Debug.Fail calls in:

• MLDsaCng.Windows.cs
• MLDsaImplementation.Windows.cs
• TypeNameParser.cs

These suppressions are necessary because:

• The new Roslyn version flags .ToString() calls in string interpolation as redundant
• For netstandard2.0 targets, ReadOnlySpan<char> cannot be directly interpolated, requiring .ToString()

However, PR #123527 (already mentioned as superseding this PR) may handle this globally via .editorconfig. Adding per-file suppressions here could create duplicate/conflicting solutions.

Recommendation: Verify whether #123527's approach obviates these local suppressions.

Flagged by: Claude, GPT

---

✅ Null-conditional transformations — Semantically correct

All the if (x != null) { x.Method(); x = null; } → x?.Method(); x = null; transformations are semantically equivalent. The only difference is that x = null is now always executed even when x was already null, which is benign. Verified patterns include:

• Dispose patterns
• Event unsubscription
• Method calls like RemoveRef(), DangerousRelease(), etc.

Flagged by: Claude, GPT

---

✅ TargetRegistry.cs linked-list refactor — Correct transformation

The linked list removal refactoring:

// Before:
if (node.Previous != null) { node.Previous.Next = next; node.Previous = null; }
if (node.Next != null) { node.Next.Previous = previous; node.Next = null; }

// After:
previous?.Next = next; node.Previous = null;
next?.Previous = previous; node.Next = null;

This is semantically equivalent — the original code only set node.Previous = null when node.Previous != null (tautologically — if it's already null, setting it to null has no effect). The refactored version unconditionally nulls the fields, which is correct and slightly more efficient (fewer branches).

Flagged by: Claude, GPT

---

✅ XmlNode.cs pattern matching — Improves nullable flow analysis

// Before:
if (firstChildTextLikeNode != null) { firstChildTextLikeNode.Value = sb.ToString(); ... }

// After:
if (firstChildTextLikeNode is XmlNode node) { node.Value = sb.ToString(); ... }

This addresses nullable analysis by extracting to a non-null local variable node, avoiding potential warnings about firstChildTextLikeNode being dereferenced. This is a safe and desirable pattern.

Flagged by: Claude, GPT

---

✅ BatchBlock.cs changes — Standard pattern improvements

• Using if (_boundingState is BoundingState boundingState) instead of if (_boundingState != null) is idiomatic C# pattern matching
• Changing += 1 to ++ is accepted style in the repo

Both are no-op changes from a behavior perspective.

Flagged by: Claude, GPT

---

💡 BOM Removal — Correct but incidental

Two files had UTF-8 BOM removed (DsesFilterAndTransform.cs, TarEntry.cs). This is correct cleanup aligning with repository conventions, but unrelated to the stated PR purpose.

Flagged by: GPT

---

Summary

Needs Changes. The version update is sound and necessary, but this PR mixes essential infrastructure fixes with extensive style changes across 85+ files. Given that:

1. Maintainers have already expressed skepticism about the style changes
2. PR Update Roslyn to 5.0.0-2.26070.104 and suppress IDE0071/IDE0031 #123527 may supersede the core version update
3. The style changes add review burden without functional benefit

Recommended actions:

1. Verify whether Update Roslyn to 5.0.0-2.26070.104 and suppress IDE0071/IDE0031 #123527 addresses the version update — if so, this PR's core purpose is already handled
2. If the version update is still needed, consider splitting this PR to separate the essential fix from optional style cleanup
3. If the style changes are to be kept, they could be a follow-up PR with appropriate justification

Review synthesized from: Claude Sonnet 4, GPT-5.1