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

Improve error reporting: Member implementation with tuple argument(s) #1203

Closed
Tracked by #1103
JanWosnitza opened this issue May 18, 2016 · 9 comments · Fixed by #1229
Closed
Tracked by #1103

Improve error reporting: Member implementation with tuple argument(s) #1203

JanWosnitza opened this issue May 18, 2016 · 9 comments · Fixed by #1229

Comments

@JanWosnitza
Copy link
Contributor

Related to #1200

What

There is a difference between int32 * int32 -> unit and (int32 * int32) -> unit. The first one generates 2 parameters and the second one generates 1 tuple parameter. One might stumble upon this error:

type IInterface =
    abstract Function : (int32 * int32) -> unit

{ new IInterface with
    member this.Function (i, j) = ()
}

FS0768: The member 'Function' does not accept the correct number of arguments, 1 arguments are expected

Why

The error message is confusing, because one might not expect the brackets to change the overall function signature and assume to the result as with:

type IInterface =
    abstract Function : int32 * int32 -> unit

{ new IInterface with
    member this.Function (i, j) = ()
}

How

Trying to address @forki proposals ( #1200 (comment) ):

  • it should be singular
  • it could hint the signature of Function
  • it could tell something about tuples vs. "C# style parameters"

E.g.:

FS0768: The member 'Function : (int32 * int32) -> unit' does not accept the correct number of arguments. Expecting 1 argument
    (int32 * int32)
but given are 2 arguments
    int32 * int32
Member functions with a single tuple argument are compiled to C#-ish parameters instead of a single parameter of type tuple, unless forced otherwise.

I am unsure about the "tuples vs C# style parameters" explanation (also a native speaker should choose the exact wording :) )

@JanWosnitza JanWosnitza changed the title Improve error reporting: Partially applied generic value vs function Improve error reporting: Member tuple parameter implementation May 18, 2016
@JanWosnitza JanWosnitza changed the title Improve error reporting: Member tuple parameter implementation Improve error reporting: Member implementation with tuple argument(s) May 18, 2016
@JanWosnitza JanWosnitza reopened this May 18, 2016
@JanWosnitza
Copy link
Contributor Author

erm.. oups

@forki
Copy link
Contributor

forki commented May 31, 2016

would that help:

image

?

I assume we would need to remove the word "abstract" from the message. and then it would already be better.

@forki
Copy link
Contributor

forki commented May 31, 2016

image

But we still somehow need to explain the tuple issue.

@forki
Copy link
Contributor

forki commented May 31, 2016

WIP PR in #1229

@forki
Copy link
Contributor

forki commented May 31, 2016

image

like this?

forki added a commit to forki/visualfsharp that referenced this issue May 31, 2016
forki added a commit to forki/visualfsharp that referenced this issue May 31, 2016
@forki
Copy link
Contributor

forki commented May 31, 2016

related PR: #1230

@cartermp
Copy link
Contributor

cartermp commented May 31, 2016

I'm leaving feedback about the error message here rather than in the PR so that code can be what's discussed there.

I like your initial changes @forki. I've bolded my suggestions.

Before:

The member 'Function' does not accept the correct number of arguments. 1 argument(s) are expected. The required signature is 'member IInterface.Function: (int32 * int32) -> unit'.
A tuple type was required in one of the parameters. Consider to put the given arguments in additional parentheses.

After:

The member 'Function' does not accept the correct number of arguments. 1 argument(s) are expected, but 2 were given. The required signature is 'member IInterface.Function: (int32 * int32) -> unit'.
A tuple type is required for one or more parameters. Consider wrapping the given arguments in additional parentheses.

My rationale for these suggestions:

  • I think that specifying the number of arguments that were passed clarifies the mistake
  • I think that changing from past to present tense is a bit clearer.

Thoughts?

@vilinski
Copy link

Just some naive thoughts.
The elm compiler gives sometimes the code examples. I think here it will be extremely useful also, with some explanation e.g. why there is just one argument expected and (i, j) are two, and how to make them one.

Consider wrapping the given arguments in additional parentheses:
(some code)
or fix the interface definition:
(some interface code)

forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jun 2, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jun 2, 2016
@JanWosnitza
Copy link
Contributor Author

@cartermp mentioning the given amount would be incredibly useful.

And also printing the given signature would help with understanding what's wrong with the given implementation (as I proposed initially):

The member 'IInterface.Function' does not accept the correct number of arguments.
The required signature has 1 argument(s):
    (int32 * int32) -> unit
but the given implementation has 2 arguments:
    int32 * int32 -> unit
A tuple type is required for one or more parameters. Consider wrapping the given arguments in additional parentheses.

@vilinski I believe this would require special case checking. But if you start adding them to some messages people might start to complain about all the other missing special cases. It's going to be a never ending story. (Though I don't know if they started similar stuff on other messages.) IMHO the compiler should print only enough information to be able to clearly understand what the problem is. Your drawn conclusion may greatly differ depending on what you trying to do.

forki added a commit to forki/visualfsharp that referenced this issue Jun 22, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jun 22, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jul 22, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jul 22, 2016
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 a pull request may close this issue.

5 participants