Entry 2: SR-7177, Adding a diagnostic

In Entry 0 I pattern matched to add a missing constraint to a KeyPath subscript. In Entry 1 I improved an error message. Now it’s time to put combine my skills to really improve a diagnostic (a term that encompasses both errors and warnings).

This one was a bit of a ride.

Starting point

SR-7177 was filed because of a confusing diagnostic. At some point, that diagnostic improved. But it was still a bit confusing:

var a = [1,2,3,4]
var b = a.popFirst()

error: '[Int]' requires the types '[Int]' and 'ArraySlice<Int>' be equivalent to use 'popFirst'
var b = a.popFirst()
          ^

Uh… okay? Who does [Int] think it is? And why does it require [Int] and ArraySlice<Int> be equivalent? Those will never be equivalent. Where does this popFirst come from?

I was genuinely confused by this one. Looking up the documentation for popFirst() didn’t turn up anything useful, although I did find that it was defined on Collection. (I’m not the only one who can’t keep all the these protocols straight, right?)

But looking up the definition of popFirst() cleared things up:

extension Collection where Self == Self.SubSequence {
    /// Removes and returns the first element of the collection.
    ///
    /// - Returns: The first element of the collection if the collection is
    ///   not empty; otherwise, `nil`.
    ///
    /// - Complexity: O(1)
    public mutating func popFirst() -> Self.Element?
}

You can’t use popFirst on an Array because Array.SubSequence == ArraySlice. It sure would be nice if the diagnostic would say that!

Finding the diagnostic

The first step was to find the existing diagnostic. A quick search for be equivalent to use turned up the definition in DiagnosticsSema.def:

ERROR(types_not_equal_in_call,none,
      "%0 requires the types %1 and %2 be equivalent to use %3",
      (Type, Type, Type, DeclName))

Searching for that id, types_not_equal_in_call, pinpointed where this was called in CSDiag.cpp (the file where diagnostics are created from the constraint solver) inside a function called diagnoseTypeRequirementFailure.

This function is used to diagnose errors related to type requirements (duh!). In other words, errors related to where clauses.

Inside is this code, which chooses a diagnostic based on the type of failed constraint in the clause:

  switch (constraint->getKind()) {
  case ConstraintKind::ConformsTo:
    TC.diagnose(anchor->getLoc(), diag::type_does_not_conform_owner, ownerType,
                lhs, rhs);
    return true;

  case ConstraintKind::Subtype: // superclass
    TC.diagnose(anchor->getLoc(), diag::type_does_not_inherit, ownerType, lhs,
                rhs);
    return true;

  case ConstraintKind::Equal: { // same type
    if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor)) {
      TC.diagnose(UDE->getLoc(), diag::types_not_equal_in_call, ownerType, lhs,
                  rhs, UDE->getName());
    } else {
      TC.diagnose(anchor->getLoc(), diag::types_not_equal, ownerType, lhs, rhs);
    }
    return true;
  }

Since this is a == constraint, the last case is used. We can see that it already does a little pattern matching. If it’s an unresolved dot expression—something that uses a method or property or an unknown (hence unresolved) type—then the error includes the name of the method.

That matches what we saw:

error: '[Int]' requires the types '[Int]' and 'ArraySlice<Int>' be equivalent to use 'popFirst'
var b = a.popFirst()
          ^

What would a better error be?

I thought this would ideally match the definition as much as possible. Perhaps:

error: 'Collection' requires the types 'Self' ('[Int]') and 'Self.SubSequence' ('ArraySlice<Int>') be equivalent to use 'popFirst'
var b = a.popFirst()
          ^

And in many places, Swift will give you “note” that tells you where a method was declared. So ideally this would include a note like this too:

Swift.Collection:2:37: note: 'popFirst()' declared here
    @inlinable public mutating func popFirst() -> Self.Element?
                                    ^

Then I wouldn’t have had to hunt around for the definition. I (hopefully) could just click on it in Xcode.

Attempt #1

I started by trying to add the note, since I thought I could figure that out more easily. And since it’s always easiest to copy existing code, I tried to find the source of one of the existing declared here notes—mostly because I couldn’t remember the exact text.

