Merge pull request swiftlang#36091 from CodaFi/extensionating-circumstances

[5.4] Correct an Incremental Miscompile For Edits to Overloaded Extension Members

Author: CodaFi

include/swift/AST/DeclContext.h

@@ -881,6 +881,8 @@ class IterableDeclContext {
   /// available.
   Optional<Fingerprint> getBodyFingerprint() const;
 
+  bool areTokensHashedForThisBodyInsteadOfInterfaceHash() const;
+
 private:
   /// Add a member to the list for iteration purposes, but do not notify the
   /// subclass that we have done so.

lib/AST/DeclContext.cpp

@@ -1032,6 +1032,16 @@ Optional<Fingerprint> IterableDeclContext::getBodyFingerprint() const {
       .fingerprint;
 }
 
+bool IterableDeclContext::areTokensHashedForThisBodyInsteadOfInterfaceHash()
+    const {
+  // Do not keep separate hashes for extension bodies because the dependencies
+  // can miss the addition of a member in an extension because there is nothing
+  // corresponding to the fingerprinted nominal dependency node.
+  if (isa<ExtensionDecl>(this))
+    return false;
+  return true;
+}
+
 /// Return the DeclContext to compare when checking private access in
 /// Swift 4 mode. The context returned is the type declaration if the context
 /// and the type declaration are in the same file, otherwise it is the types

lib/Parse/ParseDecl.cpp

@@ -4751,9 +4751,12 @@ Parser::parseDeclList(SourceLoc LBLoc, SourceLoc &RBLoc, Diag<> ErrorDiag,
 
   // If we're hashing the type body separately, record the curly braces but
   // nothing inside for the interface hash.
-  llvm::SaveAndRestore<Optional<llvm::MD5>> MemberHashingScope{CurrentTokenHash, llvm::MD5()};
-  recordTokenHash("{");
-  recordTokenHash("}");
+  Optional<llvm::SaveAndRestore<Optional<llvm::MD5>>> MemberHashingScope;
+  if (IDC->areTokensHashedForThisBodyInsteadOfInterfaceHash()) {
+    recordTokenHash("{");
+    recordTokenHash("}");
+    MemberHashingScope.emplace(CurrentTokenHash, llvm::MD5());
+  }
 
   std::vector<Decl *> decls;
   ParserStatus Status;
@@ -4785,7 +4788,8 @@ Parser::parseDeclList(SourceLoc LBLoc, SourceLoc &RBLoc, Diag<> ErrorDiag,
     hadError = true;
 
   llvm::MD5::MD5Result result;
-  CurrentTokenHash->final(result);
+  auto declListHash = MemberHashingScope ? *CurrentTokenHash : llvm::MD5();
+  declListHash.final(result);
   return std::make_pair(decls, Fingerprint{std::move(result)});
 }
 

test/Incremental/Fingerprints/Inputs/extension-adds-member/definesAB-after.swift

@@ -3,7 +3,7 @@ struct A {
 struct B {
 }
 extension A {
-  init(_ x: String = "") {}
+  var x: Int {17}
 }
 extension B {
 }

test/Incremental/Fingerprints/Inputs/extension-changes-member/definesS-after.swift

@@ -0,0 +1,8 @@
+public struct S {
+  private // commenting out this line works
+  static func foo(_ i: Int) {print("1: other:2 commented out")}
+}
+extension S {
+  // private // commenting out this line fails
+  static func foo2(_ i: Int) {print("2: other:6 commented out")}
+}

test/Incremental/Fingerprints/Inputs/extension-changes-member/definesS-before.swift

@@ -0,0 +1,8 @@
+public struct S {
+  private // commenting out this line works
+  static func foo(_ i: Int) {print("1: other:2 commented out")}
+}
+extension S {
+  private // commenting out this line fails
+  static func foo2(_ i: Int) {print("2: other:6 commented out")}
+}

test/Incremental/Fingerprints/Inputs/extension-changes-member/main.swift

@@ -0,0 +1,11 @@
+extension S {
+  static func foo<I: SignedInteger>(_ si: I) {
+    print("1: other:2 not commented out")
+  }
+  static func foo2<I: SignedInteger>(_ si: I) {
+    print("2: other:6 not commented out")
+  }
+}
+
+S.foo(3)
+S.foo2(3)

test/Incremental/Fingerprints/Inputs/extension-changes-member/ofm.json

@@ -0,0 +1,14 @@
+{
+  "main.swift": {
+    "object": "./main.o",
+    "swift-dependencies": "./main.swiftdeps"
+  },
+  "definesS.swift": {
+    "object": "./definesS.o",
+    "swift-dependencies": "./definesS.swiftdeps"
+  },
+  "": {
+    "swift-dependencies": "./main~buildrecord.swiftdeps"
+  }
+}
+

test/Incremental/Fingerprints/extension-changes-member.swift

@@ -0,0 +1,32 @@
+// Test per-type-body fingerprints using simple extensions
+//
+// If the parser is allowed to use a body fingerprint for an extension
+// this test will fail because usesA.swift won't be recompiled for the
+// last step.
+
+// Establish status quo
+
+// RUN: %empty-directory(%t)
+// RUN: cp %S/Inputs/extension-changes-member/* %t
+// RUN: cp %t/definesS{-before,}.swift
+
+// Seeing weird failure on CI, so set the mod times
+// RUN: touch -t 200101010101 %t/*.swift
+
+// RUN: cd %t && %target-swiftc_driver  -enable-batch-mode -j2 -incremental -driver-show-incremental main.swift definesS.swift -module-name main -output-file-map ofm.json  >& %t/output3
+
+// Change one type, only uses of that type get recompiled
+
+// RUN: cp %t/definesS{-after,}.swift
+
+// Seeing weird failure on CI, so ensure that definesS.swift is newer
+// RUN: touch -t 200201010101 %t/*
+// RUN: touch -t 200101010101 %t/*.swift
+// RUN: touch -t 200301010101 %t/definesS.swift
+
+// RUN: cd %t && %target-swiftc_driver  -enable-batch-mode -j2 -incremental -driver-show-incremental main.swift definesS.swift -module-name main -output-file-map ofm.json  >& %t/output4
+
+// RUN: %FileCheck -check-prefix=CHECK-RECOMPILED-W %s < %t/output4
+
+// CHECK-RECOMPILED-W: {compile: definesS.o <= definesS.swift}
+// CHECK-RECOMPILED-W: {compile: main.o <= main.swift}