Fix TSC diagnostic file parsing (handle parsing diagnostics with multiple blocks)

This patch fixes parts of the .dia file reading. As originally
implemented, the reader would fail and emit an error if a diagnostic had
sub-diagnostics, i.e, a warning with notes.

Part of the issue was that the parser could only handle one block at a
time and couldn't handle the nesting of blocks. After fixing this, it
became apparent that there were some other issues. The parser was
consuming the records in a post-order traversal of the diagnostic graph,
when it really needs to be handling them in a pre-order ordering. This
wasn't an issue as a post-order and pre-ordering are the same if you can
only handle trees with a root node. Emitted in the wrong order, the
filemap, flagmap, and categorymap wouldn't be updated correctly,
resulting on dropping diagnostic information on the floor, when really
the information would come later.

In the end, I recorded the diagnostic records in the pre-order traversal
order into the `diagnosticRecords` property. When exiting from the last
active blockID in a diagnostic tree, I would run over the lists of
records, emitting a diagnostic for each. This way we emit diagnostics in
a pre-order ordering once we have collected all of the information for
the diagnostics in the tree.

Now, there's even more fun to be had. When constructing the diagnostic,
the code made an assumption that filename records, category records, and
flag records would come strictly before the diagnosticInfo, source
range, and fixIt records. This isn't the case. To work around that, I process
filenames, categories, and flags, while visiting the record tree to fully
populate the respective maps. I'm doing it this way because it means that
we don't have to iterate over the records twice in the `Diagnostic`
initializer, and don't need to mutate the maps either. So now we emit full
diagnostics, even when the tagged records come in different orders.

Author: etcwilde

Sources/TSCUtility/SerializedDiagnostics.swift