Untitled.swift:1:5: error: value of type 'String' has no member 'coun'; did you mean 'count'?
_ = "".coun()
    ^~ ~~~~
       count
Swift.String:10:16: note: 'count' declared here
    public var count: Int { get }
               ^
Swift.Collection:5:16: note: 'count' declared here
    public var count: Int { get }
               ^

Bingo.

Searching back in DiagnosticsSema.def for declared here turned up a number of results. Most of them either lacked a format parameter or, based on the name, were clearly not what I was looking for. But decl_declared_here looked promising:

NOTE(decl_declared_here,none,
     "%0 declared here", (DeclName))

Searching turned up a lot of usages like this:

tc.diagnose(func, diag::decl_declared_here, func->getFullName());

So what was a DeclName and how could I get one?

Since the types_not_equal_in_call diagnostic that I was trying to improve also passed a DeclName, I tried copying that.

TC.diagnose(UDE->getLoc(), diag::decl_declared_here, UDE->getName());

But that printed a note that pointed to the line that was using popFirst, which was neither true nor helpful. 🙃

note: 'popFirst' declared here
var b = a.popFirst()
          ^

UDE is an UnresolvedDotExpr—a method call where the type wasn’t known. But I knew that the type was known. If it wasn’t, then we wouldn’t get an error about the type requirements.

At first, I thought I might be able to get this information from the type parameter constraint. But dumping the constraint system showed where that information resided:

Score: 0 0 0 0 0 0 0 0 0 0 0
  Fix-it applied, fixed expression was:
Type Variables:
  $T0 [lvalue allowed] as @lvalue [Int] @ locator@0x11d087600 [DeclRef@7177.swift:2:9]
  $T1 [lvalue allowed] as () -> Int? @ locator@0x11d087680 [UnresolvedDot@7177.swift:2:11 -> member]
  $T2 as [Int] @ locator@0x11d0876e0 [UnresolvedDot@7177.swift:2:11 -> member -> archetype 'Self']

Active Constraints:

Inactive Constraints:
Resolved overloads:
  selected overload set choice @lvalue [Int].popFirst: $T1 == () -> $T2.Element?
  selected overload set choice a: $T0 == @lvalue [Int]


Opened types:
    cs.dump()
  locator@0x11d087680 [UnresolvedDot@7177.swift:2:11 -> member] opens τ_0_0 -> $T2

