Invalid comparison in MSBuild's ignore-conditions mode in outer build of multitargeted project

[rainersigwald]

Originally reported by @batzen at https://github.com/dotnet/cli/issues/12267#issuecomment-522067627

• .NET Core Version: 3.0.100-preview9-013744
• Windows version: 10.0.18362.239
• Does the bug reproduce also in WPF for .NET Framework 4.8?: No

Problem description:

A multitargeted WPF project throws an exception when evaluated in MSBuild's "ignore conditions" mode, which is often used in design-time scenarios.

This is because while the conditions here are sufficient at build time for real projects, when conditions are ignored MSBuild thinks the project is invalid (dotnet/msbuild#4622)

wpf/packaging/Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props

Lines 98 to 121 in d6c7ae5

	<ItemGroup Condition="('$(TargetFrameworkIdentifier)' == '.NETFramework') And ('$(_TargetFrameworkVersionWithoutV)' != '') And
	('$(_TargetFrameworkVersionWithoutV)' >= '$(_WindowsDesktopSdkTargetFrameworkVersionFloor)')">
	<!--
	The following 3 _WpfCommonNetFxReference items normally require Condition="'$(_TargetFrameworkVersionWithoutV)' >= '3.0'", since
	they are supported on .NET Framework 3.0 and above.
	This condition is implicitly satisfied by '$(_TargetFrameworkVersionWithoutV)' >= '$(_WindowsDesktopSdkTargetFrameworkVersionFloor)'
	in the outer ItemGroup
	-->
	<_WpfCommonNetFxReference Include="WindowsBase" />
	<_WpfCommonNetFxReference Include="PresentationCore" />
	<_WpfCommonNetFxReference Include="PresentationFramework" />
	<_WpfCommonNetFxReference Include="System.Xaml" Condition="'$(_TargetFrameworkVersionWithoutV)' >= '4.0'">
	<RequiredTargetFramework>4.0</RequiredTargetFramework>
	</_WpfCommonNetFxReference>
	<_WpfCommonNetFxReference Include="UIAutomationClient" Condition="'$(_TargetFrameworkVersionWithoutV)' >= '4.0'" />
	<_WpfCommonNetFxReference Include="UIAutomationClientSideProviders" Condition="'$(_TargetFrameworkVersionWithoutV)' >= '4.0'" />
	<_WpfCommonNetFxReference Include="UIAutomationProvider" Condition="'$(_TargetFrameworkVersionWithoutV)' >= '4.0'" />
	<_WpfCommonNetFxReference Include="UIAutomationTypes" Condition="'$(_TargetFrameworkVersionWithoutV)' >= '4.0'" />
	<_WpfCommonNetFxReference Include="System.Windows.Controls.Ribbon" Condition="'$(_TargetFrameworkVersionWithoutV)' >= '4.5'" />
	</ItemGroup>

Actual behavior:

error: A numeric comparison was attempted on "$(_TargetFrameworkVersionWithoutV)" that evaluates to "" instead of a number, in condition "'$(_TargetFrameworkVersionWithoutV)' >= '4.5'".  c:\program files\dotnet\sdk\3.0.100-preview8-013656\Sdks\Microsoft.NET.Sdk.WindowsDesktop\targets\Microsoft.NET.Sdk.WindowsDesktop.props

Expected behavior:

No error

Minimal repro:

• dotnet new wpf
• Replace TargetFramework with <TargetFrameworks>netcoreapp3.0;net45</TargetFrameworks>
• dotnet restore
• dotnet list package

[rainersigwald]

I believe this could be worked around by moving that ItemGroup into a conditionally-imported file, and ensuring that when the condition on the import is true, _TargetFrameworkVersionWithoutV will be defined.

If this doesn't meet the bar for 3.0 (I wouldn't push for it, personally), it might be better to use dotnet/msbuild#3212, but that's not yet implemented.

[batzen]

This also affects loading projects/folders in VS code because omnisharp fails with

