[swift-dev] RFC: Better diagnostics through simplification

[John McCall]

> On Aug 23, 2017, at 11:00 PM, David Zarzycki via swift-dev <swift-dev at swift.org> wrote:
> 
> 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.

I think that's an interesting improvement in this specific message, but it's not obvious that it would be an improvement to all messages.  Generally the way we did things like this in Clang was to add a modifier to the diagnostic string, so that it would look something like "%type1", which would be understood to expand to a noun appropriate for the type.  That way, it was easy to take advantage of it for a specific diagnostic, but you weren't forced into it by default.  I think that's still the right approach here.

A few comments on the nouns you've picked:

> +  case TypeKind::Function:             return "function ";
> +  case TypeKind::GenericFunction:      return "function ";

'function' would be misleading here, since it's actually referring to a function *type*.  Consider your example above: "class 'A' cannot subclass function '(Int) -> ()'".

> +  case TypeKind::ProtocolComposition:  return "protocol ";

"cannot convert from protocol 'A & B' to ..."?  Hmm.  Not sure about this one.

> +  case TypeKind::Module:               return "module ";

This has a lot of the same problems as "function", I think.

> +  case TypeKind::DynamicSelf:          return "dynamic type ";

This is almost certainly a situation where you're not going to do better than "type".

> +  case TypeKind::Unresolved:           return "unresolved 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 ";

If these ever do show up in diagnostics, I'm not sure a good noun will help.

> +  case TypeKind::WeakStorage:
> +  case TypeKind::UnownedStorage:
> +  case TypeKind::UnmanagedStorage:
> +    llvm_unreachable("Not a part of the user facing type system");

This seems inconsistent given the treatment for SILFunctionType.

> +  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 ";
> +  }

I'm not sure that being more specific about archetypes is actually helpful to users.

> +  case TypeKind::DependentMember: {
> +    while (auto depTy = dyn_cast<DependentMemberType>(type.getPointer()))
> +      type = depTy->getBase();
> +    return "dependent " + computeDiagnosticTypeKindString(type);
> +  }

This is definitely going to be more confusing than not.

> +  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);
> +  }

Interesting.  "metatype of optional struct" for Array?.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 ";

Same problem as "function".

> +  case TypeKind::Optional:
> +    type = type->getAnyOptionalObjectType();
> +    return "optional " + computeDiagnosticTypeKindString(type);

This is interesting but could get pretty verbose.

> +  case TypeKind::ImplicitlyUnwrappedOptional:
> +    type = type->getAnyOptionalObjectType();
> +    return "implicitly unwrapped optional "
> +      + computeDiagnosticTypeKindString(type);

This is *already* pretty verbose to throw into random diagnostics.

John.

> +  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);
> +  }
> +}
> +
>  /// \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;
> _______________________________________________
> swift-dev mailing list
> swift-dev at swift.org
> https://lists.swift.org/mailman/listinfo/swift-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20170824/c6a809c6/attachment.html>