@@ -80,9 +80,9 @@ extension SerializedDiagnostics {
     public var fixIts: [FixIt]
 
     fileprivate init(records: [SerializedDiagnostics.OwnedRecord],
-                     filenameMap: inout [UInt64: String],
-                     flagMap: inout [UInt64: String],
-                     categoryMap: inout [UInt64: String]) throws {
+                     filenameMap: [UInt64: String],
+                     flagMap: [UInt64: String],
+                     categoryMap: [UInt64: String]) throws {
       var text: String? = nil
       var level: Level? = nil
       var location: SourceLocation? = nil
@@ -93,6 +93,7 @@ extension SerializedDiagnostics {
 
       for record in records {
         switch SerializedDiagnostics.RecordID(rawValue: record.id) {
+        case .flag, .category, .filename: continue
         case .diagnosticInfo:
           guard record.fields.count == 8,
                 case .blob(let diagnosticBlob) = record.payload
@@ -114,34 +115,6 @@ extension SerializedDiagnostics {
                                       filenameMap: filenameMap) {
               ranges.append((start, end))
           }
-        case .flag:
-          guard record.fields.count == 2,
-                case .blob(let flagBlob) = record.payload
-          else { throw Error.malformedRecord }
-
-          let flagText = String(decoding: flagBlob, as: UTF8.self)
-          let diagnosticID = record.fields[0]
-          flagMap[diagnosticID] = flagText
-
-        case .category:
-          guard record.fields.count == 2,
-                case .blob(let categoryBlob) = record.payload
-          else { throw Error.malformedRecord }
-
-          let categoryText = String(decoding: categoryBlob, as: UTF8.self)
-          let categoryID = record.fields[0]
-          categoryMap[categoryID] = categoryText
-
-        case .filename:
-          guard record.fields.count == 4,
-                case .blob(let filenameBlob) = record.payload
-          else { throw Error.malformedRecord }
-
-          let filenameText = String(decoding: filenameBlob, as: UTF8.self)
-          let filenameID = record.fields[0]
-          // record.fields[1] and record.fields[2] are no longer used.
-          filenameMap[filenameID] = filenameText
-
         case .fixit:
           guard record.fields.count == 9,
                 case .blob(let fixItBlob) = record.payload
@@ -206,48 +179,78 @@ extension SerializedDiagnostics {
 
 extension SerializedDiagnostics {
   private struct Reader: BitstreamVisitor {
-    var currentBlockID: BlockID? = nil
+    var diagnosticRecords: [[OwnedRecord]] = []
+    var activeBlocks: [BlockID] = []
+    var currentBlockID: BlockID? { activeBlocks.last }
 
     var diagnostics: [Diagnostic] = []
     var versionNumber: Int? = nil
     var filenameMap = [UInt64: String]()
     var flagMap = [UInt64: String]()
     var categoryMap = [UInt64: String]()
 
-    var currentDiagnosticRecords: [OwnedRecord] = []
-
     func validate(signature: Bitcode.Signature) throws {
       guard signature == .init(string: "DIAG") else { throw Error.badMagic }
     }
 
     mutating func shouldEnterBlock(id: UInt64) throws -> Bool {
       guard let blockID = BlockID(rawValue: id) else { throw Error.unknownBlock }
-      guard currentBlockID == nil else { throw Error.unexpectedSubblock }
-      currentBlockID = blockID
+      activeBlocks.append(blockID)
+      if currentBlockID == .diagnostic {
+        diagnosticRecords.append([])
+      }
       return true
     }
 
     mutating func didExitBlock() throws {
-      if currentBlockID == .diagnostic {
-        diagnostics.append(try Diagnostic(records: currentDiagnosticRecords,
-                                          filenameMap: &filenameMap,
-                                          flagMap: &flagMap,
-                                          categoryMap: &categoryMap))
-        currentDiagnosticRecords = []
+      activeBlocks.removeLast()
+      if activeBlocks.isEmpty {
+        for records in diagnosticRecords where !records.isEmpty {
+          diagnostics.append(try Diagnostic(records: records,
+                                            filenameMap: filenameMap,
+                                            flagMap: flagMap,
+                                            categoryMap: categoryMap))
+        }
+        diagnosticRecords = []
       }
-      currentBlockID = nil
     }
 
     mutating func visit(record: BitcodeElement.Record) throws {
       switch currentBlockID {
       case .metadata:
         guard record.id == RecordID.version.rawValue,
-              record.fields.count == 1 else {
-          throw Error.malformedRecord
-        }
+              record.fields.count == 1 else { throw Error.malformedRecord }
         versionNumber = Int(record.fields[0])
       case .diagnostic:
-        currentDiagnosticRecords.append(SerializedDiagnostics.OwnedRecord(record))
+        switch SerializedDiagnostics.RecordID(rawValue: record.id) {
+        case .filename:
+          guard record.fields.count == 4,
+                case .blob(let filenameBlob) = record.payload
+          else { throw Error.malformedRecord }
+
+          let filenameText = String(decoding: filenameBlob, as: UTF8.self)
+          let filenameID = record.fields[0]
+          // record.fields[1] and record.fields[2] are no longer used.
+          filenameMap[filenameID] = filenameText
+        case .category:
+          guard record.fields.count == 2,
+                case .blob(let categoryBlob) = record.payload
+          else { throw Error.malformedRecord }
+
+          let categoryText = String(decoding: categoryBlob, as: UTF8.self)
+          let categoryID = record.fields[0]
+          categoryMap[categoryID] = categoryText
+        case .flag:
+          guard record.fields.count == 2,
+                case .blob(let flagBlob) = record.payload
+          else { throw Error.malformedRecord }
+
+          let flagText = String(decoding: flagBlob, as: UTF8.self)
+          let diagnosticID = record.fields[0]
+          flagMap[diagnosticID] = flagText
+        default:
+          diagnosticRecords[diagnosticRecords.count - 1].append(SerializedDiagnostics.OwnedRecord(record))
+        }
       case nil:
         throw Error.unexpectedTopLevelRecord
       }

Tests/TSCUtilityTests/Inputs/multiblock.dia

@@ -14,6 +14,88 @@ import TSCBasic
 import TSCUtility
 
 final class SerializedDiagnosticsTests: XCTestCase {
+  func testReadSwiftDiagnosticWithNote() throws {
+    let serializedDiagnosticsPath = AbsolutePath(#file).parentDirectory
+      .appending(components: "Inputs", "multiblock.dia")
+    let contents = try localFileSystem.readFileContents(serializedDiagnosticsPath)
+    let serializedDiags = try SerializedDiagnostics(bytes: contents)
+
+    XCTAssertEqual(serializedDiags.versionNumber, 2)
+    XCTAssertEqual(serializedDiags.diagnostics.count, 7)
+
+    struct TestSrcLoc: Equatable {
+        var filename: String
+        var line: UInt64
+        var column: UInt64
+        var offset: UInt64
+
+        init(filename: String, line: UInt64, column: UInt64, offset: UInt64) {
+            self.filename = filename
+            self.line = line
+            self.column = column
+            self.offset = offset
+        }
+
+        init(_ original: SerializedDiagnostics.SourceLocation) {
+            self.filename = original.filename
+            self.line = original.line
+            self.column = original.column
+            self.offset = original.offset
+        }
+    }
+
+    struct TestDiag {
+        var text: String
+        var level: SerializedDiagnostics.Diagnostic.Level
+        var location: TestSrcLoc?
+        var category: String?
+        var flag: String?
+
+    }
+
+    let expectedResults = [
+        TestDiag(text: "type 'A' does not conform to protocol 'P'",
+                 level: .error,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 5, column: 7, offset: 35)),
+        TestDiag(text: "candidate is 'async', but protocol requirement is not",
+                 level: .note,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 6, column: 10, offset: 51)),
+        TestDiag(text: "do you want to add protocol stubs?",
+                 level: .note,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 5, column: 7, offset: 35)),
+        TestDiag(text: "initialization of immutable value 'a' was never used",
+                 level: .warning,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 7, column: 13, offset: 75)),
+        TestDiag(text: "consider replacing with '_' or removing it",
+                 level: .note,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 7, column: 13, offset: 75)),
+        TestDiag(text: "initialization of immutable value 'b' was never used",
+                 level: .warning,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 8, column: 13, offset: 94)),
+        TestDiag(text: "consider replacing with '_' or removing it",
+                 level: .note,
+                 location: TestSrcLoc(filename: "test.swift",
+                                      line: 8, column: 13, offset: 94))
+    ]
+
+    for case let (diag, expected) in zip(serializedDiags.diagnostics,
+        expectedResults) {
+        XCTAssertEqual(diag.text, expected.text, "Mismatched Diagnostic Text")
+        XCTAssertEqual(diag.level, expected.level, "Mismatched Diagnostic Level")
+
+        XCTAssertEqual((diag.location == nil), (expected.location == nil), "Unexpected Diagnostic Location")
+        if let diagLoc = diag.location, let expectedLoc = expected.location {
+            XCTAssertEqual(TestSrcLoc(diagLoc), expectedLoc, "Mismatched Diagnostic Location")
+        }
+    }
+  }
+
   func testReadSwiftSerializedDiags() throws {
     let serializedDiagnosticsPath = AbsolutePath(#file).parentDirectory
         .appending(components: "Inputs", "serialized.dia")