[Diagnostics][NFC] Introduce Structured fix-its

owenv · owenv · commit dd3d2cf95996 · 2019-08-14T20:22:51.000-05:00
These are defined with macros like errors/warnings/notes, and
make use of format strings and diagnostic arguments. The intent
is to leverage diagnostic arguments in the future to disambiguate
ambiguously spelled types.

Ported a few miscellaneous fix-its to the new system
diff --git a/include/swift/AST/DiagnosticConsumer.h b/include/swift/AST/DiagnosticConsumer.h
@@ -49,8 +49,7 @@ struct DiagnosticInfo {
     std::string Text;
 
   public:
-    FixIt(CharSourceRange R, StringRef Str)
-        : Range(R), Text(Str) {}
+    FixIt(CharSourceRange R, StringRef Str, ArrayRef<DiagnosticArgument> Args);
 
     CharSourceRange getRange() const { return Range; }
     StringRef getText() const { return Text; }
diff --git a/include/swift/AST/DiagnosticEngine.h b/include/swift/AST/DiagnosticEngine.h
@@ -309,8 +309,22 @@ namespace swift {
         : OpeningQuotationMark("'"), ClosingQuotationMark("'"),
           AKAFormatString("'%s' (aka '%s')"),
           OpaqueResultFormatString("'%s' (%s of '%s')") {}
+
+    /// When formatting fix-it arguments, don't include quotes or other
+    /// additions which would result in invalid code.
+    static DiagnosticFormatOptions formatForFixits() {
+      return DiagnosticFormatOptions("", "", "%s", "%s");
+    }
   };
-  
+
+  enum class FixItID : uint32_t;
+
+  /// StructuredFixIt - Represents a fix-it defined in FixIts.def with a format
+  /// string and optional DiagnosticArguments. The template parameters
+  /// allow the fixIt... methods on InflightDiagnostic to infer their own
+  /// template params.
+  template <typename... ArgTypes> struct StructuredFixIt { FixItID ID; };
+
   /// Diagnostic - This is a specific instance of a diagnostic along with all of
   /// the DiagnosticArguments that it requires. 
   class Diagnostic {
@@ -425,25 +439,89 @@ namespace swift {
     /// Add a character-based range to the currently-active diagnostic.
     InFlightDiagnostic &highlightChars(SourceLoc Start, SourceLoc End);
 
+    static const char *fixItStringFor(const FixItID id);
+
+    /// Add a token-based replacement fix-it to the currently-active
+    /// diagnostic.
+    template <typename... ArgTypes>
+    InFlightDiagnostic &
+    fixItReplace(SourceRange R, StructuredFixIt<ArgTypes...> fixIt,
+                 typename detail::PassArgument<ArgTypes>::type... VArgs) {
+      SmallVector<DiagnosticArgument, 3> Args;
+      DiagnosticArgument DiagArgs[] = {
+        DiagnosticArgument(0), std::move(VArgs)...
+      };
+      Args.append(DiagArgs + 1, DiagArgs + 1 + sizeof...(VArgs));
+      return fixItReplace(R, fixItStringFor(fixIt.ID), std::move(Args));
+    }
+
+    /// Add a character-based replacement fix-it to the currently-active
+    /// diagnostic.
+    template <typename... ArgTypes>
+    InFlightDiagnostic &
+    fixItReplaceChars(SourceLoc Start, SourceLoc End,
+                      StructuredFixIt<ArgTypes...> fixIt,
+                      typename detail::PassArgument<ArgTypes>::type... VArgs) {
+      SmallVector<DiagnosticArgument, 3> Args;
+      DiagnosticArgument DiagArgs[] = {
+        DiagnosticArgument(0), std::move(VArgs)...
+      };
+      Args.append(DiagArgs + 1, DiagArgs + 1 + sizeof...(VArgs));
+      return fixItReplaceChars(Start, End, fixItStringFor(fixIt.ID),
+                               std::move(Args));
+    }
+
+    /// Add an insertion fix-it to the currently-active diagnostic.
+    template <typename... ArgTypes>
+    InFlightDiagnostic &
+    fixItInsert(SourceLoc L, StructuredFixIt<ArgTypes...> fixIt,
+                typename detail::PassArgument<ArgTypes>::type... VArgs) {
+      SmallVector<DiagnosticArgument, 3> Args;
+      DiagnosticArgument DiagArgs[] = {
+        DiagnosticArgument(0), std::move(VArgs)...
+      };
+      Args.append(DiagArgs + 1, DiagArgs + 1 + sizeof...(VArgs));
+      return fixItReplaceChars(L, L, fixItStringFor(fixIt.ID), std::move(Args));
+    }
+
+    /// Add an insertion fix-it to the currently-active diagnostic.  The
+    /// text is inserted immediately *after* the token specified.
+    template <typename... ArgTypes>
+    InFlightDiagnostic &
+    fixItInsertAfter(SourceLoc L, StructuredFixIt<ArgTypes...> fixIt,
+                     typename detail::PassArgument<ArgTypes>::type... VArgs) {
+      SmallVector<DiagnosticArgument, 3> Args;
+      DiagnosticArgument DiagArgs[] = {
+        DiagnosticArgument(0), std::move(VArgs)...
+      };
+      Args.append(DiagArgs + 1, DiagArgs + 1 + sizeof...(VArgs));
+      return fixItInsertAfter(L, fixItStringFor(fixIt.ID), std::move(Args));
+    }
+
     /// Add a token-based replacement fix-it to the currently-active
     /// diagnostic.
-    InFlightDiagnostic &fixItReplace(SourceRange R, StringRef Str);
+    InFlightDiagnostic &fixItReplace(SourceRange R, StringRef Str) {
+      return fixItReplace(R, Str, {});
+    }
 
     /// Add a character-based replacement fix-it to the currently-active
     /// diagnostic.
     InFlightDiagnostic &fixItReplaceChars(SourceLoc Start, SourceLoc End,
-                                          StringRef Str);
+                                          StringRef Str) {
+      return fixItReplaceChars(Start, End, Str, {});
+    }
 
     /// Add an insertion fix-it to the currently-active diagnostic.
     InFlightDiagnostic &fixItInsert(SourceLoc L, StringRef Str) {
-      return fixItReplaceChars(L, L, Str);
+      return fixItReplaceChars(L, L, Str, {});
     }
 
     /// Add an insertion fix-it to the currently-active diagnostic.  The
     /// text is inserted immediately *after* the token specified.
-    ///
-    InFlightDiagnostic &fixItInsertAfter(SourceLoc L, StringRef);
-
+    InFlightDiagnostic &fixItInsertAfter(SourceLoc L, StringRef Str) {
+      return fixItInsertAfter(L, Str, {});
+    }
+    
     /// Add a token-based removal fix-it to the currently-active
     /// diagnostic.
     InFlightDiagnostic &fixItRemove(SourceRange R);
@@ -457,6 +535,22 @@ namespace swift {
     /// Add two replacement fix-it exchanging source ranges to the
     /// currently-active diagnostic.
     InFlightDiagnostic &fixItExchange(SourceRange R1, SourceRange R2);
+    
+    private:
+    InFlightDiagnostic &fixItReplace(SourceRange R, StringRef Str,
+                                     ArrayRef<DiagnosticArgument> Args);
+
+    InFlightDiagnostic &fixItReplaceChars(SourceLoc Start, SourceLoc End,
+                                          StringRef Str,
+                                          ArrayRef<DiagnosticArgument> Args);
+
+    InFlightDiagnostic &fixItInsert(SourceLoc L, StringRef Str,
+                                    ArrayRef<DiagnosticArgument> Args) {
+      return fixItReplaceChars(L, L, Str, Args);
+    }
+
+    InFlightDiagnostic &fixItInsertAfter(SourceLoc L, StringRef,
+                                         ArrayRef<DiagnosticArgument> Args);
   };
 
   /// Class to track, map, and remap diagnostic severity and fatality
diff --git a/include/swift/AST/DiagnosticsCommon.h b/include/swift/AST/DiagnosticsCommon.h
@@ -28,14 +28,24 @@ namespace swift {
   struct Diag;
 
   namespace detail {
+    // These templates are used to help extract the type arguments of the
+    // DIAG/ERROR/WARNING/NOTE/REMARK/FIXIT macros.
     template<typename T>
     struct DiagWithArguments;
     
     template<typename ...ArgTypes>
     struct DiagWithArguments<void(ArgTypes...)> {
       typedef Diag<ArgTypes...> type;
     };
-  }
+
+    template <typename T>
+    struct StructuredFixItWithArguments;
+
+    template <typename... ArgTypes>
+    struct StructuredFixItWithArguments<void(ArgTypes...)> {
+      typedef StructuredFixIt<ArgTypes...> type;
+    };
+  } // end namespace detail
 
   enum class StaticSpellingKind : uint8_t;
 
@@ -49,7 +59,14 @@ namespace swift {
 #define DIAG(KIND,ID,Options,Text,Signature) \
     extern detail::DiagWithArguments<void Signature>::type ID;
 #include "DiagnosticsCommon.def"
-  }
-}
+  } // end namespace diag
+
+  namespace fixIt {
+  // Declare structured fix-it objects with the appropriate type.
+#define FIXIT(ID, Text, Signature) \
+  extern detail::StructuredFixItWithArguments<void Signature>::type ID;
+#include "FixIts.def"
+  } // end namespace fixIt
+} // end namespace swift
 
 #endif
