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

ntrel · 2018-01-02T13:30:35Z

This is ready now #7554 has been merged.

Show expected number of arguments when too few/too many arguments are passed.
Show expected number of variadic static array arguments on mismatch.

dlang-bot · 2018-01-02T13:30:37Z

Thanks for your pull request and interest in making D better, @ntrel! 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

Auto-close	Bugzilla	Severity	Description
✓	16165	enhancement	Show expected number of function arguments on mismatch

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#7584"

marler8997 · 2018-01-30T17:14:12Z

Thank you very much for doing this work. I think this is VERY IMPORTANT. That being said, I've thought about this and wanted to share my thoughts on how we could improve error messages in the best possible way. I haven't looked at the implementation enough to know whether or not this would be easy, this is just based on what I'd like to see as a developer.

I think the first thing we should check is if there is a mismatch in argument count. If so, then it likely means the developer either missed or added an extra argument. In this case, I think the most helpful message would be:

Error: function `foo` takes 10 arguments but got 9
       argument 4 of type `string` is not convertible to `int`

Note that in the case there are no type mismatches, this likely means they just forgot the last argument, so the first line of the previous error message alone should suffice. If there is more than 1 type mismatch, then it's probably better to only print the first one because missing or adding an extra argument is likely to propogate "red herring" errors down the line.

If the argument count is correct, then we must have one or more type mismatches. In the case of 1 type mismatch, a short error message like this should work well:

Error: expected argument 4 of function `foo` to be `int` but got `string`

If there is more than one type mismatch, then a multiline error message would probably be better:

Error: function `foo` was called with invalid arguments
       argument 4 of type `string` is not convertible to `int`
       argument 8 of type 'MyCoolType!(int,byte)` is not convertible to `AnotherType!(int,byte)`

In the case of overloads, just seeing the types is often too little information, however, you also don't want to print the full error message for each overload. I think the current method of printing all the candidates is great, simply adding a special character that shows where the type mismatches are would work well here. Also, printing when there is argument count mismatch may be worth the extra characters, i.e.

Error: none of the overloads of `foo` are callable using argument types `(int,string)`, candidates are:
       foo() : requires 0 arguments but got 2
       foo(int) : requires 1 argument but got 2
       foo(! short)
       foo(int, ! int)
       foo(! short, ! byte, ! short) : requires 3 arguments but got 2
       foo(int,string, ! short) : requires 3 arguments but got 2

Note that type mismatches are indicated by a ! in the location where a type mismatch occurs.

wilzbach · 2018-01-30T17:18:12Z

@ntrel are you aware of the CI failures? Though I don't know either what could have introduced them..

Assertion failed: (t), function source_name, file cppmangle.c, line 185.
posix.mak:450: recipe for target '../generated/freebsd/release/32/dmd_frontend' failed
gmake[1]: *** [../generated/freebsd/release/32/dmd_frontend] Abort trap: 6 (core dumped)
gmake[1]: *** Waiting for unfinished jobs....
Assertion failed: (t), function source_name, file cppmangle.c, line 185.
Assertion failed: (t), function source_name, file cppmangle.c, line 185.
posix.mak:585: recipe for target '../generated/freebsd/release/32/cxx-unittest' failed
gmake[1]: *** [../generated/freebsd/release/32/cxx-unittest] Abort trap: 6 (core dumped)
posix.mak:457: recipe for target '../generated/freebsd/release/32/dmd' failed
gmake[1]: *** [../generated/freebsd/release/32/dmd] Abort trap: 6 (core dumped)

timotheecour · 2018-02-01T11:38:04Z

Note that type mismatches are indicated by a ! in the location where a type mismatch occurs.

and color-highlight mis-matches when color=on

marler8997 · 2018-02-02T16:27:13Z

I'm not sure if @ntrel is for or against the suggestions I've made. I'll wait to see if/when this PR gets integrated and afterwards probably look into making those changes myself.

