Improve template constraint error messages

[dayllenger]

Hello. I suppose, everyone agrees that the output on unsatisfied template constraints is very poor. This PR proposes a simple way to tell actually what's gone wrong on template matching.

Related to issue 9626.

Example

This code

import std.algorithm : sort;

char[] arr = ['a', 'b'];
sort(arr);

gives such error message:

./scribble.d(8): Error: template std.algorithm.sorting.sort cannot deduce function from argument types !()(char[]), candidates are:
/usr/include/dmd/phobos/std/algorithm/sorting.d(1855):        std.algorithm.sorting.sort(alias less = "a < b", SwapStrategy ss = SwapStrategy.unstable, Range)(Range r) if ((ss == SwapStrategy.unstable && (hasSwappableElements!Range || hasAssignableElements!Range) || ss != SwapStrategy.unstable && hasAssignableElements!Range) && isRandomAccessRange!Range && hasSlicing!Range && hasLength!Range)

"WHAT? Char array cannot be sorted?"

Now let's see how it will look with this PR:

./scribble.d(8): Error: template std.algorithm.sorting.sort cannot deduce function from argument types !()(char[]), candidates are:
/usr/include/dmd/phobos/std/algorithm/sorting.d(1855):        sort(alias less = "a < b", SwapStrategy ss = SwapStrategy.unstable, Range)(Range r),
  whose parameters have the following constraints:
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        ss == SwapStrategy.unstable
          > hasSwappableElements!Range
            or:
          > hasAssignableElements!Range
        or:
      > ss != SwapStrategy.unstable
      - hasAssignableElements!Range
  - isRandomAccessRange!Range
  - hasSlicing!Range
  - hasLength!Range
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  Tip: not satisfied constraints are marked with >

The output is structured like a tree. It shares exact information about which clauses have failed. Here we can read that our char[]does not have swappable and/or assignable elements, which is true, because it's UTF-8 string. Not obvious though, so thinking from the user perspective, we should move the last three checks to the beginning to improve the message.

Another (artificial) example:

import std.algorithm : countUntil, initializeAll;

int[] arr;
void* i;
countUntil(arr, i);
initializeAll(i);

Output:

./scribble.d(16): Error: template std.algorithm.searching.countUntil cannot deduce function from argument types !()(int[], void*), candidates are:
/usr/include/dmd/phobos/std/algorithm/searching.d(768):        countUntil(alias pred = "a == b", R, Rs...)(R haystack, Rs needles),
  whose parameters have the following constraints:
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    isForwardRange!R
    Rs.length > 0
    isForwardRange!(Rs[0]) == isInputRange!(Rs[0])
  > is(typeof(startsWith!pred(haystack, needles[0])))
      - Rs.length == 1
        or:
      - is(typeof(countUntil!pred(haystack, needles[1..__dollar])))
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/dmd/phobos/std/algorithm/searching.d(856):        countUntil(alias pred = "a == b", R, N)(R haystack, N needle),
  whose parameters have the following constraints:
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    isInputRange!R
  > is(typeof(binaryFun!pred(haystack.front, needle)) : bool)
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/dmd/phobos/std/algorithm/searching.d(915):        countUntil(alias pred, R)(R haystack)
  Tip: not satisfied constraints are marked with >
./scribble.d(17): Error: template std.algorithm.mutation.initializeAll cannot deduce function from argument types !()(void*), candidates are:
/usr/include/dmd/phobos/std/algorithm/mutation.d(864):        initializeAll(Range)(Range range),
  whose parameters have the following constraints:
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  > isInputRange!Range
  - hasLvalueElements!Range
  - hasAssignableElements!Range
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/dmd/phobos/std/algorithm/mutation.d(913):        initializeAll(Range)(Range range),
  whose parameters have the following constraints:
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  > is(Range == char[])
    or:
  > is(Range == wchar[])
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  Tip: not satisfied constraints are marked with >

The output is colored. It would be prettier if I didn't care about possible breakages (e.g. it needs to move signatures that don't fit to the next line).

Why my solution is cool:

• no language changes, no DIPs
• no need to rewrite existing codebases
• may be applied to static assert and probably, in some way, to static if
• can disassemble arbitrarily complex combinations of !, &&, ||, and ?:

The drawback is that it is shallow, it cannot say "does not implement popFront()", it can only point to the constraint isInputRange!R, though their names are mostly quite descriptive.

Old && approach to make assert messages can work here to produce some readable results, for example:

void stuff(T, U)()
if (isNice!T && "must be nice" && isFine!U && "must be fine")
{}

will print something like

    isNice!T
    "must be nice"
  > isFine!U
  - "must be fine"

I could handle this case specially and print it better, in one line (edit: and print evaluated expression), but it's too much for this PR.

Tools

Do we have any special output for them?

IDE can simply show that message in a tooltip. Theoretically, it can parse it and highlight failed clauses (because the message contains all parts of the constraint in order from left to right). Brittle thing, we should define a special compiler flag for error formats, and form JSON.

Then we can greatly improve the error messages for humans.

Update

Now the full tree view with the tip message is shown only in verbose mode. In the usual, it only shows failed constraints in a compact form.

A new section was added. It clarifies what type/value the passed parameters have. It prints well usual and variadic parameters, except for lambdas (__lambda1), enum members (cast(Enum)0), maybe something else.

Examples:

./scribble.d(67): Error: template instance `std.array.Appender!int` does not match template declaration Appender(A)
  with A = int
  must satisfy the following constraint:
       isDynamicArray!A

