Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't pretend we found a match #1842

Merged
merged 1 commit into from
Apr 6, 2013
Merged

Conversation

WalterBright
Copy link
Member

Changed it so it does not attempt to recover from function overload errors by picking one essentially at random. Now it marks the call as invalid.

@ghost
Copy link

ghost commented Apr 5, 2013

This is a bad patch:

void foo()
{
}

void main()
{
    foo("");
}
test.d(9): Error: function test.foo () is not callable using argument types (string)

There are no overloads here. There are no overloads in diag9420.d which you've modified either. And again you didn't add any test-cases...

@WalterBright
Copy link
Member Author

What's wrong with the error message? It looks right on target to me.

The test cases already exist - diag9420.d is an example.

@ghost
Copy link

ghost commented Apr 5, 2013

What's wrong with the error message? It looks right on target to me.

It used to be:

test.d(7): Error: function test.foo () is not callable using argument types (string)
test.d(7): Error: expected 0 arguments, not 1 for non-variadic function type void()

@WalterBright
Copy link
Member Author

Yes, I know what it used to be. The second message is either redundant or very misleading, because the previous code would just "pick any" in the event of overload resolution failure, and then you'd get a weird message based on trying to match up with that "any" function.

@WalterBright
Copy link
Member Author

Hmm, did some new update of Phobos break this?

@ghost
Copy link

ghost commented Apr 5, 2013

Yes, my Phobos pull was bad but I just reverted it, all should be fine soon enough.

@ghost
Copy link

ghost commented Apr 5, 2013

The second message is either redundant or very misleading, because the previous code would just "pick any"

But there is no other symbol to pick, there are no overloads of foo.

@WalterBright
Copy link
Member Author

The error message is fine. I really don't understand what the problem is.

@ghost
Copy link

ghost commented Apr 5, 2013

void foo(int, int, int)
{
}

void main()
{
    foo(1, 2.0, 3);
}

Before your pull:

test.d(7): Error: function test.foo (int _param_0, int _param_1, int _param_2) is not callable using argument types (int,double,int)
test.d(7): Error: cannot implicitly convert expression (2.00000) of type double to int

After:

test.d(7): Error: function test.foo (int _param_0, int _param_1, int _param_2) is not callable using argument types (int,double,int)

Why get rid of the informative error message?

@WalterBright
Copy link
Member Author

As I said before,

  1. it is, in the best case, redundant
  2. it the worst case, it is misleading because, as I said, the "error recovery" used to be "pick any function" and then issue error messages on that.

@ghost
Copy link

ghost commented Apr 5, 2013

Well ok, but I hope nobody files a regression.

@yebblies
Copy link
Member

yebblies commented Apr 6, 2013

Yes you can extract the information manually but why should you? When there is only one overload, the second error is never wrong. Is it that hard to keep it in that case?

@WalterBright
Copy link
Member Author

  1. I just don't see a problem with the error message.
  2. I do see a problem with the often bogus second message.
  3. The change in the code was so that the caller to resolveFuncCall() would know that it failed.

@yebblies
Copy link
Member

yebblies commented Apr 6, 2013

I just don't see a problem with the error message.

Ok, spot the mismatch:

test.d(49): Error: function windebug.CreateProcessA (const(char)* lpApplicationName, char* lpCommandLine, SECURITY_ATTRIBUTES* lpProcessAttributes, SECURITY_ATTRIBUTES* lpThreadAttributes, int bInheritHandles, uint dwCreationFlags, void* lpEnvironment, const(char)* lpCurrentDirectory, STARTUPINFO* lpStartupInfo, PROCESS_INFORMATION* lpProcessInformation) is not callable using argument types (immutable(char)*,typeof(null),typeof(null),typeof(null),bool,int,int,typeof(null), STARTUPINFO*,PROCESS_INFORMATION*)

Not perfect, but it helps.

deblink.d(49): Error: cannot implicitly convert expression (0) of type int to void*

I do see a problem with the often bogus second message.

No argument there. Just don't get rid of the non-bogus error.

The change in the code was so that the caller to resolveFuncCall() would know that it failed.

Sure, and if it had no matches from multiple overloads, the error message is less helpful. If it had no matches from a single function, the second error is helpful.

@WalterBright
Copy link
Member Author

  1. Your test.d(49) message is not what the compiler prints out.
  2. The second error message was generated somewhere completely else in the compiler. It is the result of the failed error recovery, not improved diagnostics, which is why it is often bogus. That is what I've been trying to eliminate.
  3. If there are a lot of parameters, the deblink.d(49) message is just as unclear which argument failed, i.e. it's not an improvement.

@int-index
Copy link

Less redundant error messages are good and actually help to find a bug in your program. I don't want to waste my time checking two error messages instead of one, so the patch is perfectly nice in my opinion.

@9rnsr
Copy link
Contributor

9rnsr commented Apr 6, 2013

I think this is reasonable change. The second error "expected 0 arguments, not 1 for non-variadic function" is redundant (At minimum it should be "supplemental" error).

9rnsr added a commit that referenced this pull request Apr 6, 2013
@9rnsr 9rnsr merged commit a48b62f into dlang:master Apr 6, 2013
@WalterBright WalterBright deleted the no_guessing branch April 6, 2013 18:34
@CyberShadow
Copy link
Member

This pull request may have introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=17518

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants