From 98f6c45f9aa81ed760fa1b79aaf32617b933d7b2 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 18 Jul 2024 11:11:17 +0200 Subject: [PATCH 1/7] add fast paths --- src/Build/Evaluation/Expander.cs | 81 +++++++++++++++++++++++++++++++- src/Build/Microsoft.Build.csproj | 1 + 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index cc4562be381..786878d63c5 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -5,6 +5,7 @@ using System.Collections; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; @@ -20,6 +21,7 @@ using Microsoft.Build.Internal; using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; +using Microsoft.Build.Utilities; using Microsoft.NET.StringTools; using Microsoft.Win32; using AvailableStaticMethods = Microsoft.Build.Internal.AvailableStaticMethods; @@ -3566,6 +3568,7 @@ internal object Execute(object objectInstance, IPropertyProvider properties, } else { + Debugger.Launch(); bool wellKnownFunctionSuccess = false; try @@ -3864,6 +3867,14 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } + else if (string.Equals(_methodMethodName, nameof(String.Equals), StringComparison.OrdinalIgnoreCase)) + { + if (TryGetArg(args, out string arg0)) + { + returnVal = text.Equals(arg0); + return true; + } + } } else if (objectInstance is string[] stringArray) { @@ -4310,6 +4321,22 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } + else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.NormalizeDirectory), StringComparison.OrdinalIgnoreCase) && args.Length == 1) + { + if (TryGetArg(args, out string arg0)) + { + returnVal = IntrinsicFunctions.NormalizeDirectory(arg0); + return true; + } + } + else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.IsOSPlatform), StringComparison.OrdinalIgnoreCase) && args.Length == 1) + { + if (TryGetArg(args, out string arg0)) + { + returnVal = IntrinsicFunctions.IsOSPlatform(arg0); + return true; + } + } } else if (_receiverType == typeof(Path)) { @@ -4407,6 +4434,14 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } + else if (string.Equals(_methodMethodName, nameof(Path.GetFileNameWithoutExtension), StringComparison.OrdinalIgnoreCase)) + { + if (TryGetArg(args, out string arg0)) + { + returnVal = Path.GetFileNameWithoutExtension(arg0); + return true; + } + } } else if (_receiverType == typeof(Version)) { @@ -4419,7 +4454,7 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } } } - else if (_receiverType == typeof(System.Guid)) + else if (_receiverType == typeof(Guid)) { if (string.Equals(_methodMethodName, nameof(Guid.NewGuid), StringComparison.OrdinalIgnoreCase)) { @@ -4430,8 +4465,50 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } } } + else if (_receiverType == typeof(ToolLocationHelper)) + { + if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKLocation), StringComparison.OrdinalIgnoreCase)) + { + if (TryGetArg([args[0]], out string arg0) && TryGetArg([args[1]], out string arg1)) + { + returnVal = ToolLocationHelper.GetPlatformSDKLocation(arg0, arg1); + return true; + } + } + else if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKDisplayName), StringComparison.OrdinalIgnoreCase)) + { + if (TryGetArg([args[0]], out string arg0) && TryGetArg([args[1]], out string arg1)) + { + returnVal = ToolLocationHelper.GetPlatformSDKDisplayName(arg0, arg1); + return true; + } + } + } + } + else if (string.Equals(_methodMethodName, nameof(Version.ToString), StringComparison.OrdinalIgnoreCase) && objectInstance is Version v) + { + if (TryGetArg(args, out int arg0)) + { + returnVal = v.ToString(arg0); + return true; + } + } + else if (string.Equals(_methodMethodName, nameof(Regex.Replace), StringComparison.OrdinalIgnoreCase) && args.Length == 3) + { + if (TryGetArg([args[0]], out string arg1) && TryGetArg([args[1]], out string arg2) && TryGetArg([args[2]], out string arg3)) + { + returnVal = Regex.Replace(arg1, arg2, arg3); + return true; + } + } + else if (string.Equals(_methodMethodName, nameof(Int32.ToString), StringComparison.OrdinalIgnoreCase) && objectInstance is int i) + { + if (TryGetArg(args, out string arg0)) + { + returnVal = i.ToString(arg0); + return true; + } } - if (Traits.Instance.LogPropertyFunctionsRequiringReflection) { LogFunctionCall("PropertyFunctionsRequiringReflection", objectInstance, args); diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index 88962e732ae..49f317426f7 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -30,6 +30,7 @@ + From 74cf25672889a97b64c4924d8d6b0876e7b660cf Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 18 Jul 2024 14:49:37 +0200 Subject: [PATCH 2/7] cover fast paths with tests --- .../Evaluation/Expander_Tests.cs | 38 +++++++++++++++++++ src/Build/Evaluation/Expander.cs | 19 +++++----- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 25187464656..2b3534e5859 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -7,6 +7,7 @@ using System.IO; using System.Linq; using System.Runtime.InteropServices; +using System.Runtime.InteropServices.ComTypes; using System.Runtime.Versioning; using System.Text; using System.Threading; @@ -5064,6 +5065,43 @@ public void GetTypeMethod_ShouldBeAllowed_EnabledByEnvVariable(string methodName } } + [Theory] + [InlineData("$([System.Version]::Parse('17.12.11.10').ToString(2))")] + [InlineData("$([System.Text.RegularExpressions.Regex]::Replace('abc123def', 'abc', ''))")] + [InlineData("$([System.String]::new('Hi').Equals('Hello'))")] + [InlineData("$([System.IO.Path]::GetFileNameWithoutExtension('C:\\folder\\file.txt'))")] + [InlineData("$([System.Int32]::new(123).ToString('mm')")] + [InlineData("$([Microsoft.Build.Utilities.ToolLocationHelper]::GetPlatformSDKLocation('10.0.19041.0', 'Windows'))")] + [InlineData("$([Microsoft.Build.Utilities.ToolLocationHelper]::GetPlatformSDKDisplayName('10.0.19041.0', 'Windows'))")] + [InlineData("$([Microsoft.Build.Evaluation.IntrinsicFunctions]::NormalizeDirectory('C:/folder1/./folder2/'))")] + [InlineData("$([Microsoft.Build.Evaluation.IntrinsicFunctions]::IsOSPlatform('Windows'))")] + public void FastPathValidationTest(string methodInvocationMetadata) + { + using (var env = TestEnvironment.Create()) + { + // Setting this env variable allows to track if expander was using reflection for a function invocation. + env.SetEnvironmentVariable("MSBuildLogPropertyFunctionsRequiringReflection", "1"); + + var logger = new MockLogger(); + ILoggingService loggingService = LoggingService.CreateLoggingService(LoggerMode.Synchronous, 1); + loggingService.RegisterLogger(logger); + var loggingContext = new MockLoggingContext( + loggingService, + new BuildEventContext(0, 0, BuildEventContext.InvalidProjectContextId, 0, 0)); + + _ = new Expander( + new PropertyDictionary(), + FileSystems.Default, + loggingContext) + .ExpandIntoStringLeaveEscaped(methodInvocationMetadata, ExpanderOptions.ExpandProperties, MockElementLocation.Instance); + + string reflectionInfoPath = Path.Combine(Directory.GetCurrentDirectory(), "PropertyFunctionsRequiringReflection"); + + // the fast path was successfully resolved without reflection. + File.Exists(reflectionInfoPath).ShouldBeFalse(); + } + } + /// /// Determines if ICU mode is enabled. /// Copied from: https://learn.microsoft.com/en-us/dotnet/core/extensions/globalization-icu#determine-if-your-app-is-using-icu diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 786878d63c5..3687f005e95 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -3568,7 +3568,6 @@ internal object Execute(object objectInstance, IPropertyProvider properties, } else { - Debugger.Launch(); bool wellKnownFunctionSuccess = false; try @@ -3867,7 +3866,7 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } - else if (string.Equals(_methodMethodName, nameof(String.Equals), StringComparison.OrdinalIgnoreCase)) + else if (string.Equals(_methodMethodName, nameof(string.Equals), StringComparison.OrdinalIgnoreCase)) { if (TryGetArg(args, out string arg0)) { @@ -4484,6 +4483,14 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } } } + else if (string.Equals(_methodMethodName, nameof(Regex.Replace), StringComparison.OrdinalIgnoreCase) && args.Length == 3) + { + if (TryGetArg([args[0]], out string arg1) && TryGetArg([args[1]], out string arg2) && TryGetArg([args[2]], out string arg3)) + { + returnVal = Regex.Replace(arg1, arg2, arg3); + return true; + } + } } else if (string.Equals(_methodMethodName, nameof(Version.ToString), StringComparison.OrdinalIgnoreCase) && objectInstance is Version v) { @@ -4493,14 +4500,6 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } - else if (string.Equals(_methodMethodName, nameof(Regex.Replace), StringComparison.OrdinalIgnoreCase) && args.Length == 3) - { - if (TryGetArg([args[0]], out string arg1) && TryGetArg([args[1]], out string arg2) && TryGetArg([args[2]], out string arg3)) - { - returnVal = Regex.Replace(arg1, arg2, arg3); - return true; - } - } else if (string.Equals(_methodMethodName, nameof(Int32.ToString), StringComparison.OrdinalIgnoreCase) && objectInstance is int i) { if (TryGetArg(args, out string arg0)) From 068200d830bfac2149f3afc8602eaf90f5f1c3e6 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 18 Jul 2024 14:52:52 +0200 Subject: [PATCH 3/7] remove unused refs --- src/Build.UnitTests/Evaluation/Expander_Tests.cs | 1 - src/Build/Evaluation/Expander.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 2b3534e5859..532aa6e1280 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -7,7 +7,6 @@ using System.IO; using System.Linq; using System.Runtime.InteropServices; -using System.Runtime.InteropServices.ComTypes; using System.Runtime.Versioning; using System.Text; using System.Threading; diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 3687f005e95..df9a971aab1 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -5,7 +5,6 @@ using System.Collections; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; From fb0ddb21bcaa80e1286c589a1160db9a06ec2fdb Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 18 Jul 2024 14:54:01 +0200 Subject: [PATCH 4/7] cleanup --- src/Build/Evaluation/Expander.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index df9a971aab1..ae6dfb0c1ac 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4319,7 +4319,7 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } - else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.NormalizeDirectory), StringComparison.OrdinalIgnoreCase) && args.Length == 1) + else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.NormalizeDirectory), StringComparison.OrdinalIgnoreCase)) { if (TryGetArg(args, out string arg0)) { @@ -4327,7 +4327,7 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } - else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.IsOSPlatform), StringComparison.OrdinalIgnoreCase) && args.Length == 1) + else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.IsOSPlatform), StringComparison.OrdinalIgnoreCase)) { if (TryGetArg(args, out string arg0)) { From 34a897362fff4623ca2cb7cb2e1bddc98400b0b0 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com> Date: Thu, 18 Jul 2024 15:36:48 +0200 Subject: [PATCH 5/7] add length check --- src/Build/Evaluation/Expander.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index ae6dfb0c1ac..475b0396ec9 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4463,9 +4463,10 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } } } + // Length check needed due to existing overloads. else if (_receiverType == typeof(ToolLocationHelper)) { - if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKLocation), StringComparison.OrdinalIgnoreCase)) + if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKLocation), StringComparison.OrdinalIgnoreCase) && args.Length == 2) { if (TryGetArg([args[0]], out string arg0) && TryGetArg([args[1]], out string arg1)) { @@ -4473,7 +4474,7 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst return true; } } - else if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKDisplayName), StringComparison.OrdinalIgnoreCase)) + else if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKDisplayName), StringComparison.OrdinalIgnoreCase) && args.Length == 2) { if (TryGetArg([args[0]], out string arg0) && TryGetArg([args[1]], out string arg1)) { From fae36a463696755a336493e9a43056b9c73cca33 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com> Date: Thu, 18 Jul 2024 16:04:19 +0200 Subject: [PATCH 6/7] remove reference to Microsoft.Build.Utilities --- src/Build/Evaluation/Expander.cs | 21 --------------------- src/Build/Microsoft.Build.csproj | 1 - 2 files changed, 22 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 475b0396ec9..75f0216028f 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -20,7 +20,6 @@ using Microsoft.Build.Internal; using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; -using Microsoft.Build.Utilities; using Microsoft.NET.StringTools; using Microsoft.Win32; using AvailableStaticMethods = Microsoft.Build.Internal.AvailableStaticMethods; @@ -4463,26 +4462,6 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } } } - // Length check needed due to existing overloads. - else if (_receiverType == typeof(ToolLocationHelper)) - { - if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKLocation), StringComparison.OrdinalIgnoreCase) && args.Length == 2) - { - if (TryGetArg([args[0]], out string arg0) && TryGetArg([args[1]], out string arg1)) - { - returnVal = ToolLocationHelper.GetPlatformSDKLocation(arg0, arg1); - return true; - } - } - else if (string.Equals(_methodMethodName, nameof(ToolLocationHelper.GetPlatformSDKDisplayName), StringComparison.OrdinalIgnoreCase) && args.Length == 2) - { - if (TryGetArg([args[0]], out string arg0) && TryGetArg([args[1]], out string arg1)) - { - returnVal = ToolLocationHelper.GetPlatformSDKDisplayName(arg0, arg1); - return true; - } - } - } else if (string.Equals(_methodMethodName, nameof(Regex.Replace), StringComparison.OrdinalIgnoreCase) && args.Length == 3) { if (TryGetArg([args[0]], out string arg1) && TryGetArg([args[1]], out string arg2) && TryGetArg([args[2]], out string arg3)) diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index 49f317426f7..88962e732ae 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -30,7 +30,6 @@ - From 286d049a07c004c63ea91104385bdb76d415a122 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com> Date: Thu, 18 Jul 2024 16:54:19 +0200 Subject: [PATCH 7/7] remove extra tests --- src/Build.UnitTests/Evaluation/Expander_Tests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 532aa6e1280..f137cf89960 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -5070,8 +5070,6 @@ public void GetTypeMethod_ShouldBeAllowed_EnabledByEnvVariable(string methodName [InlineData("$([System.String]::new('Hi').Equals('Hello'))")] [InlineData("$([System.IO.Path]::GetFileNameWithoutExtension('C:\\folder\\file.txt'))")] [InlineData("$([System.Int32]::new(123).ToString('mm')")] - [InlineData("$([Microsoft.Build.Utilities.ToolLocationHelper]::GetPlatformSDKLocation('10.0.19041.0', 'Windows'))")] - [InlineData("$([Microsoft.Build.Utilities.ToolLocationHelper]::GetPlatformSDKDisplayName('10.0.19041.0', 'Windows'))")] [InlineData("$([Microsoft.Build.Evaluation.IntrinsicFunctions]::NormalizeDirectory('C:/folder1/./folder2/'))")] [InlineData("$([Microsoft.Build.Evaluation.IntrinsicFunctions]::IsOSPlatform('Windows'))")] public void FastPathValidationTest(string methodInvocationMetadata)