[teo-tsirpanis]

Context

Extracted from #12231.

The use of Microsoft.IO.Path.GetFullPath in .NET Framework is expected to improve performance by forgoing path validation, and improve consistency with modern .NET.

Changes Made

Removed FileUtilities.GetFullPath and replaced its uses with NewPath.GetFullPath. Also removed other newly dead code.

Testing

Existing testing coverage. Some test code under #if FEATURE_LEGACY_GETFULLPATH was removed.

Notes

[teo-tsirpanis]

This is ready for review. Let me know how you feel about this behavior change.

[OvesN]

I will run experimental VS insertion on this PR

[OvesN]

Insertion seems fine. https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/740089

@rainersigwald can you please take a look on this PR?

[rainersigwald]

In general, while I'd love to have the consistency, I'm concerned about removing/shifting errors in the VS MSBuild. Most of these paths weren't already moved because of the tests pinning the current early-failure behavior, which can be user-visible.

I'm not 100% on this, though--mostly it will shift the failure to a later spot (when the path is used rather than constructed). But I'm leery of introducing silent failures that would have been caught before.

[Copilot]

Pull request overview

This PR removes the FEATURE_LEGACY_GETFULLPATH code path and standardizes .NET Framework path normalization on Microsoft.IO.Path.GetFullPath (via NewPath.GetFullPath) to improve performance and align behavior with modern .NET, along with deleting newly-dead code and associated test conditionals.

Changes:

• Removed FEATURE_LEGACY_GETFULLPATH and the associated FileUtilities.GetFullPath implementation (and related UNC/security validation helpers).
• Switched key FileUtilities callers to NewPath.GetFullPath on .NET Framework.
• Cleaned up tests that depended on the legacy validation behavior and updated one evaluation test expectation.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Utilities.UnitTests/TrackedDependencies/TrackedDependenciesTests.cs	Removes tests that asserted warnings/exceptions for malformed tlog paths/rooting markers under the legacy behavior.
src/Framework/NativeMethods.cs	Removes the Windows interop-based GetFullPath helper previously used by the legacy implementation.
src/Framework/FileUtilities.cs	Replaces internal uses of the removed helper with NewPath.GetFullPath and deletes the legacy GetFullPath/UNC validation code.
src/Framework.UnitTests/FileUtilities_Tests.cs	Removes .NET Framework-only path validation tests and simplifies one conditional assertion tied to the legacy symbol.
src/Directory.BeforeCommon.targets	Stops defining FEATURE_LEGACY_GETFULLPATH.
src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs	Updates the expected logging behavior for invalid import project values (no longer expecting MSB4102 in one case).
src/Build.UnitTests/Evaluation/Expander_Tests.cs	Removes tests that were compiled only under FEATURE_LEGACY_GETFULLPATH.

[DustinCampbell]

What a lovely almost-completely-red change. 😄

@JeremyKuhne might want to have a look too, since he's the source of Microsoft.IO.Redist knowledge and is pretty deep in the MSBuild code base these days.

[ViktorHofer]

/review

[AlesProkop]

@JeremyKuhne Would you mind taking another look?

[github-actions]

✅ Expert Code Review (command) completed successfully!

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Expert review of PR #13769 completed by subagent. Review posted with 3 inline comments and a summary covering: test coverage gaps for removed tlog tests, the MSB4102 behavior change, the removed \?\globalroot security check, and a minor allocation regression note. No further action needed from this agent.

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Review: Remove FEATURE_LEGACY_GETFULLPATH

The core goal — eliminating the custom Win32 GetFullPathName wrapper and letting NewPath.GetFullPath() (= Microsoft.IO.Redist.Path on .NET FX, = System.IO.Path on .NET Core) handle path resolution — is correct and well-targeted. The Directory.BeforeCommon.targets cleanup and NativeMethods.txt/NativeMethods.cs deletions are clean.

✅ What's right

