[swift-dev] RFC: Better diagnostics through simplification

[David Zarzycki]

Hello,

I’d like to propose improving the quality, consistency, and future preparedness of diagnostics through simplification. Right now, a lot of diagnostics output tends to look like this (for example):

	type ‘Foo’ cannot subclass type ‘Bar’

Instead of hard coding the word “type” into the message, I’d like to change the diagnostics engine to dynamically introspect the types and print out the type kind instead. Therefore, after this change, the message output would look like this:

	class ‘Foo’ cannot subclass struct ‘Bar’

Which is clearly more better. And it requires less work and specialization on behalf of diagnostics clients because the messages within the compiler “defs” files would change from:

	“type %0 cannot subclass type %1”

To:

	“%0 cannot subclass %1”

Which is obviously simpler, better, and more future proof. In particular, clients of the diagnostics engine would no longer need to haphazardly customize messages to acknowledge optionals, metatypes, etc as they do today. The engine itself would deal with that.

What do people think? The meat of the change is tiny and below. The vast majority of the work is the long and painful fallout in the test suite, which I’m cautiously (and perhaps foolishly) stepping up to doing.

Cheers,
Dave




diff --git c/lib/AST/DiagnosticEngine.cpp i/lib/AST/DiagnosticEngine.cpp
index c99833d373..32ba196f04 100644
--- c/lib/AST/DiagnosticEngine.cpp
+++ i/lib/AST/DiagnosticEngine.cpp
@@ -373,6 +373,121 @@ static bool shouldShowAKA(Type type, StringRef typeName) {
   return true;
 }
 
