[Parse][GSB] Add fix-it to remove redundant ': Any' constraints

hamishknight · hamishknight · commit 099430d1d5d6 · 2018-07-29T19:33:37.000+01:00
Note that this doesn't handle constraints that are parsed as inheritance clauses, such as &lt;T : Any&gt; as we don't currently have a custom data structure by which to represent it in the GSB.
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -1012,6 +1012,12 @@ enum class RequirementReprKind : unsigned {
 /// \c GenericParamList assumes these are POD-like.
 class RequirementRepr {
   SourceLoc SeparatorLoc;
+
+  /// This range includes everything that needs to be removed to keep the
+  /// program syntactically valid (e.g. where and commas), unlike just from
+  /// the start of \c FirstType to the end of \c SecondType.
+  SourceRange RemovalRange;
+
   RequirementReprKind Kind : 2;
   bool Invalid : 1;
   TypeLoc FirstType;
@@ -1027,15 +1033,17 @@ class RequirementRepr {
   /// for the generated interface.
   StringRef AsWrittenString;
 
-  RequirementRepr(SourceLoc SeparatorLoc, RequirementReprKind Kind,
-                  TypeLoc FirstType, TypeLoc SecondType)
-    : SeparatorLoc(SeparatorLoc), Kind(Kind), Invalid(false),
-      FirstType(FirstType), SecondType(SecondType) { }
+  RequirementRepr(SourceLoc SeparatorLoc, SourceRange RemovalRange,
+                  RequirementReprKind Kind, TypeLoc FirstType,
+                  TypeLoc SecondType)
+      : SeparatorLoc(SeparatorLoc), RemovalRange(RemovalRange), Kind(Kind),
+        Invalid(false), FirstType(FirstType), SecondType(SecondType) {}
 
-  RequirementRepr(SourceLoc SeparatorLoc, RequirementReprKind Kind,
-                  TypeLoc FirstType, LayoutConstraintLoc SecondLayout)
-    : SeparatorLoc(SeparatorLoc), Kind(Kind), Invalid(false),
-      FirstType(FirstType), SecondLayout(SecondLayout) { }
+  RequirementRepr(SourceLoc SeparatorLoc, SourceRange RemovalRange,
+                  RequirementReprKind Kind, TypeLoc FirstType,
+                  LayoutConstraintLoc SecondLayout)
+      : SeparatorLoc(SeparatorLoc), RemovalRange(RemovalRange), Kind(Kind),
+        Invalid(false), FirstType(FirstType), SecondLayout(SecondLayout) {}
 
   void printImpl(ASTPrinter &OS, bool AsWritten) const;
 
@@ -1048,10 +1056,15 @@ class RequirementRepr {
   /// this requirement was implied.
   /// \param Constraint The protocol or protocol composition to which the
   /// subject must conform, or superclass from which the subject must inherit.
-  static RequirementRepr getTypeConstraint(TypeLoc Subject,
-                                           SourceLoc ColonLoc,
-                                           TypeLoc Constraint) {
-    return { ColonLoc, RequirementReprKind::TypeConstraint, Subject, Constraint };
+  /// \param RemovalRange A source range suitable for removing the requirement
+  /// from the associated where clause, including everything that needs to be
+  /// removed to keep the program syntactically valid (e.g. the 'where' keyword
+  /// and commas).
+  static RequirementRepr getTypeConstraint(TypeLoc Subject, SourceLoc ColonLoc,
+                                           TypeLoc Constraint,
+                                           SourceRange RemovalRange) {
+    return {ColonLoc, RemovalRange, RequirementReprKind::TypeConstraint,
+            Subject, Constraint};
   }
 
   /// \brief Construct a new same-type requirement.
@@ -1060,25 +1073,34 @@ class RequirementRepr {
   /// \param EqualLoc The location of the '==' in the same-type constraint, or
   /// an invalid location if this requirement was implied.
   /// \param SecondType The second type.
-  static RequirementRepr getSameType(TypeLoc FirstType,
-                                     SourceLoc EqualLoc,
-                                     TypeLoc SecondType) {
-    return { EqualLoc, RequirementReprKind::SameType, FirstType, SecondType };
+  /// \param RemovalRange A source range suitable for removing the requirement
+  /// from the associated where clause, including everything that needs to be
+  /// removed to keep the program syntactically valid (e.g. the 'where' keyword
+  /// and commas).
+  static RequirementRepr getSameType(TypeLoc FirstType, SourceLoc EqualLoc,
+                                     TypeLoc SecondType,
+                                     SourceRange RemovalRange) {
+    return {EqualLoc, RemovalRange, RequirementReprKind::SameType, FirstType,
+            SecondType};
   }
 
   /// \brief Construct a new layout-constraint requirement.
   ///
-  /// \param Subject The type that must conform to the given layout 
-  /// requirement.
+  /// \param Subject The type that must conform to the given layout requirement.
   /// \param ColonLoc The location of the ':', or an invalid location if
   /// this requirement was implied.
   /// \param Layout The layout requirement to which the
   /// subject must conform.
+  /// \param RemovalRange A source range suitable for removing the requirement
+  /// from the associated where clause, including everything that needs to be
+  /// removed to keep the program syntactically valid (e.g. the 'where' keyword
+  /// and commas).
   static RequirementRepr getLayoutConstraint(TypeLoc Subject,
                                              SourceLoc ColonLoc,
-                                             LayoutConstraintLoc Layout) {
-    return {ColonLoc, RequirementReprKind::LayoutConstraint, Subject,
-            Layout};
+                                             LayoutConstraintLoc Layout,
+                                             SourceRange RemovalRange) {
+    return {ColonLoc, RemovalRange, RequirementReprKind::LayoutConstraint,
+            Subject, Layout};
   }
 
   /// \brief Determine the kind of requirement
@@ -1161,6 +1183,20 @@ class RequirementRepr {
     return SeparatorLoc;
   }
 
+  /// Retrieve the source range suitable for removing the requirement. Unlike
+  /// \c getSourceRange(), this range includes everything that needs to be
+  /// removed to keep the program syntactically valid (e.g. the 'where' keyword
+  /// and commas).
+  SourceRange getRemovalRange() const { return RemovalRange; }
+
+  /// Set the source range suitable for removing the requirement. Unlike
+  /// \c getSourceRange(), this range includes everything that needs to be
+  /// removed to keep the program syntactically valid (e.g. the 'where' keyword
+  /// and commas).
+  void setRemovalRange(SourceRange removalRange) {
+    RemovalRange = removalRange;
+  }
+
   /// \brief Retrieve the first type of a same-type requirement.
   Type getFirstType() const {
     assert(getKind() == RequirementReprKind::SameType);
diff --git a/include/swift/AST/GenericSignatureBuilder.h b/include/swift/AST/GenericSignatureBuilder.h
@@ -1282,6 +1282,12 @@ class GenericSignatureBuilder::RequirementSource final
   /// Retrieve the source range for the requirement, if any.
   SourceRange getSourceRange() const;
 
+  /// Retrieve the source range suitable for removing the requirement, if any.
+  /// Unlike \c getSourceRange(), this range includes everything that needs to
+  /// be removed to keep the program syntactically valid (e.g. the 'where'
+  /// keyword and commas).
+  SourceRange getRemovalRange() const;
+
   /// Compare two requirement sources to determine which has the more
   /// optimal path.
   ///
@@ -1458,6 +1464,12 @@ class GenericSignatureBuilder::FloatingRequirementSource {
   /// Retrieve the source range for this requirement, if any.
   SourceRange getSourceRange() const;
 
+  /// Retrieve the source range suitable for removing the requirement, if any.
+  /// Unlike \c getSourceRange(), this range includes everything that needs to
+  /// be removed to keep the program syntactically valid (e.g. the 'where'
+  /// keyword and commas).
+  SourceRange getRemovalRange() const;
+
   /// Whether this is an explicitly-stated requirement.
   bool isExplicit() const;
 
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -614,7 +614,8 @@ GenericParamList::clone(DeclContext *dc) const {
       reqt = RequirementRepr::getTypeConstraint(
           first.clone(ctx),
           reqt.getColonLoc(),
-          second.clone(ctx));
+          second.clone(ctx),
+          reqt.getRemovalRange());
       break;
     }
     case RequirementReprKind::SameType: {
@@ -623,7 +624,8 @@ GenericParamList::clone(DeclContext *dc) const {
       reqt = RequirementRepr::getSameType(
           first.clone(ctx),
           reqt.getEqualLoc(),
-          second.clone(ctx));
+          second.clone(ctx),
+          reqt.getRemovalRange());
       break;
     }
     case RequirementReprKind::LayoutConstraint: {
@@ -632,7 +634,8 @@ GenericParamList::clone(DeclContext *dc) const {
       reqt = RequirementRepr::getLayoutConstraint(
           first.clone(ctx),
           reqt.getColonLoc(),
-          layout);
+          layout,
+          reqt.getRemovalRange());
       break;
     }
     }
diff --git a/lib/AST/GenericSignatureBuilder.cpp b/lib/AST/GenericSignatureBuilder.cpp
@@ -1479,6 +1479,14 @@ SourceRange RequirementSource::getSourceRange() const {
   return SourceRange();
 }
 
+SourceRange RequirementSource::getRemovalRange() const {
+  if (auto requirementRepr = getRequirementRepr())
+    return requirementRepr->getRemovalRange();
+
+  // FIX-ME: Handle removal ranges for inherited types.
+  return SourceRange();
+}
+
 /// Compute the path length of a requirement source, counting only the number
 /// of \c ProtocolRequirement elements.
 static unsigned sourcePathLength(const RequirementSource *source) {
@@ -1726,6 +1734,23 @@ SourceRange FloatingRequirementSource::getSourceRange() const {
   return SourceRange();
 }
 
+SourceRange FloatingRequirementSource::getRemovalRange() const {
+  // If this is a source for a protocol requirement, get the removal range
+  // for that requirement.
+  if (kind == Kind::AbstractProtocol)
+    if (auto *written = protocolReq.written.dyn_cast<const RequirementRepr *>())
+      return written->getRemovalRange();
+
+  if (auto *requirement = storage.dyn_cast<const RequirementRepr *>())
+    return requirement->getRemovalRange();
+
+  if (auto *source = storage.dyn_cast<const RequirementSource *>())
+    return source->getRemovalRange();
+
+  // FIX-ME: Handle removal ranges for inherited types.
+  return SourceRange();
+}
+
 bool FloatingRequirementSource::isExplicit() const {
   switch (kind) {
   case Explicit:
@@ -4840,7 +4865,8 @@ ConstraintResult GenericSignatureBuilder::addTypeRequirement(
 
     Diags.diagnose(diagLoc, diag::redundant_conformance_constraint,
                    subjectType, constraintType)
-         .highlight(source.getSourceRange());
+         .highlight(source.getSourceRange())
+         .fixItRemove(source.getRemovalRange());
     Diags.diagnose(diagLoc, diag::all_types_implicitly_conform_to,
                    constraintType);
   }
diff --git a/lib/Parse/ParseGeneric.cpp b/lib/Parse/ParseGeneric.cpp
@@ -14,11 +14,12 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "swift/Parse/Parser.h"
 #include "swift/AST/DiagnosticsParse.h"
+#include "swift/Basic/Defer.h"
 #include "swift/Parse/CodeCompletionCallbacks.h"
-#include "swift/Parse/SyntaxParsingContext.h"
 #include "swift/Parse/Lexer.h"
+#include "swift/Parse/Parser.h"
+#include "swift/Parse/SyntaxParsingContext.h"
 #include "swift/Syntax/SyntaxBuilders.h"
 #include "swift/Syntax/SyntaxNodes.h"
 using namespace swift;
@@ -273,21 +274,25 @@ ParserStatus Parser::parseGenericWhereClause(
   SyntaxParsingContext ReqListContext(SyntaxContext,
                                       SyntaxKind::GenericRequirementList);
   bool HasNextReq;
+  SourceLoc LastCommaLoc;
   do {
     SyntaxParsingContext ReqContext(SyntaxContext, SyntaxContextKind::Syntax);
     // Parse the leading type-identifier.
-    ParserResult<TypeRepr> FirstType = parseTypeIdentifier();
+    ParserResult<TypeRepr> FirstTypeResult = parseTypeIdentifier();
 
-    if (FirstType.hasCodeCompletion()) {
+    if (FirstTypeResult.hasCodeCompletion()) {
       Status.setHasCodeCompletion();
       FirstTypeInComplete = true;
     }
 
-    if (FirstType.isNull()) {
+    if (FirstTypeResult.isNull()) {
       Status.setIsParseError();
       break;
     }
 
+    auto FirstType = FirstTypeResult.get();
+    auto FirstTypeLoc = FirstType->getStartLoc();
+
     if (Tok.is(tok::colon)) {
       // A conformance-requirement.
       SourceLoc ColonLoc = consumeToken();
@@ -311,8 +316,8 @@ ParserStatus Parser::parseGenericWhereClause(
         } else {
           // Add the layout requirement.
           Requirements.push_back(RequirementRepr::getLayoutConstraint(
-              FirstType.get(), ColonLoc,
-              LayoutConstraintLoc(Layout, LayoutLoc)));
+              FirstType, ColonLoc, LayoutConstraintLoc(Layout, LayoutLoc),
+              SourceRange(FirstTypeLoc, PreviousLoc)));
         }
       } else {
         // Parse the protocol or composition.
@@ -327,7 +332,8 @@ ParserStatus Parser::parseGenericWhereClause(
 
         // Add the requirement.
         Requirements.push_back(RequirementRepr::getTypeConstraint(
-            FirstType.get(), ColonLoc, Protocol.get()));
+            FirstType, ColonLoc, Protocol.get(),
+            SourceRange(FirstTypeLoc, PreviousLoc)));
       }
     } else if ((Tok.isAnyOperator() && Tok.getText() == "==") ||
                Tok.is(tok::equal)) {
@@ -349,15 +355,37 @@ ParserStatus Parser::parseGenericWhereClause(
       }
 
       // Add the requirement
-      Requirements.push_back(RequirementRepr::getSameType(FirstType.get(),
-                                                      EqualLoc,
-                                                      SecondType.get()));
+      Requirements.push_back(RequirementRepr::getSameType(
+          FirstType, EqualLoc, SecondType.get(),
+          SourceRange(FirstTypeLoc, PreviousLoc)));
     } else {
       diagnose(Tok, diag::expected_requirement_delim);
       Status.setIsParseError();
       break;
     }
-    HasNextReq = consumeIf(tok::comma);
+    SourceLoc NextCommaLoc;
+    HasNextReq = consumeIf(tok::comma, NextCommaLoc);
+    SWIFT_DEFER { LastCommaLoc = NextCommaLoc; };
+
+    auto &req = Requirements.back();
+    auto removalRange = req.getRemovalRange();
+
+    if (!HasNextReq && LastCommaLoc.isInvalid()) {
+      // Include the where clause in the removal range for the first and only
+      // requirement.
+      removalRange.Start = WhereLoc;
+    }
+    if (!HasNextReq && LastCommaLoc.isValid()) {
+      // Include the previous comma in the removal range of the last
+      // requirement.
+      removalRange.Start = LastCommaLoc;
+    }
+    if (HasNextReq) {
+      // Include the next comma in the removal range of a middle requirement.
+      removalRange.End = NextCommaLoc;
+    }
+    req.setRemovalRange(removalRange);
+
     // If there's a comma, keep parsing the list.
   } while (HasNextReq);
 
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -1819,11 +1819,13 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
       if (interfaceFirstType->hasTypeParameter()) {
         resolvedRequirements.push_back(RequirementRepr::getSameType(
             TypeLoc(req.getFirstTypeRepr(), genericType), req.getEqualLoc(),
-            TypeLoc(req.getSecondTypeRepr(), concreteType)));
+            TypeLoc(req.getSecondTypeRepr(), concreteType),
+            req.getRemovalRange()));
       } else {
         resolvedRequirements.push_back(RequirementRepr::getSameType(
             TypeLoc(req.getFirstTypeRepr(), concreteType), req.getEqualLoc(),
-            TypeLoc(req.getSecondTypeRepr(), genericType)));
+            TypeLoc(req.getSecondTypeRepr(), genericType),
+            req.getRemovalRange()));
       }
 
       // Convert the requirement into a form which uses canonical interface
@@ -1855,8 +1857,8 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
       // Re-create a requirement using the resolved interface types.
       auto resolvedReq = RequirementRepr::getLayoutConstraint(
           TypeLoc(req.getSubjectRepr(), interfaceSubjectType),
-          req.getColonLoc(),
-          req.getLayoutConstraintLoc());
+          req.getColonLoc(), req.getLayoutConstraintLoc(),
+          req.getRemovalRange());
 
       // Add a resolved requirement.
       resolvedRequirements.push_back(resolvedReq);
@@ -1897,7 +1899,8 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
       auto resolvedReq = RequirementRepr::getTypeConstraint(
           TypeLoc(req.getSubjectRepr(), interfaceSubjectType),
           req.getColonLoc(),
-          TypeLoc(req.getConstraintRepr(), interfaceLayoutConstraint));
+          TypeLoc(req.getConstraintRepr(), interfaceLayoutConstraint),
+          req.getRemovalRange());
 
       // Add a resolved requirement.
       resolvedRequirements.push_back(resolvedReq);
diff --git a/test/Generics/redundant_constraints.swift b/test/Generics/redundant_constraints.swift
