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.

Sign up for GitHub

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

Jump to bottom

Better error message when abstract member impl is wrong #1229

Merged
merged 1 commit into from
Jul 22, 2016
Merged

Better error message when abstract member impl is wrong #1229

merged 1 commit into from
Jul 22, 2016

Conversation

forki
Copy link
Contributor

@forki forki commented May 31, 2016

fixes #1203

@forki forki force-pushed the tuple branch 2 times, most recently from c125a53 to 6a54b7f Compare May 31, 2016 11:41
@forki forki mentioned this pull request May 31, 2016
Closed
@forki forki force-pushed the tuple branch 2 times, most recently from 3f73fdf to ed025ed Compare May 31, 2016 13:22
@forki forki mentioned this pull request May 31, 2016
Merged
@forki forki force-pushed the tuple branch 3 times, most recently from 2a55ec6 to fbede5c Compare June 2, 2016 08:06
@forki
Copy link
Contributor Author

forki commented Jun 2, 2016

image

@forki forki changed the title WIP Better error message when abstract member impl is wrong Better error message when abstract member impl is wrong Jun 2, 2016
@forki
Copy link
Contributor Author

forki commented Jun 2, 2016

This one is ready for review.

@cartermp cartermp mentioned this pull request Jun 2, 2016
Merged
dsyme
dsyme reviewed Jun 21, 2016
View reviewed changes
src/fsharp/TypeChecker.fs

let getSignature absSlot = (NicePrint.stringOfMethInfo cenv.amap mBinding env.DisplayEnv absSlot).Replace("abstract ","")
let getDetails (absSlot:MethInfo) =
if absSlot.GetParamTypes(cenv.amap,mBinding,[]) |> List.exists (fun xs -> xs |> List.exists (isTupleTy cenv.g)) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be Lists.existsSquared

@KevinRansom
Copy link
Member

I think the error message does not really make clear why the additional parenthesis is needed perhaps this:

The member 'Function' expects a tupled argument.  The required signature is 'member IInterface.Function : (int32 * int32) -> unit.  A Member function uses parenthesis' and commas to indicate sequential arguments.  Use additional parenthesis' to indicate the argument is tupled.  E.g.
        member this.Function ( (i, j) )

@forki
Copy link
Contributor Author

forki commented Jun 22, 2016

@KevinRansom the issue is that we can't really decide if the implementation is wrong or someone already did a wrong interface definition

@forki forki force-pushed the tuple branch 2 times, most recently from d042924 to ebe9a8a Compare June 22, 2016 07:31
@KevinRansom
Copy link
Member

@forki the interface exists and is what is being implemented, so it can be assumed to be correct and thus the implementation must be at fault by not specifying a tuple. My real point is that the revised error message didn't really explain why the extra parenthesis' is needed.

@forki
Copy link
Contributor Author

forki commented Jun 24, 2016

In the reported case the interface contained the bug. But I understand what
you mean
On Jun 24, 2016 4:49 PM, "Kevin Ransom (msft)" [email protected]
wrote:

@forki https://github.com/forki the interface exists and is what is
being implemented, so it can be assumed to be correct and thus the
implementation must be at fault by not specifying a tuple. My real point is
that the revised error message didn't really explain why the extra
parenthesis' is needed.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1229 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNJq-sWL2Ejr79hSBncI0Fx2sDteWks5qO-6EgaJpZM4IqX-q
.

@cartermp
Copy link
Contributor

the issue is that we can't really decide if the implementation is wrong or someone already did a wrong interface definition

We should bias towards the interface as-written to be intended here.

@dsyme
Copy link
Contributor

dsyme commented Jul 22, 2016

@forki Please fix merge conflicts when you get the chance, thanks

@forki
Copy link
Contributor Author

forki commented Jul 22, 2016

done. but these were non-trivial so we better wait for green.

@forki
Copy link
Contributor Author

forki commented Jul 22, 2016

ok good. seems to work

@dsyme dsyme merged commit c184a34 into dotnet:master Jul 22, 2016
@dsyme
Copy link
Contributor

dsyme commented Jul 22, 2016

All green apart from AppVeyor, which is taking a while to catch up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve error reporting: Member implementation with tuple argument(s)
5 participants
@forki @KevinRansom @cartermp @dsyme @msftclas