[Built-in analyzer] String comparison is not properly quoted

[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: String comparison in conditions should use single quotes around the strings, e.g. '$(MyProp)' == ''.

Notes

While not strictly needed for non-empty strings, the use of quotes makes the condition work for empty strings as well and is considered a good practice.

[rainersigwald]

There's debate on this; see https://github.com/mhutch/MonoDevelop.MSBuildEditor/issues/136. I continue to favor the "redundant" quotes but @KirillOsenkov and @mhutch disagreeing with me is very strong evidence.

[ladipro]

@JanKrivanek this is the candidate for an analyzer that would do static analysis "in node". The rule appears to be controversial. The other one we've considered for this category was detecting unused properties/items. That one you could argue is not super important and detecting the use of undefined properties (#9883) has a similar effect in that it flags potential typos.

[JanKrivanek]

Ok - let's probably ditch this one. Implementing would get quite nontrivial (we do not have ASTs of conditions) with questionable value

[rainersigwald]

we do not have ASTs of conditions

We do actually! It's like the only grown-up PL part of MSBuild. Check out src\Build\Evaluation\Conditionals\EqualExpressionNode.cs and that folder.

But that doesn't change anything about this rule.

[mhutch]

Ok - let's probably ditch this one. Implementing would get quite nontrivial (we do not have ASTs of conditions) with questionable value

Y'all are welcome to borrow my expression/condition parser, which produces an AST 🙂

[JanKrivanek]

@mhutch - I suppose you are reffering to https://github.com/mhutch/MonoDevelop.MSBuildEditor/blob/main/MonoDevelop.MSBuild/Language/Expressions/ExpressionParser.Conditions.cs#L7, correct?

[mhutch]

@JanKrivanek yes. I can't guarantee perfect compatibility but it does have a pretty good suite of tests, and it uses the same AST model for expressions and conditions. Plus it has positional information, for better error reporting. If you do decide to use it let's figure out a way to share the code properly 🙂

[sharwell]

I'm with @rainersigwald here. Missing quotes in property evaluation is a huge red flag.

• We should not request removing unnecessary quotes, as the resulting text requires a higher minimum level of understanding/experience to review
• We should suggest adding quotes, as there is no negative impact to users ability to read the resulting text