-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Display an error if an ITaskFactory does not set the TaskType property. #2363
Conversation
If TaskType is null it can cause a NullReferenceException in our code. This change ensures that ITaskFactory implementations return a value for this property. Fixes dotnet#1508
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, test isn't passing though!
src/Build/Instance/TaskRegistry.cs
Outdated
@@ -1425,6 +1431,10 @@ private bool GetTaskFactory(TargetLoggingContext targetLoggingContext, ElementLo | |||
return false; | |||
} | |||
} | |||
catch(InvalidProjectFileException) | |||
{ | |||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new ProjectErrorUtilities.ThrowInvalidProject()
is in a try...catch
which wraps the exception again. This new catch
is to allow the original exception to be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment, please :)
I'm also not sure I think it's worth the unique error code for this if the message is good. But it's already done so go with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I've removed the unique error code and thrown an InvalidOperationException
so the overall error contains the message
nulltasktype.proj(10,5): error MSB4175: The task factory "Microsoft.Build.UnitTests.BackEnd.TaskRegistry_Tests+NullTaskTypeTaskFactory" could not be loaded from the assembly "D:\msbuild\bin\Debug\AnyCPU\Windows_NT\Output\Microsoft.Build.Engine.UnitTests.dll". The task factory must return a value for the "TaskType" property.
@dotnet-bot test Windows_NT Build for CoreCLR please |
src/Build/Instance/TaskRegistry.cs
Outdated
@@ -1425,6 +1431,10 @@ private bool GetTaskFactory(TargetLoggingContext targetLoggingContext, ElementLo | |||
return false; | |||
} | |||
} | |||
catch(InvalidProjectFileException) | |||
{ | |||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment, please :)
I'm also not sure I think it's worth the unique error code for this if the message is good. But it's already done so go with it.
If TaskType is null it can cause a NullReferenceException in our code. This change ensures that ITaskFactory implementations return a value for this property.
Fixes #1508