MSBuild linter

[rainersigwald]

MSBuild could have an opt-in mode that would apply rules and heuristics to give suggestions about project "health".

Possible warnings:

• You have something that is almost an item transform but not well formed so falls back to string (like @(I->'%(Identity)) with a missing closing ').
• You have referred to a property that is not defined and it expanded to the empty string.
• You have referred to a property that is not defined, but an item of the same name is defined.
• You have referred to an item that is not defined, but a property of the same name is defined.

[rainersigwald]

Thanks, @danmosemsft: #1774 (comment).

[stan-sz]

My favorite linter rule would be to catch ItemGroups with wildcards that scan the entire drive:

<ItemGroup>
  <MyItem Include="$(SomeProperty)/**" />
</ItemGroup>

Where SomeProperty resolves to empty sting.

[rainersigwald]

@stan-sz that is a great linter use case but also see #3642 (comment) and the upcoming #7029.

[stan-sz]

Another case are conditions in form of

Condition="$(SomeProperty)">

where at least the linter should signal the missing single quotes and comparison with empty string like:

Condition=" '$(SomeProperty)' != '' ">

[stan-sz]

Some of the items listed in the initial scope for linter are captured in Feature: Warning Waves

[hknielsen]

We had an interesting issue today in our team, where:

<ProjectReference...><Properties></Properties>

Broke Publishing, and was not trivial what was happening, until we remembered that ProjectReference mapped to MsBuild task and removed the Properties delegated trough, removing the TargetFramework.

Interesting case as well for a linter warning, suggest to use AdditinalProperties instead

[stan-sz]

For the record: a linter to catch missing single quotes in string comparison

Condition=" $(SomeProperty) != 'true' ">

[KalleOlaviNiemitalo]

What's the difference between $(SomeProperty) and '$(SomeProperty)'?
It looks like Microsoft.Build.Evaluation.Parser.Arg(string expression) creates a StringExpressionNode in both cases.

[stan-sz]

This is to handle case when a property evaluates to an empty value. From: https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-conditions?view=vs-2022

Single quotes are not required for simple alphanumeric strings or boolean values. However, single quotes are required for empty values. This check is case insensitive.

[KalleOlaviNiemitalo]

quoted.proj:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Run">
  <PropertyGroup>
    <Property />
  </PropertyGroup>

  <Target Name="Run">
    <Message Condition="$(Property) == ''" Importance="high" Text="Is empty without quotation marks" />
    <Message Condition="$(Property) != ''" Importance="high" Text="Is not empty without quotation marks" />
    <Message Condition="'$(Property)' == ''" Importance="high" Text="Is empty with quotation marks" />
    <Message Condition="'$(Property)' != ''" Importance="high" Text="Is not empty with quotation marks" />
  </Target>
</Project>

C:\Projects\quoted>C:\Windows\Microsoft.NET\Framework64\v2.0.50727\MSBuild.exe quoted.proj
Microsoft (R) Build Engine Version 2.0.50727.9149
[Microsoft .NET Framework, Version 2.0.50727.9174]
Copyright (C) Microsoft Corporation 2005. All rights reserved.

Build started 12.9.2023 21.12.13.
__________________________________________________
Project "C:\Projects\quoted\quoted.proj" (default targets):

Target Run:
    Is empty without quotation marks
    Is empty with quotation marks

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.26

$ dotnet msbuild quoted.proj
MSBuild version 17.8.0-preview-23367-03+0ff2a83e9 for .NET
  Is empty without quotation marks
  Is empty with quotation marks

Perhaps the documentation means you cannot compare to an empty unquoted string literal like Condition="$(Property) == ".

[Piedone]

This could also include formatting checks like Prettier for XML does.

Ideally, I'm thinking of full static code analysis for csproj, props, and targets files.

[danmoseley]

Another possibility (in strict mode?) could be to help with Boolean properties. Properties of course have no type but in practice many are Boolean existing in a world where empty string is default. This can lead to ambiguity about what blank means, and since often we don't want to add a line to explicitly set a default, blank is treated as default. If that means blank is true, then properties end up with confusing negative names like say "DoNotOptimize". The obvious sources of errors here are the confusion of seeing a negative property to false, or assuming blank means true (or false) when it doesn't, or comparing against blank when it was set explicitly, or setting anything other than true or false as the value.

Years ago when we wrote the original "targets" I tried to have a rule that either blank was assumed to be false or else the value was explicitly defaulted to "true", then comparisons were always and only against "true", "false" and "" never appeared in conditionals on these Booleans, and hopefully property names were not negative (in practice some were inherited from the old format and already were).

A linter could possibly help by

• flagging comparisons between a literal Boolean and empty string
• inferring a property is Boolean (maybe indirectly) and tracking flow and doing something similar
• flagging where a property held literal true or false but is set to anything else

This would of course be noisy. Perhaps the real answer is to have some way to annotate a property as Boolean with a default.

Note of course that the conditional parser IIRC does have some special casing for Booleans for example you can negate: "'$(Optimize)' == !'$(Debug)'" ...IIRC

[stan-sz]

Another linting ideas:

• Show warning when referencing an Item as a Property or viceversa #348
• no-op property overwrite (e.g. when .csproj sets the same property with the same value as already defined/inherited from Directory.Build.props). This would help keep projects minimal

[stan-sz]

Another linter idea: #6277

[stan-sz]

Another idea: #3976

[JustinSchneiderPBI]

Fail\warn on property overwrite without Overwrite="true" metadata.

[MattKotsenas]

Another idea:

A colleague just ran into a case where a project specified <TargetFrameworks> (plural) that was silently overwritten by an imported <TargetFramework> (singular).

[DaveCousineau]

Not sure if it's a bug, something to catch with a linter, or maybe if there's already a workaround, but a problem I'm noticing is that this is an error (MSB4035):

<ProjectReference Include="" />

But these are not errors or even warnings:

<ProjectReference Include="$(empty)" />
<ProjectReference Include=" " />