[Built-in analyzer] Undefined property is used

[ladipro]

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: No property is used before it's defined, except for conditions.

Notes

A non-existent property is silently expanded to an empty string, which can lead to unexpected behavior. We would like to flag this as a violation, except for cases where the property is tested in a condition like so '$(MyProp)' == ''.

Implementation Update

Fully working prototype: https://github.com/dotnet/msbuild/pull/10009 For easier reviewing and less conflucts during other ongoing work, the payload will be delivered in 3 subsequent payloads:

• [x] Expander refactoring https://github.com/dotnet/msbuild/pull/10102
• [ ] Wiring the data https://github.com/dotnet/msbuild/pull/10237
• [ ] Actuall BuildCheck implementation - this is blocked by:
  • [ ] https://github.com/dotnet/msbuild/issues/10123
  • [ ] https://github.com/orgs/dotnet/projects/373/views/7?pane=issue&itemId=57851137

[KalleOlaviNiemitalo]

How about self-concatenation:

<PropertyGroup>
  <DefineConstants>$(DefineConstants);DEBUG</DefineConstants>
</PropertyGroup>

[JanKrivanek]

How about self-concatenation:

<PropertyGroup>
  <DefineConstants>$(DefineConstants);DEBUG</DefineConstants>
</PropertyGroup>

Ideally that should be excluded as well... but depends on the way we go about implementing this.

In any case - this reminds me we should as well build the configurable ChainablePropertyRedefinedInNonchainableWay analyzer

[rainersigwald]

Prior art here is https://github.com/dotnet/msbuild/blob/7adbbc162c3d9f37d8b287dd03f637b1831441fa/src/Build/Evaluation/Expander.cs#L372-L376

(and it may be enough to just use that, changing the severity of the event it fires)

[JanKrivanek]

Btw. this might be something that we might want to ship as message severity (or even disabled) in initial versions. It seems to be quite common pattern where custom scripts support optional passing of build parameter from commandline and counting on conditions to eval to false on undefined value.

[ladipro]

Conditions evaluating to false on undefined value should be allowed. The hope is that actually using the value of an undefined property (e.g. passing $(MyPath)\myfile to a task) is not commonly used for legitimate purposes and flagging it would be valuable. But yes, it remains to be seen how noisy it is after we implement it and we can adjust the severity accordingly.

[JanKrivanek]

Oh I missed the part about allowing conditions - sorry. Yeah - it makes it much more usefull (as well as bit more challenging to implement :-))

[JanKrivanek]

Prior art here is

https://github.com/dotnet/msbuild/blob/7adbbc162c3d9f37d8b287dd03f637b1831441fa/src/Build/Evaluation/Expander.cs#L372-L376

(and it may be enough to just use that, changing the severity of the event it fires)

Playing with this - it does seem to catch only a case where property was used unitialized, referenced during evaluation, and then later on set to same value later during the evaluation (MSB4211: The property "{0}" is being set to a value for the first time, but it was already consumed at "{1}".).

It doesn't catch:

• totally unknown property referenced during evaluation (PropertyTrackingEvaluatorDataWrapper can fill the gap)
• uninitialized property used during target/task execution (regardless whether it's initialized later on or not)

So this will need to be reworked/adapted a bit.

In any case - it seems to keep the discussed permitted cases in mind:

https://github.com/dotnet/msbuild/blob/cd64b7b4a690d809cf14fe2af807a328cce04e54/src/Build/Evaluation/Expander.cs#L1521-L1525

[sharwell]

... except for cases where the property is tested in a condition like so '$(MyProp)' == ''

My preference is to further specialize this by stating the condition occurs as part of setting that same property:

<MyProp Condition="'$(MyProp)' == ''"

 ^^^^^^   matches   ^^^^^^^^^

[JanKrivanek]

... except for cases where the property is tested in a condition like so '$(MyProp)' == ''

My preference is to further specialize this by stating the condition occurs as part of setting that same property:

<MyProp Condition="'$(MyProp)' == ''"

 ^^^^^^   matches   ^^^^^^^^^

I quess matching versus empty in general is a reasonable exception from flagging. Even common targets use this practice quite a bit - e.g.:

https://github.com/dotnet/msbuild/blob/de776177f6d540e656e6b0c6d5bb07f2ff518c19/src/Tasks/Microsoft.Common.tasks#L44

I'm wondering whether there are other cases where usage of unitialized property in conditions might be reasonable - e.g. something like:

<Target Name="MyTarget" AfterTargets="Build" Condition="'$(OptOutFromMyTarget)' != 'true'">
...

Or maybe couple others that lead to decision for the pre-existing check to completely skip conditions. As an alternative, we might make the Check accept an optional config argument (we introduced an option to pass this via .editorconfig) for opting in checking of the conditions (excluding the self initialization, or possibly the comparison to empty in general).

Btw. this check will as well likely need to accept the scope configurability (to allow users to tune it just for their project files or their imports - but skipping sdk).