Fixing #4967

[forki]

internal error when compiling interface implementation lacking an overloaded method implementation - fixes #4967

[forki]

So this didn't break anything, so I guess I only have to add the test

[forki]

Ready.

[KevinRansom]

@forki

Well ... it turns out that this is a breaking change, go figure ... no good deed goes unpunished. If we compile and run your repro, it runs fine, Of course that is because the type is not used at all, however, There is a non-zero chance that a developer has code with this error, but because the code isn't actually used their project compiles and works fine. We have plenty of customers who are reluctant to upgrade their compiler if they think they will have to crack open and fix perfectly good changes like this.

c:\temp>type foo.fs
//<Expects id="FS0366" status="error">No implementation was given for</Expects>
open System
open System.IO

type IFoo =
  abstract member Foo : t:Type * r:TextReader -> obj
  abstract member Foo<'t> : TextReader -> 't

type Foo() =
  interface IFoo with
    member x.Foo(t, reader) = obj()

exit 1
c:\temp>fsc foo.fs
Microsoft (R) F# Compiler version 10.1.0 for F# 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

c:\temp>foo

c:\temp>echo %errorlevel%
1

So I would like us to consider:

1. Making this a warning.
2. Turning the warning on as an error by default in new legacy templates.
3. Turning the warning on as an error by default in the coreclr Compiler.

--warnaserror[+|-]                       Report all warnings as errors
--warnaserror[+|-]:<warn;...>            Report specific warnings as errors
--warn:<n>                               Set a warning level (0-5)
--nowarn:<warn;...>                      Disable specific warning messages
--warnon:<warn;...>                      Enable specific warnings that may be off by default
--consolecolors[+|-]                     Output warning and error messages in color

[KevinRansom]

Please note: I said "Consider", which should see if we believe the change is only "breaking sorta"

[forki]

@KevinRansom I don't understand. Which case does it break? We had compile error before for the given repro and compile error now. Nothing changed in that regards

[KevinRansom]

As can be seen below:

A program that exhibits the error can compile and run.

c:\temp>type foo.fs
//<Expects id="FS0366" status="error">No implementation was given for</Expects>
open System
open System.IO

type IFoo =
  abstract member Foo : t:Type * r:TextReader -> obj
  abstract member Foo<'t> : TextReader -> 't

type Foo() =
  interface IFoo with
    member x.Foo(t, reader) = obj()

exit 1
c:\temp>fsc foo.fs
Microsoft (R) F# Compiler version 10.1.0 for F# 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

c:\temp>foo

c:\temp>echo %errorlevel%
1

Now we could make the argument, that: the current behaviour is clearly wrong ... and the app only works, because the broken bit isn't touched.

I think the winning argument for taking the change is that the built binary is not verifiable:

c:\temp>peverify foo.exe

Microsoft (R) .NET Framework PE Verifier.  Version  4.0.30319.0
Copyright (c) Microsoft Corporation.  All rights reserved.

[MD]: Error: Class implements interface but not method (class:0x02000004; interface:0x02000003; method:0x06000002). [token:0x09000001]
1 Error(s) Verifying foo.exe
:

Our goal is to document the reasons for taking the change, so that when someone complains that their code was broken we can say why making them fix their broken code is the correct outcome. I imagine this change would even meet the C# breaking change bar, and their bar is way higher than ours.

[forki]

@KevinRansom my point is: it did not compile for me and not for @smoothdeveloper in #4967, right? we got internal error.

[smoothdeveloper]

Mmh, actually my code hasn't left FSI, while in the issue I said compile, it was evaluate instead.

I'll try to make it more precise in future.

@KevinRansom what happens when referencing that assembly from C# and issuing typeof(Foo):

C:\dev\projects\github.com\microsoft\foo>type l.fs
namespace Cool
open System
open System.IO
type IFoo =
  abstract member Foo : t:Type * r:TextReader -> obj
  abstract member Foo<'t> : TextReader -> 't


type Foo() =
  interface IFoo with
    member x.Foo(t, reader) = obj()
    //member x.Foo<'t>(reader) = obj() :?> 't
C:\dev\projects\github.com\microsoft\foo>type a.cs
using Cool;
using System;
public class ThisIsBoring {
        public static void Main(){
                Console.WriteLine(typeof(Foo).Name);
        }
}
C:\dev\projects\github.com\microsoft\foo>fsc --target:library l.fs
Microsoft (R) F# Compiler version 10.1.0 for F# 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

C:\dev\projects\github.com\microsoft\foo>csc /reference:l.dll a.cs
Microsoft (R) Visual C# Compiler version 2.8.0.62830 (e595ee27)
Copyright (C) Microsoft Corporation. All rights reserved.

warning CS1668: Invalid search path 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.13.26128\lib\x86' specified in 'LIB environment variable' -- 'directory does not exist'

C:\dev\projects\github.com\microsoft\foo>a

Unhandled Exception: System.TypeLoadException: Method 'Foo' in type 'Cool.Foo' from assembly 'l, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
   at ThisIsBoring.Main()

Better the compiler reports an error because the interface is not implemented and I risk runtime error (like above), but I understand the breaking change argument.

That being said the chance for someone to get upset because compiler reports malformed code where previous version didn't feel slim, more chance to be grateful or being in wrong profession.

In FSI, I'd the error to not be "internal" and report the actual issue, at least in the way I get runtime exception above.

[KevinRansom]

@smoothdeveloper

I am pretty sure that the CLR will fail to load the type due to the missing method. So any method that needs jitting that references the type will likely fail. I am fairly confident that the break only occurs when the type is unused. I.e dead code, or a rarely used codepath.

Ngen on the exe will fail too:

c:\temp>ngen foo.exe
Microsoft (R) CLR Native Image Generator - Version 4.7.2556.0
Copyright (c) Microsoft Corporation.  All rights reserved.
WARNING: This syntax is deprecated or you mis-typed your command.  Run "ngen /?" to display a list of the currently supported parameters.
1>    Compiling assembly c:\temp\foo.exe (CLR v4.0.30319) ...
1>Warning: System.TypeLoadException: Method 'Foo' in type 'Foo' from assembly 'foo, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation. while resolving 0x2000004 - Foo.
1>Could not load type 'Foo' from assembly 'foo, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. while compiling method token 0x6000003
1>Could not load type 'Foo' from assembly 'foo, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. while compiling method token 0x6000004

It is most definitely a bug, and the fix looks good, and works. We just need to understand how likely a developer's project is to be affected by this change. I think the likelyhood of encountering this warning in an existing project, is probably non zero, but likely very low, for that reason, I am okay with taking this change as is, correctness in this case is preferable to compatability.

[KevinRansom]

@forki, thanks for this mate.

[smoothdeveloper]

this should be at the end of the file.