[RequirementMachine] Preserve structural requirements during rule construction,

and diagnose trivially redundant rules in the rewrite system.

Author: hborla

lib/AST/RequirementMachine/ConcreteTypeWitness.cpp

@@ -578,8 +578,11 @@ void PropertyMap::inferConditionalRequirements(
         for (const auto &rule : builder.PermanentRules)
           System.addPermanentRule(rule.first, rule.second);
 
-        for (const auto &rule : builder.RequirementRules)
-          System.addExplicitRule(rule.first, rule.second);
+        for (const auto &rule : builder.RequirementRules) {
+          auto lhs = std::get<0>(rule);
+          auto rhs = std::get<1>(rule);
+          System.addExplicitRule(lhs, rhs, /*requirementID=*/None);
+        }
       }
     }
 

lib/AST/RequirementMachine/RequirementLowering.cpp

@@ -77,14 +77,14 @@ static void desugarSameTypeRequirement(Type lhs, Type rhs, SourceLoc loc,
                   Type sugaredFirstType) {
       if (firstType->isTypeParameter() && secondType->isTypeParameter()) {
         result.emplace_back(RequirementKind::SameType,
-                            firstType, secondType);
+                            sugaredFirstType, secondType);
         recordedRequirements = true;
         return true;
       }
 
       if (firstType->isTypeParameter()) {
         result.emplace_back(RequirementKind::SameType,
-                            firstType, secondType);
+                            sugaredFirstType, secondType);
         recordedRequirements = true;
         return true;
       }
