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

error messages: report a list of missing overrides #4983

Merged

Conversation

smoothdeveloper
Copy link
Contributor

@smoothdeveloper smoothdeveloper commented May 23, 2018

This is going to be for #4982, right now, it is just a first pass at looking at the code and cleaning a bit without any behaviour change.

I intend to look at noimpl which is the location we report the error.

Instead I plan to make it an accumulator which will raise the error (with the expected message) if more than 10 missing overrides have been encountered, otherwise it will append, and then it will be checked before returning to construct the expected message if needed and raise it.

Please let me know if you see any issue with that plan.

Status:

  • main logic implementation
  • reorganize error message (and fix their names)
  • add exhaustive tests for each variant of the message
  • make it green

elif argTys.Length <> vargtys.Length then
fail(Error(FSComp.SR.typrelMemberDoesNotHaveCorrectNumberOfArguments(FormatOverride denv overrideBy, FormatMethInfoSig g amap m denv dispatchSlot), overrideBy.Range))
elif not (listSameLength argTys vargtys) then
fail(Error(FSComp.SR.typrelMemberDoesNotHaveCorrectNumberOfArguments(FormatOverride denv overrideBy, formatMethodInfoSignature dispatchSlot), overrideBy.Range))
elif mtps.Length <> fvmtps.Length then
Copy link
Contributor

Choose a reason for hiding this comment

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

If mtps is a list, use listSameLength here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmh it is a Typars, which in fact is a list, I'll change it as well.

It would be nice if type aliases in a tooltip were shown at the bottom.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is already an issue/suggestion. It would be easy to implement, the hard part is to decide how to represent it on the tooltip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3110 revolves around similar topic, in this case it would display:

image

Typars = Typar list

@smoothdeveloper
Copy link
Contributor Author

I've added the main logic to CheckDispatchSlotsAreImplemented, few decisions to make:

  • do we want to keep the same compiler error number among the variants of the message?
  • do we want to reduce the variants of message regarding the one with suggestion related to interface? I believe this should be handled differently by appending another message rather than having a couple of redundant messages with just the end being different (see changes in FSComp.txt)

I'll be working on few code samples to add to the test suite, is fsharpqa\Source\ErrorMessages\MissingOverrides a good location for it?

If you have evil code idea to check I'm not breaking anything or that we have all cases cornered, please share.

Sample output for now:

Microsoft (R) F# Interactive version 10.1.0 for F# 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> open System
- open System.IO;;
>
- type IFoo1 =
-   abstract member Foo : t:Type * r:TextReader -> obj
-   abstract member Bar<'t> : TextReader -> 't
-   abstract member Baz<'t> : TextReader -> 't
-
- type Foo1() =
-   interface IFoo1 with
-     member x.Foo(t, reader) = obj()
- ;;

    interface IFoo1 with
  ------------^^^^^

stdin(10,13): error FS0366: No implementation was given for those members:
        'abstract member IFoo1.Bar : TextReader -> 't'
        'abstract member IFoo1.Baz : TextReader -> 't'
 Note that all interface members must be implemented and listed under an appropriate 'interface' declaration, e.g. 'interface ... with member ...'.

> type IFoo2 =
-   abstract member Foo : t:Type * r:TextReader -> obj
-   abstract member Bar<'t> : TextReader -> 't
-   abstract member Baz<'t> : TextReader -> 't
-   abstract member Baz1<'t> : TextReader -> 't
-   abstract member Baz2<'t> : TextReader -> 't
-   abstract member Baz3<'t> : TextReader -> 't
-   abstract member Baz4<'t> : TextReader -> 't
-   abstract member Baz5<'t> : TextReader -> 't
-   abstract member Baz6<'t> : TextReader -> 't
-   abstract member Baz7<'t> : TextReader -> 't
-   abstract member Baz8<'t> : TextReader -> 't
-   abstract member Baz9<'t> : TextReader -> 't
-
- type Foo2() =
-   interface IFoo2 with
-     member x.Foo(t, reader) = obj()
-     ;;

    interface IFoo2 with
  ------------^^^^^

stdin(28,13): error FS0366: No implementation was given for those members (some results omitted):
        'abstract member IFoo2.Bar : TextReader -> 't'
        'abstract member IFoo2.Baz : TextReader -> 't'
        'abstract member IFoo2.Baz1 : TextReader -> 't'
        'abstract member IFoo2.Baz2 : TextReader -> 't'
        'abstract member IFoo2.Baz3 : TextReader -> 't'
        'abstract member IFoo2.Baz4 : TextReader -> 't'
        'abstract member IFoo2.Baz5 : TextReader -> 't'
        'abstract member IFoo2.Baz6 : TextReader -> 't'
        'abstract member IFoo2.Baz7 : TextReader -> 't'
        'abstract member IFoo2.Baz8 : TextReader -> 't'
 Note that all interface members must be implemented and listed under an appropriate 'interface' declaration, e.g. 'interface ... with member ...'.
>

@smoothdeveloper smoothdeveloper force-pushed the better-overrides-issue-reporting branch from e2a8a07 to c76b594 Compare June 1, 2018 08:58
@smoothdeveloper
Copy link
Contributor Author

I'm not understanding why CI part 3 & 4 fail with this error:

