GitHub merge Migrator -> swift 4.0 branch

include/swift/Migrator/FixitFilter.h

@@ -77,6 +77,19 @@ struct FixitFilter {
       return false;
     }
 
+    // With SE-110, the migrator may get a recommendation to add a Void
+    // placeholder in the call to f in:
+    // func foo(f: (Void) -> ()) {
+    //   f()
+    // }
+    // Here, f was () -> () in Swift 3 but now (()) -> () in Swift 4. Adding a
+    // type placeholder in the f() call isn't helpful for migration, although
+    // this particular fix-it should be to fix the f parameter's type.
+    if (Info.ID == diag::missing_argument_named.ID ||
+        Info.ID == diag::missing_argument_positional.ID) {
+      return false;
+    }
+
     if (Kind == DiagnosticKind::Error)
       return true;
 

include/swift/Migrator/MigrationState.h

@@ -74,8 +74,8 @@ struct MigrationState : public llvm::ThreadSafeRefCountedBase<MigrationState> {
   /// input file, output file, replacements, syntax trees, etc.
   bool print(size_t StateNumber, StringRef OutDir) const;
 
-  bool outputDiffersFromInput() const {
-    return InputBufferID != OutputBufferID;
+  bool noChangesOccurred() const {
+    return InputBufferID == OutputBufferID;
   }
 
   static RC<MigrationState>

include/swift/Migrator/Migrator.h

@@ -27,17 +27,16 @@ namespace migrator {
 
 /// Run the migrator on the compiler invocation's input file and emit a
 /// "replacement map" describing the requested changes to the source file.
-bool updateCodeAndEmitRemap(CompilerInstance &Instance,
+bool updateCodeAndEmitRemap(CompilerInstance *Instance,
                             const CompilerInvocation &Invocation);
 
-class Migrator {
-  CompilerInstance &StartInstance;
+struct Migrator {
+  CompilerInstance *StartInstance;
   const CompilerInvocation &StartInvocation;
   SourceManager SrcMgr;
   std::vector<RC<MigrationState>> States;
 
-public:
-  Migrator(CompilerInstance &StartInstance,
+  Migrator(CompilerInstance *StartInstance,
            const CompilerInvocation &StartInvocation);
 
   /// The maximum number of times to run the compiler over the input to get
@@ -48,11 +47,21 @@ class Migrator {
   /// Repeatedly perform a number of compiler-fix-it migrations in a row, until
   /// there are no new suggestions from the compiler or some other error
   /// occurred.
-  void repeatFixitMigrations(const unsigned Iterations);
+  ///
+  /// Returns the last CompilerInstance used in the iterations, provided
+  /// that the CompilerInvocation used to set it up was successful. Otherwise,
+  /// returns nullptr.
+  std::unique_ptr<swift::CompilerInstance>
+  repeatFixitMigrations(const unsigned Iterations,
+                        swift::version::Version SwiftLanguageVersion);
 
   /// Perform a single compiler fix-it migration on the last state, and push
   /// the result onto the state history.
-  llvm::Optional<RC<MigrationState>> performAFixItMigration();
+  ///
+  /// Returns the CompilerInstance used for the fix-it run, provided its
+  /// setup from a CompilerInvocation was successful.
+  std::unique_ptr<swift::CompilerInstance>
+  performAFixItMigration(swift::version::Version SwiftLanguageVersion);
 
   /// Starting with the last state, perform the following migration passes.
   ///

lib/FrontendTool/FrontendTool.cpp

@@ -517,9 +517,8 @@ static bool performCompile(std::unique_ptr<CompilerInstance> &Instance,
 
   ASTContext &Context = Instance->getASTContext();
 
-  if (!Context.hadError() &&
-      Invocation.getMigratorOptions().shouldRunMigrator()) {
-    migrator::updateCodeAndEmitRemap(*Instance, Invocation);
+  if (Invocation.getMigratorOptions().shouldRunMigrator()) {
+    migrator::updateCodeAndEmitRemap(Instance.get(), Invocation);
   }
 
   if (Action == FrontendOptions::REPL) {

lib/Migrator/Migrator.cpp

@@ -25,25 +25,41 @@
 using namespace swift;
 using namespace swift::migrator;
 
-bool migrator::updateCodeAndEmitRemap(CompilerInstance &Instance,
+bool migrator::updateCodeAndEmitRemap(CompilerInstance *Instance,
                                       const CompilerInvocation &Invocation) {
   Migrator M { Instance, Invocation }; // Provide inputs and configuration
 
-  // Phase 1:
-  // Perform any syntactic transformations if requested.
+  // Phase 1: Pre Fix-it passes
+  // These uses the initial frontend invocation to apply any obvious fix-its
+  // to see if we can get an error-free AST to get to Phase 2.
+  std::unique_ptr<swift::CompilerInstance> PreFixItInstance;
+  if (Instance->getASTContext().hadError()) {
+    PreFixItInstance = M.repeatFixitMigrations(2,
+      Invocation.getLangOptions().EffectiveLanguageVersion);
+
+    // If we still couldn't fix all of the errors, give up.
+    if (PreFixItInstance->getASTContext().hadError()) {
+      return true;
+    }
+    M.StartInstance = PreFixItInstance.get();
+  }
 
+  // Phase 2: Syntactic Transformations
   auto FailedSyntacticPasses = M.performSyntacticPasses();
   if (FailedSyntacticPasses) {
     return true;
   }
 
-  // Phase 2:
+  // Phase 3: Post Fix-it Passes
   // Perform fix-it based migrations on the compiler, some number of times in
   // order to give the compiler an opportunity to
   // take its time reaching a fixed point.
+  // This is the end of the pipeline, so we throw away the compiler instance(s)
+  // we used in these fix-it runs.
 
   if (M.getMigratorOptions().EnableMigratorFixits) {
-    M.repeatFixitMigrations(Migrator::MaxCompilerFixitPassIterations);
+    M.repeatFixitMigrations(Migrator::MaxCompilerFixitPassIterations,
+                            {4, 0, 0});
   }
 
   // OK, we have a final resulting text. Now we compare against the input
@@ -58,7 +74,7 @@ bool migrator::updateCodeAndEmitRemap(CompilerInstance &Instance,
   return EmitRemapFailed || EmitMigratedFailed || DumpMigrationStatesFailed;
 }
 
-Migrator::Migrator(CompilerInstance &StartInstance,
+Migrator::Migrator(CompilerInstance *StartInstance,
                    const CompilerInvocation &StartInvocation)
   : StartInstance(StartInstance), StartInvocation(StartInvocation) {
 
@@ -68,33 +84,32 @@ Migrator::Migrator(CompilerInstance &StartInstance,
     States.push_back(MigrationState::start(SrcMgr, StartBufferID));
 }
 
-void Migrator::
-repeatFixitMigrations(const unsigned Iterations) {
+std::unique_ptr<swift::CompilerInstance>
+Migrator::repeatFixitMigrations(const unsigned Iterations,
+                                version::Version SwiftLanguageVersion) {
   for (unsigned i = 0; i < Iterations; ++i) {
-    auto ThisResult = performAFixItMigration();
-    if (!ThisResult.hasValue()) {
-      // Something went wrong? Track error in the state?
+    auto ThisInstance = performAFixItMigration(SwiftLanguageVersion);
+    if (ThisInstance == nullptr) {
       break;
     } else {
-      if (ThisResult.getValue()->outputDiffersFromInput()) {
-        States.push_back(ThisResult.getValue());
-      } else {
-        break;
+      if (States.back()->noChangesOccurred()) {
+        return ThisInstance;
       }
     }
   }
+  return nullptr;
 }
 
-llvm::Optional<RC<MigrationState>>
-Migrator::performAFixItMigration() {
+std::unique_ptr<swift::CompilerInstance>
+Migrator::performAFixItMigration(version::Version SwiftLanguageVersion) {
   auto InputState = States.back();
   auto InputBuffer =
     llvm::MemoryBuffer::getMemBufferCopy(InputState->getOutputText(),
                                          getInputFilename());
 
   CompilerInvocation Invocation { StartInvocation };
   Invocation.clearInputs();
-  Invocation.getLangOptions().EffectiveLanguageVersion = { 4, 0, 0 };
+  Invocation.getLangOptions().EffectiveLanguageVersion = SwiftLanguageVersion;
 
   // SE-0160: When migrating, always use the Swift 3 @objc inference rules,
   // which drives warnings with the "@objc" Fix-Its.
@@ -132,18 +147,18 @@ Migrator::performAFixItMigration() {
     PrimaryIndex, SelectedInput::InputKind::Buffer
   };
 
-  CompilerInstance Instance;
-  if (Instance.setup(Invocation)) {
-    return None;
+  auto Instance = llvm::make_unique<swift::CompilerInstance>();
+  if (Instance->setup(Invocation)) {
+    return nullptr;
   }
 
   FixitApplyDiagnosticConsumer FixitApplyConsumer {
     InputState->getOutputText(),
     getInputFilename(),
   };
-  Instance.addDiagnosticConsumer(&FixitApplyConsumer);
+  Instance->addDiagnosticConsumer(&FixitApplyConsumer);
 
-  Instance.performSema();
+  Instance->performSema();
 
   StringRef ResultText = InputState->getOutputText();
   unsigned ResultBufferID = InputState->getOutputBufferID();
@@ -157,9 +172,10 @@ Migrator::performAFixItMigration() {
     ResultBufferID = SrcMgr.addNewSourceBuffer(std::move(ResultBuffer));
   }
 
-  return MigrationState::make(MigrationKind::CompilerFixits,
-                              SrcMgr, InputState->getOutputBufferID(),
-                              ResultBufferID);
+  States.push_back(MigrationState::make(MigrationKind::CompilerFixits,
+                                        SrcMgr, InputState->getOutputBufferID(),
+                                        ResultBufferID));
+  return Instance;
 }
 
 bool Migrator::performSyntacticPasses() {
@@ -181,7 +197,7 @@ bool Migrator::performSyntacticPasses() {
 
   auto InputState = States.back();
 
-  EditorAdapter Editor { StartInstance.getSourceMgr(), ClangSourceManager };
+  EditorAdapter Editor { StartInstance->getSourceMgr(), ClangSourceManager };
 
   // const auto SF = Instance.getPrimarySourceFile();
 
@@ -195,7 +211,7 @@ bool Migrator::performSyntacticPasses() {
   // Once it has run, push the edits into Edits above:
   // Edits.commit(YourPass.getEdits());
 
-  SyntacticMigratorPass SPass(Editor, StartInstance.getPrimarySourceFile(),
+  SyntacticMigratorPass SPass(Editor, StartInstance->getPrimarySourceFile(),
     getMigratorOptions());
   SPass.run();
   Edits.commit(SPass.getEdits());
@@ -207,7 +223,7 @@ bool Migrator::performSyntacticPasses() {
   RewriteBufferEditsReceiver Rewriter {
     ClangSourceManager,
     Editor.getClangFileIDForSwiftBufferID(
-      StartInstance.getPrimarySourceFile()->getBufferID().getValue()),
+      StartInstance->getPrimarySourceFile()->getBufferID().getValue()),
     InputState->getOutputText()
   };
 

test/Migrator/Inputs/ignore_type_placeholder.swift

@@ -0,0 +1,3 @@
+func foo(f: (Void) -> ()) {
+  f()
+}

test/Migrator/Inputs/string_to_substring_conversion.swift

@@ -0,0 +1,46 @@
+let s = "foo"
+let ss = s[s.startIndex..<s.endIndex]
+
+// CTP_ReturnStmt
+func returnStringButWantedSubstring() -> Substring {
+  return s
+}
+
+// CTP_DefaultParameter
+// Never do this.
+func defaultArgIsSubstring(ss: Substring = s) {}
+
+// CTP_CalleeResult
+func returnString() -> String {
+  return s
+}
+let s2: Substring = returnString()
+
+// CTP_CallArgument
+func takeString(s: String) {}
+func takeSubstring(ss: Substring) {}
+
+takeSubstring(ss)
+takeSubstring(s)
+
+// CTP_ClosureResult
+[1,2,3].map { (x: Int) -> Substring in
+  return s
+}
+
+// CTP_ArrayElement
+let a: [Substring] = [ s ]
+
+// CTP_DictionaryKey
+// CTP_DictionaryValue
+let d: [Substring : Substring] = [
+  "CTP_DictionaryValue" : s,
+  s : "CTP_DictionaryKey",
+]
+
+// CTP_CoerceOperand
+let s3: Substring = s as Substring
+
+// CTP_AssignSource
+let s4: Substring = s
+

test/Migrator/Inputs/substring_to_string_conversion.swift

@@ -0,0 +1,46 @@
+let s = "foo"
+let ss = s[s.startIndex..<s.endIndex]
+
+// CTP_ReturnStmt
+
+func returnSubstringButWantedString() -> String {
+  return ss
+}
+
+// CTP_DefaultParameter
+
+// Never do this.
+func defaultArgIsString(s: String = ss) {}
+
+// CTP_CalleeResult
+func returnSubstring() -> Substring {
+  return ss
+}
+let s2: String = returnSubstring()
+
+// CTP_CallArgument
+func takeString(_ s: String) {}
+
+takeString(s)
+takeString(ss)
+
+// CTP_ClosureResult
+[1,2,3].map { (x: Int) -> String in
+  return ss
+}
+
+// CTP_ArrayElement
+let a: [String] = [ ss ]
+
+// CTP_DictionaryKey
+// CTP_DictionaryValue
+let d: [String : String] = [
+  "CTP_DictionaryValue" : ss,
+  ss : "CTP_DictionaryKey",
+]
+
+// CTP_CoerceOperand
+let s3: String = ss as String
+
+// CTP_AssignSource
+let s4: String = ss

test/Migrator/ignore_type_placeholder.swift

@@ -0,0 +1,3 @@
+// RUN: rm -rf %t && mkdir -p %t && %target-swift-frontend -c -primary-file %S/Inputs/ignore_type_placeholder.swift -emit-migrated-file-path %t/ignore_type_placeholder.swift.result -emit-remap-file-path %t/ignore_type_placeholder.swift.remap
+// RUN: %FileCheck %s < %t/ignore_type_placeholder.swift.result
+// CHECK-NOT: f(<#Void#>)

test/Migrator/pre_fixit_pass.swift

@@ -0,0 +1,15 @@
+// REQUIRES: objc_interop
+// RUN: rm -rf %t && mkdir -p %t && not %target-swift-frontend -c -update-code -primary-file %s -F %S/mock-sdk -api-diff-data-file %S/API.json -emit-migrated-file-path %t/pre_fixit_pass.swift.result -o /dev/null
+// RUN: diff -u %S/pre_fixit_pass.swift.expected %t/pre_fixit_pass.swift.result
+
+import Bar
+
+struct New {}
+@available(*, unavailable, renamed: "New")
+struct Old {}
+Old()
+
+func foo(_ a : PropertyUserInterface) {
+  a.setField(1)
+  _ = a.field()
+}

test/Migrator/pre_fixit_pass.swift.expected

@@ -0,0 +1,15 @@
+// REQUIRES: objc_interop
+// RUN: rm -rf %t && mkdir -p %t && not %target-swift-frontend -c -update-code -primary-file %s -F %S/mock-sdk -api-diff-data-file %S/API.json -emit-migrated-file-path %t/pre_fixit_pass.swift.result -o /dev/null
+// RUN: diff -u %S/pre_fixit_pass.swift.expected %t/pre_fixit_pass.swift.result
+
+import Bar
+
+struct New {}
+@available(*, unavailable, renamed: "New")
+struct Old {}
+New()
+
+func foo(_ a : PropertyUserInterface) {
+  a.Field = 1
+  _ = a.field
+}

test/Migrator/pre_fixit_pass_still_failed.swift

@@ -0,0 +1,9 @@
+// RUN: rm -rf %t && mkdir -p %t && not %target-swift-frontend -c -update-code -primary-file %s -emit-migrated-file-path %t/pre_fixit_pass.swift.result -o /dev/null
+
+// We shouldn't be able to successfully use the pre-fix-it passes to
+// fix this file before migration because there is a use of an
+// unresolved identifier 'bar'.
+
+func foo(s: String) {}
+foo("Hello")
+bar("Hello") // Unresolved failure