From e02b3d998f7c4b39bd960f75446a74cda489bc27 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 13 Sep 2021 15:07:26 -0700 Subject: [PATCH 01/10] Early exit --- .../Conditionals/StringExpressionNode.cs | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs index 61eabd438fd..a8bb0fd20af 100644 --- a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs @@ -3,7 +3,6 @@ using System; using System.Diagnostics; - using Microsoft.Build.Shared; namespace Microsoft.Build.Evaluation @@ -98,6 +97,25 @@ internal override bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationS { if (_expandable) { + switch (_value.Length) + { + case 0: + _cachedExpandedValue = String.Empty; + return true; + // If the length is 1 or 2, it can't possibly be a property, item, or metadata, and it isn't empty. + case 1: + case 2: + _cachedExpandedValue = _value; + return false; + default: + if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@')) + { + // This isn't just a property, item, or metadata value, and it isn't empty. + return false; + } + break; + } + string expandBreakEarly = state.ExpandIntoStringBreakEarly(_value); if (expandBreakEarly == null) From 64354ffaa325fa0ddace363f9790d4e462b1a3a7 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 13 Sep 2021 15:09:23 -0700 Subject: [PATCH 02/10] Prevent double computation When calculating a value to see if we can do a type of comparison, store that value so we don't have to calculate it again. Use Try*Evaluate instead of Can*Evaluate and *Evaluate. --- .../Conditionals/GenericExpressionNode.cs | 3 ++ .../MultipleComparisonExpressionNode.cs | 47 ++++++++++++------- .../Conditionals/NumericExpressionNode.cs | 19 +++++++- .../Conditionals/OperatorExpressionNode.cs | 18 +++++++ .../Conditionals/StringExpressionNode.cs | 31 ++++++++++++ src/Shared/ConversionUtilities.cs | 46 +++++++++++++----- 6 files changed, 133 insertions(+), 31 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs index d4862fec251..94fa1ae00ce 100644 --- a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs @@ -18,6 +18,9 @@ internal abstract class GenericExpressionNode internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state); internal abstract double NumericEvaluate(ConditionEvaluator.IConditionEvaluationState state); internal abstract Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state); + internal abstract bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result); + internal abstract bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result); + internal abstract bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result); /// /// Returns true if this node evaluates to an empty string, diff --git a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs index 6a457794da3..28ca1d9c132 100644 --- a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs @@ -48,36 +48,47 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState // and we know which do, then we already have enough information to evaluate this expression. // That means we don't have to fully expand a condition like " '@(X)' == '' " // which is a performance advantage if @(X) is a huge item list. - if (LeftChild.EvaluatesToEmpty(state) || RightChild.EvaluatesToEmpty(state)) + bool leftEmpty = LeftChild.EvaluatesToEmpty(state); + bool rightEmpty = RightChild.EvaluatesToEmpty(state); + if (leftEmpty || rightEmpty) { UpdateConditionedProperties(state); - return Compare(LeftChild.EvaluatesToEmpty(state), RightChild.EvaluatesToEmpty(state)); + return Compare(leftEmpty, rightEmpty); } - if (LeftChild.CanNumericEvaluate(state) && RightChild.CanNumericEvaluate(state)) + if (LeftChild.TryNumericEvaluate(state, out double leftNumericValue)) { - return Compare(LeftChild.NumericEvaluate(state), RightChild.NumericEvaluate(state)); + // The left child evaluating to a number and the right child not evaluating to a number + // is insufficient to say they are not equal because $(MSBuildToolsVersion) evaluates to + // the string "Current" most of the time but when doing numeric comparisons, is treated + // as a version and returns "17.0" (or whatever the current tools version is). This means + // that if '$(MSBuildToolsVersion)' is "equal" to BOTH '17.0' and 'Current' (if 'Current' + // is 17.0). + if (RightChild.TryNumericEvaluate(state, out double rightNumericValue)) + { + return Compare(leftNumericValue, rightNumericValue); + } } - else if (LeftChild.CanBoolEvaluate(state) && RightChild.CanBoolEvaluate(state)) + + if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue)) { - return Compare(LeftChild.BoolEvaluate(state), RightChild.BoolEvaluate(state)); + RightChild.TryBoolEvaluate(state, out bool rightBoolValue); + return Compare(leftBoolValue, rightBoolValue); } - else // string comparison - { - string leftExpandedValue = LeftChild.GetExpandedValue(state); - string rightExpandedValue = RightChild.GetExpandedValue(state); - ProjectErrorUtilities.VerifyThrowInvalidProject - (leftExpandedValue != null && rightExpandedValue != null, - state.ElementLocation, - "IllFormedCondition", - state.Condition); + string leftExpandedValue = LeftChild.GetExpandedValue(state); + string rightExpandedValue = RightChild.GetExpandedValue(state); - UpdateConditionedProperties(state); + ProjectErrorUtilities.VerifyThrowInvalidProject + (leftExpandedValue != null && rightExpandedValue != null, + state.ElementLocation, + "IllFormedCondition", + state.Condition); - return Compare(leftExpandedValue, rightExpandedValue); - } + UpdateConditionedProperties(state); + + return Compare(leftExpandedValue, rightExpandedValue); } /// diff --git a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs index 306db6d802b..9583a67e024 100644 --- a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs @@ -73,8 +73,23 @@ internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluatio { // Check if the value can be formatted as a Version number // This is needed for nodes that identify as Numeric but can't be parsed as numbers (e.g. 8.1.1.0 vs 8.1) - Version unused; - return Version.TryParse(_value, out unused); + return Version.TryParse(_value, out _); + } + + internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) + { + result = default; + return false; + } + + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) + { + return ConversionUtilities.TryConvertDecimalOrHexToDouble(_value, out result); + } + + internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result) + { + return Version.TryParse(_value, out result); } /// diff --git a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs index 918e188ff85..c67eea8c863 100644 --- a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs @@ -56,6 +56,24 @@ internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluatio return false; } + internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) + { + result = BoolEvaluate(state); + return true; + } + + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) + { + result = default; + return false; + } + + internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result) + { + result = default; + return false; + } + /// /// Value after any item and property expressions are expanded /// diff --git a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs index a8bb0fd20af..97544e6f263 100644 --- a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs @@ -84,6 +84,37 @@ internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluatio return Version.TryParse(GetExpandedValue(state), out _); } + internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) + { + return ConversionUtilities.TryConvertStringToBool(GetExpandedValue(state), out result); + } + + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) + { + if (ShouldBeTreatedAsVisualStudioVersion(state)) + { + result = ConversionUtilities.ConvertDecimalOrHexToDouble(MSBuildConstants.CurrentVisualStudioVersion); + return true; + } + else + { + return ConversionUtilities.TryConvertDecimalOrHexToDouble(GetExpandedValue(state), out result); + } + } + + internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result) + { + if (ShouldBeTreatedAsVisualStudioVersion(state)) + { + result = Version.Parse(MSBuildConstants.CurrentVisualStudioVersion); + return true; + } + else + { + return Version.TryParse(GetExpandedValue(state), out result); + } + } + /// /// Returns true if this node evaluates to an empty string, /// otherwise false. diff --git a/src/Shared/ConversionUtilities.cs b/src/Shared/ConversionUtilities.cs index 351b3b04e46..c2e424e322e 100644 --- a/src/Shared/ConversionUtilities.cs +++ b/src/Shared/ConversionUtilities.cs @@ -64,6 +64,21 @@ internal static string ConvertByteArrayToHex(byte[] bytes) return sb.ToString(); } + internal static bool TryConvertStringToBool(string parameterValue, out bool boolValue) + { + boolValue = false; + if (ValidBooleanTrue(parameterValue)) + { + boolValue = true; + return true; + } + else if (ValidBooleanFalse(parameterValue)) + { + return true; + } + return false; + } + /// /// Returns true if the string can be successfully converted to a bool, /// such as "on" or "yes" @@ -123,30 +138,40 @@ internal static double ConvertHexToDouble(string number) /// internal static double ConvertDecimalOrHexToDouble(string number) { - if (ConversionUtilities.ValidDecimalNumber(number)) + if (TryConvertDecimalOrHexToDouble(number, out double result)) { - return ConversionUtilities.ConvertDecimalToDouble(number); + return result; } - else if (ConversionUtilities.ValidHexNumber(number)) + ErrorUtilities.VerifyThrow(false, "Cannot numeric evaluate"); + return 0.0D; + } + + internal static bool TryConvertDecimalOrHexToDouble(string number, out double doubleValue) + { + if (ConversionUtilities.ValidDecimalNumber(number, out doubleValue)) { - return ConversionUtilities.ConvertHexToDouble(number); + return true; + } + else if (ConversionUtilities.ValidHexNumber(number, out int hexValue)) + { + doubleValue = (double)hexValue; + return true; } else { - ErrorUtilities.VerifyThrow(false, "Cannot numeric evaluate"); - return 0.0D; + return false; } } /// /// Returns true if the string is a valid hex number, like "0xABC" /// - private static bool ValidHexNumber(string number) + private static bool ValidHexNumber(string number, out int value) { bool canConvert = false; + value = 0; if (number.Length >= 3 && number[0] == '0' && (number[1] == 'x' || number[1] == 'X')) { - int value; canConvert = Int32.TryParse(number.Substring(2), NumberStyles.AllowHexSpecifier, CultureInfo.InvariantCulture.NumberFormat, out value); } return canConvert; @@ -155,9 +180,8 @@ private static bool ValidHexNumber(string number) /// /// Returns true if the string is a valid decimal number, like "-123.456" /// - private static bool ValidDecimalNumber(string number) + private static bool ValidDecimalNumber(string number, out double value) { - double value; return Double.TryParse(number, NumberStyles.AllowDecimalPoint | NumberStyles.AllowLeadingSign, CultureInfo.InvariantCulture.NumberFormat, out value) && !double.IsInfinity(value); } @@ -166,7 +190,7 @@ private static bool ValidDecimalNumber(string number) /// internal static bool ValidDecimalOrHexNumber(string number) { - return ValidDecimalNumber(number) || ValidHexNumber(number); + return ValidDecimalNumber(number, out _) || ValidHexNumber(number, out _); } } } From 762ac1d16cef349166315d4ebc8067eccee38724 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 14 Sep 2021 09:31:42 -0700 Subject: [PATCH 03/10] Use search for @ --- src/Build/Evaluation/ExpressionShredder.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/ExpressionShredder.cs b/src/Build/Evaluation/ExpressionShredder.cs index ffa04158d9d..afb12090a4d 100644 --- a/src/Build/Evaluation/ExpressionShredder.cs +++ b/src/Build/Evaluation/ExpressionShredder.cs @@ -110,12 +110,14 @@ internal static List GetReferencedItemExpressions(string { List subExpressions = null; - if (expression.IndexOf('@') < 0) + int startInd = expression.IndexOf('@', start, end - start); + + if (startInd < 0) { return null; } - for (int i = start; i < end; i++) + for (int i = startInd; i < end; i++) { int restartPoint; int startPoint; From f9456c464db1abc03b9eebf4e70d67ffbe75ea9f Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Wed, 22 Sep 2021 16:41:23 -0700 Subject: [PATCH 04/10] Fix incorrect bool check --- .../Conditionals/MultipleComparisonExpressionNode.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs index 28ca1d9c132..6e1fd4e1e7d 100644 --- a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs @@ -73,8 +73,9 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue)) { - RightChild.TryBoolEvaluate(state, out bool rightBoolValue); - return Compare(leftBoolValue, rightBoolValue); + return RightChild.TryBoolEvaluate(state, out bool rightBoolValue) ? + Compare(leftBoolValue, rightBoolValue) : + this is NotEqualExpressionNode; // If the left child is a bool, and the right child is not, then they are not equal. Return true for != and false for == } string leftExpandedValue = LeftChild.GetExpandedValue(state); From 16a30b3d743048fda810e0169573845630616656 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 4 Oct 2021 12:34:41 -0700 Subject: [PATCH 05/10] Throw error --- .../MultipleComparisonExpressionNode.cs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs index 6e1fd4e1e7d..18e1caad924 100644 --- a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs @@ -56,8 +56,7 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState return Compare(leftEmpty, rightEmpty); } - - if (LeftChild.TryNumericEvaluate(state, out double leftNumericValue)) + else if (LeftChild.TryNumericEvaluate(state, out double leftNumericValue) && RightChild.TryNumericEvaluate(state, out double rightNumericValue)) { // The left child evaluating to a number and the right child not evaluating to a number // is insufficient to say they are not equal because $(MSBuildToolsVersion) evaluates to @@ -65,17 +64,11 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState // as a version and returns "17.0" (or whatever the current tools version is). This means // that if '$(MSBuildToolsVersion)' is "equal" to BOTH '17.0' and 'Current' (if 'Current' // is 17.0). - if (RightChild.TryNumericEvaluate(state, out double rightNumericValue)) - { - return Compare(leftNumericValue, rightNumericValue); - } + return Compare(leftNumericValue, rightNumericValue); } - - if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue)) + else if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue) && RightChild.TryBoolEvaluate(state, out bool rightBoolValue)) { - return RightChild.TryBoolEvaluate(state, out bool rightBoolValue) ? - Compare(leftBoolValue, rightBoolValue) : - this is NotEqualExpressionNode; // If the left child is a bool, and the right child is not, then they are not equal. Return true for != and false for == + return Compare(leftBoolValue, rightBoolValue); } string leftExpandedValue = LeftChild.GetExpandedValue(state); From 6e4c0d40640287f3350e59b858cb2b11d733e49e Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 11 Oct 2021 10:17:55 -0700 Subject: [PATCH 06/10] Replace Can*Evaluate and *Evaluate with Try*Evaluate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes all *Evaluate and Can*Evaluate except in OperatorExpressionNodes, for which CanBoolEvaluate always returns true, so using a simple BoolEvaluate makes sense. Note that for the NumericComparisonExpressionNode, we could save a tiny amount of time by only calling TryEvaluate when it's actually used (and using if/else as before), but that would since something can be both a number and a version, that would mean we would have to check both regardless of what we had previously found, which means it would be noticeably messier for (probably) very little perf gain, so I opted against that. Switch statements are so pretty 🙂 --- .../Conditionals/AndExpressionNode.cs | 8 +-- .../Conditionals/GenericExpressionNode.cs | 10 +--- .../Conditionals/NotExpressionNode.cs | 8 +-- .../NumericComparisonExpressionNode.cs | 49 +++++----------- .../Conditionals/NumericExpressionNode.cs | 55 ------------------ .../Conditionals/OperatorExpressionNode.cs | 47 +--------------- .../Conditionals/OrExpressionNode.cs | 8 +-- .../Conditionals/StringExpressionNode.cs | 56 ------------------- 8 files changed, 29 insertions(+), 212 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/AndExpressionNode.cs b/src/Build/Evaluation/Conditionals/AndExpressionNode.cs index 7b3e099fb4c..31d790c5b3a 100644 --- a/src/Build/Evaluation/Conditionals/AndExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/AndExpressionNode.cs @@ -19,14 +19,14 @@ internal sealed class AndExpressionNode : OperatorExpressionNode internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) { ProjectErrorUtilities.VerifyThrowInvalidProject - (LeftChild.CanBoolEvaluate(state), + (LeftChild.TryBoolEvaluate(state, out bool leftBool), state.ElementLocation, "ExpressionDoesNotEvaluateToBoolean", LeftChild.GetUnexpandedValue(state), LeftChild.GetExpandedValue(state), state.Condition); - if (!LeftChild.BoolEvaluate(state)) + if (!leftBool) { // Short circuit return false; @@ -34,14 +34,14 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState else { ProjectErrorUtilities.VerifyThrowInvalidProject - (RightChild.CanBoolEvaluate(state), + (RightChild.TryBoolEvaluate(state, out bool rightBool), state.ElementLocation, "ExpressionDoesNotEvaluateToBoolean", RightChild.GetUnexpandedValue(state), RightChild.GetExpandedValue(state), state.Condition); - return RightChild.BoolEvaluate(state); + return rightBool; } } diff --git a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs index 94fa1ae00ce..b9f9d92eff2 100644 --- a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs @@ -12,12 +12,6 @@ namespace Microsoft.Build.Evaluation /// internal abstract class GenericExpressionNode { - internal abstract bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state); - internal abstract bool CanNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state); - internal abstract bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state); - internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state); - internal abstract double NumericEvaluate(ConditionEvaluator.IConditionEvaluationState state); - internal abstract Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state); internal abstract bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result); internal abstract bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result); internal abstract bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result); @@ -60,13 +54,13 @@ internal virtual bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationSt internal bool Evaluate(ConditionEvaluator.IConditionEvaluationState state) { ProjectErrorUtilities.VerifyThrowInvalidProject( - CanBoolEvaluate(state), + TryBoolEvaluate(state, out bool boolValue), state.ElementLocation, "ConditionNotBooleanDetail", state.Condition, GetExpandedValue(state)); - return BoolEvaluate(state); + return boolValue; ; } /// diff --git a/src/Build/Evaluation/Conditionals/NotExpressionNode.cs b/src/Build/Evaluation/Conditionals/NotExpressionNode.cs index 0a2521feda7..5a8d3516574 100644 --- a/src/Build/Evaluation/Conditionals/NotExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NotExpressionNode.cs @@ -17,12 +17,8 @@ internal sealed class NotExpressionNode : OperatorExpressionNode /// internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) { - return !LeftChild.BoolEvaluate(state); - } - - internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return LeftChild.CanBoolEvaluate(state); + LeftChild.TryBoolEvaluate(state, out bool boolValue); + return !boolValue; } /// diff --git a/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs index 0bd30600cbf..7d1a6f82939 100644 --- a/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs @@ -38,46 +38,27 @@ internal abstract class NumericComparisonExpressionNode : OperatorExpressionNode /// internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) { - bool isLeftNum = LeftChild.CanNumericEvaluate(state); - bool isLeftVersion = LeftChild.CanVersionEvaluate(state); - bool isRightNum = RightChild.CanNumericEvaluate(state); - bool isRightVersion = RightChild.CanVersionEvaluate(state); - bool isNumeric = isLeftNum && isRightNum; - bool isVersion = isLeftVersion && isRightVersion; - bool isValidComparison = isNumeric || isVersion || (isLeftNum && isRightVersion) || (isLeftVersion && isRightNum); + bool isLeftNum = LeftChild.TryNumericEvaluate(state, out double leftNum); + bool isLeftVersion = LeftChild.TryVersionEvaluate(state, out Version leftVersion); + bool isRightNum = RightChild.TryNumericEvaluate(state, out double rightNum); + bool isRightVersion = RightChild.TryVersionEvaluate(state, out Version rightVersion); - ProjectErrorUtilities.VerifyThrowInvalidProject - (isValidComparison, + return isLeftNum, isLeftVersion, isRightNum, isRightVersion switch + { + true, _, true, _ => Compare(leftNum, rightNum), + _, true, _, true => Compare(leftVersion, rightVersion), + true, _, _, true => Compare(leftNum, rightVersion), + _, true, true, _ => Compare(leftVersion, rightNum), + + _ => ProjectErrorUtilities.VerifyThrowInvalidProject + (false, state.ElementLocation, "ComparisonOnNonNumericExpression", state.Condition, /* helpfully display unexpanded token and expanded result in error message */ - LeftChild.CanNumericEvaluate(state) ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), - LeftChild.CanNumericEvaluate(state) ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); - - // If the values identify as numeric, make that comparison instead of the Version comparison since numeric has a stricter definition - if (isNumeric) - { - return Compare(LeftChild.NumericEvaluate(state), RightChild.NumericEvaluate(state)); - } - else if (isVersion) - { - return Compare(LeftChild.VersionEvaluate(state), RightChild.VersionEvaluate(state)); + isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), + isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); false } - - // If the numbers are of a mixed type, call that specific Compare method - if (isLeftNum && isRightVersion) - { - return Compare(LeftChild.NumericEvaluate(state), RightChild.VersionEvaluate(state)); - } - else if (isLeftVersion && isRightNum) - { - return Compare(LeftChild.VersionEvaluate(state), RightChild.NumericEvaluate(state)); - } - - // Throw error here as this code should be unreachable - ErrorUtilities.ThrowInternalErrorUnreachable(); - return false; } } } diff --git a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs index 9583a67e024..fe21a15a1ad 100644 --- a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs @@ -21,61 +21,6 @@ internal NumericExpressionNode(string value) _value = value; } - /// - /// Evaluate as boolean - /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - // Should be unreachable: all calls check CanBoolEvaluate() first - ErrorUtilities.VerifyThrow(false, "Can't evaluate a numeric expression as boolean."); - return false; - } - - /// - /// Evaluate as numeric - /// - internal override double NumericEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return ConversionUtilities.ConvertDecimalOrHexToDouble(_value); - } - - /// - /// Evaluate as a Version - /// - internal override Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return Version.Parse(_value); - } - - /// - /// Whether it can be evaluated as a boolean: never allowed for numerics - /// - internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - // Numeric expressions are never allowed to be treated as booleans. - return false; - } - - /// - /// Whether it can be evaluated as numeric - /// - internal override bool CanNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - // It is not always possible to numerically evaluate even a numerical expression - - // for example, it may overflow a double. So check here. - return ConversionUtilities.ValidDecimalOrHexNumber(_value); - } - - /// - /// Whether it can be evaluated as a Version - /// - internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - // Check if the value can be formatted as a Version number - // This is needed for nodes that identify as Numeric but can't be parsed as numbers (e.g. 8.1.1.0 vs 8.1) - return Version.TryParse(_value, out _); - } - internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) { result = default; diff --git a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs index c67eea8c863..d5520e01bd7 100644 --- a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs @@ -11,57 +11,14 @@ namespace Microsoft.Build.Evaluation /// internal abstract class OperatorExpressionNode : GenericExpressionNode { - /// - /// Numeric evaluation is never allowed for operators - /// - internal override double NumericEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - // Should be unreachable: all calls check CanNumericEvaluate() first - ErrorUtilities.VerifyThrow(false, "Cannot numeric evaluate an operator"); - return 0.0D; - } - - /// - /// Version evaluation is never allowed for operators - /// - internal override Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - ErrorUtilities.VerifyThrow(false, "Cannot version evaluate an operator"); - return null; - } - - /// - /// Whether boolean evaluation is allowed: always allowed for operators - /// - internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return true; - } - - /// - /// Whether the node can be evaluated as a numeric: by default, - /// this is not allowed - /// - internal override bool CanNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return false; - } - - /// - /// Whether the node can be evaluated as a version: by default, - /// this is not allowed - /// - internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return false; - } - internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) { result = BoolEvaluate(state); return true; } + internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state); + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) { result = default; diff --git a/src/Build/Evaluation/Conditionals/OrExpressionNode.cs b/src/Build/Evaluation/Conditionals/OrExpressionNode.cs index e9469f07aca..73a91600f9b 100644 --- a/src/Build/Evaluation/Conditionals/OrExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OrExpressionNode.cs @@ -19,14 +19,14 @@ internal sealed class OrExpressionNode : OperatorExpressionNode internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) { ProjectErrorUtilities.VerifyThrowInvalidProject - (LeftChild.CanBoolEvaluate(state), + (LeftChild.TryBoolEvaluate(state, out bool leftBool), state.ElementLocation, "ExpressionDoesNotEvaluateToBoolean", LeftChild.GetUnexpandedValue(state), LeftChild.GetExpandedValue(state), state.Condition); - if (LeftChild.BoolEvaluate(state)) + if (leftBool) { // Short circuit return true; @@ -34,14 +34,14 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState else { ProjectErrorUtilities.VerifyThrowInvalidProject - (RightChild.CanBoolEvaluate(state), + (RightChild.TryBoolEvaluate(state, out bool rightBool), state.ElementLocation, "ExpressionDoesNotEvaluateToBoolean", RightChild.GetUnexpandedValue(state), RightChild.GetExpandedValue(state), state.Condition); - return RightChild.BoolEvaluate(state); + return rightBool; } } diff --git a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs index 97544e6f263..4ad13c491fd 100644 --- a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs @@ -28,62 +28,6 @@ internal StringExpressionNode(string value, bool expandable) _expandable = expandable; } - /// - /// Evaluate as boolean - /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return ConversionUtilities.ConvertStringToBool(GetExpandedValue(state)); - } - - /// - /// Evaluate as numeric - /// - internal override double NumericEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - if (ShouldBeTreatedAsVisualStudioVersion(state)) - { - return ConversionUtilities.ConvertDecimalOrHexToDouble(MSBuildConstants.CurrentVisualStudioVersion); - } - - return ConversionUtilities.ConvertDecimalOrHexToDouble(GetExpandedValue(state)); - } - - internal override Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - if (ShouldBeTreatedAsVisualStudioVersion(state)) - { - return Version.Parse(MSBuildConstants.CurrentVisualStudioVersion); - } - - return Version.Parse(GetExpandedValue(state)); - } - - internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - return ConversionUtilities.CanConvertStringToBool(GetExpandedValue(state)); - } - - internal override bool CanNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - if (ShouldBeTreatedAsVisualStudioVersion(state)) - { - return true; - } - - return ConversionUtilities.ValidDecimalOrHexNumber(GetExpandedValue(state)); - } - - internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state) - { - if (ShouldBeTreatedAsVisualStudioVersion(state)) - { - return true; - } - - return Version.TryParse(GetExpandedValue(state), out _); - } - internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) { return ConversionUtilities.TryConvertStringToBool(GetExpandedValue(state), out result); From 2aac6e2b1ded22944aab261b64860b6f6617f665 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 11 Oct 2021 10:18:11 -0700 Subject: [PATCH 07/10] Do not abbreviate variable name --- src/Build/Evaluation/ExpressionShredder.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Build/Evaluation/ExpressionShredder.cs b/src/Build/Evaluation/ExpressionShredder.cs index afb12090a4d..1ea8786cf67 100644 --- a/src/Build/Evaluation/ExpressionShredder.cs +++ b/src/Build/Evaluation/ExpressionShredder.cs @@ -110,14 +110,14 @@ internal static List GetReferencedItemExpressions(string { List subExpressions = null; - int startInd = expression.IndexOf('@', start, end - start); + int startIndex = expression.IndexOf('@', start, end - start); - if (startInd < 0) + if (startIndex < 0) { return null; } - for (int i = startInd; i < end; i++) + for (int i = startIndex; i < end; i++) { int restartPoint; int startPoint; From 0134385bfb480d51f7f2a5a54cce7d95e562e6c5 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 11 Oct 2021 10:53:04 -0700 Subject: [PATCH 08/10] Switch switch to tuple Apparently that's the best supported way --- .../NumericComparisonExpressionNode.cs | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs index 7d1a6f82939..dec7c6e2fe2 100644 --- a/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs @@ -43,22 +43,24 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState bool isRightNum = RightChild.TryNumericEvaluate(state, out double rightNum); bool isRightVersion = RightChild.TryVersionEvaluate(state, out Version rightVersion); - return isLeftNum, isLeftVersion, isRightNum, isRightVersion switch - { - true, _, true, _ => Compare(leftNum, rightNum), - _, true, _, true => Compare(leftVersion, rightVersion), - true, _, _, true => Compare(leftNum, rightVersion), - _, true, true, _ => Compare(leftVersion, rightNum), - - _ => ProjectErrorUtilities.VerifyThrowInvalidProject - (false, + ProjectErrorUtilities.VerifyThrowInvalidProject + ((isLeftNum || isLeftVersion) && (isRightNum || isRightVersion), state.ElementLocation, "ComparisonOnNonNumericExpression", state.Condition, /* helpfully display unexpanded token and expanded result in error message */ isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), - isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); false - } + isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); + + return (isLeftNum, isLeftVersion, isRightNum, isRightVersion) switch + { + (true, _, true, _) => Compare(leftNum, rightNum), + (_, true, _, true) => Compare(leftVersion, rightVersion), + (true, _, _, true) => Compare(leftNum, rightVersion), + (_, true, true, _) => Compare(leftVersion, rightNum), + + _ => false + }; } } } From 625fd3f3e587b238514542d8c358376c0c57aa19 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 11 Oct 2021 11:31:46 -0700 Subject: [PATCH 09/10] Switch order of check I tested once with and once without this change, and with this change, Evaluate was 3% faster...that sounds noisy, but I'll still take it. --- src/Build/Evaluation/Conditionals/StringExpressionNode.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs index 4ad13c491fd..54afb1dc6b8 100644 --- a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs @@ -83,7 +83,7 @@ internal override bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationS _cachedExpandedValue = _value; return false; default: - if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@')) + if (_value[1] != '(' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@') || _value[_value.Length - 1] != ')') { // This isn't just a property, item, or metadata value, and it isn't empty. return false; From 62760c1cac87d8436dd446dd60624b89834b6e12 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 11 Oct 2021 11:48:31 -0700 Subject: [PATCH 10/10] Throw exception where necessary --- .../Evaluation/Conditionals/GenericExpressionNode.cs | 2 +- src/Build/Evaluation/Conditionals/NotExpressionNode.cs | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs index b9f9d92eff2..5963a4a61e7 100644 --- a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs @@ -60,7 +60,7 @@ internal bool Evaluate(ConditionEvaluator.IConditionEvaluationState state) state.Condition, GetExpandedValue(state)); - return boolValue; ; + return boolValue; } /// diff --git a/src/Build/Evaluation/Conditionals/NotExpressionNode.cs b/src/Build/Evaluation/Conditionals/NotExpressionNode.cs index 5a8d3516574..de73b72e736 100644 --- a/src/Build/Evaluation/Conditionals/NotExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NotExpressionNode.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.Shared; using System.Diagnostics; namespace Microsoft.Build.Evaluation @@ -17,7 +18,13 @@ internal sealed class NotExpressionNode : OperatorExpressionNode /// internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) { - LeftChild.TryBoolEvaluate(state, out bool boolValue); + ProjectErrorUtilities.VerifyThrowInvalidProject + (LeftChild.TryBoolEvaluate(state, out bool boolValue), + state.ElementLocation, + "ExpressionDoesNotEvaluateToBoolean", + LeftChild.GetUnexpandedValue(state), + LeftChild.GetExpandedValue(state), + state.Condition); return !boolValue; }