Fix Issue 15613, 11529: Show parameter mismatch and rvalue/lvalue ref message #7554

ntrel · 2017-12-30T16:25:13Z

When there's only one overload to consider:

Add a supplemental message showing the first argument that failed to match the corresponding function parameter.
Alter the messsage to show if an rvalue is being passed to a ref/out parameter.
When necessary for disambiguation, show the qualified type of the parameter as well.

dlang-bot · 2017-12-30T16:25:15Z

Thanks for your pull request, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✗	9631	Error message not using fully qualified name when appropriate.
✓	11529	Unclear error message when rvalue is passed as `ref'
✓	15613	Parameter type mismatch error message are not very helpful

nordlow · 2017-12-31T01:59:14Z

Better diagnostics is exactly what D needs right now! Great work!

WalterBright · 2017-12-31T06:29:19Z

src/dmd/func.d

@@ -3618,3 +3620,21 @@ extern (C++) final class DeleteDeclaration : FuncDeclaration
        v.visit(this);
    }
 }
+
+void showArgMismatch(Loc loc, Expressions* fargs, TypeFunction tf, size_t failIndex)


Please add Ddoc style function comment.

WalterBright · 2017-12-31T06:31:28Z

src/dmd/func.d

+        auto msg = "cannot pass %sargument `%s` of type `%s` to parameter `%s`";
+        // don't print parameter type if it's already in the parameter string
+        if (strcmp(par.type.toChars(), ts[1]) != 0)
+            msg ~= " of type `%s`";


This isn't guaranteed to produce a 0 terminated array.

Just use two different msg strings.

i.e.:

msg = strcmp() ? "cannot pass ..." : "cannot pass ... of type ...";

Fixed by adding \0 to 2nd string, hope that's OK?

WalterBright · 2017-12-31T06:33:32Z

src/dmd/hdrgen.d

@@ -3433,3 +3433,17 @@ extern (C++) const(char)* parametersTypeToChars(Parameters* parameters, int vara
    v.parametersToBuffer(parameters, varargs);
    return buf.extractString();
 }
+
+extern (C++) const(char)* parameterToChars(Parameter parameter, int varargs)


Add Ddoc function comment.

JinShil · 2018-01-01T12:59:15Z

src/dmd/mtype.d

@@ -5935,6 +5936,7 @@ extern (C++) final class TypeFunction : TypeNext
            }
        }

+        auto u = size_t.max; // used for failedIndex if no match


I would prefer having a bool failed and a size_t failedIndex and use failed to determine whether or not failedIndex holds a valid value.

We can just check the return value against MATCH.nomatch to determine if failedIndex is valid.

JinShil · 2018-01-01T13:02:09Z

src/dmd/hdrgen.d

+ *  parameter = parameter to print.
+ *  varargs = Kind of varargs, see TypeFunction.varargs.
+ * Returns: NT string representing parameter. */
+extern (C++) const(char)* parameterToChars(Parameter parameter, int varargs)


DDoc also needs a summary, and what does "NT" mean? If "null-terminated", spell it out please.

JinShil · 2018-01-01T13:02:19Z

src/dmd/hdrgen.d

+/** Params:
+ *  parameters = parameters to print, such as TypeFunction.parameters.
+ *  varargs = Kind of varargs, see TypeFunction.varargs.
+ * Returns: NT string representing parameters. */
 extern (C++) const(char)* parametersTypeToChars(Parameters* parameters, int varargs)


DDoc also needs a summary, and what does "NT" mean? If "null-terminated", spell it out please.

Please don't abbreviate null-terminated.

JinShil · 2018-01-02T14:03:51Z

test/fail_compilation/bug9631.d

+TEST_OUTPUT:
+---
+fail_compilation/bug9631.d(79): Error: function bug9631.arg.f `(int i, S s)` is not callable using argument types `(int, S)`
+fail_compilation/bug9631.d(79):        cannot pass argument `y` of type `bug9631.tem!().S` to parameter `S s` of type `bug9631.S`


function bug9631.arg.f `(int i, S s)` is not callable using argument types `(int, S)`

Tick marks should surround the entire function declaration (e.g `bug9631.arg.f (int i, S s)`). That may be a difficult thing to do and I'd be willing to pass on the tick marks for now

The function accepts (int, S) but the error message says it's not callable using (int, S). Does this need fully qualified names or something?

cannot pass argument y of type bug9631.tem!().S to parameter S s of type bug9631.S

I suspect this line is supposed the clarify the first, but it's awfully confusing. Can't we have just one line with fully qualified type names?

bug9631.arg.f `(int i, S s)`

This was introduced by Walter before this PR. I tried to fix it but can't work out why exactly it happens.

JinShil · 2018-01-02T14:08:31Z

test/fail_compilation/bug9631.d

+fail_compilation/bug9631.d(80): Error: function literal `__lambda2(S s)` is not callable using argument types `(S)`
+fail_compilation/bug9631.d(80):        cannot pass argument `x` of type `bug9631.S` to parameter `S s` of type `bug9631.tem!().S`
+fail_compilation/bug9631.d(86): Error: constructor bug9631.arg.A.this `(S _param_0)` is not callable using argument types `(S)`
+fail_compilation/bug9631.d(86):        cannot pass argument `S(0)` of type `bug9631.tem!().S` to parameter `S _param_0` of type `bug9631.S`


cannot pass argument `S(0)`

~~`S(0)` doesn't look like an argument; it looks like a function invocation~~ OK, I get it now, though I'm not sure we're making much progress on clarifying these messages.

to parameter `S _param_0` of type `bug9631.S`

I think this should read "... to parameter `bug9631.S _param_0`" without redundantly specifying one unqualified type name `S` and one qualified type name `bug9631.S`

JinShil · 2018-01-02T14:19:49Z

test/fail_compilation/diag8101b.d

+fail_compilation/diag8101b.d(18):        `diag8101b.S.foo(int _param_0)`
+fail_compilation/diag8101b.d(19):        `diag8101b.S.foo(int _param_0, int _param_1)`
+fail_compilation/diag8101b.d(29): Error: function diag8101b.S.bar `(int _param_0)` is not callable using argument types `(double)`
+fail_compilation/diag8101b.d(29):        cannot pass argument `1.00000` of type `double` to parameter `int _param_0`


Can we shoot for something like:

function diag8101b.S.bar (int _param_0) is not callable using argument 1.00000 of type double to parameter int _param_0

You might even be able to make it less verbose with...

function diag8101b.S.bar is not callable using argument 1.00000 of type double to parameter int _param_0

I think it's useful to see the full parameter list, so we know which function the compiler means. If we combine both messages I think there's too much information in one sentence.

I think it's useful to see all the argument types too.

JinShil · 2018-01-02T14:32:01Z

test/fail_compilation/diag8101b.d

-fail_compilation/diag8101b.d(17):        `diag8101b.S.foo(int _param_0)`
-fail_compilation/diag8101b.d(18):        `diag8101b.S.foo(int _param_0, int _param_1)`
-fail_compilation/diag8101b.d(33): Error: mutable method `diag8101b.S.bar` is not callable using a const object
+fail_compilation/diag8101b.d(27): Error: none of the overloads of `foo` are callable using argument types `(double)`, candidates are:


The term "argument types" seems odd here. What about...

none of the overloads of foo are callable using parameter list (double), candidates are:

This also removes the plural "types" with the singular parameter type oddity.

Well, it's talking about arguments so "argument list" would work. But this is not part of the PR, I expect argument types is used elsewhere too.

ntrel · 2018-01-02T16:39:34Z

BTW, I also have code to do the same for deduced template function calls (if there are no overloads), but I'll wait until this is merged to submit.

WalterBright · 2018-01-06T10:45:30Z

src/dmd/hdrgen.d

+    parameter.accept(v);
+    if (varargs)
+    {
+        buf.writestring("...");


need test case to cover this new line

Added test, turns out it was wrong for variadic parameters :-/

WalterBright · 2018-01-06T10:46:36Z

src/dmd/hdrgen.d

+ *  parameter = parameter to print.
+ *  varargs = kind of varargs, see TypeFunction.varargs.
+ *  fullQual = whether to fully qualify types.
+ * Returns: Null-terminated string representing parameters. */


Nit: Please conform to the existing way Ddoc comments are formatted in the front end. To wit:

/**** * blah blah */

not:

/** blah blah * blah blah */

Done for this module.

WalterBright · 2018-01-06T10:50:37Z

src/dmd/mtype.d

@@ -9224,7 +9228,7 @@ const(char*)[2] toAutoQualChars(Type t1, Type t2)
 {
    auto s1 = t1.toChars();
    auto s2 = t2.toChars();
-    if (strcmp(s1, s2) == 0)
+    if (!t1.equals(t2) && strcmp(s1, s2) == 0)


when would the type strings be the same but they are different types?

When the strings aren't the full qualified type, e.g. S vs tem!().S.

Please add comment to that effect.

WalterBright · 2018-01-06T10:52:35Z

src/dmd/hdrgen.d

@@ -3433,3 +3438,24 @@ extern (C++) const(char)* parametersTypeToChars(Parameters* parameters, int vara
    v.parametersToBuffer(parameters, varargs);
    return buf.extractString();
 }
+
+/** Pretty print function parameters.


This has the same description and return value as the previous function - what does it do different?

Typo, should be parameter. Fixed.

WalterBright · 2018-01-06T10:54:11Z

src/dmd/expressionsem.d

-
+                    .error(exp.loc, "`%s%s` is not callable using argument types `%s`",
+                        exp.e1.toChars(), parametersTypeToChars(tf.parameters, tf.varargs), buf.peekString());
+                    showArgMismatch(exp.loc, exp.arguments, tf, failIndex);


It'd be nice to have test coverage for the new code.

BTW, what does TOKvar mean? I'm trying to hit this case.

TOKvar distinguishes a VarExp from other Expression types.

I don't know how to test for the case when TOKvar is a function. Function pointers are handled differently.

WalterBright · 2018-01-06T10:54:57Z

src/dmd/dtemplate.d

@@ -2325,8 +2325,10 @@ extern (C++) final class TypeDeduced : Type
 *      tiargs          initial list of template arguments
 *      tthis           if !NULL, the 'this' pointer argument
 *      fargs           arguments to function
+ *      failedIndex     address to store argument index of first type mismatch


note that failedIndex can be null

I've noted this for pMessage (which replaced failedIndex).

This should be in Ddoc style:

Params:

use = - then Ddoc can would throw an error about the non-existing failedIndex

JinShil

Definitely an improvement.

wilzbach · 2018-01-17T23:53:20Z

Rebased to upstream/master

wilzbach

Really cool!

ntrel · 2018-01-19T11:03:16Z

@wilzbach Thanks for rebasing. I changed the ddoc comments to use Params: as suggested.

Show the first argument that fails to match the function parameters. This is also part of: Issue 9631 - Error message not using fully qualified name when appropriate

Also hide separate parameter type when already shown.

wilzbach · 2018-01-25T15:23:36Z

Rebased again ;-)

Geod24 · 2018-05-04T14:48:14Z

So yesterday I dusted off an old project and tried to compile. Sure thing, I ran out of memory. This project used to take 2, max 3 GBs of memory. Now it runs out of memory after reaching 7 GBs.

As a result I started to bisect, which led me to this commit after a bit of gymnastic (having -de in the Makefile is really annoying when doing that exercise).
Trying to bisect this PR was rather annoying: the first commit (48c6b49) segfault on this code:

class Bar { this(int) {} }
void main () { scope b = new Bar; }

but that was fixed by later refactoring. Second commit (ff99241) doesn't compile because it was blindly rebased after the STCref => STC.ref_ refactoring, and so everything before commit 6 (15b1b50) does not compile. That makes solving issues more complicated than it needs to be.

Now the cherry on the top: this pull request (or more precisely, commit 6) allocates an error message for every matching failure during call resolution. And that memory is allocated with mem.xmalloc and never freed. I added scope (exit) rmem.xfree here and there but it did not help (because of the bump the pointer I guess, or the very high fragmentation it creates?).

It comes down to commit 6, which eagerly generates error message. @ntrel what was the motivation for it ?

ntrel · 2018-05-05T16:39:29Z

@Geod24 Thanks for finding this. failedIndex is not enough information to generate the (updated) error message on the caller side.

I think the problem is the call to functionResolve - it won't ~~immediately~~ necessarily error after that call so failMessage should not be passed there. Instead we can re-call the function where failMessage is actually needed (warning: not tested), please try:
https://github.com/ntrel/dmd/tree/resolve-msg

Geod24 · 2018-05-06T01:23:29Z

@ntrel I had a go at reverting this commit and it passes the test suite: #8216

ntrel requested a review from WalterBright as a code owner December 30, 2017 16:25

dlang-bot added the Enhancement label Dec 30, 2017

ntrel force-pushed the qual-arg branch 3 times, most recently from 5ecde07 to 244000b Compare December 30, 2017 18:40

WalterBright reviewed Dec 31, 2017

View reviewed changes

JinShil reviewed Jan 1, 2018

View reviewed changes

JinShil suggested changes Jan 1, 2018

View reviewed changes

ntrel force-pushed the qual-arg branch from 702be4f to fb1e928 Compare January 1, 2018 13:18

JinShil added the Diagnostic Messages label Jan 2, 2018

ntrel mentioned this pull request Jan 2, 2018

Fix Issue 16165 - Show expected number of function arguments on mismatch #7584

Merged

JinShil suggested changes Jan 2, 2018

View reviewed changes

JinShil reviewed Jan 2, 2018

View reviewed changes

dlang-bot added Needs Rebase Needs Work labels Jan 4, 2018

ntrel force-pushed the qual-arg branch from 02b244b to efe30e9 Compare January 4, 2018 12:09

dlang-bot removed Needs Rebase Needs Work labels Jan 4, 2018

WalterBright reviewed Jan 6, 2018

View reviewed changes

JinShil mentioned this pull request Jan 12, 2018

display overload type mismatches as highlighted comments #6806

Closed

JinShil approved these changes Jan 13, 2018

View reviewed changes

wilzbach force-pushed the qual-arg branch from 70f46cc to 24f52a2 Compare January 17, 2018 23:52

wilzbach closed this Jan 17, 2018

wilzbach reopened this Jan 17, 2018

wilzbach approved these changes Jan 18, 2018

View reviewed changes

wilzbach added this to the 2.079.0 milestone Jan 18, 2018

ntrel force-pushed the qual-arg branch from 972de68 to d3265a6 Compare January 19, 2018 12:16

ntrel added 10 commits January 25, 2018 16:21

Fix Issue 15613 - Parameter type mismatch error message not very helpful

48c6b49

Show the first argument that fails to match the function parameters. This is also part of: Issue 9631 - Error message not using fully qualified name when appropriate

Fix Issue 11529 - Unclear error message when rvalue is passed as `ref'