Microsoft.Build.Exceptions.InvalidProjectFileException: A numeric comparison was attempted on "$(_TargetFrameworkVersionWithoutV)" that evaluates to "" instead of a number, in condition "'$(_TargetFrameworkVersionWithoutV)' >= '4.5'".  c:\program files\dotnet\sdk\3.0.100-preview8-013656\Sdks\Microsoft.NET.Sdk.WindowsDesktop\targets\Microsoft.NET.Sdk.WindowsDesktop.props
   at Microsoft.Build.Shared.ProjectErrorUtilities.ThrowInvalidProject(String errorSubCategoryResourceName, IElementLocation elementLocation, String resourceName, Object[] args)
   at Microsoft.Build.Shared.ProjectErrorUtilities.VerifyThrowInvalidProject[T1,T2,T3](Boolean condition, String errorSubCategoryResourceName, IElementLocation elementLocation, String resourceName, T1 arg0, T2 arg1, T3 arg2)
   at Microsoft.Build.Evaluation.NumericComparisonExpressionNode.BoolEvaluate(IConditionEvaluationState state)
   at Microsoft.Build.Evaluation.ConditionEvaluator.EvaluateConditionCollectingConditionedProperties[P,I](String condition, ParserOptions options, Expander`2 expander, ExpanderOptions expanderOptions, Dictionary`2 conditionedPropertiesTable, String evaluationDirectory, ElementLocation elementLocation, ILoggingService loggingServices, BuildEventContext buildEventContext, IFileSystem fileSystem, ProjectRootElementCache projectRootElementCache)
   at Microsoft.Build.Evaluation.ConditionEvaluator.EvaluateCondition[P,I](String condition, ParserOptions options, Expander`2 expander, ExpanderOptions expanderOptions, String evaluationDirectory, ElementLocation elementLocation, ILoggingService loggingServices, BuildEventContext buildEventContext, IFileSystem fileSystem, ProjectRootElementCache projectRootElementCache)
   at Microsoft.Build.Evaluation.LazyItemEvaluator`4.EvaluateCondition(String condition, ProjectElement element, ExpanderOptions expanderOptions, ParserOptions parserOptions, Expander`2 expander, LazyItemEvaluator`4 lazyEvaluator)
   at Microsoft.Build.Evaluation.LazyItemEvaluator`4.EvaluateCondition(ProjectElement element, ExpanderOptions expanderOptions, ParserOptions parserOptions, Expander`2 expander, LazyItemEvaluator`4 lazyEvaluator)
   at Microsoft.Build.Evaluation.LazyItemEvaluator`4.EvaluateConditionWithCurrentState(ProjectElement element, ExpanderOptions expanderOptions, ParserOptions parserOptions)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateItemElement(Boolean itemGroupConditionResult, ProjectItemElement itemElement, LazyItemEvaluator`4 lazyEvaluator)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateItemGroupElement(ProjectItemGroupElement itemGroupElement, LazyItemEvaluator`4 lazyEvaluator)
   at Microsoft.Build.Evaluation.Evaluator`4.Evaluate(ILoggingService loggingService, BuildEventContext buildEventContext)
   at Microsoft.Build.Evaluation.Project.Reevaluate(ILoggingService loggingServiceForEvaluation, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
   at Microsoft.Build.Evaluation.Project.ReevaluateIfNecessary(ILoggingService loggingServiceForEvaluation, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
   at Microsoft.Build.Evaluation.Project.Initialize(IDictionary`2 globalProperties, String toolsVersion, String subToolsetVersion, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
   at Microsoft.Build.Evaluation.Project..ctor(String projectFile, IDictionary`2 globalProperties, String toolsVersion, String subToolsetVersion, ProjectCollection projectCollection, ProjectLoadSettings loadSettings, EvaluationContext evaluationContext)
   at Microsoft.Build.Evaluation.ProjectCollection.LoadProject(String fileName, IDictionary`2 globalProperties, String toolsVersion)
   at OmniSharp.MSBuild.ProjectLoader.EvaluateProjectFileCore(String filePath) in C:\projects\omnisharp-roslyn\src\OmniSharp.MSBuild\ProjectLoader.cs:line 129
   at OmniSharp.MSBuild.ProjectLoader.BuildProject(String filePath) in C:\projects\omnisharp-roslyn\src\OmniSharp.MSBuild\ProjectLoader.cs:line 72
   at OmniSharp.MSBuild.ProjectFile.ProjectFileInfo.Load(String filePath, ProjectIdInfo projectIdInfo, ProjectLoader loader) in C:\projects\omnisharp-roslyn\src\OmniSharp.MSBuild\ProjectFile\ProjectFileInfo.cs:line 95
   at OmniSharp.MSBuild.ProjectManager.LoadOrReloadProject(String projectFilePath, Func`1 loader) in C:\projects\omnisharp-roslyn\src\OmniSharp.MSBuild\ProjectManager.cs:line 308

This seems to cause excessive CPU usage from VS code.
Opening the repro project/folder causes this:

And VS code constantly displays:

I still think that's a bug in msbuild, because it should respect conditions on outer scopes.

[vatsan-madhavan]

Why is msbuild (or Omnisharp) evaluating the numeric condition after failing the previous ('$(_TargetFrameworkVersionWithoutV)' != '') ? Or is something else going on here?

Refactoring into a separate file etc. seems like a tedious approach to re-invent short-circuit evaluation of boolean operators. Can you just fix dotnet/msbuild#3212 so we can use that?