search

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.21k stars 1.35k forks source link

[Built-in analyzer] Property that starts with an underscore is set #9891

Open ladipro opened 6 months ago

ladipro commented 6 months ago

Background

This issue tracks one of the BuildCheck analyzers we would like to ship in-box with MSBuild.

Goal

Implement an analyzer with the following rule: Properties whose name starts with an underscore should not be set in project files that are part of the project (as opposed to SDK project files).

Notes

By convention, the leading underscore is used to mark properties as internal to common targets or SDK, and not expected to be modified by the user.

KalleOlaviNiemitalo commented 6 months ago

How about props/targets files in NuGet packages; are underscores okay in those? I hope it won't be based on whether the package name starts with "Microsoft."

rainersigwald commented 6 months ago

"What is the scope of this analysis" is definitely the key question here. I think initially it should be "code in the repo doesn't do this"; eventually a nice extension would be "don't touch private variables that don't belong to you", with some kind of detection where like paired props/targets or "within a NuGet package" are allowed but not touching "someone else's".

JanKrivanek commented 4 months ago

The MSBuild prefix might want to be included as well (https://github.com/dotnet/msbuild/pull/10102#discussion_r1598285182)

rainersigwald commented 4 months ago

@JanKrivanek many MSBuild-prefix properties are intended to be set in user code, like MSBuildTreatWarningsAsErrors.

JanKrivanek commented 4 months ago

We might seed this with some initial list of reserved properties (beyond the '_' prefix), plus an optional custom configuration key - something like 'forbidden_write_properties_csv'

rainersigwald commented 4 months ago

I think, as a user, that I'd prefer to have that split in three error codes:

  1. "privates" (_ prefix)
  2. MSBuild-team reserved properties
  3. configurable deny-list

Mostly just so the docs page for the error can be super clear about remediation, especially around 3.

JanKrivanek commented 4 months ago

I like that idea. (all those rules would still be contained within single executing BuildCheck)