Merge pull request #519 from sauyon/fix-consistency

Extract files for error locations

extractor-smoke-test/expected.csv

@@ -1,12 +1,12 @@
-"nd","col1"
+"1","assignment to i"
+"assignment to i","selection of Println"
+"call to Println","exit"
 "entry","skip"
 "entry","skip"
 "function declaration","exit"
-"assignment to i","selection of Println"
-"skip","skip"
-"skip","function declaration"
-"skip","1"
-"1","assignment to i"
-"call to Println","exit"
-"selection of Println","i"
 "i","call to Println"
+"nd","col1"
+"selection of Println","i"
+"skip","1"
+"skip","function declaration"
+"skip","skip"

extractor-smoke-test/test.sh

@@ -8,9 +8,10 @@ cd $DIR
 rm -rf testdb
 
 codeql database create --language=go testdb --search-path ..
+codeql dataset check testdb/db-go
 codeql query run ../ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.ql --database=testdb --output=notracing-out.bqrs --search-path ..
 codeql bqrs decode notracing-out.bqrs --format=csv --output=notracing-out.csv
-diff -w -u notracing-out.csv expected.csv
+diff -w -u <(sort notracing-out.csv) expected.csv
 
 # Now do it again with tracing enabled
 
@@ -19,6 +20,7 @@ export CODEQL_EXTRACTOR_GO_BUILD_TRACING=on
 rm -rf testdb
 
 codeql database create --language=go testdb --search-path ..
+codeql dataset check testdb/db-go
 codeql query run ../ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.ql --database=testdb --output=tracing-out.bqrs --search-path ..
 codeql bqrs decode tracing-out.bqrs --format=csv --output=tracing-out.csv
-diff -w -u tracing-out.csv expected.csv
+diff -w -u <(sort tracing-out.csv) expected.csv

extractor/extractor.go

@@ -236,6 +236,11 @@ type FileInfo struct {
 	NextErr int
 }
 
