CxxPreprocessor: fix handling of SquidAstVisitorContext

* `SquidAstVisitorContext::getFile()` never returns `null` during productive execution
* the only exception is preprocessing inside of preprocessor's constructor
  (error-prone design in my humble opinion)

* Fixes:
   * wrong assumptions in `CxxPreprocessor` were fixed
   * documentation was added
   * poorly parametrized mocks were fixed in the unit tests
   * minor refactoring

Author: ivangalkin

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java

@@ -35,11 +35,7 @@
 import com.sonar.sslr.impl.Parser;
 import java.io.File;
 import java.io.IOException;
-import java.net.URI;
-import java.net.URISyntaxException;
 import java.nio.charset.Charset;
-import java.nio.file.FileSystemNotFoundException;
-import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -49,6 +45,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
@@ -93,7 +90,6 @@ public class CxxPreprocessor extends Preprocessor {
 
   private final CxxLanguage language;
   private File currentContextFile;
-  private String rootFilePath;
 
   private final Parser<Grammar> pplineParser;
   private final MapChain<String, Macro> fixedMacros = new MapChain<>();
@@ -107,6 +103,7 @@ public class CxxPreprocessor extends Preprocessor {
   private CxxCompilationUnitSettings compilationUnitSettings;
   private final Multimap<String, Include> includedFiles = HashMultimap.create();
   private final Multimap<String, Include> missingIncludeFiles = HashMultimap.create();
+  private boolean ctorInProgress = true;
 
   private State currentFileState = new State(null);
   private final Deque<State> globalStateStack = new LinkedList<>();
@@ -159,6 +156,7 @@ public CxxPreprocessor(SquidAstVisitorContext<Grammar> context,
       }
     } finally {
       getMacros().setHighPrio(false);
+      ctorInProgress = false;
     }
   }
 