diff --git a/include/swift/AST/FixIts.def b/include/swift/AST/FixIts.def
@@ -0,0 +1,32 @@
+//===--- FixIts.def - Fix-Its Text ------------------------------*- C++ -*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2019 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
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines fix-its which can be emitted alongside a diagnostic.
+//  Each fix-it is described by a unique identifier and text, and is
+//  followed by a signature describing the diagnostic argument kinds.
+//
+//===----------------------------------------------------------------------===//
+
+#if (!(defined(FIXIT)))
+#  error Must define FIXIT
+#endif
+
+FIXIT(insert_type_coercion, " %select{as!|as}0 %1", (bool, Type))
+
+FIXIT(replace_with_type, "%0", (Type))
+FIXIT(insert_type_qualification, "%0.", (Type))
+
+FIXIT(insert_closure_return_type, "%select{| () }1-> %0 %select{|in }1", (Type, bool))
+
+#ifndef DIAG_NO_UNDEF
+#  undef FIXIT
+#endif
diff --git a/lib/AST/DiagnosticConsumer.cpp b/lib/AST/DiagnosticConsumer.cpp
@@ -29,6 +29,21 @@ using namespace swift;
 
 DiagnosticConsumer::~DiagnosticConsumer() = default;
 
+DiagnosticInfo::FixIt::FixIt(CharSourceRange R, StringRef Str,
+                             ArrayRef<DiagnosticArgument> Args): Range(R) {
+  if (!Args.empty()) {
+    // FIXME: Defer text formatting to later in the pipeline.
+    SmallString<16> Buffer;
+    llvm::raw_svector_ostream OS(Buffer);
+    DiagnosticEngine::formatDiagnosticText(OS, Str, Args,
+                                           DiagnosticFormatOptions::
+                                           formatForFixits());
+    Text = OS.str();
+  } else {
+    Text = Str;
+  }
+}
+
 llvm::SMLoc DiagnosticConsumer::getRawLoc(SourceLoc loc) {
   return loc.Value;
 }
