Merge pull request #486 from davidungar/holder-enforcer

[Incremental] Centralize fingerprint/swiftmodule invariant, catch bad paths earlier

Author: CodaFi

Sources/SwiftDriver/CMakeLists.txt

@@ -43,9 +43,10 @@ add_library(SwiftDriver
   "IncrementalCompilation/BuildRecordInfo.swift"
   "IncrementalCompilation/DependencyKey.swift"
   "IncrementalCompilation/DictionaryOfDictionaries.swift"
+  "IncrementalCompilation/ExternalDependencyAndFingerprintEnforcer.swift"
   "IncrementalCompilation/IncrementalCompilationState.swift"
   "IncrementalCompilation/InputInfo.swift"
-  "IncrementalCompilation/KeyAndFingerprintEnforcer.swift"
+  "IncrementalCompilation/KeyAndFingerprintHolder.swift"
   "IncrementalCompilation/ModuleDependencyGraph.swift"
   "IncrementalCompilation/Multidictionary.swift"
   "IncrementalCompilation/SourceFileDependencyGraph.swift"

Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift

@@ -1,39 +1,47 @@
 
 import Foundation
+import TSCBasic
 
 /// A filename from another module
 /*@_spi(Testing)*/ public struct ExternalDependency: Hashable, Comparable, CustomStringConvertible {
-  let fileName: String
-  let fingerprint: String?
+  let file: VirtualPath
 
-  /*@_spi(Testing)*/ public init(_ fileName: String, fingerprint: String? = nil) {
-    self.fileName = fileName
-    self.fingerprint = fingerprint
+  /// Cache this
+  let isSwiftModule: Bool
+
+  /*@_spi(Testing)*/ public init(_ fileName: String)
+  throws {
+    self.file = try VirtualPath(path: fileName)
+    self.isSwiftModule = file.extension == FileType.swiftModule.rawValue
   }
 
-  var file: VirtualPath? {
-    try? VirtualPath(path: fileName)
+  func modTime(_ fileSystem: FileSystem) -> Date? {
+    try? fileSystem.getFileInfo(file).modTime
   }
 
   public var description: String {
-    fileName.description
+    file.name
   }
 
   public static func < (lhs: Self, rhs: Self) -> Bool {
-    lhs.fileName < rhs.fileName
+    lhs.file.name < rhs.file.name
   }
 }
 
 /// Since the integration surfaces all externalDependencies to be processed later,
 /// a combination of the dependency and fingerprint are needed.
-public struct FingerprintedExternalDependency: Hashable, Equatable {
+public struct FingerprintedExternalDependency: Hashable, Equatable, ExternalDependencyAndFingerprintEnforcer {
   let externalDependency: ExternalDependency
   let fingerprint: String?
   init(_ externalDependency: ExternalDependency, _ fingerprint: String?) {
     self.externalDependency = externalDependency
     self.fingerprint = fingerprint
+    assert(verifyExternalDependencyAndFingerprint())
+  }
+  var externalDependencyToCheck: ExternalDependency? { externalDependency }
+  var isIncremental: Bool {
+    fingerprint != nil && externalDependency.isSwiftModule
   }
-  var isIncremental: Bool { fingerprint != nil }
 }
 
 /// A `DependencyKey` carries all of the information necessary to uniquely
@@ -184,7 +192,7 @@ public struct DependencyKey: Hashable, CustomStringConvertible {
       case let .dynamicLookup(name: name):
         return "AnyObject member '\(name)'"
       case let .externalDepend(externalDependency):
-        return "module '\(externalDependency)'"
+        return "import '\(externalDependency)'"
       case let .sourceFileProvide(name: name):
         return "source file \((try? VirtualPath(path: name).basename) ?? name)"
       }

Sources/SwiftDriver/IncrementalCompilation/ExternalDependencyAndFingerprintEnforcer.swift

@@ -0,0 +1,29 @@
+//===------- FingerprintedExternalHolder.swift ----------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2020 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+/// Central place to check an invariant: If an externalDependency has a fingerprint, it should point
+/// to a swiftmodule file that contains dependency information.
+
+protocol ExternalDependencyAndFingerprintEnforcer {
+  var externalDependencyToCheck: ExternalDependency? {get}
+  var fingerprint: String? {get}
+}
+extension ExternalDependencyAndFingerprintEnforcer {
+  func verifyExternalDependencyAndFingerprint() -> Bool {
+    if let fingerprint = self.fingerprint,
+       let externalDependency = externalDependencyToCheck,
+       !externalDependency.isSwiftModule {
+      fatalError("An external dependency with a fingerprint (\(fingerprint)) must point to a swiftmodule file: \(externalDependency)")
+    }
+    return true
+  }
+}

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift

@@ -564,15 +564,14 @@ extension IncrementalCompilationState {
     var externalDependencySources = Set<DependencySource>()
     for extDepAndPrint in moduleDependencyGraph.fingerprintedExternalDependencies {
       let extDep = extDepAndPrint.externalDependency
-      let extModTime = extDep.file.flatMap {try? fileSystem.getFileInfo($0).modTime}
-        ?? Date.distantFuture
+      let extModTime = extDep.modTime(fileSystem) ?? Date.distantFuture
       if extModTime >= buildTime {
         for dependent in moduleDependencyGraph.untracedDependents(of: extDepAndPrint) {
           guard let dependencySource = dependent.dependencySource else {
             fatalError("Dependent \(dependent) does not have dependencies file!")
           }
           reporter?.report(
-            "Queuing because of external dependency on newer \(extDep.file?.basename ?? "extDep?")",
+            "Queuing because of external dependency on newer \(extDep.file.basename)",
             dependencySource.typedFile)
           externalDependencySources.insert(dependencySource)
         }

Sources/SwiftDriver/IncrementalCompilation/KeyAndFingerprintEnforcer.swift

@@ -0,0 +1,53 @@
+//===--------- KeyAndFingerprintHolder.swift ------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2021 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import Foundation
+
+
+/// Encapsulates the invariant required for anything with a DependencyKey and an fingerprint
+public struct KeyAndFingerprintHolder: ExternalDependencyAndFingerprintEnforcer {
+  /// Def->use arcs go by DependencyKey. There may be >1 node for a given key.
+  let key: DependencyKey
+
+  /// The frontend records in the fingerprint, all of the information about an
+  /// entity, such that any uses need be rebuilt only if the fingerprint
+  /// changes.
+  /// When the driver reloads a dependency graph (after a frontend job has run),
+  /// it can use the fingerprint to determine if the entity has changed and thus
+  /// if uses need to be recompiled.
+  ///
+  /// However, at present, the frontend does not record this information for
+  /// every Decl; it only records it for the source-file-as-a-whole in the
+  /// interface hash. The inteface hash is a product of all the tokens that are
+  /// not inside of function bodies. Thus, if there is no fingerprint, when the
+  /// frontend creates an interface node,
+  /// it adds a dependency to it from the implementation source file node (which
+  /// has the intefaceHash as its fingerprint).
+  let fingerprint: String?
+
+  init(_ key: DependencyKey, _ fingerprint: String?) throws {
+    self.key = key
+    self.fingerprint = fingerprint
+    assert(verifyKeyAndFingerprint())
+  }
+  var externalDependencyToCheck: ExternalDependency? {
+    key.designator.externalDependency
+  }
+  private func verifyKeyAndFingerprint() -> Bool {
+    assert(verifyExternalDependencyAndFingerprint())
+
+    if let externalDependency = externalDependencyToCheck, key.aspect != .interface {
+        fatalError("Aspect of external dependency must be interface: \(externalDependency)")
+    }
+    return true
+  }
+}

Sources/SwiftDriver/IncrementalCompilation/KeyAndFingerprintHolder.swift

@@ -213,7 +213,7 @@ extension ModuleDependencyGraph {
     guard externalDependency.isIncremental else {
       return Integrator.Results()
     }
-    let file = externalDependency.externalDependency.file!
+    let file = externalDependency.externalDependency.file
     let dependencySource = DependencySource(file)
     reporter?.report("integrating incremental external dependency",
                      dependencySource.typedFile)
@@ -596,7 +596,7 @@ extension ModuleDependencyGraph {
           let hasFingerprint = Int(record.fields[1]) != 0
           let fingerprint = hasFingerprint ? fingerprintStr : nil
           self.graph.fingerprintedExternalDependencies.insert(
-            FingerprintedExternalDependency(ExternalDependency(path), fingerprint))
+            FingerprintedExternalDependency(try ExternalDependency(path), fingerprint))
         case .identifierNode:
           guard record.fields.count == 0,
                 case .blob(let identifierBlob) = record.payload,
@@ -768,7 +768,7 @@ extension ModuleDependencyGraph {
       }
 
       for edF in graph.fingerprintedExternalDependencies {
-        self.addIdentifier(edF.externalDependency.fileName)
+        self.addIdentifier(edF.externalDependency.file.name)
       }
 
       for str in self.identifiersToWrite {
@@ -905,7 +905,7 @@ extension ModuleDependencyGraph {
           serializer.stream.writeRecord(serializer.abbreviations[.externalDepNode]!, {
             $0.append(RecordID.externalDepNode)
             $0.append(serializer.lookupIdentifierCode(
-                        for: fingerprintedExternalDependency.externalDependency.fileName))
+                        for: fingerprintedExternalDependency.externalDependency.file.name))
             $0.append((fingerprintedExternalDependency.fingerprint != nil) ? UInt32(1) : UInt32(0))
           }, 
           blob: (fingerprintedExternalDependency.fingerprint ?? ""))
@@ -963,7 +963,7 @@ fileprivate extension DependencyKey.Designator {
       self = .dynamicLookup(name: name)
     case 5:
       try mustBeEmpty(context)
-      self = .externalDepend(ExternalDependency(name))
+      self = .externalDepend(try ExternalDependency(name))
     case 6:
       try mustBeEmpty(context)
       self = .sourceFileProvide(name: name)
@@ -1016,7 +1016,7 @@ fileprivate extension DependencyKey.Designator {
     case .dynamicLookup(name: let name):
       return name
     case .externalDepend(let path):
-      return path.fileName
+      return path.file.name
     case .sourceFileProvide(name: let name):
       return name
     case .member(context: _, name: let name):

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -25,28 +25,16 @@ extension ModuleDependencyGraph {
   /// Use a class, not a struct because otherwise it would be duplicated for each thing it uses
 
   /*@_spi(Testing)*/
-  public final class Node: KeyAndFingerprintEnforcer {
+  public final class Node {
 
     /*@_spi(Testing)*/ public typealias Graph = ModuleDependencyGraph
 
-    /// Def->use arcs go by DependencyKey. There may be >1 node for a given key.
-    let key: DependencyKey
+    /// Hold these where an invariant can be checked.
+    let keyAndFingerprint: KeyAndFingerprintHolder
+
+    var key: DependencyKey { keyAndFingerprint.key }
+    var fingerprint: String? { keyAndFingerprint.fingerprint }
 
-    /// The frontend records in the fingerprint, all of the information about an
-    /// entity, such that any uses need be rebuilt only if the fingerprint
-    /// changes.
-    /// When the driver reloads a dependency graph (after a frontend job has run),
-    /// it can use the fingerprint to determine if the entity has changed and thus
-    /// if uses need to be recompiled.
-    ///
-    /// However, at present, the frontend does not record this information for
-    /// every Decl; it only records it for the source-file-as-a-whole in the
-    /// interface hash. The inteface hash is a product of all the tokens that are
-    /// not inside of function bodies. Thus, if there is no fingerprint, when the
-    /// frontend creates an interface node,
-    /// it adds a dependency to it from the implementation source file node (which
-    /// has the intefaceHash as its fingerprint).
-    let fingerprint: String?
 
 
     /// The dependencySource file that holds this entity iff the entities .swiftdeps (or in future, .swiftmodule) is known.
@@ -62,11 +50,8 @@ extension ModuleDependencyGraph {
     /// SourceFileDependencyGraph or the DependencyKeys
     init(key: DependencyKey, fingerprint: String?,
          dependencySource: DependencySource?) {
-      self.key = key
-      self.fingerprint = fingerprint
+      self.keyAndFingerprint = try! KeyAndFingerprintHolder(key, fingerprint)
       self.dependencySource = dependencySource
-
-      try! verifyKeyAndFingerprint()
     }
   }
 }