Remove early return in GetCanonicalForm, always call System.IO.Path#13532
Conversation
OvesN
changed the title
There was a problem hiding this comment.
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 inAbsolutePath.GetCanonicalForm()so it always callsPath.GetFullPath. - Updated XML docs to explicitly state invalid path characters are rejected.
- Added a unit test ensuring
GetCanonicalForm()throws the same exception type asPath.GetFullPathfor 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[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.Throwalready 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 | noneGenerated by Expert Code Review (on open) for issue #13532 · ● 2.2M
OvesN
force-pushed
the
fix/remove-getcanonicalform-optimization
branch
2 times, most recently
from
0336eab to
1e6dec1
Compare
OvesN
changed the title
…ullPath The optimization in GetCanonicalForm() that skipped Path.GetFullPath for paths without relative segments, separator issues, or consecutive separators caused invalid path characters to be silently accepted. This broke tasks that relied on Path.GetFullPath throwing for invalid characters. Simplify GetCanonicalForm() to always call Path.GetFullPath, ensuring consistent behavior with the BCL including rejection of invalid path characters. Add test verifying GetCanonicalForm() throws the same exception as Path.GetFullPath for paths containing invalid characters (null character). Fixes dotnet#13514 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OvesN
force-pushed
the
fix/remove-getcanonicalform-optimization
branch
from
0d7dafd to
7cf86ab
Compare
OvesN
changed the title
Fixes #13514
Context
AbsolutePath.GetCanonicalForm()silently accepted invalid path characters thatPath.GetFullPathwould reject. An optimization branch skipped callingPath.GetFullPathwhen the path had no relative segments, separator issues, or consecutive separators — causing invalid characters to never be validated.Changes Made
AbsolutePath.csGetCanonicalForm()that skipped callingPath.GetFullPathfor already-canonical-looking paths.GetCanonicalForm()now always callsSystem.IO.Path.GetFullPath.Testing
AbsolutePath_Tests.csGetCanonicalForm_InvalidPathCharacters_ShouldThrowSameAsPathGetFullPath— verifies thatGetCanonicalForm()throws the same exception type asPath.GetFullPathfor paths containing invalid characters.