ff99241

Also hide separate parameter type when already shown.

Fix null termination, add ddoc

c48e834

Fully qualify parameter instead of showing type separately; fix ddoc

66a74ee

Fix backticks after rebase

7421577

Fix ddoc, fix variadic parameterToChars() & add test

2ef0b7d

Make callMatch set an error message; remove showArgMismatch

15b1b50

Fix possible wrong argument index, minor tweaks

a693e69

Fix triggering CallExp error message

0948ed9

Convert ddoc Input -> Params, fix wrong param

04f41db

wilzbach force-pushed the qual-arg branch from d3265a6 to 04f41db Compare January 25, 2018 15:23

wilzbach added the auto-merge label Jan 25, 2018

dlang-bot merged commit fcc69c1 into dlang:master Jan 26, 2018

ntrel deleted the qual-arg branch January 30, 2018 16:30

ntrel mentioned this pull request May 6, 2018

Stop eagerly generating error message for mismatch arguments #8216

Closed

ntrel mentioned this pull request May 23, 2018

[perf] Don't allocate argument mismatch string when errors gagged #8281

Merged

Fix Issue 15613, 11529: Show parameter mismatch and rvalue/lvalue ref message #7554

Fix Issue 15613, 11529: Show parameter mismatch and rvalue/lvalue ref message #7554

