Matt Diephouse

Entry 6: Unexpected Arguments Diagnostic

24 July 2018

As promised, I returned to SR-4270. This code used to return a really terrible diagnostic:

let x: Bool = true
enum A {
    case a
    case b
}
func f(_ a: A) {}
let a: A = .a
f(x ? .a(a) : .b)

In Swift 4.2, it returned a much improved diagnostic:

error: member 'a' takes no arguments
f(x ? .a(a) : .b)
       ^~~~

But Jordan Rose suggested that it would be better if it explicitly mentioned enums.

I followed my usual approach for improving diagnostics:

  1. Search DiagnosticsSema.def for the ID of the diagnostic I want to improve
  2. Search the code for that ID
  3. Actually improve the code there

I quickly found the relevant code in CSDiag.cpp:

case CC_GeneralMismatch: { // Something else is wrong.
  // If an argument value was specified, but this member expects no
  // arguments,
  // then we fail with a nice error message.
  if (!candidateArgTy) {
    if (CS.getType(argExpr)->isVoid()) {
      diagnose(E->getNameLoc(),
               diag::unexpected_parens_in_contextual_member, E->getName())
          .fixItRemove(E->getArgument()->getSourceRange());
    } else {
      diagnose(E->getNameLoc(),
               diag::unexpected_argument_in_contextual_member, E->getName())
          .highlight(E->getArgument()->getSourceRange());
    }
    return true;
  }

  return false;
}

This code used 2 diagnostics:

ERROR(unexpected_argument_in_contextual_member,none,
      "member %0 takes no arguments", (DeclName))
ERROR(unexpected_parens_in_contextual_member,none,
      "member %0 is not a function", (DeclName))

Pretty straightforward. If arguments were given, but none were expected, then the compiler either (1) suggests that you remove the parens if they’re empty or (2) points out that it doesn’t take any arguments.

The hardest part of improving diagnostics is always deciding what the improvement should actually be.

Attempt 1

My first thought was to add a check for an enum and add separate diagnostics in that case. I wasn’t sure how to do that, but I started looking through the rest of the method I was in.

I found that several other diagnostics used CS.getContextualType()—including a test that checked what kind of type it was, CS.getContextualType()->is<UnboundGenericType>(). (CS is commonly used as an abbreviation for constraint system within the compiler.)

The contextual type looked like the right thing and it made sense—this was a contextual member lookup. In other words, the compiler had .foo, a member foo, and had to look up what it referred to by using the context—the type wasn’t explicitly given.

So I added 2 new diagnostics:

ERROR(unexpected_argument_in_enum_case,none,
      "enum case %0 takes no arguments", (DeclName))
ERROR(unexpected_parens_in_enum_case,none,
      "enum case %0 is not a function", (DeclName))

And added code that used them:

if (CS.getContextualType()->is<EnumType>()) {
    if (CS.getType(argExpr)->isVoid()) {
      diagnose(E->getNameLoc(),
               diag::unexpected_parens_in_enum_case, E->getName())
          .fixItRemove(E->getArgument()->getSourceRange());
    } else {
      diagnose(E->getNameLoc(),
               diag::unexpected_argument_in_enum_case, E->getName())
          .highlight(E->getArgument()->getSourceRange());
    }
} else {
    if (CS.getType(argExpr)->isVoid()) {
      diagnose(E->getNameLoc(),
               diag::unexpected_parens_in_contextual_member, E->getName())
          .fixItRemove(E->getArgument()->getSourceRange());
    } else {
      diagnose(E->getNameLoc(),
               diag::unexpected_argument_in_contextual_member, E->getName())
          .highlight(E->getArgument()->getSourceRange());
    }
}

That worked!

error: enum case 'a' takes no arguments
f(x ? .a(a) : .b)
       ^~~~

But the code seemed awfully repetitive, so I wasn’t happy with it.

Attempt 2

In Entry 5, I discovered that DescriptiveDeclKinds can be passed to a diagnostic to print a description of the kind of declaration. (I had changed the description of “enum elements” to say “enum case”.) I thought that I might be able to use that here—if I could figure out how to get a DescriptiveDeclKind.

First I updated the original diagnostics:

ERROR(unexpected_argument_in_contextual_member,none,
      "%0 %1 takes no arguments", (DescriptiveDeclKind, DeclName))
ERROR(unexpected_parens_in_contextual_member,none,
      "%0 %1 is not a function", (DescriptiveDeclKind, DeclName))

I knew that I’d have to get this from the Decl because of the method I’d updated previously:

DescriptiveDeclKind Decl::getDescriptiveKind() const

A Decl is a declaration. It refers to the declaration of anything in Swift—a function, a type, an enum case, etc. In this case, the compiler was trying to pick the right declaration. Looking through the method, I found that there would only be one candidate at this point:

// Dump all of our viable candidates into a CalleeCandidateInfo & sort it
// out.
CalleeCandidateInfo candidateInfo(Type(), candidates, hasTrailingClosure,
                                  CS);

// Filter the candidate list based on the argument we may or may not have.
candidateInfo.filterContextualMemberList(E->getArgument());

// If we have multiple candidates, then we have an ambiguity.
if (candidateInfo.size() != 1) {
  SourceRange argRange;
  if (auto arg = E->getArgument())
    argRange = arg->getSourceRange();
  diagnose(E->getNameLoc(), diag::ambiguous_member_overload_set,
           E->getName())
      .highlight(argRange);
  candidateInfo.suggestPotentialOverloads(E->getNameLoc().getBaseNameLoc());
  return true;
}

So I looked through the types to find that CalleeCandidateInfo had a list of UncurriedCandidates (I knew there’d only be 1 at this point) which had a getDecl() method. Bingo!

So now I could get the DescriptiveDeclKind and pass it to my diagnostic:

