Merge pull request swiftlang#39189 from slavapestov/misc-rqm-fixes-2

RequirementMachine: More miscellaneous fixes

Author: slavapestov

include/swift/AST/DiagnosticsCommon.def

@@ -209,13 +209,6 @@ REMARK(remark_save_cache, none,
 // MARK: custom attribute diagnostics
 //------------------------------------------------------------------------------
 
-ERROR(ambiguous_custom_attribute_ref,none,
-      "ambiguous use of attribute %0", (Identifier))
-NOTE(ambiguous_custom_attribute_ref_fix,none,
-     "use '%0.' to reference the attribute %1 in module %2",
-     (StringRef, Identifier, Identifier))
-NOTE(found_attribute_candidate,none,
-     "found this attribute", ())
 ERROR(unknown_attribute,none,
       "unknown attribute '%0'", (StringRef))
 

lib/AST/GenericSignature.cpp

@@ -23,6 +23,7 @@
 #include "swift/AST/Module.h"
 #include "swift/AST/PrettyStackTrace.h"
 #include "swift/AST/Types.h"
+#include "swift/Basic/SourceManager.h"
 #include "swift/Basic/STLExtras.h"
 #include "RequirementMachine/RequirementMachine.h"
 #include <functional>
@@ -1492,9 +1493,12 @@ int swift::compareAssociatedTypes(AssociatedTypeDecl *assocType1,
     return compareProtocols;
 
   // Error case: if we have two associated types with the same name in the
-  // same protocol, just tie-break based on address.
-  if (assocType1 != assocType2)
-    return assocType1 < assocType2 ? -1 : +1;
+  // same protocol, just tie-break based on source location.
+  if (assocType1 != assocType2) {
+    auto &ctx = assocType1->getASTContext();
+    return ctx.SourceMgr.isBeforeInBuffer(assocType1->getLoc(),
+                                          assocType2->getLoc()) ? -1 : +1;
+  }
 
   return 0;
 }

lib/AST/GenericSignatureBuilder.cpp

@@ -4012,7 +4012,7 @@ ConstraintResult GenericSignatureBuilder::expandConformanceRequirement(
     // Add all of the inherited protocol requirements, recursively.
     auto inheritedReqResult =
       addInheritedRequirements(proto, selfType.getUnresolvedType(), source,
-                               proto->getModuleContext());
+                               nullptr);
     if (isErrorResult(inheritedReqResult))
       return inheritedReqResult;
   }

lib/AST/NameLookup.cpp

@@ -2105,11 +2105,22 @@ directReferencesForUnqualifiedTypeLookup(DeclNameRef name,
   auto descriptor = UnqualifiedLookupDescriptor(name, dc, loc, options);
   auto lookup = evaluateOrDefault(ctx.evaluator,
                                   UnqualifiedLookupRequest{descriptor}, {});
+
+  unsigned nominalTypeDeclCount = 0;
   for (const auto &result : lookup.allResults()) {
     auto typeDecl = cast<TypeDecl>(result.getValueDecl());
+
+    if (isa<NominalTypeDecl>(typeDecl))
+      nominalTypeDeclCount++;
+
     results.push_back(typeDecl);
   }
 
+  // If we saw multiple nominal type declarations with the same name,
+  // the result of the lookup is definitely ambiguous.
+  if (nominalTypeDeclCount > 1)
+    results.clear();
+
   return results;
 }
 
@@ -2607,54 +2618,6 @@ CustomAttrNominalRequest::evaluate(Evaluator &evaluator,
     }
   }
 
