Redo PR 241: parsing complex scenarios of quoting and escaping. (#243)

* Redo PR 241: parsing complex scenarios of quoting and escaping.

* add temporary logging to see how much of the new test is executing

* Break up the new windows test in two to compare smaller baseline/output strings.

* Try cl instead of gcc so that the CI machines don't find the tool in their path and report full (machine dependent) path for compiler path.

* Undo previous mistake, we need gcc not cl but ensure preconfigure adds a path to these compilers inside our repository

* Cover more interesting scenarios for compiler path while also ensuring uniformity between systems

* Fix linux test baseline according to latest changes in the dryrun

Author: andreeis

src/parser.ts

@@ -492,14 +492,14 @@ async function parseAnySwitchFromToolArguments(args: string, excludeArgs: string
         };
 
         let stderr: any = (result: string): void => {
-            logger.messageNoCR(`Error while running the compiler args parser script '${parseCompilerArgsScriptFile}'` +
-                `for regions "${compilerArgRegions}": "${result}"`, "Normal");
+            logger.message(`Error while running the compiler args parser script '${parseCompilerArgsScriptFile}'` +
+                `for regions ("${compilerArgRegions})": "${result}"`, "Normal");
         };
 
         // Running the compiler arguments parsing script can use the system locale.
         const result: util.SpawnProcessResult = await util.spawnChildProcess(runCommand, scriptArgs, util.getWorkspaceRoot(), false, stdout, stderr);
         if (result.returnCode !== 0) {
-            logger.messageNoCR(`The compiler args parser script '${parseCompilerArgsScriptFile}' failed with error code ${result.returnCode}`, "Normal");
+            logger.message(`The compiler args parser script '${parseCompilerArgsScriptFile}' failed with error code ${result.returnCode} for regions (${compilerArgRegions})`, "Normal");
         }
     } catch (error) {
         logger.message(error);
@@ -532,16 +532,30 @@ function parseMultipleSwitchFromToolArguments(args: string, sw: string, removeSu
     //    (example): -DMY_DEFINE='"SOME_VALUE"'
 
     function anythingBetweenQuotes(fullyQuoted: boolean): string {
-        let anythingBetweenReverseQuote: string = '\\`[^\\`]*?\\`';
-        let anythingBetweenSingleQuote: string = "\\'[^\\']*?\\'";
-        let anythingBetweenDoubleQuote: string = '\\"[^\\"]*?\\"';
-
-        // If the switch is fully quoted with ', like ('-DMY_DEFINE="MyValue"'), don't allow single quotes
-        // inside the switch value.
-        // One example of what can be broken if we don't do this: gcc '-DDEF1=' '-DDef2=val2'
-        // in which case DEF1 would be seen as DEF1=' ' instead of empty =
-        let str: string = anythingBetweenReverseQuote + '|' + anythingBetweenDoubleQuote + (fullyQuoted ? "" : '|' + anythingBetweenSingleQuote);
-        return str;
+      // The basic pattern for anything between quotes accepts equally single quote, double quote or back tick.
+      // One pattern that is accepted is to wrap between escaped quotes and allow inside anything (including non-escaped quotes) except escaped quotes.
+      // Another pattern that is accepted is to wrap between non-escaped quotes and allow inside anything (including escaped quotes) except non-escaped quotes.
+      // One problem with the "..." pattern is that a simple "\" (or anything ending with \") will not know if the backslash is part of the inside of quote-quote
+      // or together with the following quote represents a \" and needs to look forward for another ending quote.
+      // If there is another quote somewhere else later in the command line (another -D or a file name wrapped in quotes) everything until that first upcoming quote
+      // will be included.
+      // Example that doesn't work: -DSLASH_DEFINE="\" -DSOME_OTHER_SWITCH "drive:\folder\file.extension"
+      //                            SLASH_DEFINE is equal to '\" -DSOME_OTHER_SWITCH '
+      // Example that works: -DGIT_VERSION=" \" 1.2.3 \" "
+      //                      GIT_VERSION is equal to ' \" 1.2.3 \" '
+      // Unfortunately, we also can't identify this to log in the output channel for later analysis of more makefile switch and quoting user scenarios.
+      // Fortunately, we didn't encounter the last scenario, only the first.
+      function anythingBetweenQuotesBasicPattern(quoteChar: string): string {
+         return '\\\\\\' + quoteChar + "((?!\\\\\\" + quoteChar + ").)*\\\\\\" + quoteChar + "|" +  // \" anything(except \") \"
+                "\\" + quoteChar + "(\\\\\\" + quoteChar + "|[^\\" + quoteChar + "])*?[^\\\\\\" + quoteChar + "]?\\" + quoteChar; // " anything (except ") "
+      }
+
+      // If the switch is fully quoted with ', like ('-DMY_DEFINE="MyValue"'), don't allow single quotes
+      // inside the switch value.
+      // One example of what can be broken if we don't do this: gcc '-DDEF1=' '-DDef2=val2'
+      // in which case DEF1 would be seen as DEF1=' ' instead of empty =
+      let str: string = anythingBetweenQuotesBasicPattern("`") + '|' + anythingBetweenQuotesBasicPattern('"') + (fullyQuoted ? "" : '|' + anythingBetweenQuotesBasicPattern("'"));
+      return str;
     }
 
     function mainPattern(fullyQuoted: boolean): string {
@@ -584,7 +598,7 @@ function parseMultipleSwitchFromToolArguments(args: string, sw: string, removeSu
 
     match = regexp.exec(args);
     while (match) {
-        let matchIndex: number = (match[2].startsWith("'") && match[2].endsWith("'")) ? 8 : 18;
+        let matchIndex: number = (match[2].startsWith("'") && match[2].endsWith("'")) ? 8 : 26;
         let result: string = match[matchIndex];
         if (result) {
            if (removeSurroundingQuotes) {

src/test/fakeSuite/Repros/.vscode/preconfigure.bat

@@ -1,3 +1,4 @@
 @echo off
 set PATH="%cd%\bin\InterestingSmallMakefile\CL\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\12.34.56789\bin\Hostx86\x86";%PATH%
+set PATH=%cd%\bin\GitHub\compilers;%PATH%
 

src/test/fakeSuite/Repros/.vscode/settings.json

@@ -3,8 +3,17 @@
     "makefile.buildLog": "./dummy_dryrun.log",
     "makefile.makePath": "c:/some/other/fake/path",
     "makefile.loggingLevel": "Debug",
+    "makefile.additionalCompilerNames": ["MyOwnFakeCompiler"],
     "makefile.configureOnOpen": false,
     "makefile.configurations": [
+        {
+            "name": "complex_escaped_quotes",
+            "buildLog": "./complex_escaped_quotes_dryrun.log"
+        },
+        {
+            "name": "complex_escaped_quotes_winOnly",
+            "buildLog": "./complex_escaped_quotes_winOnly_dryrun.log"
+        },
         {
             "name": "InterestingSmallMakefile_windows_configDebug",
             "buildLog": "./InterestingSmallMakefile_windows_dryrunDebug.log"

src/test/fakeSuite/Repros/8cc_linux_baseline.out

@@ -6,6 +6,7 @@ Found build log path setting "./8cc_linux_dryrun.log" defined for configuration
 Resolving build log path to "{REPO:VSCODE-MAKEFILE-TOOLS}/src/test/fakeSuite/Repros/8cc_linux_dryrun.log"
 Deduced command 'c:/some/other/fake/path ' for configuration 8cc_linux
 Saving opened files before build.
+Loading configurations from cache is not necessary.
 Preprocessing: "{REPO:VSCODE-MAKEFILE-TOOLS}/src/test/fakeSuite/Repros/8cc_linux_dryrun.log"
 Preprocess elapsed time: 0
 Parsing for IntelliSense.
@@ -183,7 +184,6 @@ Found the following 29 build targets defined in the makefile: 8cc;all;buffer.o;c
 Complete list of build targets: 8cc;all;buffer.o;clean;cleanobj;cpp.o;debug.o;dict.o;encoding.o;error.o;file.o;fulltest;gen.o;lex.o;main.o;map.o;parse.o;path.o;runtests;self;set.o;stage1;stage2;stage3;test;testtest;utiltest;utiltest.o;vector.o
 Parsing build targets elapsed time: 0
 Configure finished. The status for all the subphases that ran:
-loadFromCache: return code = -3, elapsed time = 0
 generateParseContent: return code = 0, elapsed time = 0
 preprocessParseContent: return code = 0, elapsed time = 0
 parseIntelliSense: return code = 0, elapsed time = 0

src/test/fakeSuite/Repros/InterestingSmallMakefile_windows_baseline.out

@@ -8,6 +8,7 @@ Found build log path setting "./InterestingSmallMakefile_windows_dryrunDebug.log
 Resolving build log path to "{REPO:VSCODE-MAKEFILE-TOOLS}\src\test\fakeSuite\Repros\InterestingSmallMakefile_windows_dryrunDebug.log"
 Deduced command 'c:\some\other\fake\path.exe ' for configuration InterestingSmallMakefile_windows_configDebug
 Saving opened files before build.
+Loading configurations from cache is not necessary.
 Preprocessing: "{REPO:VSCODE-MAKEFILE-TOOLS}\src\test\fakeSuite\Repros\InterestingSmallMakefile_windows_dryrunDebug.log"
 Preprocess elapsed time: 0
 Parsing for IntelliSense.
@@ -193,7 +194,6 @@ Found the following 21 build targets defined in the makefile: Arch1_main.exe;Arc
 Complete list of build targets: Arch1_main.exe;Arch2_main.exe;Arch3_main.exe;Execute_Arch1;Execute_Arch2;Execute_Arch3;all;buildArch1;buildArch2;buildArch3;builds;clean;create_dirs;create_dirs_Arch1;create_dirs_Arch2;create_dirs_Arch3;install;rebuild;runs;simple;simple/simple.exe
 Parsing build targets elapsed time: 0
 Configure finished. The status for all the subphases that ran:
-loadFromCache: return code = -3, elapsed time = 0
 generateParseContent: return code = 0, elapsed time = 0
 preprocessParseContent: return code = 0, elapsed time = 0
 parseIntelliSense: return code = 0, elapsed time = 0