Sema: Diagnose invalid CustomAttrs using type resolution

Now that directReferencesForTypeRepr() no longer returns
ambiguous results, move the special ambiguity diagnostic out
of CustomAttrNominalRequest::evaluate(), and instead
diagnose these situations in TypeCheckAttr.cpp.

Special-case the 'unknown type' diagnostic though, to report
'unknown attribute' in the common case where the name lookup
failed.

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/NameLookup.cpp

@@ -2618,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/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}}
 }