ntrel · 2018-02-06T15:42:01Z

@marler8997: Here I'm avoiding changing the signature of callMatch, so I'm stuck with a single specific error message. I do think we should keep the printing of function parameters and provided argument types - this is useful.

Given that, I've added 2 commits to help the user pin-point the failing parameter match, rather than just saying the argument/parameter count.

For overloads, one option would be to print the specific error only when dmd -v is used - this keeps the number of output lines as is. Personally I'd be happy if they were always printed.

ntrel · 2018-02-06T15:48:09Z

@wilzbach:

are you aware of the CI failures? Though I don't know either what could have introduced them..
Assertion failed: (t), function source_name, file cppmangle.c, line 185.

I don't know why that's happening.

wilzbach · 2018-02-06T15:54:09Z

No idea about the auto-tester failures, looks like an issue with 2.068.2. If your OS doesn't use hardening, you can set the host compiler with HOST_DMD. Download with e.g.

curl https://dlang.org | bash -s dmd-2.068.2

CircleCi + Semaphore are known - I haven't had time to look into to that though :/

test_results/d_do_test: symbol lookup error: test_results/d_do_test: undefined symbol: _D3std9exception__T7enforceZ__TQmTbZQrFNaNfbLAxaAyamZb
Makefile:209: recipe for target 'test_results/runnable/helloUTF16BE.d.out' failed

edit: I'm proposing to revert the Phobos PR for now: dlang/phobos#6130

marler8997 · 2018-02-06T16:03:52Z

@ntrel Ok it sounds like my suggestions would require a bigger set of changes. Go ahead with this, any improvement in the current situation is good. If I have time after this PR is merged I might look into this.

wilzbach · 2018-02-23T04:31:49Z

Okay looks like we got over almost all failures, though the auto-tester is still complaining:

Assertion failed: (t), function source_name, file cppmangle.c, line 185.
gmake[1]: *** [../generated/freebsd/release/32/dmd_frontend] Abort trap: 6 (core dumped)
gmake[1]: *** Waiting for unfinished jobs....
Assertion failed: (t), function source_name, file cppmangle.c, line 185.
Assertion failed: (t), function source_name, file cppmangle.c, line 185.

RazvanN7 · 2018-05-30T11:11:07Z

@ntrel do you plan on pursuing this? I think it is very important that this gets merged eventually, so will you take it to completion or should someone adopt it?

wilzbach · 2018-05-31T10:37:03Z

You could please do a rebase instead?
We try to keep the git history as short as possible.

jacob-carlborg · 2018-05-31T11:19:30Z

Are all these commits necessary, would it be better to squash them instead?

ntrel · 2018-05-31T11:30:30Z

