dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.23k stars 1.35k forks source link

[Feature Request]: Make error message more helpful when `TaskItem.ItemSpec` is not set #10660

Open MangelMaxime opened 2 months ago

MangelMaxime commented 2 months ago

Summary

Hello,

Improve error message if ItemSpec is missing on a TaskItem

Background and Motivation

When creating a custom MSBuild task, it took me several hours to figure out why I was getting this error:

error MSB4028: The "EasyBuild.PackageReleaseNotes.Tasks.GetCurrentReleaseTask" task's outputs could not be retrieved from the "CurrentRelease" parameter. Parameter "includeEscaped" cannot have zero length.

The reason was that I didn't set the TaskItem.ItemSpec value.

Proposed Feature

Improve the error message to mention that ItemSpec should be set.

The "EasyBuild.PackageReleaseNotes.Tasks.GetCurrentReleaseTask" task's outputs could not be retrieved from the "CurrentRelease" parameter. Please set the value of ItemSpec, in your task.

Alternative Designs

Another solution would be to change the constructor of TaskItem to take a mandatory ItemSpec value, but I suspect it would be too big of a breaking change.

baronfel commented 2 months ago

I think this could be solvable by introducing a more specific error message here - right now we have a generic 'could not bind property' message, but for something as important as the Identity/ItemSpec it makes sense to have a more directed message.

rainersigwald commented 1 month ago

@MangelMaxime can you confirm that you called new TaskItem() (the parameterless constructor)?

We should document that that is not ever intended to be called in normal code, it's just there as a trap :(

MangelMaxime commented 1 month ago

@rainersigwald

This is indeed what I am doing:

this.CurrentRelease <- TaskItem()
this.CurrentRelease.ItemSpec <- version.Version.ToString()
this.CurrentRelease.SetMetadata("Version", version.Version.ToString())

Based on your message, I suppose TaskItem have other constructors, but I didn't think to check for them. Because it didn't occurred to me that a "trap constructor" would exist 😅

rainersigwald commented 1 month ago

Yeah that's completely fair!

I'd recommend using TaskItem(string itemSpec, IDictionary itemMetadata), or new TaskItem(string) and then set metadata.

. . . now we should make the docs say the same. And maybe even hide that "so this type is COM-createable" ctor from IntelliSense or something so it's less tempting?