-  // If we have more than one attribute declaration, we have an ambiguity.
-  // So, emit an ambiguity diagnostic.
-  if (auto typeRepr = attr->getTypeRepr()) {
-    if (nominals.size() > 1) {
-      SmallVector<NominalTypeDecl *, 4> ambiguousCandidates;
-      // Filter out declarations that cannot be attributes.
-      for (auto decl : nominals) {
-        if (isa<ProtocolDecl>(decl)) {
-          continue;
-        }
-        ambiguousCandidates.push_back(decl);
-      }
-      if (ambiguousCandidates.size() > 1) {
-        auto attrName = nominals.front()->getName();
-        ctx.Diags.diagnose(typeRepr->getLoc(),
-                           diag::ambiguous_custom_attribute_ref, attrName);
-        for (auto candidate : ambiguousCandidates) {
-          ctx.Diags.diagnose(candidate->getLoc(),
-                             diag::found_attribute_candidate);
-          // If the candidate is a top-level attribute, let's suggest
-          // adding module name to resolve the ambiguity.
-          if (candidate->getDeclContext()->isModuleScopeContext()) {
-            auto moduleName = candidate->getParentModule()->getName();
-            ctx.Diags
-                .diagnose(typeRepr->getLoc(),
-                          diag::ambiguous_custom_attribute_ref_fix,
-                          moduleName.str(), attrName, moduleName)
-                .fixItInsert(typeRepr->getLoc(), moduleName.str().str() + ".");
-          }
-        }
-        return nullptr;
-      }
-    }
-  }
-
-  // There is no nominal type with this name, so complain about this being
-  // an unknown attribute.
-  std::string typeName;
-  if (auto typeRepr = attr->getTypeRepr()) {
-    llvm::raw_string_ostream out(typeName);
-    typeRepr->print(out);
-  } else {
-    typeName = attr->getType().getString();
-  }
-
-  ctx.Diags.diagnose(attr->getLocation(), diag::unknown_attribute, typeName);
-  attr->setInvalid();
-
   return nullptr;
 }
 

lib/Sema/TypeCheckAttr.cpp

