Skip to content

Commit

Permalink
Fix regex culture bug (#7498)
Browse files Browse the repository at this point in the history
Fixes #7154.

The CSC compiler does not unquote disabled warnings eg `/nowarn:1701,1702,1705,NETSDK1138,CS8969,"IDE0161",NU5105,RS0041,CA1416,1701,1702` will not disable `IDE0161`. MSBuild has logic to quote strings that it believes must be quoted to be passed on a command line. It has a bug causing it to do this over eagerly in the tr-TR culture, which exposes this compiler limitation.

To fix this Regex should use RegexOptions.CultureInvariant unless they are specifically intended to have culture specific  behavior. In this case the purpose is simply to determine whether a string needs to be quoted on a command line. An example is "SYSLIB0003". The `I` does not match `[a-z]` case insensitively in tr-TR culture because lower casing `I` does not produce `i` in this culture, it produces `ı`, which does not match the pattern. Assuming that it is safe to pass `I` on a command line in an OS set to Turkish culture -- I assume this is the case, but if it isn't, quoting will not help -- we can change the pattern to match in a culture invariant way. This causes the regex engine to operate more-or-less with en-US casing rules, so `I` and `i` are allowed. (Turkish `ı` or `İ` will still be quoted, see below.) 

Added the option to the regex. Also added it to the other related one, even though it has no relevant parts in the pattern, for consistency. I did not add it to the place where MSBuild looks for "error" or "warning" -- it probably should be there, but as they don't include "i", it doesn't matter and can be changed separately.

Note: this does not address the issue that a quoted warning is not unquoted by the compiler. The code will still add quotes if eg., the string has a space in, and most likely this will then fail to be unquoted by the compiler. If htis is an issue then we should have a separate issue for it against the compiler.

This change makes things strictly better and solves the reported issue.
  • Loading branch information
danmoseley committed Mar 28, 2022
1 parent 058a026 commit 1160f4e
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
20 changes: 20 additions & 0 deletions src/Utilities.UnitTests/CommandLineBuilder_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Globalization;
using System.IO;
using System.Threading;
using Microsoft.Build.Shared;
using Microsoft.Build.Utilities;
using Shouldly;
Expand Down Expand Up @@ -54,6 +56,24 @@ public void AppendSwitchWithSpacesInParameter()
c.ShouldBe("/animal:\"dog and pony\"");
}

[Fact]
public void AppendSwitchWithIShouldNotNeedQuotingInTurkishLocale()
{
var currentCulture = Thread.CurrentThread.CurrentCulture;
try
{
Thread.CurrentThread.CurrentCulture = new CultureInfo("tr-TR"); // Turkish

CommandLineBuilder c = new CommandLineBuilder();
c.AppendSwitchIfNotNull("/i:", "iI");
c.ShouldBe("/i:iI");
}
finally
{
Thread.CurrentThread.CurrentCulture = currentCulture;
}
}

/// <summary>
/// Test for AppendSwitchIfNotNull for the ITaskItem version
/// </summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Utilities/CommandLineBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,13 @@ public CommandLineBuilder(bool quoteHyphensOnCommandLine, bool useNewLineSeparat
/// Use a private property so that we can lazy initialize the regex
/// </summary>
private Regex DefinitelyNeedQuotes => _definitelyNeedQuotes
?? (_definitelyNeedQuotes = new Regex(_quoteHyphens ? s_definitelyNeedQuotesRegexWithHyphen : s_definitelyNeedQuotesRegexNoHyphen, RegexOptions.None));
?? (_definitelyNeedQuotes = new Regex(_quoteHyphens ? s_definitelyNeedQuotesRegexWithHyphen : s_definitelyNeedQuotesRegexNoHyphen, RegexOptions.CultureInvariant));

/// <summary>
/// Use a private getter property to we can lazy initialize the regex
/// </summary>
private Regex AllowedUnquoted => _allowedUnquoted
?? (_allowedUnquoted = new Regex(_quoteHyphens ? s_allowedUnquotedRegexNoHyphen : s_allowedUnquotedRegexWithHyphen, RegexOptions.IgnoreCase));
?? (_allowedUnquoted = new Regex(_quoteHyphens ? s_allowedUnquotedRegexNoHyphen : s_allowedUnquotedRegexWithHyphen, RegexOptions.IgnoreCase | RegexOptions.CultureInvariant));

/// <summary>
/// Checks the given switch parameter to see if it must/can be quoted.
Expand Down

0 comments on commit 1160f4e

Please sign in to comment.