[Support] Fix Windows Command Shell Command line parsing for quoted arguments

[MyDeveloperDay]

Summary

PR38471 - Check readability-identifier-naming is not working on Windows 10

clang-tidy command line parsing fails to read single quoted -checks when using windows cmd.exe prompt

The following will fail to read the values of -checks due argv not removing the single quote (') on windows cmd shell (unlike unix variants & cygwin)
see (https://bugs.llvm.org/show_bug.cgi?id=38471) for more details

clang-tidy.exe -checks='-*,readability-identifier-naming' test.cpp -- -std=c++11

this results in Windows only supporting double quote usage and this is confusing with lots of the examples using single quotes

clang-tidy.exe -checks="-*,readability-identifier-naming" test.cpp -- -std=c++11

As I'm fairly new to LLVM this may not be the best place to do this, whilst this was reported up in clang-tidy I suspect it could have an effect on any of the llvm tools being run in a windows command shell (but not inside bash.exe)

If you think this can be done better elsewhere or is unnecessary I'll be happy to change/abandon it

Picking reviewers from code owners and common reviewers to changes in this area feel free to adjust as necessary.

[JonasToth]

Looking through the clang-tidy eyes as well, my thoughts.

lib/Support/CommandLine.cpp

447

I think more specific StringRef methods should be used here for readability.

Maybe

if (Value.size() >= 2 && Value.startswith('\'') && Value.endswith('\''))
    Value = Value.slice(1, Value.size() - 1); // minus one, because slice takes the index, not the count as substr

I would prefer the >= 2 as it seems clearer to me, that at least 2 character are necessary for this logic. This is highly subjective and I would not insist on anything.

unittests/Support/CommandLineTest.cpp

916

changing the file-mode for this file is not necessary, is it?

941

Could you please add a test with empty input (-checks='', -checks=).

[alexfh]

This is not in any way specific to clang-tidy. Since it will affect the behavior of Clang on Windows, I'd like Reid or someone else who's familiar with the topic to have a look.

Where does this problem come from? Do we have any documentation that mentions single quotes in the context of command line? I personally doubt that the use of single quotes is common for windows CLI tools. Since single quotes don't have any special meaning on Windows unlike double quotes (used to enclose argument values with spaces in them, IIRC).

I believe, the confusion the user had could be addressed by clang-tidy being a bit stricter about the set of characters it accepts in the -checks= option and issue a warning/error in case it sees something weird.

unittests/Support/CommandLineTest.cpp

925

Since the check name has no special meaning to the functionality being proposed, I'd refrain from using it.

[MyDeveloperDay]

In D56901#1363040, @alexfh wrote:

Where does this problem come from?

https://bugs.llvm.org/show_bug.cgi?id=38471 (in case you didn't see that)

Do we have any documentation that mentions single quotes in the context of command line?

we have bit, but not as much as I originally thought.

http://clang.llvm.org/extra/clang-tidy/ (look for format-style), There are some others in ClangFormat too..

I personally doubt that the use of single quotes is common for windows CLI tools. Since single quotes don't have any special meaning on Windows unlike double quotes (used to enclose argument values with spaces in them, IIRC).

Most likely it comes about by the need to put the argument into quotes (double or single) due to the often used wild cards '-*,-modernize-*' I notice it always fails if you use it for the --config='....' argument because the YAML fails so as in the bug report the user was using single quote for one argument and double quote for another.

I also think the fact it works on linux shells means those venturing into using clang-tidy on windows are probably copy and pasting examples they might see online which have single quotes in:

https://stackoverflow.com/questions/40723903/how-to-use-clang-tidy-configuration
https://stackoverflow.com/questions/44360961/selectively-disable-clang-tidy-warnings
https://stackoverflow.com/questions/36604325/cppcheck-vs-clang-tidy-explict-constructor-initializer-list

I believe, the confusion the user had could be addressed by clang-tidy being a bit stricter about the set of characters it accepts in the -checks= option and issue a warning/error in case it sees something weird.

To be honest @alexfh, I feel like I'm using a sledgehammer to crack a nut here!... this isn't exactly the "2nd" commit I wanted to be making given all the tools it could affect, I have also no connection with the original bug, I was just trying to be a good newbie and go fix something small from the bug list as a means of getting more experience.. I may have chosen unwisely ;-)

So I think I like your idea of moving extra error checking up into clang-tidy instead, This might help a little because it will prevent clang-tidy from silently failing but also will have the desired effect, and hopefully won't break the world.

So I let me abandon this review and resubmit something smaller to the clang-tidy guys like @JonasToth and I'll reference this review when I do, so lets not waste any more time on it here if that ok with you guys.

but thanks of your time, and hey I learnt a lot!

[MyDeveloperDay]

Abandoning revision on the basis of perhaps going for a more clang-tidy only directed error message.

[MyDeveloperDay]

lib/Support/CommandLine.cpp

447

sounds good, when I rewrite it I'll do it that way. (or something like it)

unittests/Support/CommandLineTest.cpp

941

let me do this when I do it up in clang-tidy.

[alexfh]

In D56901#1363088, @MyDeveloperDay wrote:

In D56901#1363040, @alexfh wrote:

Where does this problem come from?

https://bugs.llvm.org/show_bug.cgi?id=38471 (in case you didn't see that)

I saw that. I was mostly wondering how someone comes with an idea of using single quotes. Copying from stackoverflow seems like a good guess ;)

