Clean-up unnecessary compatibility polyfills and feature definitions.#12231
Open
teo-tsirpanis wants to merge 18 commits into
Open
Clean-up unnecessary compatibility polyfills and feature definitions.#12231teo-tsirpanis wants to merge 18 commits into
teo-tsirpanis wants to merge 18 commits into
Conversation
teo-tsirpanis
force-pushed
the
rm-compat-code
branch
4 times, most recently
from
3085937 to
e030e20
Compare
This comment was marked as resolved.
This comment was marked as resolved.
teo-tsirpanis
force-pushed
the
rm-compat-code
branch
from
3134c2b to
c73a5f7
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up unnecessary compatibility polyfills and feature definitions that were previously required for earlier .NET Core and .NET Standard versions but are no longer needed since MSBuild now targets only modern .NET frameworks.
- Removes calls to
Type.GetTypeInfo()and replaces them with directTypeproperty access - Removes Assembly polyfills and the
FEATURE_CULTUREINFO_GETCULTURESandFEATURE_GET_COMMANDLINEfeature flags - Simplifies code by using native framework APIs directly instead of compatibility wrappers
Reviewed Changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Utilities/AssemblyResources.cs | Remove GetTypeInfo() calls for ResourceManager assembly access |
| src/Utilities.UnitTests/ToolLocationHelper_Tests.cs | Remove GetTypeInfo() calls for Module.FullyQualifiedName access |
| src/Utilities.UnitTests/MockTask.cs | Remove GetTypeInfo() call for ResourceManager assembly access |
| src/UnitTests.Shared/MockLogger.cs | Remove GetTypeInfo() call for Assembly access in resource manager |
| src/Tasks/RoslynCodeTaskFactory/RoslynCodeTaskFactory.cs | Remove GetTypeInfo() call for assembly directory path calculation |
| src/Tasks/CultureInfoCache.cs | Remove FEATURE_CULTUREINFO_GETCULTURES feature flag and hardcoded culture names list |
| src/Tasks/AssemblyResources.cs | Remove GetTypeInfo() calls for ResourceManager assembly access |
| src/Shared/UnitTests/TypeLoader_Tests.cs | Remove FEATURE_ASSEMBLY_LOCATION feature flag and add assembly load context checks |
| src/Shared/Tracing.cs | Remove GetTypeInfo() call for assembly name access |
| src/Shared/TaskParameterTypeVerifier.cs | Remove GetTypeInfo() calls for type property checks |
| src/Shared/TaskParameter.cs | Remove GetTypeInfo() calls for type assignability checks |
| src/Shared/TaskLoader.cs | Remove GetTypeInfo() calls for task class validation |
| src/Shared/LoadedType.cs | Remove GetTypeInfo() calls for attribute checks |
| src/Shared/FileUtilities.cs | Remove GetTypeInfo() call for executing assembly path |
| src/Shared/CoreCLRAssemblyLoader.cs | Add well-known assembly name check for load optimization |
| src/Shared/CommunicationsUtilities.cs | Remove GetTypeInfo() call for CLR version check |
| src/Shared/BuildEnvironmentHelper.cs | Simplify process path detection by removing Assembly utility wrapper |
| src/Samples/PortableTask/ShowItems.cs | Remove GetTypeInfo() call for core assembly access |
| src/MSBuild/XMake.cs | Remove FEATURE_GET_COMMANDLINE feature flag and command line parameter handling |
| src/MSBuild/OutOfProcTaskHostNode.cs | Remove GetTypeInfo() call for serialization check |
| src/MSBuild/MSBuildClientApp.cs | Remove FEATURE_GET_COMMANDLINE feature flag conditionals |
| src/MSBuild/AssemblyResources.cs | Remove GetTypeInfo() calls for ResourceManager assembly access |
| src/MSBuild.UnitTests/XMake_Tests.cs | Remove FEATURE_GET_COMMANDLINE conditionals in test methods |
| src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs | Remove FEATURE_GET_COMMANDLINE conditionals in test execution |
| src/Framework/TaskPropertyInfo.cs | Remove GetTypeInfo() call for value type check |
| src/Framework/NativeMethods.cs | Remove GetTypeInfo() call for framework path detection |
| src/Framework/AssemblyUtilities.cs | Remove all polyfill methods and compatibility code |
| src/Framework.UnitTests/Attribute_Tests.cs | Remove GetTypeInfo() call for custom attribute access |
| src/Directory.BeforeCommon.targets | Remove FEATURE_ASSEMBLY_LOCATION, FEATURE_CULTUREINFO_GETCULTURES, and FEATURE_GET_COMMANDLINE defines |
| src/BuildCheck.UnitTests/EditorConfig_Tests.cs | Remove GetTypeInfo() calls for type checks |
| src/Build/Resources/MSBuildAssemblyFileVersion.cs | Remove GetTypeInfo() call for assembly attribute access |
| src/Build/Resources/AssemblyResources.cs | Remove GetTypeInfo() calls for ResourceManager assembly access |
| src/Build/Logging/LoggerDescription.cs | Remove GetTypeInfo() calls for logger class validation |
| src/Build/Instance/TaskRegistry.cs | Remove GetTypeInfo() calls for task factory class validation |
| src/Build/Instance/TaskFactoryLoggingHost.cs | Remove GetTypeInfo() call for serialization check |
| src/Build/Instance/TaskFactories/TaskHostTask.cs | Remove GetTypeInfo() calls for assembly location access |
| src/Build/Evaluation/Expander.cs | Remove GetTypeInfo() call for enum type check |
| src/Build/Definition/ProjectCollection.cs | Remove GetTypeInfo() call for assembly version access |
| src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | Remove GetTypeInfo() calls for assembly access |
| src/Build/BackEnd/Node/ServerNodeBuildCommand.cs | Remove FEATURE_GET_COMMANDLINE conditionals |
| src/Build/BackEnd/Node/OutOfProcServerNode.cs | Remove FEATURE_GET_COMMANDLINE conditionals |
| src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs | Remove GetTypeInfo() call for type name check |
| src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs | Remove GetTypeInfo() calls for type filtering |
| src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs | Remove GetTypeInfo() call for serialization check |
| src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs | Remove GetTypeInfo() call for exception type check |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTask.cs | Remove GetTypeInfo() call for base type access |
| src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs | Remove GetTypeInfo() calls for type filtering |
| src/Build/BackEnd/Components/Logging/LoggingService.cs | Remove GetTypeInfo() call for assembly access |
| src/Build/BackEnd/Client/MSBuildClient.cs | Remove FEATURE_GET_COMMANDLINE conditionals |
| src/Build/BackEnd/BuildManager/BuildManager.cs | Remove GetTypeInfo() calls for assembly name access |
| src/Build.UnitTests/Utilities_Tests.cs | Remove FEATURE_GET_COMMANDLINE conditionals in test execution |
| src/Build.UnitTests/Evaluation/Expander_Tests.cs | Remove FEATURE_CULTUREINFO_GETCULTURES conditionals |
| src/Build.UnitTests/EscapingInProjects_Tests.cs | Remove FEATURE_ASSEMBLY_LOCATION conditionals and update assembly access |
| src/Build.UnitTests/Definition/ItemDefinitionGroup_Tests.cs | Remove FEATURE_ASSEMBLY_LOCATION conditionals and update assembly access |
| src/Build.UnitTests/BackEnd/TranslationHelpers.cs | Add assembly load context comparison for exception types |
| src/Build.UnitTests/BackEnd/TaskRegistry_Tests.cs | Remove FEATURE_ASSEMBLY_LOCATION conditionals and update assembly location access |
| src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs | Remove GetTypeInfo() calls for type checks |
| src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs | Remove GetTypeInfo() calls for assembly access |
| src/Build.UnitTests/BackEnd/LoggingService_Tests.cs | Remove FEATURE_ASSEMBLY_LOCATION conditionals and update assembly access |
| src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs | Remove FEATURE_ASSEMBLY_LOCATION conditionals and update assembly load info |
| src/Build.OM.UnitTests/Definition/ProjectCollection_Tests.cs | Remove GetTypeInfo() call for assembly version access |
Comments suppressed due to low confidence (1)
teo-tsirpanis
force-pushed
the
rm-compat-code
branch
from
c73a5f7 to
119759e
Compare
teo-tsirpanis
commented
Aug 3, 2025
Comment on lines
+58
to
+66
| AssemblyName assemblyName = AssemblyLoadContext.GetAssemblyName(fullPath); | ||
| if (MSBuildLoadContext.WellKnownAssemblyNames.Contains(assemblyName.Name)) | ||
| { | ||
| // If this is a well-known assembly, load it directly, ensuring that | ||
| // it is loaded only once. Some tests might pass a well-known assembly | ||
| // to this method. | ||
| return Assembly.Load(assemblyName); | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This is the only non-trivial change in the PR.
Member
|
hi @teo-tsirpanis , Could you please update this PR if you want to have it in? |
teo-tsirpanis
force-pushed
the
rm-compat-code
branch
2 times, most recently
from
a478eb2 to
5bf56e7
Compare
Fixes at least one failing test. Type checking in tests was also enhanced to better catch these kinds of errors.
teo-tsirpanis
force-pushed
the
rm-compat-code
branch
from
5bf56e7 to
bb84e0a
Compare
Contributor
Author
|
@YuliiaKovalova conflicts resolved. |
Contributor
|
This pull request has been automatically closed because it has been open for more than 180 days with no recent activity. If you believe this work is still relevant, please feel free to reopen or create a new pull request. Thank you for your contribution!
|
Contributor
Author
|
@YuliiaKovalova could you reopen this? |
This was referenced Apr 20, 2026
Member
|
Let's get #13576 in before bothering to fix conflicts here again; I suspect it'll cause more. |
Member
|
@teo-tsirpanis will you update this pr? |
Contributor
Author
|
@JanProvaznik done. |
Member
|
/review |
Contributor
|
✅ Expert Code Review (command) completed successfully! Expert reviewer agent completed and posted its review directly to PR #12231. The review found no blocking issues. The agent posted inline review comments and a final COMMENT-event review covering: (1) CoreCLRAssemblyLoader behavioral change, (2) MSBuildAssemblyFileVersion blank line artifact, and (3) XMake.cs NodeExeLocation source change on .NET Core. All |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary
Clean, well-executed modernization PR. Reviewed across the key MSBuild dimensions (correctness, backwards compatibility, performance, dead code, test coverage).
Verification ✅
- No orphaned references: Searched the entire
src/tree forGetTypeInfo(),GetAssemblyLocation,AssemblyUtilities.EntryAssembly,AssemblyUtilities.GetAllCultures,CultureInfoHasGetCultures,FEATURE_ASSEMBLY_LOCATION, andFEATURE_CULTUREINFO_GETCULTURES— all return zero matches. The cleanup is thorough. GetTypeInfo()removal: All 40+ call sites are mechanical.GetTypeInfo().Foo→.Fooreplacements. SinceTypeandTypeInfoshare these members (andGetTypeInfo()was only needed for .NET Standard 1.x), all transformations are semantically equivalent.FEATURE_ASSEMBLY_LOCATIONremoval: This flag was only defined fornet4*targets. On .NET Framework,Assembly.Locationwas always available. On .NET Core, it's also always available (not .NET Standard 1.x). Safe to remove.FEATURE_CULTUREINFO_GETCULTURESremoval: Same situation —CultureInfo.GetCulturesis available on all current target frameworks.CultureInfoCachehardcoded list removal: The 800+ hardcoded culture names were a fallback for platforms withoutCultureInfo.GetCultures. Since that API is now always available, this dead code is correctly removed.
Items Worth Noting
CoreCLRAssemblyLoader.csbehavioral change (lines 60-67): Not just a cleanup — adds a well-known assembly early-return guard. Consistent with existingMSBuildLoadContextpatterns but worth calling out in PR description.MSBuildAssemblyFileVersion.csline 40: Minor blank line artifact from removing.GetTypeInfo().XMake.csline 1509: On .NET Core, theNodeExeLocationsource changed fromBuildEnvironmentHelper.Instance.CurrentMSBuildExePathtoAssembly.GetExecutingAssembly().Location. These are equivalent for MSBuild's deployment model but the latter has less fallback robustness.
Overall Assessment
No correctness or compatibility concerns. The CoreCLRAssemblyLoader change is the only one that goes beyond pure cleanup and could use a note in the PR description. Everything else is straightforward dead code removal.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #12231
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #12231
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 (command) for issue #12231 · ● 5.7M
teo-tsirpanis
force-pushed
the
rm-compat-code
branch
from
d8f46a5 to
6eeb05e
Compare
JanProvaznik
approved these changes
May 12, 2026
Member
There was a problem hiding this comment.
I will do an experimental insertion as a smoke test for the flagged comment, but it seems minor. Otherwise lgtm
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/738320
Member
|
lgtm except it does not compile now 😄 |
teo-tsirpanis
commented
May 13, 2026
Comment on lines
+683
to
+706
| private static string GetFullPath(string path) | ||
| { | ||
| #if FEATURE_LEGACY_GETFULLPATH | ||
| if (NativeMethods.IsWindows) | ||
| { | ||
| string uncheckedFullPath = NativeMethods.GetFullPath(path); | ||
|
|
||
| if (IsPathTooLong(uncheckedFullPath)) | ||
| { | ||
| throw new PathTooLongException(SR.FormatPathTooLong(path, NativeMethods.MaxPath)); | ||
| } | ||
|
|
||
| // We really don't care about extensions here, but Path.HasExtension provides a great way to | ||
| // invoke the CLR's invalid path checks (these are independent of path length) | ||
| Path.HasExtension(uncheckedFullPath); | ||
|
|
||
| // If we detect we are a UNC path then we need to use the regular get full path in order to do the correct checks for UNC formatting | ||
| // and security checks for strings like \\?\GlobalRoot | ||
| return IsUNCPath(uncheckedFullPath) ? Path.GetFullPath(uncheckedFullPath) : uncheckedFullPath; | ||
| } | ||
| #endif | ||
|
|
||
| return Path.GetFullPath(path); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Using NewPath.GetFullPath instead of this might be the best way forward and fixed some test failures locally, but I will do it in a separate PR to reduce risk.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
MSBuild has been using some polyfills and compatibility code, owing to it targeting earlier .NET Core and .NET Standard versions in the past. It doesn't target these frameworks anymore, so these can be removed.
Changes Made
The following were removed as unnecessary:
Type.GetTypeInfo()Assemblyclass, that are now available on all supported frameworks.FEATURE_CULTUREINFO_GETCULTURES,FEATURE_PIPE_SECURITY, andFEATURE_HTTP_LISTENERsymbols, and all code conditioned upon them not being defined.FEATURE_LEGACY_GETCURRENTDIRECTORYsymbol, and all code conditioned upon it being defined.Testing
Existing test coverage.
Notes
FEATURE_XML_SCHEMA_VALIDATIONis always available too, but since enabling it has user-visible effects, it will be done in a separate PR.