+func (extraction *Extraction) SeenFile(path string) bool {
+	_, ok := extraction.FileInfo[path]
+	return ok
+}
+
 func (extraction *Extraction) GetFileInfo(path string) *FileInfo {
 	if fileInfo, ok := extraction.FileInfo[path]; ok {
 		return fileInfo
@@ -487,13 +492,15 @@ func (extraction *Extraction) extractError(tw *trap.Writer, err packages.Error,
 	}
 	transformed := filepath.ToSlash(srcarchive.TransformPath(ffile))
 
+	extraction.extractFileInfo(tw, ffile)
+
 	extraction.Lock.Lock()
+
 	diagLbl := extraction.StatWriter.Labeler.FreshID()
-	dbscheme.DiagnosticsTable.Emit(extraction.StatWriter, diagLbl, 1, tag, err.Msg, err.Msg,
-		emitLocation(
-			extraction.StatWriter, extraction.StatWriter.Labeler.GlobalID(ffile+";sourcefile"),
-			line, col, line, col,
-		))
+	flbl := extraction.StatWriter.Labeler.FileLabelFor(ffile)
+	dbscheme.DiagnosticsTable.Emit(
+		extraction.StatWriter, diagLbl, 1, tag, err.Msg, err.Msg,
+		emitLocation(extraction.StatWriter, flbl, line, col, line, col))
 	dbscheme.DiagnosticForTable.Emit(extraction.StatWriter, diagLbl, extraction.Label, extraction.GetFileIdx(transformed), extraction.GetNextErr(transformed))
 	extraction.Lock.Unlock()
 	dbscheme.ErrorsTable.Emit(tw, lbl, kind, err.Msg, pos, transformed, line, col, pkglbl, idx)
@@ -585,6 +592,16 @@ func (extraction *Extraction) extractFileInfo(tw *trap.Writer, file string) {
 	components := strings.Split(path, "/")
 	parentPath := ""
 	var parentLbl trap.Label
+
+	// We may visit the same file twice because `extractError` calls this function to describe files containing
+	// compilation errors. It is also called for user source files being extracted.
+	extraction.Lock.Lock()
+	if extraction.SeenFile(path) {
+		extraction.Lock.Unlock()
+		return
+	}
+	extraction.Lock.Unlock()
+
 	for i, component := range components {
 		if i == 0 {
 			if component == "" {
@@ -597,12 +614,13 @@ func (extraction *Extraction) extractFileInfo(tw *trap.Writer, file string) {
 		}
 		if i == len(components)-1 {
 			stem, ext := stemAndExt(component)
-			lbl := tw.Labeler.FileLabel()
+			lbl := tw.Labeler.FileLabelFor(file)
 			dbscheme.FilesTable.Emit(tw, lbl, path, stem, ext, 0)
 			dbscheme.ContainerParentTable.Emit(tw, parentLbl, lbl)
-			extractLocation(tw, lbl, 0, 0, 0, 0)
+			dbscheme.HasLocationTable.Emit(tw, lbl, emitLocation(tw, lbl, 0, 0, 0, 0))
 			extraction.Lock.Lock()
-			dbscheme.CompilationCompilingFilesTable.Emit(extraction.StatWriter, extraction.Label, extraction.GetFileIdx(path), extraction.StatWriter.Labeler.FileLabelFor(tw))
+			slbl := extraction.StatWriter.Labeler.FileLabelFor(file)
+			dbscheme.CompilationCompilingFilesTable.Emit(extraction.StatWriter, extraction.Label, extraction.GetFileIdx(path), slbl)
 			extraction.Lock.Unlock()
 			break
 		}

extractor/trap/labels.go

@@ -62,7 +62,7 @@ func (l *Labeler) GlobalID(key string) Label {
 	return label
 }
 
-// FileLabel returns the label for the file with which the trap writer is associated
+// FileLabel returns the label for a file with path `path`.
 func (l *Labeler) FileLabel() Label {
 	if l.fileLabel == InvalidLabel {
 		l.fileLabel = l.GlobalID(l.tw.path + ";sourcefile")
@@ -71,8 +71,8 @@ func (l *Labeler) FileLabel() Label {
 }
 
 // FileLabelFor returns the label for the file for which the trap writer `tw` is associated
-func (l *Labeler) FileLabelFor(tw *Writer) Label {
-	return l.GlobalID(tw.path + ";sourcefile")
+func (l *Labeler) FileLabelFor(path string) Label {
+	return l.GlobalID(path + ";sourcefile")
 }
 
 // LocalID associates a label with the given AST node `nd` and returns it

ql/src/semmle/go/Files.qll

@@ -180,9 +180,22 @@ class Folder extends Container, @folder {
 /** A file. */
 class File extends Container, @file, Documentable, ExprParent, GoModExprParent, DeclParent,
   ScopeNode {
+  string path;
+
+  File() {
+    files(this, path, _, _, _) and
+    (
+      // Exclude `.go` files that have not been extracted. Non-extracted files only exist in the `files`
+      // table if we are reporting compilation errors relating to them in the `diagnostics` table.
+      not path.matches("%.go")
+      or
+      exists(this.getAChild())
+    )
+  }
+
   override Location getLocation() { has_location(this, result) }
 
-  override string getAbsolutePath() { files(this, result, _, _, _) }
+  override string getAbsolutePath() { result = path }
 
   /** Gets the number of lines in this file. */
   int getNumberOfLines() { numlines(this, result, _, _) }

ql/test/library-tests/semmle/go/Files/CONSISTENCY/UnexpectedFrontendErrors.expected

@@ -0,0 +1 @@
+| vendor/github.com/github/nonexistent/bad.go:1:57:1:57 | expected 'package', found 'EOF' |

ql/test/library-tests/semmle/go/Files/a.go

@@ -0,0 +1,7 @@
+package main
+
+import "github.com/github/nonexistent"
+
+func main() {
+	nonexistent.A()
+}

ql/test/library-tests/semmle/go/Files/b.go

@@ -0,0 +1 @@
+package main

ql/test/library-tests/semmle/go/Files/blah.html

@@ -0,0 +1,4 @@
+<head>
+</head>
+<body>
+</body>

ql/test/library-tests/semmle/go/Files/files.expected

@@ -0,0 +1,4 @@
+| a.go:0:0:0:0 | a.go |
+| b.go:0:0:0:0 | b.go |
+| blah.html:0:0:0:0 | blah.html |
+| go.mod:0:0:0:0 | go.mod |

ql/test/library-tests/semmle/go/Files/files.ql

@@ -0,0 +1,3 @@
+import go
+
+select any(File f)

ql/test/library-tests/semmle/go/Files/go.mod

@@ -0,0 +1,5 @@
+module codeql-go-tests/files
+
+go 1.16
+
+require github.com/github/nonexistent v0.0.0

ql/test/library-tests/semmle/go/Files/vendor/github.com/github/nonexistent/bad.go

@@ -0,0 +1 @@
+// autoformat-ignore (gofmt chokes on invalid programs)

ql/test/library-tests/semmle/go/Files/vendor/github.com/github/nonexistent/stub.go

@@ -0,0 +1,3 @@
+package nonexistent
+
+func A() {}

ql/test/library-tests/semmle/go/Files/vendor/modules.txt

@@ -0,0 +1,2 @@
+# github.com/github/nonexistent v0.0.0
+## explicit