NoWarn property is not set correct when using Turkish OS Language

[brunolins16]

Issue Description

When building the project Mvc.Api.Analyzers.Test.csproj that contains a NoWarn property configured to <NoWarn>$(NoWarn);IDE0161</NoWarn> on a Windows machine (Turkish language) fails with the following error.

aspnetcore\src\Mvc\Mvc.Api.Analyzers\test\TestFiles\ActualApiResponseMetadataFactoryTest\GetDefaultStatusCodeTest.cs(8,1): error IDE0161: Dosya kapsamlı namespace öğesine dönüştür [aspnetcore\src\Mvc\Mvc.Api.Analyzers\test\Mvc.Api.Analyzers.Test.csproj]

Steps to Reproduce

1. Open Command Prompt as administrator, create a new Repos folder under %userprofile% and navigate to it

   mkdir Repos
   cd Repos
   git clone --recursive https://github.com/dotnet/aspnetcore

2. Open Windows PowerShell, navigate to AspNetCore: cd Repros/AspNetCore
3. On Windows PowerShell, Run: Set-ExecutionPolicy RemoteSigned -Scope CurrentUser , Type Y
4. On Windows PowerShell, Run: .\restore.cmd, then change into the src/Mvc/Mvc.Api.Analyzers/test directory and run dotnet build

Expected Behavior

"C:\Program Files\Microsoft Visual Studio\2022\Preview\MSBuild\Current\Bin\Roslyn\csc.exe" 
/noconfig 
/unsafe- 
/checked-
/nowarn:1701,1702,1705,NETSDK1138,CS8969,IDE0161,NU5105,RS0041,CA1416,1701,1702 
/fullpaths /nostdlib+ /errorreport:prompt /warn:6 
/define:TRACE;DEBUG;NET;NET7_0;NETCOREAPP;NET5_0_OR_GREATER;NET6_0_OR_GREATER;NET7_0_OR_GREATER;NETCOREAPP1_0_OR_GREATER;NETCOREAPP1_1_OR_GREATER;NETCOREAPP2_0_OR_GREATER;NETCOREAPP2_1_OR_GREATER;NETCOREAPP2_2_OR_GREATER;NETCOREAPP3_0_OR_GREATER;NETCOREAPP3_1_OR_GREATER /highentropyva+ 
[additional lines ommited]
/warnaserror+:NU1605 /warnaserror-:CS1591,xUnit1004

Actual Behavior

"C:\Program Files\Microsoft Visual Studio\2022\Preview\MSBuild\Current\Bin\Roslyn\csc.exe" 
/noconfig 
/unsafe- 
/checked-
/nowarn:1701,1702,1705,NETSDK1138,CS8969,"IDE0161",NU5105,RS0041,CA1416,1701,1702 
/fullpaths /nostdlib+ /errorreport:prompt /warn:6 
/define:TRACE;DEBUG;NET;NET7_0;NETCOREAPP;NET5_0_OR_GREATER;NET6_0_OR_GREATER;NET7_0_OR_GREATER;NETCOREAPP1_0_OR_GREATER;NETCOREAPP1_1_OR_GREATER;NETCOREAPP2_0_OR_GREATER;NETCOREAPP2_1_OR_GREATER;NETCOREAPP2_2_OR_GREATER;NETCOREAPP3_0_OR_GREATER;NETCOREAPP3_1_OR_GREATER /highentropyva+ 
[additional lines ommited]
/warnaserror+:NU1605 /warnaserror-:CS1591,xUnit1004

Versions & Configurations

OS: Win10 x64 20H2 TRK

Attach a binlog

BinLogs.zip

[rainersigwald]

What a great bug report! Thanks for the detail

I thought it might be a Roslyn bug but it is in fact in MSBuild. This code that decides if we should quote a part of a string is going wrong:

msbuild/src/Utilities/CommandLineBuilder.cs

Lines 194 to 204 in 386a830

	bool hasAllUnquotedCharacters = AllowedUnquoted.IsMatch(parameter);
	bool hasSomeQuotedCharacters = DefinitelyNeedQuotes.IsMatch(parameter);
	isQuotingRequired = !hasAllUnquotedCharacters;
	isQuotingRequired = isQuotingRequired || hasSomeQuotedCharacters;
	Debug.Assert(!hasAllUnquotedCharacters || !hasSomeQuotedCharacters,
	"At least one of allowedUnquoted or definitelyNeedQuotes is wrong.");
	}
	return isQuotingRequired;

I suspect the Turkish I ı İ i.