./scribble.d(10): Error: template std.algorithm.sorting.sort cannot deduce function from argument types !()(FilterResult!(unaryFun, int[])), candidates are:
/usr/include/dmd/phobos/std/algorithm/sorting.d(1855):        sort(alias less = "a < b", SwapStrategy ss = SwapStrategy.unstable, Range)(Range r)
  with less = "a < b",
       ss = cast(SwapStrategy)0,
       Range = FilterResult!(unaryFun, int[])
  must satisfy the following constraint:
       isRandomAccessRange!Range

/usr/include/dmd/phobos/std/range/package.d(6264):        iota(B, E)(B begin, E end)
  with B = string,
       E = string
  must satisfy one of the following constraints:
       isIntegral!(CommonType!(B, E))
       isPointer!(CommonType!(B, E))

(omitted other 5 iota variants)

[dlang-bot]

Thanks for your pull request and interest in making D better, @dayllenger! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9715"

[jacob-carlborg]

This looks pretty awesome. Perhaps only print the non-failing constraints in verbose mode. Then the > and the tip might not be necessary.

[wilzbach]

Do we have any special output for them?

Not yet. We should really add a flag for JSON error messages. We could use the existing -verrors.

BTW I hope you knew about AUTO_UPDATE=1 and didn't update all tests manually.

[dayllenger]

I updated, but before rebase. After rebase I'm getting such errors on make -f posix.mak:

dmd/backend/cg87.d:2719: error: undefined reference to 'idxregm(code*)'
dmd/backend/cgxmm.d:290: error: undefined reference to 'idxregm(code*)'
dmd/backend/cod1.d:4485: error: undefined reference to 'movOnly(elem*)'
dmd/backend/cod4.d:465: error: undefined reference to 'idxregm(code*)'
dmd/backend/cod4.d:480: error: undefined reference to 'idxregm(code*)'
collect2: error: ld returned 1 exit status

I guess it wasn't necessary to rebase...

[wilzbach]

If you get such errors, it always helps to do a full clean.

[thewilsonator]

@dayllenger see also dlang/DIPs#131 (will be discussed at dconf) it would be good to coordinate with these improvements (in part so that we only have to update the test suite once).

[nordlow]

Awesome.

I would like to also have links to the failing restrictions be printed somehow so I can navigate to them inside my favorite editor or IDE.

[adamdruppe]

here's my idea for something similar btw:
#9419

One thing I like with mine is it shows what value the template parameters evaluate to, so like:

constraint clause isRandomAccessRange!(FilterResult!(unaryFun, int[])) was false

Shows that Range == FilterResult!(unaryFun, int[]). Your source view is more readable in some cases though... but in stuff like loops, knowing which iteration failed is quite nice. Perhaps just a supplemental line that says like "You passed FilterResult!(unaryFun, int[]) as Range" (though "you passed" is semi-lie because it is the library creating that, but meh)

[dayllenger]

At first I printed those expressions after substitution, but sometimes it looks like
is(typeof(startsWith(alias pred = (a, b) => a == b, Range, Needles...)(Range doesThisStart, Needles withOneOfThese) if (isInputRange!Range && (Needles.length > 1) && is(typeof(.startsWith!pred(doesThisStart, withOneOfThese[0])) : bool) && is(typeof(.startsWith!pred(doesThisStart, withOneOfThese[1..__dollar])) : uint))(haystack, __needles_field_0)))...

It should be easy to add a line like with R = int[], N = void*, pred = "a == b", I am already working on that.

About loops - this task is somewhat deeper than I can think over now.

[dayllenger]

See Update section in the first post.

I guess, I need to pack tests in a single file and put it into fail_compilation?

[dayllenger]

@thewilsonator did you discuss your DIP at DConf? This PR is ready, I guess people can try it out.

[nordlow]

@dayllenger, could this be extended to include transitive diagnostics of mismatched qualifiers @safe pure nothrow @nogc when calling a templated function that in turn calls a mis-qualified function?

Example:

User calls f(g(h(x))) inside a pure context, where f and g are templated non-pure-qualified (inferred pure) functions and h is a non-pure function or templated function inferred to be non-pure. Currently the compiler diagnoses this as something like:

"Cannot call unpure function f inside a pure context"

when it instead should hint that the non-purity of h is the source of the problem.

I guess a separate PR is preferred for that. But the logic for producing such diagnostics might share code with this PR...

[dayllenger]

It's a good idea, and it looks complicated for me. The logic is unlikely similar: my function recursively decompose the constraint expression, but in case of function bodies it needs to intervene somehow in the mechanism of attribute inference; it requires deep knowledge of this part of the compiler I don't have currently.

[WalterBright]

Given the large amount of code added over 3 files, please consider organizing this functionality into one place.

[dayllenger]

Currently the code in staticcond.d doesn't know anything about template constraints, so theoretically it can work with any static condition - as the module name says. The code inside dtemplate.d formats specifically the constraints and also the arguments. If I merge the parts into one, it will be unnecessary coupled.

Should I rewrite the existing commits or append a new one, for review fixes?

[JesseKPhillips]

I recommend adding fixes as new commits to review those specific changes. Use $ git commit --fixup... To be able to cleanup history quickly afterwards.

[FeepingCreature]

Ping for attention! :)

[FeepingCreature]

Slightly more insistent ping for attention! <(｀^´)>

[dayllenger]

@WalterBright I don't see here a point of arranging the code into one place. Why it should be so? What is that place?

[RazvanN7]

This looks good to me. The code is logically organized, I don't see the point in trying to put it in one place. There is no reason to stall this. Adding auto-merge.

[RazvanN7]

Thank you @dayllenger for this awesome work.