Merge pull request #3049 from asger-semmle/js/fix-cyclic-join

Approved by erik-krogh

Author: semmle-qlci

javascript/extractor/lib/typescript/src/main.ts

@@ -69,8 +69,11 @@ interface PrepareFilesCommand {
     command: "prepare-files";
     filenames: string[];
 }
+interface GetMetadataCommand {
+    command: "get-metadata";
+}
 type Command = ParseCommand | OpenProjectCommand | CloseProjectCommand
-    | GetTypeTableCommand | ResetCommand | QuitCommand | PrepareFilesCommand;
+    | GetTypeTableCommand | ResetCommand | QuitCommand | PrepareFilesCommand | GetMetadataCommand;
 
 /** The state to be shared between commands. */
 class State {
@@ -91,7 +94,7 @@ const reloadMemoryThresholdMb = getEnvironmentVariable("SEMMLE_TYPESCRIPT_MEMORY
 /**
  * Debugging method for finding cycles in the TypeScript AST. Should not be used in production.
  *
- * If cycles are found, additional properties should be added to `isBlacklistedProperty`.
+ * If cycles are found, the whitelist in `astProperties` is too permissive.
  */
 // tslint:disable-next-line:no-unused-variable
 function checkCycle(root: any) {
@@ -104,7 +107,8 @@ function checkCycle(root: any) {
         obj.$cycle_visiting = true;
         for (let k in obj) {
             if (!obj.hasOwnProperty(k)) continue;
-            if (isBlacklistedProperty(k)) continue;
+            // Ignore numeric and whitelisted properties.
+            if (+k !== +k && !astPropertySet.has(k)) continue;
             if (k === "$cycle_visiting") continue;
             let cycle = visit(obj[k]);
             if (cycle) {
@@ -122,30 +126,133 @@ function checkCycle(root: any) {
     }
 }
 
-/**
- * A property that should not be serialized as part of the AST, because they
- * lead to cycles or are just not needed.
- *
- * Because of restrictions on `JSON.stringify`, these properties may also not
- * be used as part of a command response.
- */
-function isBlacklistedProperty(k: string) {
-    return k === "parent" || k === "pos" || k === "end"
-        || k === "symbol" || k === "localSymbol"
-        || k === "flowNode" || k === "returnFlowNode" || k === "endFlowNode" || k === "fallthroughFlowNode"
-        || k === "nextContainer" || k === "locals"
-        || k === "bindDiagnostics" || k === "bindSuggestionDiagnostics";
-}
+/** Property names to extract from the TypeScript AST. */
+const astProperties: string[] = [
+    "$declarationKind",
+    "$declaredSignature",
+    "$end",
+    "$lineStarts",
+    "$overloadIndex",
+    "$pos",
+    "$resolvedSignature",
+    "$symbol",
+    "$tokens",
+    "$type",
+    "argument",
+    "argumentExpression",
+    "arguments",
+    "assertsModifier",
+    "asteriskToken",
+    "attributes",
+    "block",
+    "body",
+    "caseBlock",
+    "catchClause",
+    "checkType",
+    "children",
+    "clauses",
+    "closingElement",
+    "closingFragment",
+    "condition",
+    "constraint",
+    "constructor",
+    "declarationList",
+    "declarations",
+    "decorators",
+    "default",
+    "delete",
+    "dotDotDotToken",
+    "elements",
+    "elementType",
+    "elementTypes",
+    "elseStatement",
+    "escapedText",
+    "exclamationToken",
+    "exportClause",
+    "expression",
+    "exprName",
+    "extendsType",
+    "falseType",
+    "finallyBlock",
+    "flags",
+    "head",
+    "heritageClauses",
+    "importClause",
+    "incrementor",
+    "indexType",
+    "init",
+    "initializer",
+    "isExportEquals",
+    "isTypeOf",
+    "isTypeOnly",
+    "keywordToken",
+    "kind",
+    "label",
+    "left",
+    "literal",
+    "members",
+    "messageText",
+    "modifiers",
+    "moduleReference",
+    "moduleSpecifier",
+    "name",
+    "namedBindings",
+    "objectType",
+    "openingElement",
+    "openingFragment",
+    "operand",
+    "operator",
+    "operatorToken",
+    "parameterName",
+    "parameters",
+    "parseDiagnostics",
+    "properties",
+    "propertyName",
+    "qualifier",
+    "questionDotToken",
+    "questionToken",
+    "right",
+    "selfClosing",
+    "statement",
+    "statements",
+    "tag",
+    "tagName",
+    "template",
+    "templateSpans",
+    "text",
+    "thenStatement",
+    "token",
+    "tokenPos",
+    "trueType",
+    "tryBlock",
+    "type",
+    "typeArguments",
+    "typeName",
+    "typeParameter",
+    "typeParameters",
+    "types",
+    "variableDeclaration",
+    "whenFalse",
+    "whenTrue",
+];
+
+/** Property names used in a parse command response, in addition to the AST itself. */
+const astMetaProperties: string[] = [
+    "ast",
+    "type",
+];
+
+/** Property names to extract in an AST response. */
+const astPropertySet = new Set([...astProperties, ...astMetaProperties]);
 
 /**
- * Converts (part of) an AST to a JSON string, ignoring parent pointers.
+ * Converts (part of) an AST to a JSON string, ignoring properties we're not interested in.
  */
 function stringifyAST(obj: any) {
     return JSON.stringify(obj, (k, v) => {
-        if (isBlacklistedProperty(k)) {
-            return undefined;
-        }
-        return v;
+        // Filter out properties that aren't numeric, empty, or whitelisted.
+        // Note `k` is the empty string for the root object, which is also covered by +k === +k.
+        return (+k === +k || astPropertySet.has(k)) ? v : undefined;
     });
 }
 
@@ -155,8 +262,6 @@ function extractFile(filename: string): string {
     return stringifyAST({
         type: "ast",
         ast,
-        nodeFlags: ts.NodeFlags,
-        syntaxKinds: ts.SyntaxKind
     });
 }
 
@@ -522,6 +627,14 @@ function handlePrepareFilesCommand(command: PrepareFilesCommand) {
     });
 }
 
+function handleGetMetadataCommand(command: GetMetadataCommand) {
+    console.log(JSON.stringify({
+        type: "metadata",
+        syntaxKinds: ts.SyntaxKind,
+        nodeFlags: ts.NodeFlags,
+    }));
+}
+
 function reset() {
     state = new State();
     state.typeTable.restrictedExpansion = getEnvironmentVariable("SEMMLE_TYPESCRIPT_NO_EXPANSION", Boolean, true);
@@ -582,6 +695,9 @@ function runReadLineInterface() {
         case "reset":
             handleResetCommand(req);
             break;
+        case "get-metadata":
+            handleGetMetadataCommand(req);
+            break;
         case "quit":
             rl.close();
             break;

javascript/extractor/src/com/semmle/js/parser/TypeScriptASTConverter.java

@@ -2,9 +2,7 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -163,10 +161,7 @@
  */
 public class TypeScriptASTConverter {
   private String source;
-  private final JsonObject nodeFlags;
-  private final JsonObject syntaxKinds;
-  private final Map<Integer, String> nodeFlagMap = new LinkedHashMap<>();
-  private final Map<Integer, String> syntaxKindMap = new LinkedHashMap<>();
+  private final TypeScriptParserMetadata metadata;
   private int[] lineStarts;
 
   private int syntaxKindExtends;
@@ -180,22 +175,9 @@ public class TypeScriptASTConverter {
   private static final Pattern WHITESPACE_END_PAREN =
       Pattern.compile("^" + WHITESPACE_CHAR + "*\\)");
 
-  TypeScriptASTConverter(JsonObject nodeFlags, JsonObject syntaxKinds) {
-    this.nodeFlags = nodeFlags;
-    this.syntaxKinds = syntaxKinds;
-    makeEnumIdMap(nodeFlags, nodeFlagMap);
-    makeEnumIdMap(syntaxKinds, syntaxKindMap);
-    this.syntaxKindExtends = getSyntaxKind("ExtendsKeyword");
-  }
-
-  /** Builds a mapping from ID to name given a TypeScript enum object. */
-  private void makeEnumIdMap(JsonObject enumObject, Map<Integer, String> idToName) {
-    for (Map.Entry<String, JsonElement> entry : enumObject.entrySet()) {
-      JsonPrimitive prim = entry.getValue().getAsJsonPrimitive();
-      if (prim.isNumber() && !idToName.containsKey(prim.getAsInt())) {
-        idToName.put(prim.getAsInt(), entry.getKey());
-      }
-    }
+  TypeScriptASTConverter(TypeScriptParserMetadata metadata) {
+    this.metadata = metadata;
+    this.syntaxKindExtends = metadata.getSyntaxKindId("ExtendsKeyword");
   }
 
   /**
@@ -1617,7 +1599,7 @@ private Node convertMappedType(JsonObject node, SourceLocation loc) throws Parse
   private Node convertMetaProperty(JsonObject node, SourceLocation loc) throws ParseError {
     Position metaStart = loc.getStart();
     String keywordKind =
-        syntaxKinds.get(node.getAsJsonPrimitive("keywordToken").getAsInt() + "").getAsString();
+        metadata.getSyntaxKindName(node.getAsJsonPrimitive("keywordToken").getAsInt());
     String identifier = keywordKind.equals("ImportKeyword") ? "import" : "new";
     Position metaEnd =
         new Position(
@@ -1995,7 +1977,7 @@ private Node convertPrefixUnaryExpression(JsonObject node, SourceLocation loc) t
 
   private String getOperator(JsonObject node) throws ParseError {
     int operatorId = node.get("operator").getAsInt();
-    switch (syntaxKindMap.get(operatorId)) {
+    switch (metadata.getSyntaxKindName(operatorId)) {
       case "PlusPlusToken":
         return "++";
       case "MinusMinusToken":
@@ -2219,7 +2201,7 @@ private Node convertTypeOfExpression(JsonObject node, SourceLocation loc) throws
   }
 
   private Node convertTypeOperator(JsonObject node, SourceLocation loc) throws ParseError {
-    String operator = syntaxKinds.get("" + node.get("operator").getAsInt()).getAsString();
+    String operator = metadata.getSyntaxKindName(node.get("operator").getAsInt());
     if (operator.equals("KeyOfKeyword")) {
       return new UnaryTypeExpr(loc, UnaryTypeExpr.Kind.Keyof, convertChildAsType(node, "type"));
     }
@@ -2537,29 +2519,14 @@ private int getMemberModifierKeywords(JsonObject node) {
    * <tt>ts.NodeFlags</tt> in enum.
    */
   private boolean hasFlag(JsonObject node, String flagName) {
-    JsonElement flagDescriptor = this.nodeFlags.get(flagName);
-    if (flagDescriptor == null) {
-      throw new RuntimeException(
-          "Incompatible version of TypeScript installed. Missing node flag " + flagName);
-    }
-    int flagId = flagDescriptor.getAsInt();
+    int flagId = metadata.getNodeFlagId(flagName);
     JsonElement flags = node.get("flags");
     if (flags instanceof JsonPrimitive) {
       return (flags.getAsInt() & flagId) != 0;
     }
     return false;
   }
 
-  /** Gets the numeric value of the syntax kind enum with the given name. */
-  private int getSyntaxKind(String syntaxKind) {
-    JsonElement descriptor = this.syntaxKinds.get(syntaxKind);
-    if (descriptor == null) {
-      throw new RuntimeException(
-          "Incompatible version of TypeScript installed. Missing syntax kind " + syntaxKind);
-    }
-    return descriptor.getAsInt();
-  }
-
   /** Check whether a node has a child with a given name. */
   private boolean hasChild(JsonObject node, String prop) {
     if (!node.has(prop)) return false;
@@ -2581,7 +2548,7 @@ private String getKind(JsonElement node) {
     if (node instanceof JsonObject) {
       JsonElement kind = ((JsonObject) node).get("kind");
       if (kind instanceof JsonPrimitive && ((JsonPrimitive) kind).isNumber())
-        return syntaxKindMap.get(kind.getAsInt());
+        return metadata.getSyntaxKindName(kind.getAsInt());
     }
     return null;
   }

javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java

@@ -150,6 +150,9 @@ public class TypeScriptParser {
   /** If non-zero, we use this instead of relying on the corresponding environment variable. */
   private int typescriptRam = 0;
 
+  /** Metadata requested immediately after starting the TypeScript parser. */
+  private TypeScriptParserMetadata metadata;
+
   /** Sets the amount of RAM to allocate to the TypeScript compiler.s */
   public void setTypescriptRam(int megabytes) {
     this.typescriptRam = megabytes;
@@ -297,6 +300,7 @@ private void setupParserWrapper() {
       InputStream is = parserWrapperProcess.getInputStream();
       InputStreamReader isr = new InputStreamReader(is, "UTF-8");
       fromParserWrapper = new BufferedReader(isr);
+      this.loadMetadata();
     } catch (IOException e) {
       throw new CatastrophicError(
           "Could not start TypeScript parser wrapper " + "(command: ." + parserWrapperCommand + ")",
@@ -385,6 +389,17 @@ private RuntimeException getExceptionFromMalformedResponse(String response, Exce
     return new CatastrophicError("Unexpected response from TypeScript parser wrapper:\n" + response, e);
   }
 
+  /**
+   * Requests metadata from the TypeScript process. See {@link TypeScriptParserMetadata}.
+   */
+  private void loadMetadata() {
+    JsonObject request = new JsonObject();
+    request.add("command", new JsonPrimitive("get-metadata"));
+    JsonObject response = talkToParserWrapper(request);
+    checkResponseType(response, "metadata");
+    this.metadata = new TypeScriptParserMetadata(response);
+  }
+
   /**
    * Returns the AST for a given source file.
    *
@@ -402,11 +417,9 @@ public Result parse(File sourceFile, String source, ExtractionMetrics metrics) {
     metrics.stopPhase(ExtractionMetrics.ExtractionPhase.TypeScriptParser_talkToParserWrapper);
     try {
       checkResponseType(response, "ast");
-      JsonObject nodeFlags = response.get("nodeFlags").getAsJsonObject();
-      JsonObject syntaxKinds = response.get("syntaxKinds").getAsJsonObject();
       JsonObject ast = response.get("ast").getAsJsonObject();
       metrics.startPhase(ExtractionMetrics.ExtractionPhase.TypeScriptASTConverter_convertAST);
-      Result converted = new TypeScriptASTConverter(nodeFlags, syntaxKinds).convertAST(ast, source);
+      Result converted = new TypeScriptASTConverter(metadata).convertAST(ast, source);
       metrics.stopPhase(ExtractionMetrics.ExtractionPhase.TypeScriptASTConverter_convertAST);
       return converted;
     } catch (IllegalStateException e) {