Skip to content

Simplify verify throw and remove unnecessary usings #6999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 2, 2021
Merged

Simplify verify throw and remove unnecessary usings #6999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
110571d
Remove unnecessary usings
Forgind Oct 14, 2021
96c8fa0
Move condition outside VerifyThrow
Forgind Oct 14, 2021
c3929ad
Other VerifyThrow simplifications
Forgind Oct 14, 2021
84c7a23
Directly throw exceptions
Forgind Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
using Microsoft.Build.BackEnd.Logging;
using Microsoft.Build.BackEnd.SdkResolution;
using Microsoft.Build.Collections;
using Microsoft.Build.Construction;
using Microsoft.Build.Evaluation;
using Microsoft.Build.Eventing;
using Microsoft.Build.Exceptions;
Expand All @@ -31,7 +30,6 @@
using Microsoft.Build.Logging;
using Microsoft.Build.Shared;
using Microsoft.Build.Shared.Debugging;
using Microsoft.Build.Shared.FileSystem;
using Microsoft.Build.Utilities;
using ForwardingLoggerRecord = Microsoft.Build.Logging.ForwardingLoggerRecord;
using LoggerDescription = Microsoft.Build.Logging.LoggerDescription;
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/Logging/BuildEventArgTransportSink.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public IDictionary<int, ISet<string>> WarningsAsMessagesByProject
/// </summary>
public void Consume(BuildEventArgs buildEvent)
{
ErrorUtilities.VerifyThrow(false, "Do not use this method for the transport sink");
ErrorUtilities.ThrowInternalError("Do not use this method for the transport sink");
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/Logging/EventSourceSink.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ public void Consume(BuildEventArgs buildEvent)
}
else
{
ErrorUtilities.VerifyThrow(false, "Unknown event args type.");
ErrorUtilities.ThrowInternalError("Unknown event args type.");
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/Logging/LoggingService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1417,7 +1417,7 @@ private void RouteBuildEvent(object loggingEvent)
}
else
{
ErrorUtilities.VerifyThrow(false, "Unknown logging item in queue:" + loggingEvent.GetType().FullName);
ErrorUtilities.ThrowInternalError("Unknown logging item in queue:" + loggingEvent.GetType().FullName);
}