auto kind = candidateInfo[0].getDecl()->getDescriptiveKind();
if (CS.getType(argExpr)->isVoid()) {
  diagnose(E->getNameLoc(), diag::unexpected_parens_in_contextual_member,
           kind,
           E->getName())
       .fixItRemove(E->getArgument()->getSourceRange());
} else {
  diagnose(E->getNameLoc(), diag::unexpected_argument_in_contextual_member,
           kind, E->getName())
       .highlight(E->getArgument()->getSourceRange());
}

That also worked, but I realized that the diagnostic wasn’t great as it could be—especially if the parens were empty.

error: enum case 'a' takes no arguments
f(x ? .a(a) : .b)
       ^~~~

But I ran the test suite. I wanted to see (1) where these enum diagnostics were tested and (2) what other types of declarations would have this error. I found some enum tests in test/decl/enum/enumtest.swift, but to my surprise (horror?) I found that there weren’t any other test cases.

I thought about it and figured I could get a similar error by adding a static var:

struct A {
  static var a: A { return A() }
}
let _: A = .a()
let _: A = .a(1)

This yielded these errors:

error: static var 'a' takes no arguments
let _: A = .a()
             ^~~~

error: static var 'a' is not a function
let _: A = .a(1)
             ^~~~

The second error made sense to me, but the first didn’t. Of course a static var takes no arguments. It’s not a function.

Attempt 3

This led me to my final attempt.

I decided that there should be just 2 diagnostics:

ERROR(unexpected_arguments_in_enum_case,none,
      "enum case %0 has no associated values", (DeclName))
ERROR(unexpected_arguments_in_contextual_member,none,
      "%0 %1 is not a function", (DescriptiveDeclKind, DeclName))

The distinction between having arguments or no arguments wasn’t important for the actual message—it only mattered for whether the compiler should suggest a fixit to just remove the empty (). If the type was an enum, I wanted a message about associated values; otherwise I wanted a message about not being a function.

So I updated the code I’d written to use these:

bool isEnum = CS.getContextualType()->is<EnumType>();
bool isVoid = CS.getType(argExpr)->isVoid();
auto argumentRange = E->getArgument()->getSourceRange();
if (isEnum) {
  if (isVoid) {
    diagnose(E->getNameLoc(), diag::unexpected_arguments_in_enum_case,
             E->getName())
        .fixItRemove(argumentRange);
  } else {
    diagnose(E->getNameLoc(), diag::unexpected_arguments_in_enum_case,
             E->getName())
        .highlight(argumentRange);
  }
} else {
  auto kind = candidateInfo[0].getDecl()->getDescriptiveKind();
  if (isVoid) {
    diagnose(E->getNameLoc(),
             diag::unexpected_arguments_in_contextual_member, kind,
             E->getName())
        .fixItRemove(argumentRange);
  } else {
    diagnose(E->getNameLoc(),
             diag::unexpected_arguments_in_contextual_member, kind,
             E->getName())
        .highlight(argumentRange);
  }
}

This seemed good. It provided the diagnostics I wanted. I ran the test suite and all tests passed. So I set to work adding some tests for the static var case.

I looked through the other diagnostics in this case to find where they were tested. Then I’d have a good spot to add my additional tests. I found some code in test/Constraints/diagnostics.swift and updated it with my test:

enum Color {
  case Red
  …
  static var svar: Color { return .Red }
}

…
var someColor : Color
…
someColor = .svar() // expected-error {{static var 'svar' is not a function}}
someColor = .svar(1) // expected-error {{static var 'svar' is not a function}}

I reran these tests, expecting to see them pass. But they didn’t.

test/Constraints/diagnostics.swift:456:41: error: incorrect message found
someColor = .svar() // expected-error {{static var 'svar' is not a function}}
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                        enum case 'svar' has no associated values
test/Constraints/diagnostics.swift:457:42: error: incorrect message found
someColor = .svar(1) // expected-error {{static var 'svar' is not a function}}
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                         enum case 'svar' has no associated values

This was a lucky find. I had been testing with a static var on a struct. The test I added used a static var on an enum. That meant that the contextual type was an enum, so I was using the enum diagnostic. But that was wrong: the contextual type here was an enum, but the member was a static var.

Instead, I should have been testing whether the declaration was an enum element. Luckily, I already had the DescriptiveDeclKind. So this was an easy fix.

auto kind = candidateInfo[0].getDecl()->getDescriptiveKind();
bool isVoid = CS.getType(argExpr)->isVoid();
auto argumentRange = E->getArgument()->getSourceRange();
if (kind == DescriptiveDeclKind::EnumElement) {
  if (isVoid) {
    diagnose(E->getNameLoc(), diag::unexpected_arguments_in_enum_case,
             E->getName())
        .fixItRemove(argumentRange);
  } else {
    diagnose(E->getNameLoc(), diag::unexpected_arguments_in_enum_case,
             E->getName())
        .highlight(argumentRange);
  }
} else {
  if (isVoid) {
    diagnose(E->getNameLoc(),
             diag::unexpected_arguments_in_contextual_member, kind,
             E->getName())
        .fixItRemove(argumentRange);
  } else {
    diagnose(E->getNameLoc(),
             diag::unexpected_arguments_in_contextual_member, kind,
             E->getName())
        .highlight(argumentRange);
  }
}

I tested again to verify that it worked as expected. Then I was able open my PR. Diagnostic improved!

I still have a lot to learn about the Swift compiler, but I’m starting to feel comfortable making improvements to diagnostics. That’s pretty exciting to me: I feel passionate about improving these.

If you’re interested in contributing to Swift, please try improving a diagnostic! Hopefully what I’ve written on this blog can help. But if you’re stuck, feel free to reach out to me or join the (very unofficial) Swift Contributors Slack.