@@ -530,7 +530,7 @@ void swift::rewriting::realizeInheritedRequirements(
 /// \returns true if any errors were emitted, and false otherwise (including
 /// when only warnings were emitted).
 bool swift::rewriting::diagnoseRequirementErrors(
-    ASTContext &ctx, SmallVectorImpl<RequirementError> &errors,
+    ASTContext &ctx, ArrayRef<RequirementError> errors,
     bool allowConcreteGenericParams) {
   bool diagnosedError = false;
 
@@ -1034,7 +1034,7 @@ void RuleBuilder::addRequirements(ArrayRef<Requirement> requirements) {
 
   // Add rewrite rules for all top-level requirements.
   for (const auto &req : requirements)
-    addRequirement(req, /*proto=*/nullptr);
+    addRequirement(req, /*proto=*/nullptr, /*requirementID=*/None);
 }
 
 void RuleBuilder::addRequirements(ArrayRef<StructuralRequirement> requirements) {
@@ -1184,22 +1184,26 @@ swift::rewriting::getRuleForRequirement(const Requirement &req,
 }
 
 void RuleBuilder::addRequirement(const Requirement &req,
-                                 const ProtocolDecl *proto) {
+                                 const ProtocolDecl *proto,
+                                 Optional<unsigned> requirementID) {
   if (Dump) {
     llvm::dbgs() << "+ ";
     req.dump(llvm::dbgs());
     llvm::dbgs() << "\n";
   }
 
-  RequirementRules.push_back(
+  auto rule =
       getRuleForRequirement(req, proto, /*substitutions=*/None,
-                            Context));
+                            Context);
+  RequirementRules.push_back(
+      std::make_tuple(rule.first, rule.second, requirementID));
 }
 
 void RuleBuilder::addRequirement(const StructuralRequirement &req,
                                  const ProtocolDecl *proto) {
-  // FIXME: Preserve source location information for diagnostics.
-  addRequirement(req.req.getCanonical(), proto);
+  WrittenRequirements.push_back(req);
+  unsigned requirementID = WrittenRequirements.size() - 1;
+  addRequirement(req.req.getCanonical(), proto, requirementID);
 }
 
 /// Lowers a protocol typealias to a rewrite rule.
@@ -1231,7 +1235,8 @@ void RuleBuilder::addTypeAlias(const ProtocolTypeAlias &alias,
     constraintTerm.add(Symbol::forConcreteType(concreteType, result, Context));
   }
 
-  RequirementRules.emplace_back(subjectTerm, constraintTerm);
+  RequirementRules.emplace_back(subjectTerm, constraintTerm,
+                                /*requirementID=*/None);
 }
 
 /// Record information about a protocol if we have no seen it yet.
@@ -1292,11 +1297,11 @@ void RuleBuilder::collectRulesFromReferencedProtocols() {
         addRequirement(req, proto);
 
       for (auto req : proto->getTypeAliasRequirements())
-        addRequirement(req.getCanonical(), proto);
+        addRequirement(req.getCanonical(), proto, /*requirementID=*/None);
     } else {
       auto reqs = proto->getRequirementSignature();
       for (auto req : reqs.getRequirements())
-        addRequirement(req.getCanonical(), proto);
+        addRequirement(req.getCanonical(), proto, /*requirementID=*/None);
       for (auto alias : reqs.getTypeAliases())
         addTypeAlias(alias, proto);
     }

lib/AST/RequirementMachine/RequirementLowering.h

@@ -59,7 +59,7 @@ void realizeInheritedRequirements(TypeDecl *decl, Type type,
                                   SmallVectorImpl<RequirementError> &errors);
 
 bool diagnoseRequirementErrors(ASTContext &ctx,
-                               SmallVectorImpl<RequirementError> &errors,
+                               ArrayRef<RequirementError> errors,
                                bool allowConcreteGenericParams);
 
 std::pair<MutableTerm, MutableTerm>
@@ -104,7 +104,12 @@ struct RuleBuilder {
 
   /// New rules derived from requirements written by the user, which can be
   /// eliminated by homotopy reduction.
-  std::vector<std::pair<MutableTerm, MutableTerm>> RequirementRules;
+  std::vector<std::tuple<MutableTerm, MutableTerm, Optional<unsigned>>>
+      RequirementRules;
+
+  /// Requirements written in source code. The requirement ID in the above
+  /// \c RequirementRules vector is an index into this array.
+  std::vector<StructuralRequirement> WrittenRequirements;
 
   /// Enables debugging output. Controlled by the -dump-requirement-machine
   /// frontend flag.
@@ -123,7 +128,8 @@ struct RuleBuilder {
   void addAssociatedType(const AssociatedTypeDecl *type,
                          const ProtocolDecl *proto);
   void addRequirement(const Requirement &req,
-                      const ProtocolDecl *proto);
+                      const ProtocolDecl *proto,
+                      Optional<unsigned> requirementID);
   void addRequirement(const StructuralRequirement &req,
                       const ProtocolDecl *proto);
   void addTypeAlias(const ProtocolTypeAlias &alias,

lib/AST/RequirementMachine/RequirementMachine.cpp

@@ -96,6 +96,7 @@ RequirementMachine::initWithGenericSignature(CanGenericSignature sig) {
   // Add the initial set of rewrite rules to the rewrite system.
   System.initialize(/*recordLoops=*/false,
                     /*protos=*/ArrayRef<const ProtocolDecl *>(),
+                    std::move(builder.WrittenRequirements),
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 
@@ -138,6 +139,7 @@ RequirementMachine::initWithProtocols(ArrayRef<const ProtocolDecl *> protos) {
 
   // Add the initial set of rewrite rules to the rewrite system.
   System.initialize(/*recordLoops=*/true, protos,
+                    std::move(builder.WrittenRequirements),
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 
@@ -185,6 +187,7 @@ RequirementMachine::initWithWrittenRequirements(
   // Add the initial set of rewrite rules to the rewrite system.
   System.initialize(/*recordLoops=*/true,
                     /*protos=*/ArrayRef<const ProtocolDecl *>(),
+                    std::move(builder.WrittenRequirements),
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 

lib/AST/RequirementMachine/RequirementMachineRequests.cpp

@@ -419,6 +419,12 @@ RequirementSignatureRequestRQM::evaluate(Evaluator &evaluator,
     }
   }
 
+  if (ctx.LangOpts.RequirementMachineProtocolSignatures ==
+      RequirementMachineMode::Enabled) {
+    diagnoseRequirementErrors(ctx, machine->System.getErrors(),
+                              /*allowConcreteGenericParams=*/false);
+  }
+
   // Return the result for the specific protocol this request was kicked off on.
   return *result;
 }
@@ -668,6 +674,8 @@ InferredGenericSignatureRequestRQM::evaluate(
   if (ctx.LangOpts.RequirementMachineInferredSignatures ==
       RequirementMachineMode::Enabled) {
     hadError |= diagnoseRequirementErrors(ctx, errors, allowConcreteGenericParams);
+    hadError |= diagnoseRequirementErrors(ctx, machine->System.getErrors(),
+                                          allowConcreteGenericParams);
   }
 
   // FIXME: Handle allowConcreteGenericParams

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -276,19 +276,26 @@ RewriteSystem::~RewriteSystem() {
 
 void RewriteSystem::initialize(
     bool recordLoops, ArrayRef<const ProtocolDecl *> protos,
+    ArrayRef<StructuralRequirement> writtenRequirements,
     std::vector<std::pair<MutableTerm, MutableTerm>> &&permanentRules,
-    std::vector<std::pair<MutableTerm, MutableTerm>> &&requirementRules) {
+    std::vector<std::tuple<MutableTerm, MutableTerm, Optional<unsigned>>>
+        &&requirementRules) {
   assert(!Initialized);
   Initialized = 1;
 
   RecordLoops = recordLoops;
   Protos = protos;
+  WrittenRequirements = writtenRequirements;
 
   for (const auto &rule : permanentRules)
     addPermanentRule(rule.first, rule.second);
 
-  for (const auto &rule : requirementRules)
-    addExplicitRule(rule.first, rule.second);
+  for (const auto &rule : requirementRules) {
+    auto lhs = std::get<0>(rule);
+    auto rhs = std::get<1>(rule);
+    auto requirementID = std::get<2>(rule);
+    addExplicitRule(lhs, rhs, requirementID);
+  }
 }
 
 /// Reduce a term by applying all rewrite rules until fixed point.
@@ -487,10 +494,16 @@ bool RewriteSystem::addPermanentRule(MutableTerm lhs, MutableTerm rhs) {
 }
 
 /// Add a new rule, marking it explicit.
-bool RewriteSystem::addExplicitRule(MutableTerm lhs, MutableTerm rhs) {
+bool RewriteSystem::addExplicitRule(MutableTerm lhs, MutableTerm rhs,
+                                    Optional<unsigned> requirementID) {
   bool added = addRule(std::move(lhs), std::move(rhs));
-  if (added)
+  if (added) {
     Rules.back().markExplicit();
+  } else if (requirementID.hasValue()) {
+    auto req = WrittenRequirements[requirementID.getValue()];
+    Errors.push_back(
+        RequirementError::forRedundantRequirement(req.req, req.loc));
+  }
 
   return added;
 }

lib/AST/RequirementMachine/RewriteSystem.h

@@ -13,10 +13,12 @@
 #ifndef SWIFT_REWRITESYSTEM_H
 #define SWIFT_REWRITESYSTEM_H
 
+#include "swift/AST/Requirement.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/PointerUnion.h"
 
 #include "Debug.h"
+#include "Diagnostics.h"
 #include "RewriteLoop.h"
 #include "Symbol.h"
 #include "Term.h"
@@ -225,6 +227,12 @@ class RewriteSystem final {
   /// top-level generic signature and this array is empty.
   ArrayRef<const ProtocolDecl *> Protos;
 
+  /// The requirements written in source code.
+  std::vector<StructuralRequirement> WrittenRequirements;
+
+  /// The invalid requirements.
+  std::vector<RequirementError> Errors;
+
   /// The rules added so far, including rules from our client, as well
   /// as rules introduced by the completion procedure.
   std::vector<Rule> Rules;
@@ -278,11 +286,16 @@ class RewriteSystem final {
     return ProtocolMap;
   }
 
+  ArrayRef<RequirementError> getErrors() {
+    return Errors;
+  }
+
   DebugOptions getDebugOptions() const { return Debug; }
 
   void initialize(bool recordLoops, ArrayRef<const ProtocolDecl *> protos,
+                  ArrayRef<StructuralRequirement> writtenRequirements,
                   std::vector<std::pair<MutableTerm, MutableTerm>> &&permanentRules,
-                  std::vector<std::pair<MutableTerm, MutableTerm>> &&requirementRules);
+                  std::vector<std::tuple<MutableTerm, MutableTerm, Optional<unsigned>>> &&requirementRules);
 
   ArrayRef<const ProtocolDecl *> getProtocols() const {
     return Protos;
@@ -314,7 +327,8 @@ class RewriteSystem final {
 
   bool addPermanentRule(MutableTerm lhs, MutableTerm rhs);
 
-  bool addExplicitRule(MutableTerm lhs, MutableTerm rhs);
+  bool addExplicitRule(MutableTerm lhs, MutableTerm rhs,
+                       Optional<unsigned> requirementID);
 
   bool simplify(MutableTerm &term, RewritePath *path=nullptr) const;
 

test/Generics/requirement_machine_diagnostics.swift

@@ -82,3 +82,32 @@ func cascadingConflictingRequirement<T>(_: T) where DoesNotExist : EqualComparab
 
 func cascadingInvalidConformance<T>(_: T) where T : DoesNotExist { }
 // expected-error@-1 {{cannot find type 'DoesNotExist' in scope}}
+
+func trivialRedundant1<T>(_: T) where T: P, T: P {}
+// expected-warning@-1 {{redundant conformance constraint 'T' : 'P'}}
+
+func trivialRedundant2<T>(_: T) where T: AnyObject, T: AnyObject {}
+// expected-warning@-1 {{redundant constraint 'T' : 'AnyObject'}}
+
+func trivialRedundant3<T>(_: T) where T: C, T: C {}
+// expected-warning@-1 {{redundant superclass constraint 'T' : 'C'}}
+
+func trivialRedundant4<T>(_: T) where T == T {}
+// expected-warning@-1 {{redundant same-type constraint 'T' == 'T'}}
+
+protocol TrivialRedundantConformance: P, P {}
+// expected-warning@-1 {{redundant conformance constraint 'Self' : 'P'}}
+
+protocol TrivialRedundantLayout: AnyObject, AnyObject {}
+// expected-warning@-1 {{redundant constraint 'Self' : 'AnyObject'}}
+// expected-error@-2 {{duplicate inheritance from 'AnyObject'}}
+
+protocol TrivialRedundantSuperclass: C, C {}
+// expected-warning@-1 {{redundant superclass constraint 'Self' : 'C'}}
+// expected-error@-2 {{duplicate inheritance from 'C'}}
+
+protocol TrivialRedundantSameType where Self == Self {
+// expected-warning@-1 {{redundant same-type constraint 'Self' == 'Self'}}
+  associatedtype T where T == T
+  // expected-warning@-1 {{redundant same-type constraint 'Self.T' == 'Self.T'}}
+}