@@ -456,11 +454,19 @@ public PreprocessorAction process(List<Token> tokens) { //TODO: deprecated Prepr
     Token token = tokens.get(0);
     TokenType ttype = token.getType();
 
-    File file = getFileUnderAnalysis();
-    rootFilePath = file == null ? token.getURI().toString() : file.getAbsolutePath();
+    final String rootFilePath = getFileUnderAnalysis().getAbsolutePath();
 
-    if (context.getFile() != currentContextFile) {
+    // CxxPreprocessor::process() can be called a) while construction,
+    // b) for a new "physical" file or c) for #include directive.
+    // Make sure, that the following code is executed for a new "physical" file
+    // only.
+    final boolean processingNewSourceFile = !ctorInProgress && (context.getFile() != currentContextFile);
+
+    if (processingNewSourceFile) {
       currentContextFile = context.getFile();
+      // In case "physical" file is preprocessed, SquidAstVisitorContext::getFile() cannot return null
+      // Did you forget to setup the mock properly?
+      Objects.requireNonNull(context.getFile(), "SquidAstVisitorContext::getFile() must be non-null");
       compilationUnitSettings = conf.getCompilationUnitSettings(currentContextFile.getAbsolutePath());
 
       if (compilationUnitSettings != null) {
@@ -640,9 +646,8 @@ public String expandFunctionLikeMacro(String macroName, List<Token> restTokens)
   }
 
   public Boolean expandHasIncludeExpression(AstNode exprAst) {
-    File file = getFileUnderAnalysis();
-    String filePath = file == null ? rootFilePath : file.getAbsolutePath();
-    return findIncludedFile(exprAst, exprAst.getToken(), filePath) != null;
+    final File file = getFileUnderAnalysis();
+    return findIncludedFile(exprAst, exprAst.getToken(), file.getAbsolutePath()) != null;
   }
 
   private boolean isCFile(String filePath) {
@@ -857,7 +862,6 @@ private Macro parseMacroDefinition(String macroDef) {
 
   private File findIncludedFile(AstNode ast, Token token, String currFileName) {
     String includedFileName = null;
-    File includedFile = null;
     boolean quoted = false;
 
     AstNode node = ast.getFirstDescendant(CppGrammar.includeBodyQuoted);
@@ -908,28 +912,34 @@ private File findIncludedFile(AstNode ast, Token token, String currFileName) {
     }
 
     if (includedFileName != null) {
-      File file = getFileUnderAnalysis();
-      String dir;
-      if (file != null) {
-        dir = file.getParent();
-      } else {
-        try {
-          dir = Paths.get(new URI(currFileName)).getParent().toString();
-        } catch (IllegalArgumentException | FileSystemNotFoundException | SecurityException | URISyntaxException e) {
-          dir = "";
-        }
-      }
-      includedFile = getCodeProvider().getSourceCodeFile(includedFileName, dir, quoted);
+      final File file = getFileUnderAnalysis();
+      final String dir = file.getParent();
+      return getCodeProvider().getSourceCodeFile(includedFileName, dir, quoted);
     }
 
-    return includedFile;
+    return null;
   }
 
   private File getFileUnderAnalysis() {
-    if (currentFileState.includeUnderAnalysis == null) {
-      return context.getFile();
+    if (ctorInProgress) {
+      // a) CxxPreprocessor is parsing artificial #include and #define
+      // directives in order to initialize preprocessor with default macros
+      // and forced includes.
+      // This code is running in constructor of CxxPreprocessor. There is no
+      // information about the current file. Therefore return some artificial
+      // path under the project base directory.
+      return new File(conf.getBaseDir(), "CxxPreprocessorCtorInProgress.cpp").getAbsoluteFile();
+    } else if (currentFileState.includeUnderAnalysis != null) {
+      // b) CxxPreprocessor is called recursively in order to parse the #include
+      // directive. Return path to the included file.
+      return currentFileState.includeUnderAnalysis;
     }
-    return currentFileState.includeUnderAnalysis;
+
+    // c) CxxPreprocessor is called in the ordinary mode: it is preprocessing the
+    // file, tracked in org.sonar.squidbridge.SquidAstVisitorContext. This file cannot
+    // be null. If it is null - you forgot to setup the test mock.
+    Objects.requireNonNull(context.getFile(), "SquidAstVisitorContext::getFile() must be non-null");
+    return context.getFile();
   }
 
   PreprocessorAction handleIfLine(AstNode ast, Token token, String filename) {

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/MapChain.java

@@ -90,6 +90,7 @@ public V removeLowPrio(K key) {
    */
   public void clearLowPrio() {
     lowPrioMap.clear();
+    lowPrioDisabled.clear();
   }
 
   /**

cxx-squid/src/test/java/org/sonar/cxx/lexer/CxxLexerTest.java

@@ -20,7 +20,10 @@
 package org.sonar.cxx.lexer;
 
 import com.sonar.sslr.api.GenericTokenType;
+import com.sonar.sslr.api.Grammar;
 import com.sonar.sslr.impl.Lexer;
+
+import java.io.File;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -30,6 +33,8 @@
 import org.junit.BeforeClass;
 import org.junit.Test;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
 import org.sonar.cxx.CxxFileTesterHelper;
 import org.sonar.cxx.CxxLanguage;
 import org.sonar.cxx.api.CxxKeyword;
@@ -46,8 +51,12 @@ public class CxxLexerTest {
 
   @BeforeClass
   public static void init() {
+    File file = new File("snippet.cpp").getAbsoluteFile();
+    SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
+    when(context.getFile()).thenReturn(file);
+
     CxxLanguage language = CxxFileTesterHelper.mockCxxLanguage();
-    CxxPreprocessor cxxpp = new CxxPreprocessor(mock(SquidAstVisitorContext.class), language);
+    CxxPreprocessor cxxpp = new CxxPreprocessor(context, language);
     lexer = CxxLexer.create(cxxpp, new JoinStringsPreprocessor());
   }
 

cxx-squid/src/test/java/org/sonar/cxx/lexer/CxxLexerWithPreprocessingTest.java

@@ -52,10 +52,16 @@ public class CxxLexerWithPreprocessingTest {
 
   private static Lexer lexer;
   private final CxxLanguage language;
+  private final SquidAstVisitorContext<Grammar> context;
 
   public CxxLexerWithPreprocessingTest() {
     language = CxxFileTesterHelper.mockCxxLanguage();
-    CxxPreprocessor cxxpp = new CxxPreprocessor(mock(SquidAstVisitorContext.class), language);
+
+    File file = new File("snippet.cpp").getAbsoluteFile();
+    context = mock(SquidAstVisitorContext.class);
+    when(context.getFile()).thenReturn(file);
+
+    CxxPreprocessor cxxpp = new CxxPreprocessor(context, language);
     lexer = CxxLexer.create(cxxpp, new JoinStringsPreprocessor());
   }
 
@@ -331,7 +337,7 @@ public void bodyless_defines() {
   public void external_define() {
     CxxConfiguration conf = new CxxConfiguration();
     conf.setDefines(new String[]{"M body"});
-    CxxPreprocessor cxxpp = new CxxPreprocessor(mock(SquidAstVisitorContext.class), conf, language);
+    CxxPreprocessor cxxpp = new CxxPreprocessor(context, conf, language);
     lexer = CxxLexer.create(conf, cxxpp, new JoinStringsPreprocessor());
 
     List<Token> tokens = lexer.lex("M");
@@ -345,7 +351,7 @@ public void external_define() {
   public void external_defines_with_params() {
     CxxConfiguration conf = new CxxConfiguration();
     conf.setDefines(new String[]{"minus(a, b) a - b"});
-    CxxPreprocessor cxxpp = new CxxPreprocessor(mock(SquidAstVisitorContext.class), conf, language);
+    CxxPreprocessor cxxpp = new CxxPreprocessor(context, conf, language);
     lexer = CxxLexer.create(conf, cxxpp, new JoinStringsPreprocessor());
 
     List<Token> tokens = lexer.lex("minus(1, 2)");
@@ -612,7 +618,7 @@ public void ignore_irrelevant_preprocessor_directives() {
   public void externalMacrosCannotBeOverriden() {
     CxxConfiguration conf = mock(CxxConfiguration.class);
     when(conf.getDefines()).thenReturn(Arrays.asList("name goodvalue"));
-    CxxPreprocessor cxxpp = new CxxPreprocessor(mock(SquidAstVisitorContext.class), conf, language);
+    CxxPreprocessor cxxpp = new CxxPreprocessor(context, conf, language);
     lexer = CxxLexer.create(conf, cxxpp);
 
     List<Token> tokens = lexer.lex("#define name badvalue\n"

cxx-squid/src/test/java/org/sonar/cxx/parser/CxxParserTest.java

@@ -21,11 +21,12 @@
 
 import com.sonar.sslr.api.AstNode;
 import com.sonar.sslr.api.Grammar;
+import com.sonar.sslr.impl.Parser;
+
 import java.io.File;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import org.apache.commons.io.FileUtils;
@@ -38,7 +39,7 @@
 import org.sonar.cxx.CxxFileTesterHelper;
 import org.sonar.squidbridge.SquidAstVisitorContext;
 
-public class CxxParserTest extends ParserBaseTestHelper {
+public class CxxParserTest  {
 
   String errSources = "/parser/bad/error_recovery_declaration.cc";
   String[] goodFiles = {"own", "VC", "cli", "cuda", "examples"};
@@ -54,7 +55,7 @@ public CxxParserTest() throws URISyntaxException {
 
   @Test
   public void testParsingOnDiverseSourceFiles() {
-    Collection<File> files = listFiles(goodFiles, new String[]{"cc", "cpp", "hpp"});
+    List<File> files = listFiles(goodFiles, new String[]{"cc", "cpp", "hpp"});
     HashMap<String, Integer> map = new HashMap<String, Integer>() {
       private static final long serialVersionUID = 6029310517902718597L;
 
@@ -76,7 +77,15 @@ public void testParsingOnDiverseSourceFiles() {
         put("outbuf2.cpp", 2);
       }
     };
+
+    CxxConfiguration conf = new CxxConfiguration();
+    conf.setErrorRecoveryEnabled(false);
+
+    SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
+    Parser<Grammar> p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
+
     for (File file : files) {
+      when(context.getFile()).thenReturn(file);
       AstNode root = p.parse(file);
       CxxParser.finishedParsing(file);
       if (map.containsKey(file.getName())) {
@@ -90,7 +99,7 @@ public void testParsingOnDiverseSourceFiles() {
   @SuppressWarnings("unchecked")
   @Test
   public void testPreproccessorParsingOnDiverseSourceFiles() {
-    conf = new CxxConfiguration();
+    CxxConfiguration conf = new CxxConfiguration();
     conf.setErrorRecoveryEnabled(false);
     String baseDir = new File("src/test").getAbsolutePath();
     conf.setBaseDir(baseDir);
@@ -118,9 +127,11 @@ public void testPreproccessorParsingOnDiverseSourceFiles() {
       }
     };
 
-    p = CxxParser.create(mock(SquidAstVisitorContext.class), conf, CxxFileTesterHelper.mockCxxLanguage());
-    Collection<File> files = listFiles(preprocessorFiles, new String[]{"cc", "cpp", "hpp", "h"});
+    SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
+    Parser<Grammar> p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
+    List<File> files = listFiles(preprocessorFiles, new String[]{"cc", "cpp", "hpp", "h"});
     for (File file : files) {
+      when(context.getFile()).thenReturn(file);
       AstNode root = p.parse(file);
       CxxParser.finishedParsing(file);
       if (map.containsKey(file.getName())) {
@@ -139,13 +150,15 @@ public void testParsingInCCompatMode() { //ToDo: Fix this compatibility test - c
     // This mode works if such a file causes parsing errors when the mode
     // is switched off and doesn't, if the mode is switched on.
 
-    File cfile = (File) listFiles(cCompatibilityFiles, new String[]{"c"}).toArray()[0];
+    File cfile = listFiles(cCompatibilityFiles, new String[]{"c"}).get(0);
 
     SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
     when(context.getFile()).thenReturn(cfile);
 
+    CxxConfiguration conf = new CxxConfiguration();
+    conf.setErrorRecoveryEnabled(false);
     conf.setCFilesPatterns(new String[]{""});
-    p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
+    Parser<Grammar> p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
 
     AstNode root0 = p.parse(cfile);
     assertThat(root0.getNumberOfChildren()).isEqualTo(2);
@@ -158,6 +171,14 @@ public void testParsingInCCompatMode() { //ToDo: Fix this compatibility test - c
 
   @Test
   public void testParseErrorRecoveryDisabled() {
+    SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
+    when(context.getFile()).thenReturn(erroneousSources);
+
+    CxxConfiguration conf = new CxxConfiguration();
+    conf.setErrorRecoveryEnabled(false);
+
+    Parser<Grammar> p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
+
     // The error recovery works, if:
     // - a syntacticly incorrect file causes a parse error when recovery is disabled
     assertThatThrownBy(() -> {
@@ -168,15 +189,19 @@ public void testParseErrorRecoveryDisabled() {
   @SuppressWarnings("unchecked")
   @Test
   public void testParseErrorRecoveryEnabled() {
+    SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
+    when(context.getFile()).thenReturn(erroneousSources);
+
     // The error recovery works, if:
     // - but doesn't cause such an error if we run with default settings
+    CxxConfiguration conf = new CxxConfiguration();
     conf.setErrorRecoveryEnabled(true);
-    p = CxxParser.create(mock(SquidAstVisitorContext.class), conf, CxxFileTesterHelper.mockCxxLanguage());
+    Parser<Grammar> p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
     AstNode root = p.parse(erroneousSources); //<-- this shouldn't throw now
     assertThat(root.getNumberOfChildren()).isEqualTo(6);
   }
 
-  private Collection<File> listFiles(String[] dirs, String[] extensions) {
+  private List<File> listFiles(String[] dirs, String[] extensions) {
     List<File> files = new ArrayList<>();
     for (String dir : dirs) {
       files.addAll(FileUtils.listFiles(new File(rootDir, dir), extensions, true));

cxx-squid/src/test/java/org/sonar/cxx/parser/ParserBaseTestHelper.java

@@ -19,14 +19,24 @@
  */
 package org.sonar.cxx.parser;
 
-import com.sonar.sslr.api.Grammar;
-import com.sonar.sslr.impl.Parser;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+
 import org.sonar.cxx.CxxConfiguration;
 import org.sonar.cxx.CxxFileTesterHelper;
 import org.sonar.squidbridge.SquidAstVisitorContext;
 import org.sonar.sslr.grammar.GrammarRuleKey;
 
+import com.sonar.sslr.api.Grammar;
+import com.sonar.sslr.impl.Parser;
+
+/**
+ * SquidAstVisitorContext is mock with a fake file path. You can use this base
+ * class for preprocessing tokens. You shouldn't use it for preprocessing
+ * "physical" files.
+ */
 public class ParserBaseTestHelper {
 
   protected CxxConfiguration conf = null;
@@ -36,7 +46,12 @@ public class ParserBaseTestHelper {
   public ParserBaseTestHelper() {
     conf = new CxxConfiguration();
     conf.setErrorRecoveryEnabled(false);
-    p = CxxParser.create(mock(SquidAstVisitorContext.class), conf, CxxFileTesterHelper.mockCxxLanguage());
+
+    File file = new File("snippet.cpp").getAbsoluteFile();
+    SquidAstVisitorContext<Grammar> context = mock(SquidAstVisitorContext.class);
+    when(context.getFile()).thenReturn(file);
+
+    p = CxxParser.create(context, conf, CxxFileTesterHelper.mockCxxLanguage());
     g = p.getGrammar();
   }