Warn for conditional clauses that are either always true or never true

[danmoseley]

The below was a typo we overlooked in our repo. Strictly, the expression is "valid" but the possibility it was intended is zero.

Have you considered warning for conditions or clauses in conditions that will either ALWAYS or NEVER be true? In this case, it will always be true. I am guessing such clauses are very rarely intentional.

<Project>

   <ItemGroup Condition="'$(RunDisabledAppleSiliconTests' != 'true'"/>

   <Target Name="dummy"/>
</Project>

C:\proj\1>dotnet --version
6.0.100-preview.2.21155.3

C:\proj\1>dotnet --info
.NET SDK (reflecting any global.json):
 Version:   6.0.100-preview.2.21155.3
 Commit:    1a9103db2d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19042
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100-preview.2.21155.3\

Host (useful for support):
  Version: 6.0.0-preview.2.21154.6
  Commit:  3eaf1f316b

...

[danmoseley]

For perf reasons this might require a "lint" mode

[benvillalobos]

Team Triage: Should we consider that if what we're parsing as a property has $(, and no closing ), log a warning based off of that?

This could also be expanded to special MSBuildisms like %( @( $(.

[danmoseley]

I figured just generally warning for clauses that are always true or always false would be much easier as there's no parser work. Basically after parsing, examine any comparison of string/number/bool to string/number/bool where there are no expandable elements (e.g. no $ or % or @ ...I forget whether this falls directly out of the parsing). In fact if it can be done like that it's nearly free and wouldn't need an opt in lint mode.

[benvillalobos]

I don't understand how we can guarantee whether or not something would always be true or never be true? Unless you mean comparing different types.

eg:
"$(foo)" resolves to false, and we're comparing it to true, so no warning because they're the same type and "could have been" equal.
"$(foo" resolves to "$(foo" and is being compared to true, so log a warning that these mismatched types could never be equal.

Is this what you mean?

[danmoseley]

Yes, exactly. If your parse tree contains a subtree like

|
equal
|          \ 
string   string

and string isn't expandable, then we can immediately warn because the clause will always have the same result and at best is redundant, at worst is a typo.

Similarly for other comparisons between string/bool/number - the key being there's nothing expandable on either side.

I don't think it's necessary to consider more than individual clauses, eg to handle cases like $(A) == '1' || $(A) != '1' -- also pointless, but less likely to be the result of a typo.

If instead you extended the parser to try to catch typos, I would imagine it could be much more costly/fragile. I don't know much about parsers, but I would expect that would be better handled by changing to a parser generator, if there is one, that offers a feature for spotting expressions that are "nearly something else".

[benvillalobos]

and string isn't expandable

In this case you mean the string on both sides of the equal aren't expanded?

My understanding for cases to warn:

1. Comparison between two different types
2. Comparison between two items of the same type, where neither expanded

[danmoseley]

In this case you mean the string on both sides of the equal aren't expanded

Right

Comparison between two different types

I don't think the type is relevant as there's coercion . So '1' == 1.0 or 'YES' == true are valid but would warn but '$(foo)' == 1 is valid and would not warn.

[danmoseley]

Here's another example from today where a typo caused a condition to always be false (extra space after property name)
<EnableUnsafeUTF7Encoding Condition="'$(EnableUnsafeUTF7Encoding )' == ''">false</EnableUnsafeUTF7Encoding >

Ideally MSBuild would flag this.