Failed constraint:
  $T2 equal $T2.SubSequence [[locator@0x11d0877f8 [UnresolvedDot@7177.swift:2:11 -> member -> opened generic -> type parameter requirement #1]]];

The unresolved dot expression had a resolved overload in the constraint system. I just needed to figure out how to get it.

I searched in the dump method and found that that list was part of ConstraintSystem::resolvedOverloadSets. Searching for use of that turned up the ConstraintSystem::findResolvedMemberRef method, which seemed to do just what I wanted.

My first attempt to actually use it failed—the resolved member wasn’t found. I figured this was because I was passing in the wrong ConstraintLocator. The ConstraintLocator I was working with looked like this:

(lldb) e locator->dump(&cs)
locator@0x117187bf8 [UnresolvedDot@7177.swift:2:11 -> member -> opened generic -> type parameter requirement #1]

Each locator is made up of an _anchor—the expression it’s from—and a list of PathElements. Those elements show where the constraint came from.

Searching for other uses of findResolvedMemberRef showed that you can ask for a new ConstraintLocator with a specific type of path element:

  auto loc = cs.getConstraintLocator(UDE, ConstraintLocator::Member);
  auto *member = cs.findResolvedMemberRef(loc);

member here is a ValueDecl, which I could pass as the source of the note so that it appeared on the declaration of the method.

Now that I had the note in place, I had to figure out how to and whether I could show the types that I wanted. With my debugger parked on the existing diagnostic, I started exploring the types I had access to.

By jumping through the headers, I discovered that member had access to the name of the type it was defined on. That makes sense: Collection.popFirst should know that it’s defined on Collection.

(lldb) e member->getDeclContext()->getDeclaredInterfaceType()->dump()
(protocol_type decl=Swift.(file).Collection)

Now I could replace the first type in the diagnostic with that.

error: '[Int]' requires the types '[Int]' and 'ArraySlice<Int>' be equivalent to use 'popFirst'
var b = a.popFirst()
          ^

But I still wanted to show that this was from the Self == Self.SubSequence constraint. That constraint was available in the diagnostic method. Dumping the constraint’s types to the console gave me a clue of where to go next:

(lldb) e constraint->getFirstType()->dump()
(type_variable_type id=2)
(lldb) e constraint->getSecondType()->dump()
(dependent_member_type assoc_type=Swift.(file).Sequence.SubSequence
  (base=type_variable_type id=2))

The constraint was of the form $T2 == $T2.SubSequence because the solver was figuring out the concrete Collection type that it should use. I searched for dependent_member_type to see what that was and discovered DependentMemberType (a straightforward translation in retrospect).

DependentMemberType has a base type and either a name or associated type.

/// A type that refers to a member type of some type that is dependent on a
/// generic parameter.
class DependentMemberType : public TypeBase {
  Type Base;
  llvm::PointerUnion<Identifier, AssociatedTypeDecl *> NameOrAssocType;

  …
}

That seemed like what I’d want to include with the diagnostic. But I didn’t want to show $T2.SubSequence, I wanted to show [Int].SubSequence. So I created a new DependentMemberType that used the [Int] type I had with the .SubSequence from the existing DependentMemberType:

auto rhsType = constraint->getSecondType()->getAs<DependentMemberType>();
if (!rhsType)
  return false;
auto memberType = DependentMemberType::get(lhs, rhsType->getAssocType());

Adding a new diagnostic that included the extra type gave me a reasonable diagnostic.

error: 'Collection' requires the types '[Int]' and 'Array<Int>.SubSequence' ('ArraySlice<Int>') be equivalent to use 'popFirst'
var b = a.popFirst()
          ^
Swift.Collection:2:37: note: 'popFirst()' declared here
    @inlinable public mutating func popFirst() -> Self.Element?
                                    ^

I was pretty happy with that, so I opened a PR

Attempt #2

When I opened a PR with the above diagnostic, Pavel Yaskevich pointed out that Swift already had a similar diagnostic that would be clearer:

func countOf<C: Collection>(_ c: C) -> Int where C == C.SubSequence {
    return c.count
}
_ = countOf([1])

error: cannot invoke 'countOf(_:)' with an argument list of type '([Int])'
_ = countOf([1])
            ^
note: candidate requires that the types '[Int]' and 'ArraySlice<Int>' be equivalent (requirement specified as 'C' == 'C.SubSequence' [with C = [Int]])
func countOf<C: Collection>(_ c: C) -> Int where C == C.SubSequence {
     ^

So back I went to the code. I found where the diagnostic was generated and saw that it used a Requirement. That was a new type to me. It’s the AST-equivalent of the Constraint I used above—it represents C == C.SubSequence in the code above. From that Requirement, a Constraint is generated.

Unfortunately, I didn’t see how to work back from the Constraint to the Requirement. But Pavel very helpfully told me that I could get there from the ConstraintLocator.

auto path = locator->getPath();
if (path.empty())
  return false;

auto &last = path.back();
if (last.getKind() != ConstraintLocator::TypeParameterRequirement)
  return false;

auto req = member
  ->getAsGenericContext()
  ->getGenericSignature()
  ->getRequirements()[last.getValue()];

Remember the constraint locator I mentioned above?

(lldb) e locator->dump(&cs)
locator@0x117187bf8 [UnresolvedDot@7177.swift:2:11 -> member -> opened generic -> type parameter requirement #1]

The last item in the path is the type parameter requirement at index 1. By looking up the requirements for the generic signature of the popFirst method, I could use that index to get the requirement I was after.

Then I was able look up the diagnostic that Pavel pointed me to and use its code to create a similar diagnostic note here. And since the note pointed out that the candidate had an un-met requirement, I realized that the more general “has no member” error could be used.

error: value of type '[Int]' has no member 'popFirst'
var b = a.popFirst()
          ^
Swift.Collection:2:37: note: candidate requires that the types '[Int]' and 'ArraySlice<Int>' be equivalent (requirement specified as 'Self' == 'Self.SubSequence')
    @inlinable public mutating func popFirst() -> Self.Element?
                                    ^

I like that even more than my first error. Consistency between error messages is definitely helpful.

This attempt seems much easier—my description of it is at least much shorter—but it was actually more difficult for me to figure out. That’s in part because I used what I learned in the first attempt. But it also makes sense: I had to learn some new things to do things this way.

Detour: why did it spell out Array in my first attempt?

I was surprised to see that the error from my first attempt specified [Int] but Array<Int>.SubSequence instead of [Int].SubSequence, which I would have preferred since the connection is clearer.

error: 'Collection' requires the types '[Int]' and 'Array<Int>.SubSequence' ('ArraySlice<Int>') be equivalent to use 'popFirst'
var b = a.popFirst()
          ^

My first attempt to figure this out was to “Jump to Definition” in Xcode on the diagnose method. That didn’t lead anywhere useful since that method uses template magic.

So I resorted to searching for '.' and setting breakpoints. My search turned up a promising result in DiagnosticEngine.cpp, but the breakpoint wasn’t hit. Since I was pretty sure that DiagnosticEngine::emitDiagnostic was the correct method, I set a breakpoint at the beginning of the method.

Stepping through, nothing exciting happened until the end. I eventually realized that I should step into Consumer->handleDiagnostic since it took a list of arguments. I wasn’t sure whether the arguments would already be stringified at this point, so I kept stepping through.

This eventually lead me to the formatDiagnosticArgument function in DiagnosticEngine.cpp. That function switches on the type of argument. Since I knew I had a Type, I found the relevant case:

  case DiagnosticArgumentKind::Type: {
    assert(Modifier.empty() && "Improper modifier for Type argument");

    // Strip extraneous parentheses; they add no value.
    auto type = Arg.getAsType()->getWithoutParens();
    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());
    } else {
      Out << FormatOpts.OpeningQuotationMark << typeName
          << FormatOpts.ClosingQuotationMark;
    }
    break;
  }

type->getString() stood out. Jumping to its definition wasn’t very enlightening:

std::string TypeBase::getString(const PrintOptions &PO) const {
  std::string Result;
  llvm::raw_string_ostream OS(Result);
  print(OS, PO);
  return OS.str();
}

But jumping through a couple print functions like this eventually led me to this function:

void Type::print(ASTPrinter &Printer, const PrintOptions &PO) const {
  if (isNull())
    Printer << "<null>";
  else
    TypePrinter(Printer, PO).visit(*this);
}

TypePrinter definitely looked like it was what I needed. It had a series of visit* methods for different types. I knew I had a DependentMemberType, since I’d constructed it above, so I looked for that method.

  void visitDependentMemberType(DependentMemberType *T) {
    visitParentType(T->getBase());
    Printer << ".";
    Printer.printName(T->getName());
  }

The parent type was clearly what I was after.

  void visitParentType(Type T) {
    PrintOptions innerOptions = Options;
    innerOptions.SynthesizeSugarOnTypes = false;

    if (auto sugarType = dyn_cast<SyntaxSugarType>(T.getPointer()))
      T = sugarType->getImplementationType();

    TypePrinter(Printer, innerOptions).visit(T);
  }

SynthesizeSugarOnTypes definitely seemed like it would control this. [Int] is syntactic sugar for Array<Int>. Just to be sure, I set a debugger breakpoint after that line and ran e innerOptions.SynthesizeSugarOnTypes = true in the debugger. After continuing, I saw [Int].SubSequence just as I was hoping.

But since this was explicitly turned off, I decided to leave it for now. It seemed like a much larger and potentially unwanted change. So this ended up being an insightful, but otherwise unproductive, detour.

Finishing up

Since I wasn’t sure I could correctly find all the test cases that I’d need to change up front, I decided to let the computer do this for me. I ran ./utils/build-script --debug --test while I wrote most of this up. 😄

After the tests finished, I looked at all the failed tests. Once I’d made sure that the failure actually had the error that I’d want, I updated the test expectation to match.

You can see my PR here.