Eval cond perf (#6859)

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

* Use search for @

* Fix incorrect bool check

* Replace Can*Evaluate and *Evaluate with Try*Evaluate

* Switch order of check

src/Build/Evaluation/Conditionals/AndExpressionNode.cs

@@ -19,29 +19,29 @@ 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;
             }
             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;
             }
         }
 

src/Build/Evaluation/Conditionals/GenericExpressionNode.cs

@@ -12,12 +12,9 @@ namespace Microsoft.Build.Evaluation
     /// </summary>
     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);
 
         /// <summary>
         /// Returns true if this node evaluates to an empty string,
@@ -57,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;
         }
 
         /// <summary>

src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs

@@ -48,36 +48,41 @@ 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))
+            else if (LeftChild.TryNumericEvaluate(state, out double leftNumericValue) && RightChild.TryNumericEvaluate(state, out double rightNumericValue))
             {
-                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).
+                return Compare(leftNumericValue, rightNumericValue);
             }
-            else if (LeftChild.CanBoolEvaluate(state) && RightChild.CanBoolEvaluate(state))
+            else if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue) && RightChild.TryBoolEvaluate(state, out bool rightBoolValue))
             {
-                return Compare(LeftChild.BoolEvaluate(state), RightChild.BoolEvaluate(state));
+                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);
         }
 
         /// <summary>

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,12 +18,14 @@ internal sealed class NotExpressionNode : OperatorExpressionNode
         /// </summary>
         internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state)
         {
-            return !LeftChild.BoolEvaluate(state);
-        }
-
-        internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state)
-        {
-            return LeftChild.CanBoolEvaluate(state);
+            ProjectErrorUtilities.VerifyThrowInvalidProject
+                    (LeftChild.TryBoolEvaluate(state, out bool boolValue),
+                     state.ElementLocation,
+                     "ExpressionDoesNotEvaluateToBoolean",
+                     LeftChild.GetUnexpandedValue(state),
+                     LeftChild.GetExpandedValue(state),
+                     state.Condition);
+            return !boolValue;
         }
 
         /// <summary>

src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs

@@ -38,46 +38,29 @@ internal abstract class NumericComparisonExpressionNode : OperatorExpressionNode
         /// </summary>
         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,
+                ((isLeftNum || isLeftVersion) && (isRightNum || isRightVersion),
                  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));
+                 isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state),
+                 isLeftNum ? 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 (isLeftNum, isLeftVersion, isRightNum, isRightVersion) switch
             {
-                return Compare(LeftChild.NumericEvaluate(state), RightChild.NumericEvaluate(state));
-            }
-            else if (isVersion)
-            {
-                return Compare(LeftChild.VersionEvaluate(state), RightChild.VersionEvaluate(state));
-            }
-
-            // 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));
-            }
+                (true, _, true, _) => Compare(leftNum, rightNum),
+                (_, true, _, true) => Compare(leftVersion, rightVersion),
+                (true, _, _, true) => Compare(leftNum, rightVersion),
+                (_, true, true, _) => Compare(leftVersion, rightNum),
 
-            // Throw error here as this code should be unreachable
-            ErrorUtilities.ThrowInternalErrorUnreachable();
-            return false;
+                _ => false
+            };
         }
     }
 }

src/Build/Evaluation/Conditionals/NumericExpressionNode.cs

@@ -21,60 +21,20 @@ internal NumericExpressionNode(string value)
             _value = value;
         }
 
-        /// <summary>
-        /// Evaluate as boolean
-        /// </summary>
-        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;
-        }
-
-        /// <summary>
-        /// Evaluate as numeric
-        /// </summary>
-        internal override double NumericEvaluate(ConditionEvaluator.IConditionEvaluationState state)
-        {
-            return ConversionUtilities.ConvertDecimalOrHexToDouble(_value);
-        }
-
-        /// <summary>
-        /// Evaluate as a Version
-        /// </summary>
-        internal override Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state)
+        internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result)
         {
-            return Version.Parse(_value);
-        }
-
-        /// <summary>
-        /// Whether it can be evaluated as a boolean: never allowed for numerics
-        /// </summary>
-        internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state)
-        {
-            // Numeric expressions are never allowed to be treated as booleans.
+            result = default;
             return false;
         }
 
-        /// <summary>
-        /// Whether it can be evaluated as numeric
-        /// </summary>
-        internal override bool CanNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state)
+        internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result)
         {
-            // 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);
+            return ConversionUtilities.TryConvertDecimalOrHexToDouble(_value, out result);
         }
 
-        /// <summary>
-        /// Whether it can be evaluated as a Version
-        /// </summary>
-        internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state)
+        internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result)
         {
-            // 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 result);
         }
 
         /// <summary>

src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs

@@ -11,48 +11,23 @@ namespace Microsoft.Build.Evaluation
     /// </summary>
     internal abstract class OperatorExpressionNode : GenericExpressionNode
     {
-        /// <summary>
-        /// Numeric evaluation is never allowed for operators
-        /// </summary>
-        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;
-        }
-
-        /// <summary>
-        /// Version evaluation is never allowed for operators
-        /// </summary>
-        internal override Version VersionEvaluate(ConditionEvaluator.IConditionEvaluationState state)
-        {
-            ErrorUtilities.VerifyThrow(false, "Cannot version evaluate an operator");
-            return null;
-        }
-
-        /// <summary>
-        /// Whether boolean evaluation is allowed: always allowed for operators
-        /// </summary>
-        internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state)
+        internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result)
         {
+            result = BoolEvaluate(state);
             return true;
         }
 
-        /// <summary>
-        /// Whether the node can be evaluated as a numeric: by default,
-        /// this is not allowed
-        /// </summary>
-        internal override bool CanNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state)
+        internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state);
+
+        internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result)
         {
+            result = default;
             return false;
         }
 
-        /// <summary>
-        /// Whether the node can be evaluated as a version: by default,
-        /// this is not allowed
-        /// </summary>
-        internal override bool CanVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state)
+        internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result)
         {
+            result = default;
             return false;
         }
 

src/Build/Evaluation/Conditionals/OrExpressionNode.cs

@@ -19,29 +19,29 @@ 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;
             }
             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;
             }
         }