@jacob-carlborg f286ceb and 3437c00 are cherry-picked from stable (they're necessary to reduce unnecessary memory usage that was probably causing a timeout in a test), probably they shouldn't be squashed. The existing ones I thought I'd leave to show I've not changed anything, but could be squashed.

jacob-carlborg · 2018-05-31T11:39:08Z

ok

ntrel · 2018-05-31T12:25:25Z

@RazvanN7 Anyone is welcome to take this over. I think I've done all I can to try to fix all the checks, but still two remain failing.

wilzbach · 2018-06-26T00:19:21Z

FYI the SemaphoreCI failure is related to GDC having issues when building this:

dmd/mtype.d: In member function 'callMatch':
dmd/mtype.d:5116:21: internal compiler error: Aborted
                     *pMessage = getMatchError("missing argument for parameter #%d: `%s`",

ibuclaw · 2018-06-26T06:06:28Z

@wilzbach you forgot the ice line in the compiler itself.

cppmangle.c:185: void CppMangleVisitor::source_name(Dsymbol*, bool): Assertion `t' failed.
dmd/mtype.d: In member function 'callMatch':
dmd/mtype.d:4953:21: internal compiler error: Aborted
                     *pMessage = getMatchError("missing argument for parameter #%d: `%s`",
                     ^

src/dmd/mtype.d

jacob-carlborg · 2018-06-26T13:02:55Z

test/fail_compilation/fail332.d

@@ -1,15 +1,56 @@
 /*
 TEST_OUTPUT:
 ---
-fail_compilation/fail332.d(14): Error: function `fail332.foo(int _param_0, ...)` is not callable using argument types `()`
+fail_compilation/fail332.d(22): Error: function `fail332.foo(int _param_0, ...)` is not callable using argument types `()`
+fail_compilation/fail332.d(22):        missing argument for parameter #1: `int _param_0`


Why is _param_0 mentioned sometimes and sometimes it's not, i.e.: https://github.com/dlang/dmd/pull/7584/files#diff-534158a8eaceb0162bf8ef17709be089R33

jacob-carlborg · 2018-06-26T13:03:35Z

test/fail_compilation/diag8101.d

-fail_compilation/diag8101.d(51):        `diag8101.t_2(T1, T2, T3, T4, T5)()`
-fail_compilation/diag8101.d(62):        ... (1 more, -v to show) ...
+fail_compilation/diag8101.d(57): Error: function `diag8101.f_0(int)` is not callable using argument types `()`
+fail_compilation/diag8101.d(57):        missing argument for parameter #1: `int`


How does this error message look like with several missing arguments?

getMatchError is private

thewilsonator · 2018-11-25T03:02:44Z

@ntrel this is green. Anything more you want to add? Otherwise please squash.

ntrel · 2018-11-26T11:32:12Z

@thewilsonator That's great, I think this is good to go. I don't have my laptop where I am for the next few days, so it might take me a while to squash commits (unless there's a way to do it in a browser?).

This finds the first argument that fails to match.

Fix test line numbers, Windows backslash Show which parameter is missing an argument Don't allocate argument count mismatch string when errors gagged Mark getMatchError as private

ntrel · 2018-12-01T17:34:29Z

@thewilsonator I've now squashed 4 simple commits into one.

ntrel requested a review from WalterBright as a code owner January 2, 2018 13:30

dlang-bot added the Severity:Enhancement label Jan 2, 2018

JinShil added the Diagnostic Messages label Jan 2, 2018

dlang-bot added the Review:Needs Rebase label Jan 4, 2018

ntrel force-pushed the count-args branch from c04d96b to 24d8b7e Compare January 4, 2018 12:28

dlang-bot added Review:Needs Work Review:Needs Rebase and removed Review:Needs Rebase labels Jan 4, 2018

JinShil mentioned this pull request Jan 12, 2018

display overload type mismatches as highlighted comments #6806

Closed

ntrel force-pushed the count-args branch from 24d8b7e to 92b024a Compare January 19, 2018 12:10

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jan 19, 2018

ntrel force-pushed the count-args branch from 92b024a to cdefb24 Compare January 19, 2018 12:19

ntrel force-pushed the count-args branch from cdefb24 to 6731097 Compare January 30, 2018 16:30

ntrel changed the title ~~Fix Issue 16165 - Show count of arguments vs parameters in error message~~ Fix Issue 16165 - Show expected number of function arguments on mismatch Jan 30, 2018

ntrel force-pushed the count-args branch from 0c603e4 to 4d8c8c8 Compare February 5, 2018 17:50

wilzbach closed this Feb 23, 2018

wilzbach reopened this Feb 23, 2018

ntrel mentioned this pull request May 8, 2018

Stop eagerly generating error message for mismatch arguments #8216

Closed

ntrel mentioned this pull request May 8, 2018

Avoid eager error message allocation with functionResolve #8232

Merged

ntrel force-pushed the count-args branch 2 times, most recently from 66237e5 to 56ab089 Compare May 9, 2018 11:26

ntrel force-pushed the count-args branch from 77ab0ba to f286ceb Compare May 31, 2018 10:41

wilzbach force-pushed the count-args branch from 4db4768 to 36c8d5f Compare June 26, 2018 00:19

ibuclaw previously requested changes Jun 26, 2018

View reviewed changes

src/dmd/mtype.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Jun 26, 2018

View reviewed changes

ntrel added 2 commits November 25, 2018 09:34

Fix Issue 16165 - Show count of arguments vs parameters on mismatch

b14d8f8

Add workaround for possible OutBuffer.vprintf issue on Windows (Vista)

350b3c0

thewilsonator force-pushed the count-args branch from 704eda6 to 4f772c1 Compare November 25, 2018 01:35

thewilsonator self-assigned this Nov 25, 2018

ntrel added 2 commits November 30, 2018 15:52

Type-check preceeding arguments even when there are surplus arguments

97a1866

This finds the first argument that fails to match.

Minor fixes

bff6a03

Fix test line numbers, Windows backslash Show which parameter is missing an argument Don't allocate argument count mismatch string when errors gagged Mark getMatchError as private

ntrel force-pushed the count-args branch from 4f772c1 to bff6a03 Compare November 30, 2018 16:33

thewilsonator approved these changes Dec 1, 2018

View reviewed changes

thewilsonator merged commit f35640a into dlang:master Dec 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ntrel commented Jan 2, 2018 •

edited

Loading

dlang-bot commented Jan 2, 2018 •

edited

Loading

marler8997 commented Jan 30, 2018 •

edited

Loading

wilzbach commented Jan 30, 2018

timotheecour commented Feb 1, 2018

marler8997 commented Feb 2, 2018

ntrel commented Feb 6, 2018

ntrel commented Feb 6, 2018

wilzbach commented Feb 6, 2018 •

edited

Loading

marler8997 commented Feb 6, 2018

wilzbach commented Feb 23, 2018

RazvanN7 commented May 30, 2018

wilzbach commented May 31, 2018

jacob-carlborg commented May 31, 2018

ntrel commented May 31, 2018

jacob-carlborg commented May 31, 2018

ntrel commented May 31, 2018

wilzbach commented Jun 26, 2018

ibuclaw commented Jun 26, 2018

jacob-carlborg Jun 26, 2018

jacob-carlborg Jun 26, 2018

thewilsonator commented Nov 25, 2018

ntrel commented Nov 26, 2018

ntrel commented Dec 1, 2018

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

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

Conversation

ntrel commented Jan 2, 2018 • edited Loading

dlang-bot commented Jan 2, 2018 • edited Loading

Bugzilla references

Testing this PR locally

marler8997 commented Jan 30, 2018 • edited Loading

wilzbach commented Jan 30, 2018

timotheecour commented Feb 1, 2018

marler8997 commented Feb 2, 2018

ntrel commented Feb 6, 2018

ntrel commented Feb 6, 2018

wilzbach commented Feb 6, 2018 • edited Loading

marler8997 commented Feb 6, 2018

wilzbach commented Feb 23, 2018

RazvanN7 commented May 30, 2018

wilzbach commented May 31, 2018

jacob-carlborg commented May 31, 2018

ntrel commented May 31, 2018

jacob-carlborg commented May 31, 2018

ntrel commented May 31, 2018

wilzbach commented Jun 26, 2018

ibuclaw commented Jun 26, 2018

jacob-carlborg Jun 26, 2018

Choose a reason for hiding this comment

jacob-carlborg Jun 26, 2018

Choose a reason for hiding this comment

thewilsonator commented Nov 25, 2018

ntrel commented Nov 26, 2018

ntrel commented Dec 1, 2018

ntrel commented Jan 2, 2018 •

edited

Loading

dlang-bot commented Jan 2, 2018 •

edited

Loading

marler8997 commented Jan 30, 2018 •

edited

Loading

wilzbach commented Feb 6, 2018 •

edited

Loading