if (buildEventArgs is BuildWarningEventArgs warningEvent)
Expand Down
5 changes: 2 additions & 3 deletions src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,7 @@ ElementLocation elementLocation
// to really loop here... it's just that the foreach is the only way I can
// figure out how to get data out of the hashtable without knowing any of the
// keys!
ProjectErrorUtilities.VerifyThrowInvalidProject(false,
elementLocation, "CannotReferenceItemMetadataWithoutItemName", unqualifiedMetadataName);
ProjectErrorUtilities.ThrowInvalidProject(elementLocation, "CannotReferenceItemMetadataWithoutItemName", unqualifiedMetadataName);
}
}
else
Expand Down Expand Up @@ -410,7 +409,7 @@ ElementLocation elementLocation
}
catch (InvalidOperationException e)
{
ProjectErrorUtilities.VerifyThrowInvalidProject(false, elementLocation,
ProjectErrorUtilities.ThrowInvalidProject(elementLocation,
"CannotEvaluateItemMetadata", metadataName, e.Message);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ private List<ProjectItemInstance> FindItemsMatchingMetadataSpecification(
ItemSpec<ProjectPropertyInstance, ProjectItemInstance> itemSpec = new ItemSpec<ProjectPropertyInstance, ProjectItemInstance>(child.Remove, expander, child.RemoveLocation, Project.Directory, true);
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(
itemSpec.Fragments.All(f => f is ItemSpec<ProjectPropertyInstance, ProjectItemInstance>.ItemExpressionFragment),
new BuildEventFileInfo(string.Empty),
BuildEventFileInfo.Empty,
"OM_MatchOnMetadataIsRestrictedToReferencedItems",
child.RemoveLocation,
child.Remove);
Expand Down
4 changes: 2 additions & 2 deletions src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,7 @@ private void UpdateContinueOnError(ItemBucket bucket, TaskHost taskHost)
catch (ArgumentException e)
{
// handle errors in string-->bool conversion
ProjectErrorUtilities.VerifyThrowInvalidProject(false, _taskNode.ContinueOnErrorLocation, "InvalidContinueOnErrorAttribute", _taskNode.Name, e.Message);
ProjectErrorUtilities.ThrowInvalidProject(_taskNode.ContinueOnErrorLocation, "InvalidContinueOnErrorAttribute", _taskNode.Name, e.Message);
}
}

Expand Down Expand Up @@ -740,7 +740,7 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
if (!taskExecutionHost.SetTaskParameters(_taskNode.ParametersForBuild))
{
// The task cannot be initialized.
ProjectErrorUtilities.VerifyThrowInvalidProject(false, _targetChildInstance.Location, "TaskParametersError", _taskNode.Name, String.Empty);
ProjectErrorUtilities.ThrowInvalidProject(_targetChildInstance.Location, "TaskParametersError", _taskNode.Name, String.Empty);
}
else
{
Expand Down
16 changes: 5 additions & 11 deletions src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#endif
using System.Text;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.Build.BackEnd.Logging;
using Microsoft.Build.Construction;
Expand Down Expand Up @@ -442,9 +441,8 @@ bool ITaskExecutionHost.GatherTaskOutputs(string parameterName, ElementLocation
}
else
{
ProjectErrorUtilities.VerifyThrowInvalidProject
ProjectErrorUtilities.ThrowInvalidProject
(
false,
parameterLocation,
"UnsupportedTaskParameterTypeError",
parameter.PropertyType.FullName,
Expand Down Expand Up @@ -482,9 +480,8 @@ bool ITaskExecutionHost.GatherTaskOutputs(string parameterName, ElementLocation

// We do not recover from a task exception while getting outputs,
// so do not merely set gatheredGeneratedOutputsSuccessfully = false; here
ProjectErrorUtilities.VerifyThrowInvalidProject
ProjectErrorUtilities.ThrowInvalidProject
(
false,
parameterLocation,
"FailedToRetrieveTaskOutputs",
_taskName,
Expand All @@ -500,9 +497,8 @@ bool ITaskExecutionHost.GatherTaskOutputs(string parameterName, ElementLocation
throw;
}

ProjectErrorUtilities.VerifyThrowInvalidProject
ProjectErrorUtilities.ThrowInvalidProject
(
false,
parameterLocation,
"FailedToRetrieveTaskOutputs",
_taskName,
Expand Down Expand Up @@ -1183,11 +1179,9 @@ out bool taskParameterSet
{
// We only allow a single item to be passed into a parameter of ITaskItem.

// Some of the computation (expansion) here is expensive, so don't make the above
// "if" statement directly part of the first param to VerifyThrowInvalidProject.
ProjectErrorUtilities.VerifyThrowInvalidProject
// Some of the computation (expansion) here is expensive, so don't switch to VerifyThrowInvalidProject.
ProjectErrorUtilities.ThrowInvalidProject
(
false,
parameterLocation,
"CannotPassMultipleItemsIntoScalarParameter",
_batchBucket.Expander.ExpandIntoStringAndUnescape(parameterValue, ExpanderOptions.ExpandAll, parameterLocation),
Expand Down
2 changes: 2 additions & 0 deletions src/Build/Construction/Solution/ProjectInSolution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
using System.Security;
using System.Text;
using System.Xml;
#if !NETFRAMEWORK || MONO
using Microsoft.Build.Shared;
#endif

using XMakeAttributes = Microsoft.Build.Shared.XMakeAttributes;
using ProjectFileErrorUtilities = Microsoft.Build.Shared.ProjectFileErrorUtilities;
Expand Down
12 changes: 4 additions & 8 deletions src/Build/Construction/Solution/SolutionProjectGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,9 +1133,8 @@ private bool CanBuildDirectly(ProjectInstance traversalProject, ProjectInSolutio
{
if (!_solutionFile.ProjectsByGuid.TryGetValue(dependencyProjectGuid, out ProjectInSolution dependencyProject))
{
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile
ProjectFileErrorUtilities.ThrowInvalidProjectFile
(
false,
"SubCategoryForSolutionParsingErrors",
new BuildEventFileInfo(traversalProject.FullPath),
"SolutionParseProjectDepNotFoundError",
Expand Down Expand Up @@ -1206,9 +1205,8 @@ private ProjectInstance CreateMetaproject(ProjectInstance traversalProject, Proj
if (project.ProjectType == SolutionProjectType.WebProject)
{
#if !FEATURE_ASPNET_COMPILER
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile
ProjectFileErrorUtilities.ThrowInvalidProjectFile
(
false,
"SubCategoryForSolutionParsingErrors",
new BuildEventFileInfo(_solutionFile.FullPath),
"AspNetCompiler.UnsupportedMSBuildVersion",
Expand Down Expand Up @@ -1295,9 +1293,8 @@ private void AddMetaprojectReferenceItems(ProjectInstance traversalProject, Proj
{
if (!_solutionFile.ProjectsByGuid.TryGetValue(dependencyProjectGuid, out ProjectInSolution dependencyProject))
{
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile
ProjectFileErrorUtilities.ThrowInvalidProjectFile
(
false,
"SubCategoryForSolutionParsingErrors",
new BuildEventFileInfo(traversalProject.FullPath),
"SolutionParseProjectDepNotFoundError",
Expand Down Expand Up @@ -1557,9 +1554,8 @@ private void ValidateTargetFrameworkForWebProject(ProjectInSolution project)
}
if (!isDotNetFramework)
{
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile
ProjectFileErrorUtilities.ThrowInvalidProjectFile
(
false,
"SubCategoryForSolutionParsingErrors",
new BuildEventFileInfo(_solutionFile.FullPath),
"AspNetCompiler.InvalidTargetFrameworkMonikerNotDotNET",
Expand Down
2 changes: 1 addition & 1 deletion src/Build/Definition/Toolset.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ private void LoadAndRegisterFromTasksFile(string[] defaultTaskFiles, ILoggingSer
catch (XmlException e)
{
// handle XML errors in the default tasks file
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(false, new BuildEventFileInfo(defaultTasksFile, e), taskFileError, e.Message);
ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(defaultTasksFile, e), taskFileError, e.Message);
}
catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))
{
Expand Down
6 changes: 3 additions & 3 deletions src/Build/Errors/InternalLoggerException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public sealed class InternalLoggerException : Exception
/// <exception cref="InvalidOperationException"></exception>
public InternalLoggerException()
{
ErrorUtilities.VerifyThrowInvalidOperation(false, "InternalLoggerExceptionOnlyThrownByEngine");
ErrorUtilities.ThrowInvalidOperation("InternalLoggerExceptionOnlyThrownByEngine");
}

/// <summary>
Expand All @@ -47,7 +47,7 @@ public InternalLoggerException()
public InternalLoggerException(string message)
: base(message)
{
ErrorUtilities.VerifyThrowInvalidOperation(false, "InternalLoggerExceptionOnlyThrownByEngine");
ErrorUtilities.ThrowInvalidOperation("InternalLoggerExceptionOnlyThrownByEngine");
}

/// <summary>
Expand All @@ -62,7 +62,7 @@ public InternalLoggerException(string message)
public InternalLoggerException(string message, Exception innerException)
: base(message, innerException)
{
ErrorUtilities.VerifyThrowInvalidOperation(false, "InternalLoggerExceptionOnlyThrownByEngine");
ErrorUtilities.ThrowInvalidOperation("InternalLoggerExceptionOnlyThrownByEngine");
}

#endregion
Expand Down
22 changes: 13 additions & 9 deletions src/Build/Evaluation/Conditionals/AndExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ internal sealed class AndExpressionNode : OperatorExpressionNode
/// </summary>
internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state)
{
ProjectErrorUtilities.VerifyThrowInvalidProject
(LeftChild.TryBoolEvaluate(state, out bool leftBool),
if (!LeftChild.TryBoolEvaluate(state, out bool leftBool))
{
ProjectErrorUtilities.ThrowInvalidProject(
state.ElementLocation,
"ExpressionDoesNotEvaluateToBoolean",
LeftChild.GetUnexpandedValue(state),
LeftChild.GetExpandedValue(state),
state.Condition);
}

if (!leftBool)
{
Expand All @@ -33,13 +35,15 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState
}
else
{
ProjectErrorUtilities.VerifyThrowInvalidProject
(RightChild.TryBoolEvaluate(state, out bool rightBool),
state.ElementLocation,
"ExpressionDoesNotEvaluateToBoolean",
RightChild.GetUnexpandedValue(state),
RightChild.GetExpandedValue(state),
state.Condition);
if (!RightChild.TryBoolEvaluate(state, out bool rightBool))
{
ProjectErrorUtilities.ThrowInvalidProject(
state.ElementLocation,
"ExpressionDoesNotEvaluateToBoolean",
RightChild.GetUnexpandedValue(state),
RightChild.GetExpandedValue(state),
state.Condition);
}

return rightBool;
}
Expand Down
3 changes: 1 addition & 2 deletions src/Build/Evaluation/Conditionals/FunctionCallExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState
// We haven't implemented any other "functions"
else
{
ProjectErrorUtilities.VerifyThrowInvalidProject(
false,
ProjectErrorUtilities.ThrowInvalidProject(
state.ElementLocation,
"UndefinedFunctionCall",
state.Condition,
Expand Down
14 changes: 8 additions & 6 deletions src/Build/Evaluation/Conditionals/GenericExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,14 @@ internal virtual bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationSt
/// <returns></returns>
internal bool Evaluate(ConditionEvaluator.IConditionEvaluationState state)
{
ProjectErrorUtilities.VerifyThrowInvalidProject(
TryBoolEvaluate(state, out bool boolValue),
state.ElementLocation,
"ConditionNotBooleanDetail",
state.Condition,
GetExpandedValue(state));
if (!TryBoolEvaluate(state, out bool boolValue))
{
ProjectErrorUtilities.ThrowInvalidProject(
state.ElementLocation,
"ConditionNotBooleanDetail",
state.Condition,
GetExpandedValue(state));
}

return boolValue;
}
Expand Down
17 changes: 10 additions & 7 deletions src/Build/Evaluation/Conditionals/NotExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,16 @@ internal sealed class NotExpressionNode : OperatorExpressionNode
/// </summary>
internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state)
{
ProjectErrorUtilities.VerifyThrowInvalidProject
(LeftChild.TryBoolEvaluate(state, out bool boolValue),
state.ElementLocation,
"ExpressionDoesNotEvaluateToBoolean",
LeftChild.GetUnexpandedValue(state),
LeftChild.GetExpandedValue(state),
state.Condition);
if (!LeftChild.TryBoolEvaluate(state, out bool boolValue))
{
ProjectErrorUtilities.ThrowInvalidProject(
state.ElementLocation,
"ExpressionDoesNotEvaluateToBoolean",
LeftChild.GetUnexpandedValue(state),
LeftChild.GetExpandedValue(state),
state.Condition);
}

return !boolValue;
}

Expand Down
18 changes: 10 additions & 8 deletions src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,16 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState
bool isRightNum = RightChild.TryNumericEvaluate(state, out double rightNum);
bool isRightVersion = RightChild.TryVersionEvaluate(state, out Version rightVersion);

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));
if ((!isLeftNum && !isLeftVersion) || (!isRightNum && !isRightVersion))
Copy link
Member

@benvillalobos benvillalobos Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💗 simple change yet the code is FAR more readable

Copy link
Contributor Author

@Forgind Forgind Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to mention performant! This prevents us from randomly expanding values when we don't expect to need them.

{
ProjectErrorUtilities.ThrowInvalidProject(
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));
}

return (isLeftNum, isLeftVersion, isRightNum, isRightVersion) switch
{
Expand Down
1 change: 0 additions & 1 deletion src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using Microsoft.Build.Shared;

namespace Microsoft.Build.Evaluation
{
Expand Down
Loading