Skip to content
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

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

Open
ladipro opened this issue Mar 18, 2024 · 7 comments
Open

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

ladipro opened this issue Mar 18, 2024 · 7 comments

Comments

@ladipro
Copy link
Member

ladipro commented Mar 18, 2024

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
Copy link

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
Copy link
Member

"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
Copy link
Member

The MSBuild prefix might want to be included as well (#10102 (comment))

@rainersigwald
Copy link
Member

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

@JanKrivanek
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants