From cb0a2498b004857b5b56ce13af8836d4e8d31b23 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 3 Jun 2020 23:41:40 +0100 Subject: [PATCH 01/12] JS: Sort files --- .../com/semmle/js/extractor/AutoBuild.java | 53 +++++++++++++++---- .../js/extractor/test/AutoBuildTests.java | 5 +- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 9c31c7afc87e..402797c1bf2c 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -20,6 +20,7 @@ import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -28,6 +29,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; import java.util.stream.Stream; import com.google.gson.Gson; @@ -536,6 +538,27 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) Files.walkFileTree(externs, visitor); } + /** + * Compares files in the order they should be extracted. + *

+ * The ordering of tsconfig.json files can affect extraction results. Since we + * extract any given source file at most once, and a source file can be included from + * multiple tsconfig.json files, we sometimes have to choose arbitrarily which tsconfig.json + * to use for a given file (which is based on this ordering). + *

+ * We sort them to help ensure reproducible extraction. Additionally, deeply nested files are + * preferred over shallow ones to help ensure files are extracted with the most specific + * tsconfig.json file. + */ + public static final Comparator PATH_ORDERING = new Comparator() { + public int compare(Path f1, Path f2) { + if (f1.getNameCount() != f2.getNameCount()) { + return f2.getNameCount() - f1.getNameCount(); + } + return f1.compareTo(f2); + } + }; + /** Extract all supported candidate files that pass the filters. */ private void extractSource() throws IOException { // default extractor @@ -554,6 +577,14 @@ private void extractSource() throws IOException { Set filesToExtract = new LinkedHashSet<>(); List tsconfigFiles = new ArrayList<>(); findFilesToExtract(defaultExtractor, filesToExtract, tsconfigFiles); + + tsconfigFiles = tsconfigFiles.stream() + .sorted(PATH_ORDERING) + .collect(Collectors.toList()); + + filesToExtract = filesToExtract.stream() + .sorted(PATH_ORDERING) + .collect(Collectors.toCollection(() -> new LinkedHashSet<>())); DependencyInstallationResult dependencyInstallationResult = DependencyInstallationResult.empty; if (!tsconfigFiles.isEmpty() && this.installDependencies) { @@ -902,7 +933,7 @@ private Set extractTypeScript( logEndProcess(start, "Done opening project " + projectFile); // Extract all files belonging to this project which are also matched // by our include/exclude filters. - List typeScriptFiles = new ArrayList(); + List typeScriptFiles = new ArrayList(); for (File sourceFile : project.getSourceFiles()) { Path sourcePath = sourceFile.toPath(); if (!files.contains(normalizePath(sourcePath))) continue; @@ -912,9 +943,10 @@ private Set extractTypeScript( continue; } if (!extractedFiles.contains(sourcePath)) { - typeScriptFiles.add(sourcePath.toFile()); + typeScriptFiles.add(sourcePath); } } + typeScriptFiles.sort(PATH_ORDERING); extractTypeScriptFiles(typeScriptFiles, extractedFiles, extractor, extractorState); tsParser.closeProject(projectFile); } @@ -926,11 +958,11 @@ private Set extractTypeScript( } // Extract remaining TypeScript files. - List remainingTypeScriptFiles = new ArrayList(); + List remainingTypeScriptFiles = new ArrayList<>(); for (Path f : files) { if (!extractedFiles.contains(f) && FileType.forFileExtension(f.toFile()) == FileType.TYPESCRIPT) { - remainingTypeScriptFiles.add(f.toFile()); + remainingTypeScriptFiles.add(f); } } if (!remainingTypeScriptFiles.isEmpty()) { @@ -1018,15 +1050,18 @@ public void verifyTypeScriptInstallation(ExtractorState extractorState) { } public void extractTypeScriptFiles( - List files, + List files, Set extractedFiles, FileExtractor extractor, ExtractorState extractorState) { - extractorState.getTypeScriptParser().prepareFiles(files); - for (File f : files) { - Path path = f.toPath(); + List list = files + .stream() + .sorted(PATH_ORDERING) + .map(p -> p.toFile()).collect(Collectors.toList()); + extractorState.getTypeScriptParser().prepareFiles(list); + for (Path path : files) { extractedFiles.add(path); - extract(extractor, f.toPath(), extractorState); + extract(extractor, path, extractorState); } } diff --git a/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java b/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java index aefe68f73703..99918cde52a9 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java +++ b/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java @@ -1,6 +1,5 @@ package com.semmle.js.extractor.test; -import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.FileVisitResult; @@ -122,11 +121,11 @@ public void verifyTypeScriptInstallation(ExtractorState state) {} @Override public void extractTypeScriptFiles( - java.util.List files, + java.util.List files, java.util.Set extractedFiles, FileExtractor extractor, ExtractorState extractorState) { - for (File f : files) { + for (Path f : files) { actual.add(f.toString()); } } From aaf141782f1a3ee0bafab025a8226bdc3c35870b Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 3 Jun 2020 09:13:32 +0100 Subject: [PATCH 02/12] JS: Fix source root --- .../extractor/src/com/semmle/js/extractor/AutoBuild.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 402797c1bf2c..1bf380798178 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -712,9 +712,8 @@ protected DependencyInstallationResult installDependencies(Set filesToExtr if (!verifyYarnInstallation()) { return DependencyInstallationResult.empty; } - - final Path sourceRoot = Paths.get(".").toAbsolutePath(); - final Path virtualSourceRoot = Paths.get(EnvironmentVariables.getScratchDir()).toAbsolutePath(); + final Path sourceRoot = LGTM_SRC; + final Path virtualSourceRoot = toRealPath(Paths.get(EnvironmentVariables.getScratchDir())); // Read all package.json files and index them by name. Map packageJsonFiles = new LinkedHashMap<>(); From dceb2110210f89c8a1b1df4ef12aa49baaf028a9 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 3 Jun 2020 10:57:12 +0100 Subject: [PATCH 03/12] JS: Pass source root to Node.js process --- javascript/extractor/lib/typescript/src/main.ts | 4 +++- .../lib/typescript/src/virtual_source_root.ts | 6 +++--- .../src/com/semmle/js/extractor/AutoBuild.java | 2 +- .../extractor/DependencyInstallationResult.java | 15 ++++++++++++++- .../com/semmle/js/parser/TypeScriptParser.java | 3 +++ 5 files changed, 24 insertions(+), 6 deletions(-) diff --git a/javascript/extractor/lib/typescript/src/main.ts b/javascript/extractor/lib/typescript/src/main.ts index 55d08b5d2628..490b076278a8 100644 --- a/javascript/extractor/lib/typescript/src/main.ts +++ b/javascript/extractor/lib/typescript/src/main.ts @@ -48,6 +48,7 @@ interface ParseCommand { interface OpenProjectCommand { command: "open-project"; tsConfig: string; + sourceRoot: string | null; virtualSourceRoot: string | null; packageEntryPoints: [string, string][]; packageJsonFiles: [string, string][]; @@ -370,7 +371,7 @@ function handleOpenProjectCommand(command: OpenProjectCommand) { let packageEntryPoints = new Map(command.packageEntryPoints); let packageJsonFiles = new Map(command.packageJsonFiles); - let virtualSourceRoot = new VirtualSourceRoot(process.cwd(), command.virtualSourceRoot); + let virtualSourceRoot = new VirtualSourceRoot(command.sourceRoot, command.virtualSourceRoot); /** * Rewrites path segments of form `node_modules/PACK/suffix` to be relative to @@ -720,6 +721,7 @@ if (process.argv.length > 2) { tsConfig: argument, packageEntryPoints: [], packageJsonFiles: [], + sourceRoot: null, virtualSourceRoot: null, }); for (let sf of state.project.program.getSourceFiles()) { diff --git a/javascript/extractor/lib/typescript/src/virtual_source_root.ts b/javascript/extractor/lib/typescript/src/virtual_source_root.ts index 5f1bbf2b95ed..8c7c57c24b59 100644 --- a/javascript/extractor/lib/typescript/src/virtual_source_root.ts +++ b/javascript/extractor/lib/typescript/src/virtual_source_root.ts @@ -7,20 +7,20 @@ import * as ts from "./typescript"; */ export class VirtualSourceRoot { constructor( - private sourceRoot: string, + private sourceRoot: string | null, /** * Directory whose folder structure mirrors the real source root, but with `node_modules` installed, * or undefined if no virtual source root exists. */ - private virtualSourceRoot: string, + private virtualSourceRoot: string | null, ) {} /** * Maps a path under the real source root to the corresponding path in the virtual source root. */ public toVirtualPath(path: string) { - if (!this.virtualSourceRoot) return null; + if (!this.virtualSourceRoot || !this.sourceRoot) return null; let relative = pathlib.relative(this.sourceRoot, path); if (relative.startsWith('..') || pathlib.isAbsolute(relative)) return null; return pathlib.join(this.virtualSourceRoot, relative); diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 1bf380798178..210daa32b71c 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -837,7 +837,7 @@ protected DependencyInstallationResult installDependencies(Set filesToExtr } } - return new DependencyInstallationResult(virtualSourceRoot, packageMainFile, packagesInRepo); + return new DependencyInstallationResult(sourceRoot, virtualSourceRoot, packageMainFile, packagesInRepo); } /** diff --git a/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java b/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java index 460a6573f6b2..cb32e4b46b1d 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java +++ b/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java @@ -6,14 +6,16 @@ /** Contains the results of installing dependencies. */ public class DependencyInstallationResult { + private Path sourceRoot; private Path virtualSourceRoot; private Map packageEntryPoints; private Map packageJsonFiles; public static final DependencyInstallationResult empty = - new DependencyInstallationResult(null, Collections.emptyMap(), Collections.emptyMap()); + new DependencyInstallationResult(null, null, Collections.emptyMap(), Collections.emptyMap()); public DependencyInstallationResult( + Path sourceRoot, Path virtualSourceRoot, Map packageEntryPoints, Map packageJsonFiles) { @@ -21,6 +23,17 @@ public DependencyInstallationResult( this.packageJsonFiles = packageJsonFiles; } + /** + * Returns the source root mirrored by {@link #getVirtualSourceRoot()} or null + * if no virtual source root exists. + *

+ * When invoked from the AutoBuilder, this corresponds to the source root. When invoked + * from ODASA, there is no notion of source root, so this is always null in that context. + */ + public Path getSourceRoot() { + return sourceRoot; + } + /** * Returns the virtual source root or null if no virtual source root exists. * diff --git a/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java b/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java index 0d505ecb578d..9ff1c9b69f1f 100644 --- a/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java +++ b/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java @@ -502,6 +502,9 @@ public ParsedProject openProject(File tsConfigFile, DependencyInstallationResult request.add("tsConfig", new JsonPrimitive(tsConfigFile.getPath())); request.add("packageEntryPoints", mapToArray(deps.getPackageEntryPoints())); request.add("packageJsonFiles", mapToArray(deps.getPackageJsonFiles())); + request.add("sourceRoot", deps.getSourceRoot() == null + ? JsonNull.INSTANCE + : new JsonPrimitive(deps.getSourceRoot().toString())); request.add("virtualSourceRoot", deps.getVirtualSourceRoot() == null ? JsonNull.INSTANCE : new JsonPrimitive(deps.getVirtualSourceRoot().toString())); From ba5d6bb2e9e70f9a76d4fb4b0ce750724c068e98 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 3 Jun 2020 12:24:01 +0100 Subject: [PATCH 04/12] JS: Actually set fields --- .../com/semmle/js/extractor/DependencyInstallationResult.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java b/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java index cb32e4b46b1d..10fc1b616c60 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java +++ b/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java @@ -19,6 +19,8 @@ public DependencyInstallationResult( Path virtualSourceRoot, Map packageEntryPoints, Map packageJsonFiles) { + this.sourceRoot = sourceRoot; + this.virtualSourceRoot = virtualSourceRoot; this.packageEntryPoints = packageEntryPoints; this.packageJsonFiles = packageJsonFiles; } From 6d15397fdc7b4ee597a11666ebde56ea5a26ed2f Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 3 Jun 2020 09:19:37 +0100 Subject: [PATCH 05/12] JS: Ensure we never write outside the scratch dir --- .../src/com/semmle/js/extractor/AutoBuild.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 210daa32b71c..e87dcea27da2 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -689,6 +689,19 @@ private String getChildAsString(JsonObject obj, String name) { return null; } + /** + * Gets a relative path from from to to provided + * the latter is contained in the former. Otherwise returns null. + * @return a path or null + */ + public static Path tryRelativize(Path from, Path to) { + Path relative = from.relativize(to); + if (relative.startsWith("..") || relative.isAbsolute()) { + return null; + } + return relative; + } + /** * Installs dependencies for use by the TypeScript type checker. *

@@ -727,6 +740,9 @@ protected DependencyInstallationResult installDependencies(Set filesToExtr if (!(json instanceof JsonObject)) continue; JsonObject jsonObject = (JsonObject) json; file = file.toAbsolutePath(); + if (tryRelativize(sourceRoot, file) == null) { + continue; // Ignore package.json files outside the source root. + } packageJsonFiles.put(file, jsonObject); String name = getChildAsString(jsonObject, "name"); From 6ff81377d5b5e5fce112c5597156053caf5a10a7 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 5 Jun 2020 11:54:40 +0100 Subject: [PATCH 06/12] JS: Also sort files in legacy extractor --- .../src/com/semmle/js/extractor/AutoBuild.java | 15 ++++++++++++--- .../src/com/semmle/js/extractor/Main.java | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index e87dcea27da2..8b9a6fa8f639 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -559,6 +559,15 @@ public int compare(Path f1, Path f2) { } }; + /** + * Like {@link #PATH_ORDERING} but for {@link File} objects. + */ + public static final Comparator FILE_ORDERING = new Comparator() { + public int compare(File f1, File f2) { + return PATH_ORDERING.compare(f1.toPath(), f2.toPath()); + } + }; + /** Extract all supported candidate files that pass the filters. */ private void extractSource() throws IOException { // default extractor @@ -577,11 +586,11 @@ private void extractSource() throws IOException { Set filesToExtract = new LinkedHashSet<>(); List tsconfigFiles = new ArrayList<>(); findFilesToExtract(defaultExtractor, filesToExtract, tsconfigFiles); - + tsconfigFiles = tsconfigFiles.stream() .sorted(PATH_ORDERING) .collect(Collectors.toList()); - + filesToExtract = filesToExtract.stream() .sorted(PATH_ORDERING) .collect(Collectors.toCollection(() -> new LinkedHashSet<>())); @@ -695,7 +704,7 @@ private String getChildAsString(JsonObject obj, String name) { * @return a path or null */ public static Path tryRelativize(Path from, Path to) { - Path relative = from.relativize(to); + Path relative = from.relativize(to); if (relative.startsWith("..") || relative.isAbsolute()) { return null; } diff --git a/javascript/extractor/src/com/semmle/js/extractor/Main.java b/javascript/extractor/src/com/semmle/js/extractor/Main.java index 904969a168fb..873a84966525 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/Main.java +++ b/javascript/extractor/src/com/semmle/js/extractor/Main.java @@ -7,6 +7,7 @@ import java.util.List; import java.util.Set; import java.util.regex.Pattern; +import java.util.stream.Collectors; import com.semmle.js.extractor.ExtractorConfig.HTMLHandling; import com.semmle.js.extractor.ExtractorConfig.Platform; @@ -77,8 +78,8 @@ public class Main { private PathMatcher includeMatcher, excludeMatcher; private FileExtractor fileExtractor; private ExtractorState extractorState; - private final Set projectFiles = new LinkedHashSet<>(); - private final Set files = new LinkedHashSet<>(); + private Set projectFiles = new LinkedHashSet<>(); + private Set files = new LinkedHashSet<>(); private final Set extractedFiles = new LinkedHashSet<>(); /* used to detect cyclic directory hierarchies */ @@ -138,6 +139,16 @@ public void run(String[] args) { if (containsTypeScriptFiles()) { tsParser.verifyInstallation(!ap.has(P_QUIET)); } + + // Sort files for determinism + projectFiles = projectFiles.stream() + .sorted(AutoBuild.FILE_ORDERING) + .collect(Collectors.toCollection(() -> new LinkedHashSet<>())); + + files = files.stream() + .sorted(AutoBuild.FILE_ORDERING) + .collect(Collectors.toCollection(() -> new LinkedHashSet<>())); + for (File projectFile : projectFiles) { long start = verboseLogStartTimer(ap, "Opening project " + projectFile); From cf784757997035b07430bc1eb96068122abf2a81 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 5 Jun 2020 13:42:11 +0100 Subject: [PATCH 07/12] JS: Only extract included files with a given tsconfig --- javascript/extractor/lib/typescript/src/main.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/extractor/lib/typescript/src/main.ts b/javascript/extractor/lib/typescript/src/main.ts index 490b076278a8..7355c58b51a2 100644 --- a/javascript/extractor/lib/typescript/src/main.ts +++ b/javascript/extractor/lib/typescript/src/main.ts @@ -590,7 +590,7 @@ function handleOpenProjectCommand(command: OpenProjectCommand) { console.log(JSON.stringify({ type: "project-opened", - files: program.getSourceFiles().map(sf => pathlib.resolve(sf.fileName)), + files: config.fileNames.map(file => pathlib.resolve(file)), })); } From cc3e62f5357459aade88219cbc8d413b8ab016fa Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 12 Jun 2020 17:00:25 +0100 Subject: [PATCH 08/12] JS: Move stack trace limit to top of file --- javascript/extractor/lib/typescript/src/main.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/extractor/lib/typescript/src/main.ts b/javascript/extractor/lib/typescript/src/main.ts index 7355c58b51a2..90369fa92a39 100644 --- a/javascript/extractor/lib/typescript/src/main.ts +++ b/javascript/extractor/lib/typescript/src/main.ts @@ -41,6 +41,9 @@ import { Project } from "./common"; import { TypeTable } from "./type_table"; import { VirtualSourceRoot } from "./virtual_source_root"; +// Remove limit on stack trace depth. +Error.stackTraceLimit = Infinity; + interface ParseCommand { command: "parse"; filename: string; @@ -364,7 +367,6 @@ function parseSingleFile(filename: string): {ast: ts.SourceFile, code: string} { const nodeModulesRex = /[/\\]node_modules[/\\]((?:@[\w.-]+[/\\])?\w[\w.-]*)[/\\](.*)/; function handleOpenProjectCommand(command: OpenProjectCommand) { - Error.stackTraceLimit = Infinity; let tsConfigFilename = String(command.tsConfig); let tsConfig = ts.readConfigFile(tsConfigFilename, ts.sys.readFile); let basePath = pathlib.dirname(tsConfigFilename); From 4c4acd50bdb05ad8badb41c06213aadd90359766 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 12 Jun 2020 17:12:08 +0100 Subject: [PATCH 09/12] JS: Factor out loading of tsconfig files --- .../extractor/lib/typescript/src/main.ts | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/javascript/extractor/lib/typescript/src/main.ts b/javascript/extractor/lib/typescript/src/main.ts index 90369fa92a39..80cf70e99cbf 100644 --- a/javascript/extractor/lib/typescript/src/main.ts +++ b/javascript/extractor/lib/typescript/src/main.ts @@ -48,14 +48,16 @@ interface ParseCommand { command: "parse"; filename: string; } -interface OpenProjectCommand { - command: "open-project"; +interface LoadCommand { tsConfig: string; sourceRoot: string | null; virtualSourceRoot: string | null; packageEntryPoints: [string, string][]; packageJsonFiles: [string, string][]; } +interface OpenProjectCommand extends LoadCommand { + command: "open-project"; +} interface CloseProjectCommand { command: "close-project"; tsConfig: string; @@ -366,10 +368,17 @@ function parseSingleFile(filename: string): {ast: ts.SourceFile, code: string} { */ const nodeModulesRex = /[/\\]node_modules[/\\]((?:@[\w.-]+[/\\])?\w[\w.-]*)[/\\](.*)/; -function handleOpenProjectCommand(command: OpenProjectCommand) { - let tsConfigFilename = String(command.tsConfig); - let tsConfig = ts.readConfigFile(tsConfigFilename, ts.sys.readFile); - let basePath = pathlib.dirname(tsConfigFilename); +interface LoadedConfig { + config: ts.ParsedCommandLine; + basePath: string; + packageEntryPoints: Map; + packageJsonFiles: Map; + virtualSourceRoot: VirtualSourceRoot; +} + +function loadTsConfig(command: LoadCommand): LoadedConfig { + let tsConfig = ts.readConfigFile(command.tsConfig, ts.sys.readFile); + let basePath = pathlib.dirname(command.tsConfig); let packageEntryPoints = new Map(command.packageEntryPoints); let packageJsonFiles = new Map(command.packageJsonFiles); @@ -418,7 +427,14 @@ function handleOpenProjectCommand(command: OpenProjectCommand) { } }; let config = ts.parseJsonConfigFileContent(tsConfig.config, parseConfigHost, basePath); - let project = new Project(tsConfigFilename, config, state.typeTable, packageEntryPoints, virtualSourceRoot); + + return { config, basePath, packageJsonFiles, packageEntryPoints, virtualSourceRoot }; +} + +function handleOpenProjectCommand(command: OpenProjectCommand) { + let { config, packageEntryPoints, virtualSourceRoot, basePath } = loadTsConfig(command); + + let project = new Project(command.tsConfig, config, state.typeTable, packageEntryPoints, virtualSourceRoot); project.load(); state.project = project; From 675c64d9d4b32957c0c821c7f80d60e2bab7c3c8 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 12 Jun 2020 21:49:43 +0100 Subject: [PATCH 10/12] JS: Prefer extracting file with tsconfig that included it --- .../extractor/lib/typescript/src/main.ts | 35 ++++++++++-- .../com/semmle/js/extractor/AutoBuild.java | 13 ++++- .../src/com/semmle/js/extractor/Main.java | 2 +- .../com/semmle/js/parser/ParsedProject.java | 17 +++--- .../semmle/js/parser/TypeScriptParser.java | 56 ++++++++++++++----- 5 files changed, 96 insertions(+), 27 deletions(-) diff --git a/javascript/extractor/lib/typescript/src/main.ts b/javascript/extractor/lib/typescript/src/main.ts index 80cf70e99cbf..e4c867d0b43c 100644 --- a/javascript/extractor/lib/typescript/src/main.ts +++ b/javascript/extractor/lib/typescript/src/main.ts @@ -58,6 +58,9 @@ interface LoadCommand { interface OpenProjectCommand extends LoadCommand { command: "open-project"; } +interface GetOwnFilesCommand extends LoadCommand { + command: "get-own-files"; +} interface CloseProjectCommand { command: "close-project"; tsConfig: string; @@ -78,7 +81,7 @@ interface PrepareFilesCommand { interface GetMetadataCommand { command: "get-metadata"; } -type Command = ParseCommand | OpenProjectCommand | CloseProjectCommand +type Command = ParseCommand | OpenProjectCommand | GetOwnFilesCommand | CloseProjectCommand | GetTypeTableCommand | ResetCommand | QuitCommand | PrepareFilesCommand | GetMetadataCommand; /** The state to be shared between commands. */ @@ -374,6 +377,7 @@ interface LoadedConfig { packageEntryPoints: Map; packageJsonFiles: Map; virtualSourceRoot: VirtualSourceRoot; + ownFiles: string[]; } function loadTsConfig(command: LoadCommand): LoadedConfig { @@ -428,11 +432,26 @@ function loadTsConfig(command: LoadCommand): LoadedConfig { }; let config = ts.parseJsonConfigFileContent(tsConfig.config, parseConfigHost, basePath); - return { config, basePath, packageJsonFiles, packageEntryPoints, virtualSourceRoot }; + let ownFiles = config.fileNames.map(file => pathlib.resolve(file)); + + return { config, basePath, packageJsonFiles, packageEntryPoints, virtualSourceRoot, ownFiles }; +} + +/** + * Returns the list of files included in the given tsconfig.json file's include pattern, + * (not including those only references through imports). + */ +function handleGetFileListCommand(command: GetOwnFilesCommand) { + let { config, ownFiles } = loadTsConfig(command); + + console.log(JSON.stringify({ + type: "file-list", + ownFiles, + })); } function handleOpenProjectCommand(command: OpenProjectCommand) { - let { config, packageEntryPoints, virtualSourceRoot, basePath } = loadTsConfig(command); + let { config, packageEntryPoints, virtualSourceRoot, basePath, ownFiles } = loadTsConfig(command); let project = new Project(command.tsConfig, config, state.typeTable, packageEntryPoints, virtualSourceRoot); project.load(); @@ -606,9 +625,14 @@ function handleOpenProjectCommand(command: OpenProjectCommand) { return symbol; } + // Unlike in the get-own-files command, this command gets all files we can possibly + // extract type information for, including files referenced outside the tsconfig's inclusion pattern. + let allFiles = program.getSourceFiles().map(sf => pathlib.resolve(sf.fileName)); + console.log(JSON.stringify({ type: "project-opened", - files: config.fileNames.map(file => pathlib.resolve(file)), + ownFiles, + allFiles, })); } @@ -704,6 +728,9 @@ function runReadLineInterface() { case "open-project": handleOpenProjectCommand(req); break; + case "get-own-files": + handleGetFileListCommand(req); + break; case "close-project": handleCloseProjectCommand(req); break; diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 8b9a6fa8f639..39f144974235 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -949,6 +949,16 @@ private Set extractTypeScript( TypeScriptParser tsParser = extractorState.getTypeScriptParser(); verifyTypeScriptInstallation(extractorState); + // Collect all files included in a tsconfig.json inclusion pattern. + // If a given file is referenced by multiple tsconfig files, we prefer to extract it using + // one that includes it rather than just references it. + Set explicitlyIncludedFiles = new LinkedHashSet<>(); + if (tsconfig.size() > 1) { // No prioritization needed if there's only one tsconfig. + for (Path projectPath : tsconfig) { + explicitlyIncludedFiles.addAll(tsParser.getOwnFiles(projectPath.toFile(), deps)); + } + } + // Extract TypeScript projects for (Path projectPath : tsconfig) { File projectFile = projectPath.toFile(); @@ -958,9 +968,10 @@ private Set extractTypeScript( // Extract all files belonging to this project which are also matched // by our include/exclude filters. List typeScriptFiles = new ArrayList(); - for (File sourceFile : project.getSourceFiles()) { + for (File sourceFile : project.getAllFiles()) { Path sourcePath = sourceFile.toPath(); if (!files.contains(normalizePath(sourcePath))) continue; + if (!project.getOwnFiles().contains(sourceFile) && explicitlyIncludedFiles.contains(sourceFile)) continue; if (!FileType.TYPESCRIPT.getExtensions().contains(FileUtil.extension(sourcePath))) { // For the time being, skip non-TypeScript files, even if the TypeScript // compiler can parse them for us. diff --git a/javascript/extractor/src/com/semmle/js/extractor/Main.java b/javascript/extractor/src/com/semmle/js/extractor/Main.java index 873a84966525..feb0ebfe2bbf 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/Main.java +++ b/javascript/extractor/src/com/semmle/js/extractor/Main.java @@ -157,7 +157,7 @@ public void run(String[] args) { // Extract all files belonging to this project which are also matched // by our include/exclude filters. List filesToExtract = new ArrayList<>(); - for (File sourceFile : project.getSourceFiles()) { + for (File sourceFile : project.getOwnFiles()) { if (files.contains(normalizeFile(sourceFile)) && !extractedFiles.contains(sourceFile.getAbsoluteFile()) && FileType.TYPESCRIPT.getExtensions().contains(FileUtil.extension(sourceFile))) { diff --git a/javascript/extractor/src/com/semmle/js/parser/ParsedProject.java b/javascript/extractor/src/com/semmle/js/parser/ParsedProject.java index cb01b76f9858..49c6049d4524 100644 --- a/javascript/extractor/src/com/semmle/js/parser/ParsedProject.java +++ b/javascript/extractor/src/com/semmle/js/parser/ParsedProject.java @@ -1,15 +1,17 @@ package com.semmle.js.parser; import java.io.File; -import java.util.LinkedHashSet; import java.util.Set; public class ParsedProject { private final File tsConfigFile; - private final Set sourceFiles = new LinkedHashSet<>(); + private final Set ownFiles; + private final Set allFiles; - public ParsedProject(File tsConfigFile) { + public ParsedProject(File tsConfigFile, Set ownFiles, Set allFiles) { this.tsConfigFile = tsConfigFile; + this.ownFiles = ownFiles; + this.allFiles = allFiles; } /** Returns the tsconfig.json file that defines this project. */ @@ -18,11 +20,12 @@ public File getTsConfigFile() { } /** Absolute paths to the files included in this project. */ - public Set getSourceFiles() { - return sourceFiles; + public Set getOwnFiles() { + return allFiles; } - public void addSourceFile(File file) { - sourceFiles.add(file); + /** Absolute paths to the files included in or referenced by this project. */ + public Set getAllFiles() { + return allFiles; } } diff --git a/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java b/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java index 9ff1c9b69f1f..24451b581e0b 100644 --- a/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java +++ b/javascript/extractor/src/com/semmle/js/parser/TypeScriptParser.java @@ -15,8 +15,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import com.google.gson.JsonArray; @@ -488,6 +490,29 @@ private JsonArray mapToArray(Map map) { return result; } + private static Set getFilesFromJsonArray(JsonArray array) { + Set files = new LinkedHashSet<>(); + for (JsonElement elm : array) { + files.add(new File(elm.getAsString())); + } + return files; + } + + /** + * Returns the set of files included by the inclusion pattern in the given tsconfig.json file. + */ + public Set getOwnFiles(File tsConfigFile, DependencyInstallationResult deps) { + JsonObject request = makeLoadCommand("get-own-files", tsConfigFile, deps); + JsonObject response = talkToParserWrapper(request); + try { + checkResponseType(response, "file-list"); + return getFilesFromJsonArray(response.get("ownFiles").getAsJsonArray()); + } catch (IllegalStateException e) { + throw new CatastrophicError( + "TypeScript parser wrapper sent unexpected response: " + response, e); + } + } + /** * Opens a new project based on a tsconfig.json file. The compiler will analyze all files in the * project. @@ -497,8 +522,23 @@ private JsonArray mapToArray(Map map) { *

Only one project should be opened at once. */ public ParsedProject openProject(File tsConfigFile, DependencyInstallationResult deps) { + JsonObject request = makeLoadCommand("open-project", tsConfigFile, deps); + JsonObject response = talkToParserWrapper(request); + try { + checkResponseType(response, "project-opened"); + ParsedProject project = new ParsedProject(tsConfigFile, + getFilesFromJsonArray(response.get("ownFiles").getAsJsonArray()), + getFilesFromJsonArray(response.get("allFiles").getAsJsonArray())); + return project; + } catch (IllegalStateException e) { + throw new CatastrophicError( + "TypeScript parser wrapper sent unexpected response: " + response, e); + } + } + + private JsonObject makeLoadCommand(String command, File tsConfigFile, DependencyInstallationResult deps) { JsonObject request = new JsonObject(); - request.add("command", new JsonPrimitive("open-project")); + request.add("command", new JsonPrimitive(command)); request.add("tsConfig", new JsonPrimitive(tsConfigFile.getPath())); request.add("packageEntryPoints", mapToArray(deps.getPackageEntryPoints())); request.add("packageJsonFiles", mapToArray(deps.getPackageJsonFiles())); @@ -508,19 +548,7 @@ public ParsedProject openProject(File tsConfigFile, DependencyInstallationResult request.add("virtualSourceRoot", deps.getVirtualSourceRoot() == null ? JsonNull.INSTANCE : new JsonPrimitive(deps.getVirtualSourceRoot().toString())); - JsonObject response = talkToParserWrapper(request); - try { - checkResponseType(response, "project-opened"); - ParsedProject project = new ParsedProject(tsConfigFile); - JsonArray filesJson = response.get("files").getAsJsonArray(); - for (JsonElement elm : filesJson) { - project.addSourceFile(new File(elm.getAsString())); - } - return project; - } catch (IllegalStateException e) { - throw new CatastrophicError( - "TypeScript parser wrapper sent unexpected response: " + response, e); - } + return request; } /** From ad48c4e54d1b80c698f61f1274eea2e97c2b1593 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Mon, 1 Jun 2020 11:24:42 +0100 Subject: [PATCH 11/12] JS: Always prepare package.json files --- .../com/semmle/js/extractor/AutoBuild.java | 60 +++++++++---------- .../js/extractor/test/AutoBuildTests.java | 4 +- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 39f144974235..15c721a11c9d 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -596,8 +596,8 @@ private void extractSource() throws IOException { .collect(Collectors.toCollection(() -> new LinkedHashSet<>())); DependencyInstallationResult dependencyInstallationResult = DependencyInstallationResult.empty; - if (!tsconfigFiles.isEmpty() && this.installDependencies) { - dependencyInstallationResult = this.installDependencies(filesToExtract); + if (!tsconfigFiles.isEmpty()) { + dependencyInstallationResult = this.preparePackagesAndDependencies(filesToExtract); } // extract TypeScript projects and files @@ -712,7 +712,8 @@ public static Path tryRelativize(Path from, Path to) { } /** - * Installs dependencies for use by the TypeScript type checker. + * Prepares package.json files in a virtual source root, and, if enabled, + * installs dependencies for use by the TypeScript type checker. *

* Some packages must be downloaded while others exist within the same repo ("monorepos") * but are not in a location where TypeScript would look for it. @@ -730,10 +731,7 @@ public static Path tryRelativize(Path from, Path to) { * The TypeScript parser wrapper then overrides module resolution so packages can be found * under the virtual source root and via that package location mapping. */ - protected DependencyInstallationResult installDependencies(Set filesToExtract) { - if (!verifyYarnInstallation()) { - return DependencyInstallationResult.empty; - } +protected DependencyInstallationResult preparePackagesAndDependencies(Set filesToExtract) { final Path sourceRoot = LGTM_SRC; final Path virtualSourceRoot = toRealPath(Paths.get(EnvironmentVariables.getScratchDir())); @@ -836,29 +834,31 @@ protected DependencyInstallationResult installDependencies(Set filesToExtr } // Install dependencies - for (Path file : packageJsonFiles.keySet()) { - Path virtualFile = virtualSourceRoot.resolve(sourceRoot.relativize(file)); - System.out.println("Installing dependencies from " + virtualFile); - ProcessBuilder pb = - new ProcessBuilder( - Arrays.asList( - "yarn", - "install", - "--non-interactive", - "--ignore-scripts", - "--ignore-platform", - "--ignore-engines", - "--ignore-optional", - "--no-default-rc", - "--no-bin-links", - "--pure-lockfile")); - pb.directory(virtualFile.getParent().toFile()); - pb.redirectOutput(Redirect.INHERIT); - pb.redirectError(Redirect.INHERIT); - try { - pb.start().waitFor(this.installDependenciesTimeout, TimeUnit.MILLISECONDS); - } catch (IOException | InterruptedException ex) { - throw new ResourceError("Could not install dependencies from " + file, ex); + if (this.installDependencies && verifyYarnInstallation()) { + for (Path file : packageJsonFiles.keySet()) { + Path virtualFile = virtualSourceRoot.resolve(sourceRoot.relativize(file)); + System.out.println("Installing dependencies from " + virtualFile); + ProcessBuilder pb = + new ProcessBuilder( + Arrays.asList( + "yarn", + "install", + "--non-interactive", + "--ignore-scripts", + "--ignore-platform", + "--ignore-engines", + "--ignore-optional", + "--no-default-rc", + "--no-bin-links", + "--pure-lockfile")); + pb.directory(virtualFile.getParent().toFile()); + pb.redirectOutput(Redirect.INHERIT); + pb.redirectError(Redirect.INHERIT); + try { + pb.start().waitFor(this.installDependenciesTimeout, TimeUnit.MILLISECONDS); + } catch (IOException | InterruptedException ex) { + throw new ResourceError("Could not install dependencies from " + file, ex); + } } } diff --git a/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java b/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java index 99918cde52a9..34af4daec71e 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java +++ b/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java @@ -131,8 +131,8 @@ public void extractTypeScriptFiles( } @Override - protected DependencyInstallationResult installDependencies(Set filesToExtract) { - // never install dependencies during testing + protected DependencyInstallationResult preparePackagesAndDependencies(Set filesToExtract) { + // currently disabled in tests return DependencyInstallationResult.empty; } From e28284bd01298a83ec4bf46a4dda73c224a9af05 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 17 Jun 2020 10:19:47 +0100 Subject: [PATCH 12/12] JS: Fix javadoc --- .../com/semmle/js/extractor/DependencyInstallationResult.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java b/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java index 10fc1b616c60..5e432e4a40a1 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java +++ b/javascript/extractor/src/com/semmle/js/extractor/DependencyInstallationResult.java @@ -28,7 +28,7 @@ public DependencyInstallationResult( /** * Returns the source root mirrored by {@link #getVirtualSourceRoot()} or null * if no virtual source root exists. - *

+ *

* When invoked from the AutoBuilder, this corresponds to the source root. When invoked * from ODASA, there is no notion of source root, so this is always null in that context. */ @@ -38,7 +38,7 @@ public Path getSourceRoot() { /** * Returns the virtual source root or null if no virtual source root exists. - * + *

* The virtual source root is a directory hierarchy that mirrors the real source * root, where dependencies are installed. */