Remove `FEATURE_LEGACY_GETFULLPATH` and use `Microsoft.IO.Path.GetFullPath` in .NET Framework. by teo-tsirpanis · Pull Request #13769 · dotnet/msbuild

teo-tsirpanis · 2026-05-15T02:25:10Z

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

…lPath` in .NET Framework. The use of the new `Path.GetFullPath` in .NET Framework This is expected to improve performance by forgoing path validation, and improve consistency with modern .NET.

teo-tsirpanis · 2026-05-16T02:20:42Z

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

OvesN · 2026-05-19T13:49:57Z

I will run experimental VS insertion on this PR

OvesN · 2026-05-20T09:56:22Z

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.

rainersigwald · 2026-05-20T13:53:44Z


-#if FEATURE_WINDOWSINTEROP
-    [SupportedOSPlatform("windows6.1")]
-    internal static unsafe string GetFullPath(string path)


@JeremyKuhne since you both fixed this and did Microsoft.IO.Redist -- what do you think of this change?

I like simplicity and consistency. There is a bit more logic in the .NET Framework implementation than what is here, but it isn't that much more expensive. The biggest hit was all of the pre-parsing of the path for correctness. While nice in theory, it was super expensive and fundamentally wrong. NTFS is not the only file system that shows up on a path in Windows. :)

@teo-tsirpanis We should double check that .NET Framework scenarios aren't opting back into legacy path handling. https://learn.microsoft.com/dotnet/framework/migration-guide/mitigation-path-normalization I presume devenv isn't in compat mode, but I haven't checked.

devenv.exe.config has not enabled Switch.System.IO.UseLegacyPathHandling.

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`.

@@ -378,34 +366,6 @@ public void EmptyTLog()
            Assert.Equal(outofdate[0].ItemSpec, Path.Combine("TestFiles", "one.cpp"));
        }



@@ -537,21 +497,6 @@ public void ReadTLogWithDuplicateInRoot()
            Assert.NotEmpty(d.DependencyTable); // "Dependency Table should not be empty."
        }



@@ -2970,27 +2915,6 @@ public void SaveCompactedReadTlog_MaintainCompositeRootingMarkers()
            Assert.True(d2.DependencyTable[Path.GetFullPath(Path.Combine("TestFiles", "three.cpp")) + "|" + Path.GetFullPath(Path.Combine("TestFiles", "two.cpp"))].Values.Count == 4);
        }



DustinCampbell · 2026-06-23T16:08:57Z

    <DefineConstants>$(DefineConstants);FEATURE_HANDLEPROCESSCORRUPTEDSTATEEXCEPTIONS</DefineConstants>
    <DefineConstants>$(DefineConstants);FEATURE_INSTALLED_MSBUILD</DefineConstants>
-    <!-- Path.GetFullPath The pre .Net 4.6.2 implementation of Path.GetFullPath is slow and creates strings in its work. -->
-    <DefineConstants>$(DefineConstants);FEATURE_LEGACY_GETFULLPATH</DefineConstants>


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.

teo-tsirpanis added 2 commits May 15, 2026 05:22

Remove FEATURE_LEGACY_GETFULLPATH and use `Microsoft.IO.Path.GetFul…

24ba005

…lPath` in .NET Framework. The use of the new `Path.GetFullPath` in .NET Framework This is expected to improve performance by forgoing path validation, and improve consistency with modern .NET.

Update tests to account for new behavior.

505d24e

teo-tsirpanis marked this pull request as ready for review May 16, 2026 02:20

github-actions Bot mentioned this pull request May 18, 2026

Remove web project configuration check #12785

Closed

OvesN self-assigned this May 19, 2026

OvesN added the wait-for-exp-insertion label May 19, 2026

OvesN removed the wait-for-exp-insertion label May 20, 2026

OvesN requested a review from rainersigwald May 20, 2026 09:56

OvesN assigned rainersigwald May 20, 2026

rainersigwald reviewed May 20, 2026

View reviewed changes

github-actions Bot mentioned this pull request Jun 1, 2026

Check the unit test fail or success without code changes [DO NOT MERGE] #12839

Closed

Merge branch 'main' into newpath-getfullname

fc6e6c0

Copilot AI review requested due to automatic review settings June 23, 2026 11:13

Copilot started reviewing on behalf of teo-tsirpanis June 23, 2026 11:13 View session

Remove unused P/Invoke definitions.

6dcdebb

Copilot AI reviewed Jun 23, 2026

View reviewed changes

DustinCampbell reviewed Jun 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove `FEATURE_LEGACY_GETFULLPATH` and use `Microsoft.IO.Path.GetFullPath` in .NET Framework.#13769

Remove `FEATURE_LEGACY_GETFULLPATH` and use `Microsoft.IO.Path.GetFullPath` in .NET Framework.#13769
teo-tsirpanis wants to merge 4 commits into
dotnet:mainfrom
teo-tsirpanis:newpath-getfullname

teo-tsirpanis commented May 15, 2026 •

edited

Loading

Uh oh!

teo-tsirpanis commented May 16, 2026

Uh oh!

OvesN commented May 19, 2026

Uh oh!

OvesN commented May 20, 2026

Uh oh!

rainersigwald left a comment

Uh oh!

rainersigwald May 20, 2026

Uh oh!

JeremyKuhne Jun 23, 2026

Uh oh!

teo-tsirpanis Jun 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

DustinCampbell Jun 23, 2026

Uh oh!

DustinCampbell left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		@@ -378,34 +366,6 @@ public void EmptyTLog()
		Assert.Equal(outofdate[0].ItemSpec, Path.Combine("TestFiles", "one.cpp"));
		}

		@@ -537,21 +497,6 @@ public void ReadTLogWithDuplicateInRoot()
		Assert.NotEmpty(d.DependencyTable); // "Dependency Table should not be empty."
		}

		@@ -2970,27 +2915,6 @@ public void SaveCompactedReadTlog_MaintainCompositeRootingMarkers()
		Assert.True(d2.DependencyTable[Path.GetFullPath(Path.Combine("TestFiles", "three.cpp")) + "\|" + Path.GetFullPath(Path.Combine("TestFiles", "two.cpp"))].Values.Count == 4);
		}

Uh oh!

Conversation

teo-tsirpanis commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

teo-tsirpanis commented May 16, 2026

Uh oh!

OvesN commented May 19, 2026

Uh oh!

OvesN commented May 20, 2026

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

rainersigwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

DustinCampbell Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

teo-tsirpanis commented May 15, 2026 •

edited

Loading