Merge pull request #42133 from slavapestov/rqm-minor-diagnostics-fixes

RequirementMachine: Minor diagnostics fixes

Author: slavapestov

lib/AST/RequirementMachine/Debug.h

@@ -72,6 +72,9 @@ enum class DebugFlags : unsigned {
 
   /// Print a trace of requirement machines constructed and how long each took.
   Timers = (1<<16),
+
+  /// Print conflicting rules.
+  ConflictingRules = (1<<17),
 };
 
 using DebugOptions = OptionSet<DebugFlags>;

lib/AST/RequirementMachine/HomotopyReduction.cpp

@@ -167,30 +167,6 @@ void RewriteSystem::propagateRedundantRequirementIDs() {
   }
 }
 
-/// Process pairs of conflicting rules, marking the more specific rule as
-/// conflicting, which instructs minimization to drop this rule.
-void RewriteSystem::processConflicts() {
-  for (auto pair : ConflictingRules) {
-    auto *existingRule = &getRule(pair.first);
-    auto *newRule = &getRule(pair.second);
-
-    // The identity conformance rule ([P].[P] => [P]) will conflict with
-    // a concrete type requirement in an invalid protocol declaration
-    // where 'Self' is constrained to a type that does not conform to
-    // the protocol. This rule is permanent, so don't mark it as
-    // conflicting in this case.
-
-    if (!existingRule->isIdentityConformanceRule() &&
-        existingRule->getRHS().size() >= newRule->getRHS().size())
-      existingRule->markConflicting();
-    if (!newRule->isIdentityConformanceRule() &&
-        newRule->getRHS().size() >= existingRule->getRHS().size())
-      newRule->markConflicting();
-
-    // FIXME: Diagnose the conflict later.
-  }
-}
-
 /// Find a rule to delete by looking through all loops for rewrite rules appearing
 /// once in empty context. Returns a pair consisting of a loop ID and a rule ID,
 /// otherwise returns None.
