Remove early return in GetCanonicalForm, always call System.IO.Path

[OvesN]

Fixes #13514

Context

AbsolutePath.GetCanonicalForm() silently accepted invalid path characters that Path.GetFullPath would reject. An optimization branch skipped calling Path.GetFullPath when the path had no relative segments, separator issues, or consecutive separators — causing invalid characters to never be validated.

Changes Made

AbsolutePath.cs

• Removed the optimization branch in GetCanonicalForm() that skipped calling Path.GetFullPath for already-canonical-looking paths.
• Removed early return if value is null or empty.
• GetCanonicalForm() now always calls System.IO.Path.GetFullPath.

Testing

AbsolutePath_Tests.cs

• Added GetCanonicalForm_InvalidPathCharacters_ShouldThrowSameAsPathGetFullPath — verifies that GetCanonicalForm() throws the same exception type as Path.GetFullPath for paths containing invalid characters.

[Copilot]

Pull request overview

This PR fixes a behavioral inconsistency in Microsoft.Build.Framework.AbsolutePath.GetCanonicalForm() where some “already-canonical-looking” paths could bypass Path.GetFullPath and therefore fail to reject invalid path characters. The change aligns canonicalization behavior with the BCL so invalid paths are rejected consistently.

Changes:

• Removed the “skip Path.GetFullPath” optimization branch in AbsolutePath.GetCanonicalForm() so it always calls Path.GetFullPath.
• Updated XML docs to explicitly state invalid path characters are rejected.
• Added a unit test ensuring GetCanonicalForm() throws the same exception type as Path.GetFullPath for invalid characters.

Reviewed changes

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

File	Description
src/Framework/PathHelpers/AbsolutePath.cs	Ensures canonicalization always runs through Path.GetFullPath, fixing invalid-character validation gaps.
src/Framework.UnitTests/AbsolutePath_Tests.cs	Adds regression coverage verifying exception-type parity with Path.GetFullPath for invalid paths.

[github-actions]

Review Summary

This PR removes the ChangeWave.Wave18_6 fast-path optimization from GetCanonicalForm() and always delegates to Path.GetFullPath. The intent — simpler code and consistent BCL behavior including invalid-character rejection — is reasonable, but there's a notable performance concern.

Key Finding

Performance regression on hot paths (Medium priority): The removed optimization avoided Path.GetFullPath for already-canonical paths (which is the common case). GetCanonicalForm() is called per-item in Copy, FindUnderPath, and AssignTargetPath — all with explicit performance annotations. The AssignTargetPath source code itself contains a PERF NOTE warning that Path.GetFullPath() "consumes a lot of memory, esp. when we have a lot of items."

If the motivation is rejecting invalid path characters, consider augmenting the existing fast-path with an invalid-character check rather than removing the optimization entirely. This would preserve the performance characteristics while gaining the correctness improvement.

Minor Items

• Redundant type assertion in the test (line 313) — Should.Throw already checks type
• Manual try/catch could be simplified with Record.Exception

Dimensions with No Issues

ChangeWave discipline (reverting to simpler behavior is fine), backwards compatibility (no new warnings/errors for valid paths), cross-platform correctness, null safety, concurrency, naming, API surface — all clean.

Note

🔒 Integrity filter blocked 2 items

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

• #13532 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #13532 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 #13532 · ● 2.2M