• NewPath type alias (FileUtilities.cs lines 22–25) is already established infrastructure; this PR simply extends its usage from two call sites to five, which is consistent.
• GetCurrentDirectory removal from NativeMethods.txt is safe — confirmed no callers in NativeMethods.cs after the GetFullPath P/Invoke was deleted.
• GetFullPathName removal is safe — the only caller was the deleted NativeMethods.GetFullPath().
• NormalizePathBadUNC* / NormalizePathInvalid removals in FileUtilities_Tests.cs are correct: these were WindowsFullFrameworkOnlyFact-gated tests whose comments explicitly stated ".NET Core 2.1+ no longer validates paths". Removing them is consistent with aligning .NET FX behavior to .NET Core.
• #if FEATURE_LEGACY_GETFULLPATH removal is complete — zero remaining references across the repo.

⚠️ Behavioral changes that need scrutiny

1. Tlog warning regression for invalid-path tlog names (see inline comment)

The deleted tests InvalidReadTLogName, InvalidWriteTLogName, and InvalidFlatTrackingTLogName each verified that a tlog item spec containing | (e.g., TestFiles\|one|.tlog) caused a Tracking_RebuildingDueToInvalidTLog warning and an empty DependencyTable. This warning came from FormatNormalizedTlogRootingMarker → NormalizePath → old GetFullPath() → Path.HasExtension() throwing ArgumentException for the pipe character. With NewPath.GetFullPath(), no exception is thrown, so no warning is emitted.

In practice this matters only if a build system generates tlog item specs with invalid path characters (impossible on Windows where | is not a valid filename character). But the warning code path is now completely untested — if a regression were introduced in the warning logic, nothing would catch it.

2. Malformed UNC path \\ no longer throws (see inline comment)

The FormatNormalizedRootingMarkerTests assertion that TaskItem("\\\\") throws ArgumentException was correct for the old path. With the new path it silently returns a deterministic string. This changes observable behavior: previously a build with a \\-spec tlog item would log Tracking_RebuildingDueToInvalidTLog; now it would silently rebuild without any diagnostic.

3. .NET Framework + | import path no longer produces MSB4102 (see inline comment)

This is the correct behavior alignment, but represents a real behavioral change on .NET Framework that's documented only via the assertion flip. A PR description note or code comment would help future maintainers.

🔍 Performance note

The deleted NativeMethods.GetFullPath() had a zero-allocation optimization: it used stackalloc char[MAX_PATH] + ReadOnlySpan<char> comparison to return the original path object when input and output are identical (common for already-normalized paths). Microsoft.IO.Redist.Path.GetFullPath() does not have this optimization, so this is a minor allocation regression on the hot path (every NormalizePath call on an already-fully-qualified path now allocates a new string). Given that NormalizePath is called during project evaluation, this could be measurable in large repos.

If allocation count matters here, the optimization could be re-introduced as a wrapper:

string fullPath = NewPath.GetFullPath(path);
if (fullPath.Equals(path, StringComparison.Ordinal)) fullPath = path; // avoid duplicate string

This is a suggestion, not a blocker.

Summary

Dimension	Finding
Correctness	✅ Core changes correct; behavior differences are intentional
Backward compat	⚠️ Three behavioral changes documented via test removal, not explicitly noted
Test coverage	⚠️ Removed tests not replaced; new behavior for malformed paths is untested
Security	❓ \\?\globalroot check removed; Microsoft.IO.Redist inherits .NET Core model
Performance	⚠️ Minor allocation regression vs. old stackalloc implementation
Dead code removal	✅ Clean, no leftover references

Overall: COMMENT — the changes are directionally correct but the PR would benefit from (a) replacement tests for the new behavior and (b) inline comments documenting the intentional .NET Framework behavior changes.

Generated by Expert Code Review (command) for #13769 · 619.2 AIC · ⊞ 31.9K
_{Comment /review to run again}

[JeremyKuhne]

While this changes behavior, it aligns with what the underlying BCL does. The BCL deliberately moved away from pre-emptive validation as there is no plausible way to know what a valid path is- there are many file system drivers involved in IO, sometimes more than one in one path (through mounting).

[rainersigwald]

Let's do it, but keep an ear out for weird failures that might be related.

Thanks @teo-tsirpanis!