diff --git a/lib/AST/DiagnosticEngine.cpp b/lib/AST/DiagnosticEngine.cpp
@@ -108,6 +108,12 @@ static constexpr const char *const debugDiagnosticStrings[] = {
     "<not a diagnostic>",
 };
 
+static constexpr const char *const fixItStrings[] = {
+#define FIXIT(ID, Text, Signature) Text,
+#include "swift/AST/FixIts.def"
+    "<not a fix-it>",
+};
+
 DiagnosticState::DiagnosticState() {
   // Initialize our per-diagnostic state to default
   perDiagnosticBehavior.resize(LocalDiagID::NumDiags, Behavior::Unspecified);
@@ -162,10 +168,11 @@ InFlightDiagnostic &InFlightDiagnostic::highlightChars(SourceLoc Start,
 /// Add an insertion fix-it to the currently-active diagnostic.  The
 /// text is inserted immediately *after* the token specified.
 ///
-InFlightDiagnostic &InFlightDiagnostic::fixItInsertAfter(SourceLoc L,
-                                                         StringRef Str) {
+InFlightDiagnostic &
+InFlightDiagnostic::fixItInsertAfter(SourceLoc L, StringRef Str,
+                                     ArrayRef<DiagnosticArgument> Args) {
   L = Lexer::getLocForEndOfToken(Engine->SourceMgr, L);
-  return fixItInsert(L, Str);
+  return fixItInsert(L, Str, std::move(Args));
 }
 
 /// Add a token-based removal fix-it to the currently-active
@@ -189,13 +196,13 @@ InFlightDiagnostic &InFlightDiagnostic::fixItRemove(SourceRange R) {
     charRange = CharSourceRange(charRange.getStart(),
                                 charRange.getByteLength()+1);
   }
-  Engine->getActiveDiagnostic().addFixIt(Diagnostic::FixIt(charRange, {}));
+  Engine->getActiveDiagnostic().addFixIt(Diagnostic::FixIt(charRange, {}, {}));
   return *this;
 }
 
-
-InFlightDiagnostic &InFlightDiagnostic::fixItReplace(SourceRange R,
-                                                     StringRef Str) {
+InFlightDiagnostic &
+InFlightDiagnostic::fixItReplace(SourceRange R, StringRef Str,
+                                 ArrayRef<DiagnosticArgument> Args) {
   if (Str.empty())
     return fixItRemove(R);
 
@@ -216,17 +223,20 @@ InFlightDiagnostic &InFlightDiagnostic::fixItReplace(SourceRange R,
       Str = Str.drop_front();
   }
 
-  Engine->getActiveDiagnostic().addFixIt(Diagnostic::FixIt(charRange, Str));
+  Engine->getActiveDiagnostic().addFixIt(
+      Diagnostic::FixIt(charRange, Str, std::move(Args)));
   return *this;
 }
 
-InFlightDiagnostic &InFlightDiagnostic::fixItReplaceChars(SourceLoc Start,
-                                                          SourceLoc End,
-                                                          StringRef Str) {
+InFlightDiagnostic &
+InFlightDiagnostic::fixItReplaceChars(SourceLoc Start, SourceLoc End,
+                                      StringRef Str,
+                                      ArrayRef<DiagnosticArgument> Args) {
   assert(IsActive && "Cannot modify an inactive diagnostic");
   if (Engine && Start.isValid())
-    Engine->getActiveDiagnostic().addFixIt(Diagnostic::FixIt(
-        toCharSourceRange(Engine->SourceMgr, Start, End), Str));
+    Engine->getActiveDiagnostic().addFixIt(
+        Diagnostic::FixIt(toCharSourceRange(Engine->SourceMgr, Start, End), Str,
+                          std::move(Args)));
   return *this;
 }
 
@@ -242,10 +252,10 @@ InFlightDiagnostic &InFlightDiagnostic::fixItExchange(SourceRange R1,
   auto text1 = SM.extractText(charRange1);
   auto text2 = SM.extractText(charRange2);
 
-  Engine->getActiveDiagnostic()
-    .addFixIt(Diagnostic::FixIt(charRange1, text2));
-  Engine->getActiveDiagnostic()
-    .addFixIt(Diagnostic::FixIt(charRange2, text1));
+  Engine->getActiveDiagnostic().addFixIt(
+      Diagnostic::FixIt(charRange1, text2, {}));
+  Engine->getActiveDiagnostic().addFixIt(
+      Diagnostic::FixIt(charRange2, text1, {}));
   return *this;
 }
 
@@ -917,6 +927,10 @@ const char *DiagnosticEngine::diagnosticStringFor(const DiagID id,
   return diagnosticStrings[(unsigned)id];
 }
 
+const char *InFlightDiagnostic::fixItStringFor(const FixItID id) {
+  return fixItStrings[(unsigned)id];
+}
+
 void DiagnosticEngine::setBufferIndirectlyCausingDiagnosticToInput(
     SourceLoc loc) {
   // If in the future, nested BufferIndirectlyCausingDiagnosticRAII need be
diff --git a/lib/AST/DiagnosticList.cpp b/lib/AST/DiagnosticList.cpp
@@ -24,6 +24,11 @@ enum class swift::DiagID : uint32_t {
 static_assert(static_cast<uint32_t>(swift::DiagID::invalid_diagnostic) == 0,
               "0 is not the invalid diagnostic ID");
 
+enum class swift::FixItID : uint32_t {
+#define FIXIT(ID, Text, Signature) ID,
+#include "swift/AST/FixIts.def"
+};
+
 // Define all of the diagnostic objects and initialize them with their 
 // diagnostic IDs.
 namespace swift {
@@ -32,4 +37,10 @@ namespace swift {
     detail::DiagWithArguments<void Signature>::type ID = { DiagID::ID };
 #include "swift/AST/DiagnosticsAll.def"
   } // end namespace diag
+
+  namespace fixIt {
+#define FIXIT(ID, Text, Signature) \
+  detail::StructuredFixItWithArguments<void Signature>::type ID = {FixItID::ID};
+#include "swift/AST/FixIts.def"
+  } // end namespace fixIt
 } // end namespace swift
diff --git a/lib/Sema/CSDiag.cpp b/lib/Sema/CSDiag.cpp
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