Conversation

ntrel commented Dec 30, 2017 • edited

dlang-bot commented Dec 30, 2017 • edited

Bugzilla references

nordlow commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Jan 1, 2018 • edited

Choose a reason for hiding this comment

ntrel Jan 2, 2018 • edited

Choose a reason for hiding this comment

JinShil Jan 1, 2018 • edited

Choose a reason for hiding this comment

JinShil Jan 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntrel Jan 2, 2018 • edited

Choose a reason for hiding this comment

JinShil Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntrel commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright Jan 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil left a comment

Choose a reason for hiding this comment

wilzbach commented Jan 17, 2018

wilzbach left a comment

Choose a reason for hiding this comment

ntrel commented Jan 19, 2018

wilzbach commented Jan 25, 2018

Geod24 commented May 4, 2018

ntrel commented May 5, 2018 • edited

Geod24 commented May 6, 2018

ntrel commented Dec 30, 2017 •

edited

dlang-bot commented Dec 30, 2017 •

edited

JinShil Jan 1, 2018 •

edited

ntrel Jan 2, 2018 •

edited

JinShil Jan 1, 2018 •

edited

JinShil Jan 1, 2018 •

edited

ntrel Jan 2, 2018 •

edited

JinShil Jan 2, 2018 •

edited

JinShil Jan 2, 2018 •

edited

JinShil Jan 2, 2018 •

edited

WalterBright Jan 6, 2018 •

edited

ntrel commented May 5, 2018 •

edited