@@ -2955,7 +2955,30 @@ void AttributeChecker::visitCustomAttr(CustomAttr *attr) {
   auto nominal = evaluateOrDefault(
     Ctx.evaluator, CustomAttrNominalRequest{attr, dc}, nullptr);
 
+  // Diagnose errors.
   if (!nominal) {
+    auto typeRepr = attr->getTypeRepr();
+
+    auto type = TypeResolution::forInterface(dc, TypeResolverContext::CustomAttr,
+                                             // Unbound generics and placeholders
+                                             // are not allowed within this
+                                             // attribute.
+                                             /*unboundTyOpener*/ nullptr,
+                                             /*placeholderHandler*/ nullptr)
+        .resolveType(typeRepr);
+
+    if (type->is<ErrorType>()) {
+      // Type resolution has failed, and we should have diagnosed something already.
+      assert(Ctx.hadError());
+    } else {
+      // Otherwise, something odd happened.
+      std::string typeName;
+      llvm::raw_string_ostream out(typeName);
+      typeRepr->print(out);
+
+      Ctx.Diags.diagnose(attr->getLocation(), diag::unknown_attribute, typeName);
+    }
+
     attr->setInvalid();
     return;
   }

lib/Sema/TypeCheckType.cpp

@@ -1080,6 +1080,14 @@ static Type diagnoseUnknownType(TypeResolution resolution,
 
   // Unqualified lookup case.
   if (parentType.isNull()) {
+    // Tailored diagnostic for custom attributes.
+    if (resolution.getOptions().is(TypeResolverContext::CustomAttr)) {
+      diags.diagnose(comp->getNameLoc(), diag::unknown_attribute,
+                     comp->getNameRef().getBaseIdentifier().str());
+
+      return ErrorType::get(ctx);
+    }
+
     if (comp->getNameRef().isSimpleName(ctx.Id_Self) &&
         !isa<GenericIdentTypeRepr>(comp)) {
       DeclContext *nominalDC = nullptr;
@@ -1642,6 +1650,15 @@ resolveIdentTypeComponent(TypeResolution resolution,
       !options.is(TypeResolverContext::TypeAliasDecl)) {
 
     if (!options.contains(TypeResolutionFlags::SilenceErrors)) {
+      // Tailored diagnostic for custom attributes.
+      if (options.is(TypeResolverContext::CustomAttr)) {
+        auto &ctx = resolution.getASTContext();
+        ctx.Diags.diagnose(lastComp->getNameLoc(), diag::unknown_attribute,
+                           lastComp->getNameRef().getBaseIdentifier().str());
+
+        return ErrorType::get(ctx);
+      }
+
       diagnoseUnboundGenericType(result,
                                  lastComp->getNameLoc().getBaseNameLoc());
     }
@@ -3605,6 +3622,7 @@ NeverNullType TypeResolver::resolveImplicitlyUnwrappedOptionalType(
   case TypeResolverContext::AbstractFunctionDecl:
   case TypeResolverContext::ClosureExpr:
   case TypeResolverContext::Inherited:
+  case TypeResolverContext::CustomAttr:
     doDiag = true;
     break;
   }

lib/Sema/TypeCheckType.h

@@ -133,6 +133,9 @@ enum class TypeResolverContext : uint8_t {
 
   /// Whether this is an "inherited" type.
   Inherited,
+
+  /// Whether this is a custom attribute.
+  CustomAttr
 };
 
 /// Options that determine how type resolution should work.
@@ -215,6 +218,7 @@ class TypeResolutionOptions {
     case Context::ImmediateOptionalTypeArgument:
     case Context::AbstractFunctionDecl:
     case Context::Inherited:
+    case Context::CustomAttr:
       return false;
     }
     llvm_unreachable("unhandled kind");

test/Generics/ambiguous_protocol_inheritance.swift

@@ -0,0 +1,15 @@
+// RUN: %target-typecheck-verify-swift -requirement-machine=verify
+
+protocol Base { // expected-note {{'Base' previously declared here}}
+// expected-note@-1 {{found this candidate}}
+  associatedtype E
+}
+
+struct Base<T> {} // expected-error {{invalid redeclaration of 'Base'}}
+// expected-note@-1 {{found this candidate}}
+
+protocol Derived : Base { // expected-error {{'Base' is ambiguous for type lookup in this context}}
+  associatedtype E
+}
+
+func foo<T : Derived>(_: T.E, _: T) {}

test/Generics/requirement_inference.swift

@@ -508,13 +508,15 @@ extension X1WithP2MoreArgs {
   }
 }
 
-// Inference from protocol inheritance clauses.
+// Inference from protocol inheritance clauses is not allowed.
 typealias ExistentialP4WithP2Assoc<T: P4> = P4 where T.P4Assoc : P2
 
 protocol P37 : ExistentialP4WithP2Assoc<Self> { }
+// expected-error@-1 {{type 'Self.P4Assoc' does not conform to protocol 'P2'}}
 
 extension P37 {
   func f() {
-    _ = X5<P4Assoc>() // requires P2
+    _ = X5<P4Assoc>()
+    // expected-error@-1 {{type 'Self.P4Assoc' does not conform to protocol 'P2'}}
   }
 }

test/NameLookup/custom_attrs_ambiguous.swift

@@ -9,13 +9,9 @@ import custom_attrs_B
 
 struct Test {
   @Wrapper var x: Int = 17
-  // expected-error@-1 {{ambiguous use of attribute 'Wrapper'}}
-  // expected-note@-2 {{use 'custom_attrs_A.' to reference the attribute 'Wrapper' in module 'custom_attrs_A'}} {{4-4=custom_attrs_A.}}
-  // expected-note@-3 {{use 'custom_attrs_B.' to reference the attribute 'Wrapper' in module 'custom_attrs_B'}} {{4-4=custom_attrs_B.}}
+  // expected-error@-1 {{'Wrapper' is ambiguous for type lookup in this context}}
 
   init(@Builder closure: () -> Int) {}
-  // expected-error@-1 {{ambiguous use of attribute 'Builder'}}
-  // expected-note@-2 {{use 'custom_attrs_A.' to reference the attribute 'Builder' in module 'custom_attrs_A'}} {{9-9=custom_attrs_A.}}
-  // expected-note@-3 {{use 'custom_attrs_B.' to reference the attribute 'Builder' in module 'custom_attrs_B'}} {{9-9=custom_attrs_B.}}
+  // expected-error@-1 {{'Builder' is ambiguous for type lookup in this context}}
 }