http://clang.llvm.org/extra/clang-tidy/ (look for format-style), There are some others in ClangFormat too..

Seems like it's worth changing to double quotes.

So I think I like your idea of moving extra error checking up into clang-tidy instead, This might help a little because it will prevent clang-tidy from silently failing but also will have the desired effect, and hopefully won't break the world.

The minimal viable safeguard would be to look at the set of characters in the -checks= option value (roughly [a-zA-Z0-9-,*.]). An even better validation would be to split the checks filter by commas and verify that each part actually affects the final set of checks. Unless I'm missing something, both validation strategies should be rather straightforward to implement.

lib/Support/CommandLine.cpp

if (I == Sub.OptionsMap.end())
     return nullptr;
 
   auto O = I->second;
   if (O->getFormattingFlag() == cl::AlwaysPrefix)
     return nullptr;
 
   Value = Arg.substr(EqualPos + 1);
   Arg = Arg.substr(0, EqualPos);
+
+  // Windows Cmd Shell will not remove the '' from a --checks='abc' type command
+  // when passed via argv, unlike Linux or cygwin shells.
+  if (Triple(sys::getProcessTriple()).isOSWindows()) {
+    // Pull off the leading and trailing 's.
+    if (Value.size() > 1 && Value[0] == '\'' && Value[Value.size() - 1] == '\'')
+      Value = Value.substr(1, Value.size() - 2);
+  }
   return I->second;
 }
 
 SubCommand *CommandLineParser::LookupSubCommand(StringRef Name) {
   if (Name.empty())
     return &*TopLevelSubCommand;
   for (auto S : RegisteredSubCommands) {
     if (S == &*AllSubCommands)

unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, PrefixOptions) {
   // Test prefixed variant works with cl::AlwaysPrefix options.
   EXPECT_TRUE(MacroDefs.empty());
   const char *args6[] = {"prog", "-DHAVE_FOO"};
   EXPECT_TRUE(
       cl::ParseCommandLineOptions(2, args6, StringRef(), &llvm::nulls()));
   EXPECT_TRUE(MacroDefs.size() == 1);
   EXPECT_TRUE(MacroDefs.front().compare("HAVE_FOO") == 0);
 }
 
+TEST(CommandLineTest, WindowsCmdShellQuotesArg) {
+  if (!Triple(sys::getProcessTriple()).isOSWindows())
+    return;
+
+  StackOption<std::string, cl::opt<std::string>> Checks(
+      "checks", cl::desc("The checks"), cl::init(""));
+
+  const char *argsCmdShell[] = {"clang-tidy.exe",
+                                "-checks='-*,readability-identifier-naming'"};
+
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, argsCmdShell, StringRef(),
+                                          &llvm::nulls()));
+
+  EXPECT_TRUE(Checks.compare("-*,readability-identifier-naming") == 0);
+
+  cl::ResetAllOptionOccurrences();
+
+  const char *argsCmdShell2[] = {"clang-tidy.exe",
+                                 "-checks=-*,readability-identifier-naming"};
+
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, argsCmdShell2, StringRef(),
+                                          &llvm::nulls()));
+
+  EXPECT_TRUE(Checks.compare("-*,readability-identifier-naming") == 0);
+}
+
 } // anonymous namespace