+static std::string computeDiagnosticTypeKindString(Type &type) {
+  switch (type->getKind()) {
+#define TYPE(id, parent)
+#define BUILTIN_TYPE(id, parent) \
+  case TypeKind::id: return "builtin ";
+#include <swift/AST/TypeNodes.def>
+  case TypeKind::InOut:
+    type = type->getInOutObjectType();
+    return "inout " + computeDiagnosticTypeKindString(type);
+  case TypeKind::Function:             return "function ";
+  case TypeKind::GenericFunction:      return "function ";
+  case TypeKind::Struct:               return "struct ";
+  case TypeKind::BoundGenericStruct:   return "struct ";
+  case TypeKind::Class:                return "class ";
+  case TypeKind::BoundGenericClass:    return "class ";
+  case TypeKind::Enum:                 return "enum ";
+  case TypeKind::BoundGenericEnum:     return "enum ";
+  case TypeKind::ProtocolComposition:  return "protocol ";
+  case TypeKind::Protocol:             return "protocol ";
+  case TypeKind::Module:               return "module ";
+  case TypeKind::Unresolved:           return "unresolved type ";
+  case TypeKind::DynamicSelf:          return "dynamic type ";
+  case TypeKind::TypeVariable:         return "type-variable ";
+  case TypeKind::SILFunction:          return "SIL function ";
+  case TypeKind::SILBlockStorage:      return "SIL block storage ";
+  case TypeKind::SILBox:               return "SIL box ";
+  case TypeKind::GenericTypeParam:     return "generic parameter ";
+  case TypeKind::Archetype: {
+    auto archetypeTy = dyn_cast<ArchetypeType>(type.getPointer());
+    if (auto openedTy = archetypeTy->getOpenedExistentialType()) {
+      type = openedTy;
+      return computeDiagnosticTypeKindString(type);
+    }
+    if (archetypeTy->getAssocType())
+      return "associated type ";
+    return "generic parameter ";
+  }
+  case TypeKind::DependentMember: {
+    while (auto depTy = dyn_cast<DependentMemberType>(type.getPointer()))
+      type = depTy->getBase();
+    return "dependent " + computeDiagnosticTypeKindString(type);
+  }
+  case TypeKind::Metatype: {
+    auto metaTy = cast<MetatypeType>(type.getPointer());
+    type = metaTy->getInstanceType();
+    if (!type->isExistentialType())
+      return "metatype of " + computeDiagnosticTypeKindString(type);
+    return "concrete metatype of " + computeDiagnosticTypeKindString(type);
+  }
+  case TypeKind::ExistentialMetatype: {
+    auto metaTy = cast<ExistentialMetatypeType>(type.getPointer());
+    type = metaTy->getInstanceType();
+    return "metatype of " + computeDiagnosticTypeKindString(type);
+  }
+  case TypeKind::ArraySlice:
+    // we could in dig further, but then diag messages might become unwieldy
+    return "array slice ";
+  case TypeKind::Dictionary:
+    // we could in dig further, but then diag messages would become unwieldy
+    return "dictionary ";
+  case TypeKind::Optional:
+    type = type->getAnyOptionalObjectType();
+    return "optional " + computeDiagnosticTypeKindString(type);
+  case TypeKind::ImplicitlyUnwrappedOptional:
+    type = type->getAnyOptionalObjectType();
+    return "implicitly unwrapped optional "
+      + computeDiagnosticTypeKindString(type);
+  case TypeKind::Paren:
+    type = type->getWithoutParens();
+    return computeDiagnosticTypeKindString(type);
+  case TypeKind::Tuple: {
+    auto tempTy = type->getWithoutImmediateLabel();
+    if (tempTy->isEqual(type))
+      return "tuple ";
+    type = tempTy;
+    return computeDiagnosticTypeKindString(type);
+  }
+  case TypeKind::NameAlias: {
+    // We need to stop updating our caller's 'type' at this point.
+    auto aliasTy = cast<NameAliasType>(type.getPointer());
+    Type discardTy = aliasTy->getSinglyDesugaredType();
+    return computeDiagnosticTypeKindString(discardTy);
+  }
+  case TypeKind::Error: {
+    auto errorTy = cast<ErrorType>(type.getPointer());
+    if (auto origTy = errorTy->getOriginalType()) {
+      type = origTy;
+      return "invalid " + computeDiagnosticTypeKindString(type);
+    }
+    return "invalid type ";
+  }
+  case TypeKind::UnboundGeneric:
+    switch (type->castTo<UnboundGenericType>()->getDecl()->getKind()) {
+    case DeclKind::TypeAlias: return "typealias ";
+    case DeclKind::Protocol:  return "protocol ";
+    case DeclKind::Struct:    return "struct ";
+    case DeclKind::Class:     return "class ";
+    case DeclKind::Enum:      return "enum ";
+    default:                  break;
+    }
+    llvm_unreachable("Unknown unbound generic");
+  }
+  case TypeKind::LValue: {
+    // While LValues are not a part of the user visible type system, they do
+    // sometimes appear inside of tuple expressions. Just ignore them.
+    auto lvTy = cast<LValueType>(type.getPointer());
+    type = lvTy->getObjectType();
+    return computeDiagnosticTypeKindString(type);
+  }
+  case TypeKind::WeakStorage:
+  case TypeKind::UnownedStorage:
+  case TypeKind::UnmanagedStorage:
+    llvm_unreachable("Not a part of the user facing type system");
+}
+
 /// \brief Format a single diagnostic argument and write it to the given
 /// stream.
 static void formatDiagnosticArgument(StringRef Modifier, 
@@ -439,16 +554,18 @@ static void formatDiagnosticArgument(StringRef Modifier,
     
     // Strip extraneous parentheses; they add no value.
     auto type = Arg.getAsType()->getWithoutParens();
+    auto origType = type->getInOutObjectType();
+    std::string declKind = computeDiagnosticTypeKindString(type);
     std::string typeName = type->getString();
 
     if (shouldShowAKA(type, typeName)) {
       llvm::SmallString<256> AkaText;
       llvm::raw_svector_ostream OutAka(AkaText);
-      OutAka << type->getCanonicalType();
-      Out << llvm::format(FormatOpts.AKAFormatString.c_str(), typeName.c_str(),
-                          AkaText.c_str());
+      OutAka << origType->getCanonicalType();
+      Out << declKind << llvm::format(FormatOpts.AKAFormatString.c_str(),
+          typeName.c_str(), AkaText.c_str());
     } else {
-      Out << FormatOpts.OpeningQuotationMark << typeName
+      Out << declKind << FormatOpts.OpeningQuotationMark << typeName
           << FormatOpts.ClosingQuotationMark;
     }
     break;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20170823/eeb4b30f/attachment.html>