19:59:20 D:\j\w\release_ci_pa---c6929ad7\src\fsharp\MethodOverrides.fs(376,48): error FS0039: The field, constructor or member 'typrelNoImplementationSeveralGivenWithSuggestion' is not defined. Maybe you want one of the following:�   typrelNoImplementationGivenWithSuggestion�   typrelNoImplementationGiven�   typrelCannotResolveImplicitGenericInstantiation�   typrelOverrideImplementsMoreThenOneSlot�   typrelNamedArgumentHasBeenAssignedMoreThenOnce [D:\j\w\release_ci_pa---c6929ad7\src\buildfromsource\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
19:59:20 D:\j\w\release_ci_pa---c6929ad7\src\fsharp\MethodOverrides.fs(377,49): error FS0039: The field, constructor or member 'typrelNoImplementationSeveralGiven' is not defined. Maybe you want one of the following:�   typrelNoImplementationGiven�   typrelNoImplementationGivenWithSuggestion�   typrelCannotResolveAmbiguityInDelegate�   typrelOverrideImplementsMoreThenOneSlot�   typrelCannotResolveImplicitGenericInstantiation [D:\j\w\release_ci_pa---c6929ad7\src\buildfromsource\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
19:59:20 D:\j\w\release_ci_pa---c6929ad7\src\fsharp\MethodOverrides.fs(378,47): error FS0039: The field, constructor or member 'typrelNoImplementationSeveralTruncatedGivenWithSuggestion' is not defined. Maybe you want one of the following:�   typrelNoImplementationGivenWithSuggestion�   typrelCannotResolveImplicitGenericInstantiation�   typrelTypeImplementsIComparableShouldOverrideObjectEquals�   typrelMemberDoesNotHaveCorrectKindsOfGenericParameters�   typrelTypeImplementsIComparableDefaultObjectEqualsProvided [D:\j\w\release_ci_pa---c6929ad7\src\buildfromsource\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
19:59:20 D:\j\w\release_ci_pa---c6929ad7\src\fsharp\MethodOverrides.fs(379,48): error FS0039: The field, constructor or member 'typrelNoImplementationSeveralTruncatedGiven' is not defined. Maybe you want one of the following:�   typrelNoImplementationGivenWithSuggestion�   typrelNoImplementationGiven�   typrelSigImplNotCompatibleConstraintsDiffer�   typrelSigImplNotCompatibleConstraintsDifferRemove�   typrelCannotResolveImplicitGenericInstantiation [D:\j\w\release_ci_pa---c6929ad7\src\buildfromsource\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]

those are defined in https://github.com/Microsoft/visualfsharp/pull/4983/files#diff-0e57ebf050db850b5efdd7e2cb13e725 and it compiles in part 1 & 2.

@dsyme
Copy link
Contributor

dsyme commented Jun 5, 2018

I'm not understanding why CI part 3 & 4 fail with this error:

You have to update the FSComp files at least. See DEVGUIDE.md

    .\build net40
    copy /y src\fsharp\FSharp.Compiler.Private\obj\release\net40\FSComp.* src\buildfromsource\FSharp.Compiler.Private\

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

This is looking good, once it's green let's get it merged.

Requested a few minor changes

src/fsharp/MethodOverrides.fs Outdated Show resolved Hide resolved
src/fsharp/MethodOverrides.fs Outdated Show resolved Hide resolved
src/fsharp/MethodOverrides.fs Show resolved Hide resolved
src/fsharp/MethodOverrides.fs Outdated Show resolved Hide resolved
@smoothdeveloper smoothdeveloper force-pushed the better-overrides-issue-reporting branch from 85126d4 to 5647094 Compare June 6, 2018 07:26
build.cmd Outdated Show resolved Hide resolved
build.cmd Outdated Show resolved Hide resolved
* use mutable instead of ref
* avoid calling List.Length just to compare if the length is equal (I believe this should be reusable function outside, but don't know where it is if it exists, or where to put it)
* shorten call to FormatMethInfoSig which is repeated more than any others in many places
* for loop instead of List.iter
* declare local instead of match over long expression
* remove spurious calls to ignore and unit
* update the error names in FSComp to keep the typrelNoImplementationGiven prefix atomic
….fs fsharpqa tests. Note that multiline error messages can't be matched as precisely, we need to drop the status="error" and id="..." from the <Expects> tags to match individual lines of the message separately.
@smoothdeveloper smoothdeveloper force-pushed the better-overrides-issue-reporting branch from 5647094 to 445e82f Compare June 13, 2018 13:04
@smoothdeveloper smoothdeveloper changed the title [wip] error messages: report a list of missing overrides error messages: report a list of missing overrides Jun 13, 2018
@smoothdeveloper
Copy link
Contributor Author

This is green and I think all comments have been addressed.

@smoothdeveloper
Copy link
Contributor Author

smoothdeveloper commented Jun 14, 2018

@dsyme thanks for the review, is there a concern about https://github.com/Microsoft/visualfsharp/pull/4983/files#diff-87f5261ee04fbbcd39ff79a83af01e01R270 where we allocate an empty ResizeArray at the top level of CheckDispatchSlotsAreImplemented?

I could change this to a mutable ResizeArray option initialized to None or whatever other suggestion would apply here.

Edit: a list is initialized with empty array and first add to it pushes it to 4 elements, so this is basically not a concern.

@KevinRansom KevinRansom merged commit 14279d7 into dotnet:master Apr 11, 2019
@KevinRansom
Copy link
Member

@smoothdeveloper,

thanks for this

Kevin

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