Lightbulb for redundant quotes

[KirillOsenkov]

would be nice to let people know that ‘$(A)’ == ‘true’ all quotes are redundant

[mhutch]

Unfortunately this is gonna be nontrivial as it'll require some resolving the type of the expression inside the quotes?

I also think it's not redundant when there's a possibility of the property being empty?

[KirillOsenkov]

we could even just detect simple cases, such as '$(A)', 'true' and 'false' - this will cover 90% of all cases

[mhutch]

I must admit I'm not 100% clear on the semantics here.

AFAIK you still need the quotes around a property that could evaluate to empty, and we would need flow analysis for that, but property and item functions that return bools should be more doable.

As for unquoted 'true'/'false' literals, I'm not sure whether you ever still need those quoted? Maybe you need them quoted when the other comparand may not be able to be coerced to a bool - e.g. it may be empty string, or 'enabled'/'on' etc.

@rainersigwald are these semantics well documented anywhere?

[rainersigwald]

No, not documented that I know of.

I actually am exactly opposed to @KirillOsenkov on this one (unusual!). My experience has guided me to "always quote", even though I'm pretty sure you're right that you don't ever need to quote true/false literals.

AFAIK you still need the quotes around a property that could evaluate to empty,

This is what I thought too but I just tried it and it looks like it's not the case, though if you have $(A) OR true and A is not defined it explodes. I wonder if there's some subtlety that we both hit, or if it's something that changed in some semi-recent release.

[KirillOsenkov]

Haha, what a coincidence! I'm ALSO exactly opposed to @rainersigwald in this case! And it is indeed unusual.

This builds fine:

<Project>

  <Target Name="Build" Condition="$(NonExisting) == false">
    <Message Text="runs!" />
  </Target>

</Project>

I think it's always safe to drop quotes around properties and true/false literals.

[KirillOsenkov]

'$(NonExisting)' or true explodes just as well, so dropping quotes is even fine here

I think quotes everywhere is just cargo cult through generations (a convention that nobody bothered to stop and check)

[rainersigwald]

I definitely had some experience that led me to endorse the cult. Wish I could remember what it was to be able to falsify it!

[KirillOsenkov]

see, when I edit MSBuild, I can afford to take risks.

when YOU edit MSBuild, you can't afford risking in the files you're editing (likely common targets). So I won't blame you for being overly cautious.

In my experience I've been omitting quotes for years and seems to have been fine so far.

[mhutch]

I suspect this is one of those things that has some existentially horrifying edge cases 😛

It's also possible that this behavior has changed over time. I'm feel pretty certain there have been a bunch of cases where I needed to use quotes to stop the evaluator from exploding, but I don't recall details.

[mhutch]

I took a look at MSBuild's condition parser and it looks like unquoted bools aren't special cases, they're actually just string nodes. So you can do this too:

<Project>
  <PropertyGroup>
    <Foo>enabled</Foo>
  </PropertyGroup>
  <Target Name="Build">
    <Message Text="runs" Condition="$(Foo) == enabled"/>
    <Message Text="does not run" Condition="$(Foo) == true"/>
    <Message Text="does not run" Condition="$(Foo) == false"/>
  </Target>
</Project>

[baronfel]

This conversation is shaking my entire worldview - I was certain that the condition parser broke on unquoted missing properties.

[slang25]

Same 😆 are you telling me I put in all of those quotes for nothing

[MichaeIDietrich]

Same here. I always used quotes, since new MSBuild files in the .NET SDKs also used quotes.

If we are now at the point that there was a lot of confusion about this, then the MSBuild documentation should probably be extended to include this information and unit tests (if missing) to ensure it stays this way for the foreseeable future. 🙊