This is exactly the situation described in this Regex doc about using invariant culture comparison and I suspect that's the fix here.

[brunolins16]

@rainersigwald I think you are right about I ı İ i because I did a dummy test to set as DE0161 and it behaves correctly.

[danmoseley]

Noticed this because we've been doing thinking about case insensitivity in regex recently. I think this is the only instance of the bug in MSBuild, but a perf note: the regex engine had/has some inefficiencies when using RegexOptions.IgnoreCase. In older versions, it makes no attempt to determine apriori that casing is irrelevant to the pattern. So even if my pattern is \d+ it will dutifully ToLower() every character in the input before checking that it matches \d. Given that, adding RegexOptions.InvariantCulture if possible is an improvement, as char.ToLowerInvariant() is a little faster. Or, removing RegexOptions.IgnoreCase if it's not relevant is even better. For example, by inspection of s_filenameLocationFromOrigin regex we know that lower casing will not affect any match, so it needn't have RegexOptions.IgnoreCase.

In MSBuild I'm guessing the hottest regexes are the warning/error matchers -- which could probably be RegexOptions.InvariantCulture, since the text that matches error or warning is the same in all cultures. (In .NET 6, a-z except for i is special cased). I'm not suggesting that's worth changing unless it actually produces meaningful perf improvements.

In .NET 7 we expect all this to be taken care of automatically, and will also change to change to consistently use the culture at construction time (if ignore case is applied) -- this is tracked by dotnet/runtime#61048.

BTW, if and when MSBuild builds against net7.0, you will likely want to switch over to the regex source generator for patterns that are statically constructed instead of compiling them at runtime.

[Forgind]

I'm not sure if it's the same issue or not, but I ran build.cmd --noWarn:SYSLIB0011 and got:

MSBUILD : error MSB1025: MSBuild çalıştırılırken bir iç hata oluştu.
Microsoft.Build.Shared.InternalErrorException: MSB0001: Internal MSBuild Error: The switch name extracted from either the partially or completely unquoted arg should be the same.
   konum: Microsoft.Build.Shared.ErrorUtilities.ThrowInternalError(String message, Exception innerException, Object[] args)
   konum: Microsoft.Build.CommandLine.MSBuildApp.ExtractSwitchParameters(String commandLineArg, String unquotedCommandLineArg, Int32 doubleQuotesRemovedFromArg, String switchName, Int32 switchParameterIndicator)
   konum: Microsoft.Build.CommandLine.MSBuildApp.GatherCommandLineSwitches(List`1 commandLineArgs, CommandLineSwitches commandLineSwitches)
   konum: Microsoft.Build.CommandLine.MSBuildApp.GatherAllSwitches(String commandLine, CommandLineSwitches& switchesFromAutoResponseFile, CommandLineSwitches& switchesNotFromAutoResponseFile)
   konum: Microsoft.Build.CommandLine.MSBuildApp.Execute(String commandLine)

İşlenmeyen Özel Durum: Microsoft.Build.Shared.InternalErrorException: MSB0001: Internal MSBuild Error: The switch name extracted from either the partially or completely unquoted arg should be the same.
   konum: Microsoft.Build.Shared.ErrorUtilities.ThrowInternalError(String message, Exception innerException, Object[] args)
   konum: Microsoft.Build.CommandLine.MSBuildApp.ExtractSwitchParameters(String commandLineArg, String unquotedCommandLineArg, Int32 doubleQuotesRemovedFromArg, String switchName, Int32 switchParameterIndicator)
   konum: Microsoft.Build.CommandLine.MSBuildApp.GatherCommandLineSwitches(List`1 commandLineArgs, CommandLineSwitches commandLineSwitches)
   konum: Microsoft.Build.CommandLine.MSBuildApp.GatherAllSwitches(String commandLine, CommandLineSwitches& switchesFromAutoResponseFile, CommandLineSwitches& switchesNotFromAutoResponseFile)
   konum: Microsoft.Build.CommandLine.MSBuildApp.Execute(String commandLine)
   konum: Microsoft.Build.CommandLine.MSBuildApp.Main()
Build failed with exit code -532462766. Check errors above.

[danmoseley]

I think that's the same bug. BTW, our shipping product is containing strings like this ("The switch name extracted from either the partially or completely unquoted arg should be the same.") .. and that string has probably only been used once ever -- in this repro case. Have you considered removing all the InternalErrorException strings and replacing with codes (or just using line numbers). Seems like it could save a fair bit.

[Forgind]

I seem to get a passed build if I use a bootstrap build of current main but not if I use the msbuild from VS 17.0.5; not sure what the difference is.