@@ -477,7 +453,6 @@ void RewriteSystem::minimizeRewriteSystem() {
   Minimized = 1;
 
   propagateExplicitBits();
-  processConflicts();
 
   if (Context.getASTContext().LangOpts.EnableRequirementMachineLoopNormalization) {
     for (auto &loop : Loops) {

lib/AST/RequirementMachine/PropertyUnification.cpp

@@ -119,9 +119,37 @@ static void recordRelation(Term key,
   (void) system.addRule(lhs, rhs, &path);
 }
 
+/// Given two property rules that conflict because no concrete type
+/// can satisfy both, mark one or both rules conflicting.
+///
+/// The right hand side of one rule must be a suffix of the other
+/// (in which case the longer of the two rules is conflicting) or
+/// the right hand sides are equal (in which case both will be
+/// conflicting).
 void RewriteSystem::recordConflict(unsigned existingRuleID,
                                    unsigned newRuleID) {
   ConflictingRules.emplace_back(existingRuleID, newRuleID);
+
+  auto &existingRule = getRule(existingRuleID);
+  auto &newRule = getRule(newRuleID);
+
+  if (Debug.contains(DebugFlags::ConflictingRules)) {
+    llvm::dbgs() << "Conflicting rules:\n";
+    llvm::dbgs() << "- " << existingRule << "\n";
+    llvm::dbgs() << "- " << newRule << "\n";
+  }
+
+  // The identity conformance rule ([P].[P] => [P]) will conflict with
+  // a concrete type requirement in an invalid protocol declaration
+  // where 'Self' is constrained to a type that does not conform to
+  // the protocol. This rule is permanent, so don't mark it as
+  // conflicting in this case.
+  if (!existingRule.isIdentityConformanceRule() &&
+      existingRule.getRHS().size() >= newRule.getRHS().size())
+    existingRule.markConflicting();
+  if (!newRule.isIdentityConformanceRule() &&
+      newRule.getRHS().size() >= existingRule.getRHS().size())
+    newRule.markConflicting();
 }
 
 void PropertyMap::addConformanceProperty(
@@ -350,8 +378,10 @@ void PropertyMap::addSuperclassProperty(
     }
 
     auto &req = props->Superclasses[props->SuperclassDecl];
-    for (const auto &pair : req.SuperclassRules)
-      System.recordConflict(pair.second, ruleID);
+    for (const auto &pair : req.SuperclassRules) {
+      if (checkRulePairOnce(pair.second, ruleID))
+        System.recordConflict(pair.second, ruleID);
+    }
   }
 }
 

lib/AST/RequirementMachine/RequirementLowering.cpp

@@ -654,20 +654,35 @@ void swift::rewriting::realizeInheritedRequirements(
   }
 }
 
+static bool shouldSuggestConcreteTypeFixit(
+    Type type, AllowConcreteTypePolicy concreteTypePolicy) {
+  switch (concreteTypePolicy) {
+  case AllowConcreteTypePolicy::All:
+    return true;
+
+  case AllowConcreteTypePolicy::AssocTypes:
+    return type->is<DependentMemberType>();
+
+  case AllowConcreteTypePolicy::NestedAssocTypes:
+    if (auto *memberType = type->getAs<DependentMemberType>())
+      return memberType->getBase()->is<DependentMemberType>();
+
+    return false;
+  }
+}
+
 /// Emit diagnostics for the given \c RequirementErrors.
 ///
 /// \param ctx The AST context in which to emit diagnostics.
 /// \param errors The set of requirement diagnostics to be emitted.
-/// \param allowConcreteGenericParams Whether concrete type parameters
-/// are permitted in the generic signature. If true, diagnostics will
-/// offer fix-its to turn invalid type requirements, e.g. T: Int, into
-/// same-type requirements.
+/// \param concreteTypePolicy Whether fix-its should be offered to turn
+/// invalid type requirements, e.g. T: Int, into same-type requirements.
 ///
 /// \returns true if any errors were emitted, and false otherwise (including
 /// when only warnings were emitted).
 bool swift::rewriting::diagnoseRequirementErrors(
     ASTContext &ctx, ArrayRef<RequirementError> errors,
-    bool allowConcreteGenericParams) {
+    AllowConcreteTypePolicy concreteTypePolicy) {
   bool diagnosedError = false;
 
   for (auto error : errors) {
@@ -697,7 +712,7 @@ bool swift::rewriting::diagnoseRequirementErrors(
         return subjectTypeName;
       };
 
-      if (allowConcreteGenericParams) {
+      if (shouldSuggestConcreteTypeFixit(subjectType, concreteTypePolicy)) {
         auto options = PrintOptions::forDiagnosticArguments();
         auto subjectTypeName = subjectType.getString(options);
         auto subjectTypeNameWithoutSelf = getNameWithoutSelf(subjectTypeName);
@@ -725,16 +740,25 @@ bool swift::rewriting::diagnoseRequirementErrors(
       auto requirement = error.requirement;
       auto conflict = error.conflictingRequirement;
 
+      if (requirement.getFirstType()->hasError() ||
+          (requirement.getKind() != RequirementKind::Layout &&
+           requirement.getSecondType()->hasError())) {
+        // Don't emit a cascading error.
+        break;
+      }
+
       if (!conflict) {
-        if (requirement.getFirstType()->hasError() ||
-            requirement.getSecondType()->hasError()) {
-          // Don't emit a cascading error.
-          break;
-        }
         ctx.Diags.diagnose(loc, diag::requires_same_concrete_type,
                            requirement.getFirstType(),
                            requirement.getSecondType());
       } else {
+        if (conflict->getFirstType()->hasError() ||
+            (conflict->getKind() != RequirementKind::Layout &&
+             conflict->getSecondType()->hasError())) {
+          // Don't emit a cascading error.
+          break;
+        }
+
         auto options = PrintOptions::forDiagnosticArguments();
         std::string requirements;
         llvm::raw_string_ostream OS(requirements);
@@ -754,6 +778,13 @@ bool swift::rewriting::diagnoseRequirementErrors(
 
     case RequirementError::Kind::RedundantRequirement: {
       auto requirement = error.requirement;
+      if (requirement.getFirstType()->hasError() ||
+          (requirement.getKind() != RequirementKind::Layout &&
+           requirement.getSecondType()->hasError())) {
+        // Don't emit a cascading error.
+        break;
+      }
+
       switch (requirement.getKind()) {
       case RequirementKind::SameType:
         ctx.Diags.diagnose(loc, diag::redundant_same_type_to_concrete,
@@ -886,7 +917,8 @@ StructuralRequirementsRequest::evaluate(Evaluator &evaluator,
 
   if (ctx.LangOpts.RequirementMachineProtocolSignatures ==
       RequirementMachineMode::Enabled) {
-    diagnoseRequirementErrors(ctx, errors, /*allowConcreteGenericParams=*/false);
+    diagnoseRequirementErrors(ctx, errors,
+                              AllowConcreteTypePolicy::NestedAssocTypes);
   }
 
   return ctx.AllocateCopy(result);
@@ -1159,7 +1191,8 @@ TypeAliasRequirementsRequest::evaluate(Evaluator &evaluator,
 
   if (ctx.LangOpts.RequirementMachineProtocolSignatures ==
       RequirementMachineMode::Enabled) {
-    diagnoseRequirementErrors(ctx, errors, /*allowConcreteGenericParams=*/false);
+    diagnoseRequirementErrors(ctx, errors,
+                              AllowConcreteTypePolicy::NestedAssocTypes);
   }
 
   return ctx.AllocateCopy(result);

lib/AST/RequirementMachine/RequirementLowering.h

@@ -55,9 +55,24 @@ void realizeInheritedRequirements(TypeDecl *decl, Type type,
                                   SmallVectorImpl<StructuralRequirement> &result,
                                   SmallVectorImpl<RequirementError> &errors);
 
+/// Policy for the fixit that transforms 'T : S' where 'S' is not a protocol
+/// or a class into 'T == S'.
+enum AllowConcreteTypePolicy {
+  /// Any type parameter can be concrete.
+  All,
+
+  /// Only associated types can be concrete.
+  AssocTypes,
+
+  /// Only nested associated types can be concrete. This is for protocols,
+  /// where we don't want to suggest making an associated type member of
+  /// 'Self' concrete.
+  NestedAssocTypes
+};
+
 bool diagnoseRequirementErrors(ASTContext &ctx,
                                ArrayRef<RequirementError> errors,
-                               bool allowConcreteGenericParams);
+                               AllowConcreteTypePolicy concreteTypePolicy);
 
 // Defined in ConcreteContraction.cpp.
 bool performConcreteContraction(

lib/AST/RequirementMachine/RequirementMachineRequests.cpp

@@ -412,7 +412,7 @@ RequirementSignatureRequestRQM::evaluate(Evaluator &evaluator,
       SmallVector<RequirementError, 4> errors;
       machine->computeRequirementDiagnostics(errors, proto->getLoc());
       diagnoseRequirementErrors(ctx, errors,
-                                /*allowConcreteGenericParams=*/false);
+                                AllowConcreteTypePolicy::NestedAssocTypes);
     }
 
     if (!machine->getErrors()) {
@@ -844,7 +844,10 @@ InferredGenericSignatureRequestRQM::evaluate(
         ctx.LangOpts.RequirementMachineInferredSignatures ==
         RequirementMachineMode::Enabled) {
       machine->computeRequirementDiagnostics(errors, loc);
-      diagnoseRequirementErrors(ctx, errors, allowConcreteGenericParams);
+      diagnoseRequirementErrors(ctx, errors,
+                                allowConcreteGenericParams
+                                ? AllowConcreteTypePolicy::All
+                                : AllowConcreteTypePolicy::AssocTypes);
     }
 
     // FIXME: Handle allowConcreteGenericParams

lib/AST/RequirementMachine/RewriteContext.cpp

@@ -177,6 +177,7 @@ static DebugOptions parseDebugFlags(StringRef debugFlags) {
       .Case("concrete-contraction", DebugFlags::ConcreteContraction)
       .Case("propagate-requirement-ids", DebugFlags::PropagateRequirementIDs)
       .Case("timers", DebugFlags::Timers)
+      .Case("conflicting-rules", DebugFlags::ConflictingRules)
       .Default(None);
     if (!flag) {
       llvm::errs() << "Unknown debug flag in -debug-requirement-machine "

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -608,6 +608,41 @@ void RewriteSystem::verifyRewriteRules(ValidityPolicy policy) const {
 #undef ASSERT_RULE
 }
 
+/// Determine whether this is a redundantly inheritable Objective-C protocol.
+///
+/// A redundantly-inheritable Objective-C protocol is one where we will
+/// silently accept a directly-stated redundant conformance to this protocol,
+/// and emit this protocol in the list of "inherited" protocols. There are
+/// two cases where we allow this:
+///
+//    1) For a protocol defined in Objective-C, so that we will match Clang's
+///      behavior, and
+///   2) For an @objc protocol defined in Swift that directly inherits from
+///      JavaScriptCore's JSExport, which depends on this behavior.
+static bool isRedundantlyInheritableObjCProtocol(const ProtocolDecl *inheritingProto,
+                                                 const ProtocolDecl *proto) {
+  if (!proto->isObjC()) return false;
+
+  // Check the two conditions in which we will suppress the diagnostic and
+  // emit the redundant inheritance.
+  if (!inheritingProto->hasClangNode() && !proto->getName().is("JSExport"))
+    return false;
+
+  // If the inheriting protocol already has @_restatedObjCConformance with
+  // this protocol, we're done.
+  for (auto *attr : inheritingProto->getAttrs()
+                      .getAttributes<RestatedObjCConformanceAttr>()) {
+    if (attr->Proto == proto) return true;
+  }
+
+  // Otherwise, add @_restatedObjCConformance.
+  auto &ctx = proto->getASTContext();
+  const_cast<ProtocolDecl *>(inheritingProto)
+      ->getAttrs().add(new (ctx) RestatedObjCConformanceAttr(
+          const_cast<ProtocolDecl *>(proto)));
+  return true;
+}
+
 /// Computes the set of explicit redundant requirements to
 /// emit warnings for in the source code.
 void RewriteSystem::computeRedundantRequirementDiagnostics(
@@ -689,8 +724,18 @@ void RewriteSystem::computeRedundantRequirementDiagnostics(
   auto isRedundantRule = [&](unsigned ruleID) {
     const auto &rule = getRules()[ruleID];
 
-    return (rule.isRedundant() &&
-            nonExplicitNonRedundantRules.count(ruleID) == 0);
+    if (!rule.isRedundant())
+      return false;
+
+    if (nonExplicitNonRedundantRules.count(ruleID) > 0)
+      return false;
+
+    if (rule.isProtocolRefinementRule(Context) &&
+        isRedundantlyInheritableObjCProtocol(rule.getLHS()[0].getProtocol(),
+                                             rule.getLHS()[1].getProtocol()))
+      return false;
+
+    return true;
   };
 
   // Finally walk through the written requirements, diagnosing any that are

lib/AST/RequirementMachine/RewriteSystem.h

@@ -361,8 +361,6 @@ class RewriteSystem final {
 
   void propagateRedundantRequirementIDs();
 
-  void processConflicts();
-
   using EliminationPredicate = llvm::function_ref<bool(unsigned loopID,
                                                        unsigned ruleID)>;
 

lib/AST/RequirementMachine/Rule.h

@@ -198,9 +198,11 @@ class Rule final {
   }
 
   void markConflicting() {
+    // It's okay to mark a rule as conflicting multiple times.
+    if (Conflicting)
+      return;
+
     assert(!Frozen);
-    // It's okay to mark a rule as conflicting multiple times, but it must not
-    // be a permanent rule.
     assert(!Permanent && "Permanent rule should not conflict with anything");
     Conflicting = true;
   }